From 21343dc3aa6de5c7a1a631e410857ad6264b5e5e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 15:34:36 -0400 Subject: [PATCH 1/6] fix(plugin): the session-start Goal line cuts at a word and says where the rest is (#4036) A raw 200-char slice ended mid-word with nothing marking the cut, so a reader took half a sentence for the whole goal. _goal_line flattens the goal to one line, trims at a word break with an ellipsis, and points at enter_project(id) when it cut. Co-Authored-By: Claude Opus 5 (1M context) --- src/scribe/services/plugin_context.py | 24 +++++++++++++++++++++++- tests/test_services_plugin_context.py | 26 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 14fe9e5..6e3ff53 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -16,6 +16,7 @@ from __future__ import annotations import logging import re +import textwrap import time @@ -41,6 +42,10 @@ _MAX_CHARS = 9000 # Max chars of a Process body to fold into the auto-surface description. _PROC_PREVIEW_CHARS = 200 +# Max chars of the project goal on the session-start Goal line. The full goal is +# one enter_project away; a cut says so rather than ending mid-word (#4036). +_GOAL_CHARS = 200 + # --- Knowledge auto-inject (Path A: per-turn awareness push) ----------------- # Per-user settings (keys live in the generic settings table). The threshold is # deliberately STRICTER than the pull-search default (embeddings @@ -2140,6 +2145,23 @@ def _stamp_line(path: str, stamped: list[dict]) -> str: +def _goal_line(goal: str, project_id: int) -> str: + """The Goal line, trimmed at a word break with a visible cut. + + A raw slice ended mid-word with nothing to say more existed, so a reader + took half a sentence for the whole goal (#4036). + """ + if not goal: + return "" + flat = " ".join(goal.split()) + if len(flat) <= _GOAL_CHARS: + return f"Goal: {flat}" + short = textwrap.shorten(flat, width=_GOAL_CHARS, placeholder="…") + if short == "…": # one unbroken word longer than the cap + short = flat[: _GOAL_CHARS - 1] + "…" + return f"Goal: {short} (full goal: `enter_project({project_id})`)" + + async def build_session_context( user_id: int, project_id: int = 0, unbound_repo: str = "" ) -> dict: @@ -2185,7 +2207,7 @@ async def build_session_context( lines += [ "", f"## Active project: {project.title} (id {project.id})", - f"Goal: {goal[:200]}" if goal else "", + _goal_line(goal, project.id), f"Open todo tasks: {open_count}", ] diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 15a7af4..21f43a7 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -120,6 +120,32 @@ async def test_build_session_context_includes_project_when_scoped(): assert "Reflex:" not in out["context"] +def test_a_short_goal_is_shown_whole(): + from scribe.services.plugin_context import _goal_line + assert _goal_line("ship it", 2) == "Goal: ship it" + assert _goal_line("", 2) == "" + + +def test_a_long_goal_is_cut_at_a_word_and_says_where_the_rest_is(): + """#4036: a raw slice ended mid-word with nothing marking the cut, so a + reader took half a sentence for the whole goal.""" + from scribe.services.plugin_context import _GOAL_CHARS, _goal_line + goal = "make the record reach the next session\nso a solution is recalled " * 10 + line = _goal_line(goal, 2) + shown = line.removeprefix("Goal: ").split(" (full goal:")[0] + assert shown.endswith("…") and len(shown) <= _GOAL_CHARS + # The cut lands between words: what precedes the ellipsis is a whole word. + assert shown[:-1].rstrip().split()[-1] in goal.split() + assert "\n" not in line + assert line.endswith("(full goal: `enter_project(2)`)") + + +def test_an_unbroken_goal_still_shows_its_start(): + from scribe.services.plugin_context import _GOAL_CHARS, _goal_line + shown = _goal_line("x" * 500, 2).removeprefix("Goal: ").split(" (full goal:")[0] + assert shown == "x" * (_GOAL_CHARS - 1) + "…" + + @pytest.mark.asyncio async def test_build_session_context_pushes_the_projects_design_system(): """The gap this closes: a design system had no push channel, so its -- 2.54.0 From 9071cb05da27069abd5a0c02448f63f24dd05578 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 16:01:34 -0400 Subject: [PATCH 2/6] fix(410): using-scribe states the retrieval reflex once, and the adapter stops restating it (#4039) Step 7's live check (#4034 "a real session shows each topic stated once") found nothing copied across surfaces, but found the owner repeating itself: using-scribe said "rules are retrieved; ask before a consequential act; an empty session is not an empty rulebook" in "Do this first", in reflex 2, and twice more further down reflex 2. - using-scribe: reflex 2 now holds the one full statement (how rules arrive, why that is the surface working, what silence means, when to ask). "Do this first" keeps the enter_project step and a short lead-in pointing at reflex 2. "A retrieved rule outranks a default habit" and "Ask hardest where you feel most certain" stay, since each adds something. - Adapter static context: "Lines injected beside your work" keeps only the Claude Code timing and leaves "never the whole set" to the skill. "Keep one copy" names CLAUDE.md and auto-memory and points at using-scribe, which already says to leave a client's settings as they are. - The ownership registry's markers and statement for the topic still sit on using-scribe, and nothing else changed owner. Plugin version minted. Sizes: using-scribe 18,823 -> 18,197 chars; scribe_static_context.md 1,797 -> 1,714. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_static_context.md | 11 +++---- plugin/skills/using-scribe/SKILL.md | 46 ++++++++++----------------- 3 files changed, 23 insertions(+), 36 deletions(-) diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index f53a334..00946bb 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.14.1723", + "version": "2026.09.14.2001", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_static_context.md b/plugin/hooks/scribe_static_context.md index 5dac691..d20ddde 100644 --- a/plugin/hooks/scribe_static_context.md +++ b/plugin/hooks/scribe_static_context.md @@ -10,15 +10,14 @@ shape-accounting) carry their arcs. What only Claude Code needs said: -- **Keep one copy — in Scribe, not Claude Code's local memory.** The - operator's rules, plans and project notes go to Scribe, not also to - `CLAUDE.md` or auto-memory. Leave auto-memory at its default setting: you - replace its job by doing the work in Scribe, not by switching it off. +- **Keep one copy — in Scribe, not Claude Code's local memory.** Claude + Code's local memory is `CLAUDE.md` and auto-memory: the operator's rules, + plans and project notes go to Scribe instead, and using-scribe says how + Scribe works alongside them. - **Lines injected beside your work are retrieval.** When the operator sends a message, and before a write or a command, Scribe may add rules, preferences, notes and prior art that resemble what you are doing. Open the ones that - apply. They are what matched, never the whole set — using-scribe says how to - ask for the rest. + apply; using-scribe says what a quiet turn means. - **Compact at clean seams.** Because work is recorded as you go, a compaction is safe once in-flight state is logged. After finishing a block of work in a long session, log it to Scribe, then tell the operator it's a good moment to diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 33a8419..a63d758 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -13,23 +13,14 @@ asked for. ## Do this first (every session) -**You are not holding the operator's rules, and no call loads them all.** -There is no standing set to pull. A rule reaches you when what you are about to -do matches it — a command, code you are writing, or what the operator just -asked for — and on most turns none will. That is the surface working. - -**So the reflex is to ASK, not to load.** Before a consequential act — anything -hard to reverse or outward-facing — `search(content_type="rule")` for the thing -you are about to do. An empty session is not evidence of an empty rulebook. - If the working repo maps to a Scribe project (you're in a known repo, or `list_repo_bindings` shows a binding), call `enter_project(id)` — it returns the project plus the rules bound to the areas it works in, open tasks, and recent notes in one shot. -Do this actively. Nothing is handed to a session up front to stand in for it — -rules arrive by retrieval, when your work or the operator's message matches -one — so asking and entering the project are the reliable path. +Then **ask before you act**: before anything hard to reverse or outward-facing, +search the rules for what you are about to do. Reflex 2 below is why asking, +not loading, is how the operator's rules reach you. ## Scribe holds these functions — keep one copy @@ -59,12 +50,20 @@ Two constraints on *how* that's achieved: re-deriving it or opening a duplicate. When a project is in scope, pass its `project_id` so results stay scoped. -2. **Rules are binding, and silence does not mean there are none.** Nothing - is preloaded, so "no rule arrived" means "nothing matched" — never "no rule - exists". Ask with `search(content_type="rule")` before a consequential act, - and pull a record's full statement with `get_rule(id)` when it is about to - bite. When a project is in scope, `enter_project(id)` also returns the rules - bound to its areas. +2. **Rules are binding, and they reach you by retrieval.** No call loads the + operator's rules and no standing set is handed to a session. A rule arrives + when what you are about to do resembles what it is about — a command, the + code you are writing, the operator's message — and on most turns none will. + That is the surface working: it is what lets a rulebook keep growing, since + a rule costs nothing in a session it has nothing to do with. A rule binds + just as hard for never having been handed to you. + + So "no rule arrived" means "nothing matched", never "no rule exists" — an + empty session is not evidence of an empty rulebook. Retrieval fires when + something asks: before a consequential act, `search(content_type="rule")` on + what you are about to do, and pull a record's full statement with + `get_rule(id)` when it is about to bite. When a project is in scope, + `enter_project(id)` also returns the rules bound to its areas. **`kind` says how much force a record carries, and it is never something to infer.** A **rule** must be followed: ignoring it breaks something or @@ -84,17 +83,6 @@ Two constraints on *how* that's achieved: asks. If what you learned is that something MUST be done a certain way, that is a rule to propose, not a preference to harden in place. - Every rule is RETRIEVED: one reaches you when a command, the code you are - writing or the operator's message resembles what it is about, and a rule - binds just as hard for never having been handed to you. So before a - consequential act, `search(content_type="rule")` on what you are about to - do. An empty session is not evidence that no rule applies; it is only - evidence that nothing has matched yet, and those are different claims. - - Retrieval is what lets a rulebook keep growing — a rule costs nothing in a - session it has nothing to do with — but retrieval only fires if something - asks. - **A retrieved rule outranks a default habit.** Before a hard-to-reverse or outward-facing act — changing shared state, publishing, deleting, sending something outside the session — the operator's rules decide what to do, not -- 2.54.0 From 921565696c646ae18d0ad4467ac2f4c227ffe029 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 17:43:16 -0400 Subject: [PATCH 3/6] feat(409): an operator's own reply shapes reach the reply they are about (#4013) The reporting-back skill ships default shapes; an operator's adjustments to them are preference records. Prompt-time retrieval matches the operator's message, and a shape preference is about the reply, so those preferences were on file and never arrived. Operator's decision (logged on #4013): the server delivers them for a completion report, and the skill asks for every other kind. - Completion reports (option C): closing a task with update_task runs a kind-filtered preference search for the moment "writing the completion report after finishing a task" and returns matches as `reply_preferences` ({id, title, statement, kind}), with a sentence added to `report_back` naming the key. A preference says it is about completion reports through its own when_to_apply; no tag or column. Omitted when nothing matches, and the lookup fails open. - Telemetry: every call logs to retrieval_logs under `report_preference` (empty calls included; a search that never ran writes no row) and hits are recorded surfaced. The source is ranked, so it counts toward pull-through. The bar is the prompt arm's setting until step 6 reads this source's near misses. - Every other reply (option A): reporting-back gains "The operator's own shapes come first". Before a finding, decision, handoff or "where are we", search(content_type="rule") in the words of that moment and follow what comes back. Registered in the ownership guard with reporting-back as owner. - Loading reply shapes at session start (option B) was rejected: it would be a small copy of the preloading milestone 394 retired. Domain-neutral query (pinned); works on an install with no preferences. Plugin version minted. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/skills/reporting-back/SKILL.md | 18 +++ src/scribe/mcp/tools/tasks.py | 23 +++- src/scribe/services/reply_preferences.py | 135 +++++++++++++++++++++++ src/scribe/services/rule_usage.py | 3 + tests/test_guidance_ownership.py | 3 + tests/test_mcp_tool_report_back_cue.py | 37 ++++++- tests/test_services_reply_preferences.py | 81 ++++++++++++++ 8 files changed, 294 insertions(+), 8 deletions(-) create mode 100644 src/scribe/services/reply_preferences.py create mode 100644 tests/test_services_reply_preferences.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 00946bb..ae281d1 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.14.2001", + "version": "2026.09.14.2143", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/skills/reporting-back/SKILL.md b/plugin/skills/reporting-back/SKILL.md index f7718e3..90d91b5 100644 --- a/plugin/skills/reporting-back/SKILL.md +++ b/plugin/skills/reporting-back/SKILL.md @@ -41,6 +41,24 @@ it is wrong. So take placement from Scribe: - 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. +## The operator's own shapes come first + +The shapes below are defaults. An operator may have changed some of them — a +section they always want, an order they read faster, a kind of reply they want +shorter — and those changes are `preference` records. Where a preference and a +default differ, the preference is what they asked for. + +- **A completion report brings its preferences with it.** Closing a task with + `update_task` returns them as **`reply_preferences`** when the operator has + any; the `report_back` line says so. Nothing to search for. +- **Every other reply, ask before writing it.** A finding, a decision, a + handoff, a "where are we" — no tool call comes before these, so nothing + hands their preferences over. Once you know which kind of reply you are + writing, `search(content_type="rule")` for it in the words of that moment — + "writing a decision for the operator", "handing off to the operator" — and + follow any preference that comes back. Nothing coming back means the default + shape stands. + ## Reports — work happened | Kind | Sections | diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 6e93a9a..72fe242 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -31,6 +31,7 @@ from scribe.services.notes import minted_kind from scribe.services import placement as placement_svc from scribe.services import planning as planning_svc from scribe.services import record_batch as batch_svc +from scribe.services import reply_preferences as reply_prefs_svc from scribe.services import rulebooks as rulebooks_svc from scribe.services import systems as systems_svc from scribe.services import task_logs as task_logs_svc @@ -273,7 +274,13 @@ async def update_task( reads exactly like a real one when it is wrong. Closing a task (done or cancelled) also returns `report_back`: a one-line - reminder of what the reply to the operator should cover. + reminder of what the reply to the operator should cover. When the + operator has preferences for how a completion report is written, they + come back as `reply_preferences` ({id, title, statement, kind}) — found + by their `when_to_apply`, so a preference whose trigger is writing the + report after finishing a task is the one that arrives here. Where one + differs from the default shape, the preference is what the operator + asked for. """ uid = current_user_id() fields: dict = {} @@ -314,6 +321,14 @@ async def update_task( await placement_svc.attach_placement(uid, data, note) if status in _CLOSING_STATUSES: data["report_back"] = REPORT_BACK_CUE + # The operator's own adjustments to the completion report, retrieved + # at the one moment a server can see that report coming (milestone + # 409 step 4). Omitted rather than sent empty, like every decoration. + prefs = await reply_prefs_svc.completion_preferences( + uid, project_id=getattr(note, "project_id", None)) + if prefs: + data["reply_preferences"] = prefs + data["report_back"] = REPORT_BACK_CUE + " " + REPLY_PREFERENCES_CUE return data @@ -360,6 +375,12 @@ REPORT_BACK_CUE = ( "Reporting this to the operator? Say where it sits (from `placement`), " "what now works, what needs them, and what comes next." ) +# Appended only when `reply_preferences` is present, so the key never arrives +# unexplained and a session with no preferences reads exactly what it did. +REPLY_PREFERENCES_CUE = ( + "The operator has preferences for how this report is written — " + "follow `reply_preferences` over the default shape where they differ." +) _ITEM_KEYS = {"title", "body", "type", "status", "priority", "kind", "tags", "system_ids"} diff --git a/src/scribe/services/reply_preferences.py b/src/scribe/services/reply_preferences.py new file mode 100644 index 0000000..7065a0d --- /dev/null +++ b/src/scribe/services/reply_preferences.py @@ -0,0 +1,135 @@ +"""The operator's own preferences for a completion report, at the moment it is written. + +WHY THIS EXISTS (milestone 409 step 4) + +The reporting-back skill ships DEFAULT shapes. An operator will want some of +them different ("my completion reports also say how it was tested", "decisions +as a numbered list"), and those adjustments are `preference` records. The gap +is the query: prompt-time retrieval matches the OPERATOR'S MESSAGE, and a shape +preference is about the REPLY. "Fix the flaky test" never retrieves "completion +reports should say how it was tested", so the preference is on file and never +arrives. + +THE DECISION (operator, 2026-09-14, logged on the step): two deliveries, split +by reply kind. + + - A COMPLETION REPORT has a moment the server can see — a task closing — so + the server retrieves for it and hands the matches back beside + `report_back`. That is this module. + - EVERY OTHER REPLY KIND (a finding, a decision, a handoff…) has no tool call + in front of it, so the reporting-back skill asks: it tells the agent to + `search(content_type="rule")` for that kind before writing. + +Loading reply-shape preferences at session start was the rejected third option: +it is a small copy of the preloading milestone 394 retired, and just as +unmeasurable. + +HOW A PREFERENCE SAYS IT IS ABOUT COMPLETION REPORTS + +By its trigger, which is what it already has — no tag, no new column. A +preference's `when_to_apply` dominates its embedded document, so one written +for this moment ("writing the report after finishing a task") resembles +COMPLETION_QUERY below, and one about anything else does not. That keeps +delivery entirely in retrieval, as 394 decided, and leaves an operator nothing +new to learn: a preference reaches the completion report the same way every +other record reaches its moment. + +THE BAR IS THE PROMPT ARM'S, AND IT IS NOT YET EARNED HERE + +A fixed query against triggers is a different score distribution from an +operator's message against the same documents. Starting at the prompt arm's +setting is the value with evidence behind it, and an operator's tuning of that +bar reaches this too. Every call logs under its own source, +`report_preference`, so step 6 can read this surface's near misses apart from +the prompt arm's before anyone moves the number. +""" +from __future__ import annotations + +import logging +import time + +from scribe.services.embeddings import semantic_search_rules +from scribe.services.plugin_context import ( + PROMPTRULE_DEFAULT_THRESHOLD, + PROMPTRULE_THRESHOLD_KEY, +) +from scribe.services.retrieval_telemetry import record_retrieval +from scribe.services.rule_usage import record_rule_surfaced +from scribe.services.settings import get_setting + +logger = logging.getLogger(__name__) + +SOURCE = "report_preference" + +# Written in the vocabulary of the MOMENT, because that is what a trigger is +# written in and what this query is scored against. Domain-neutral on purpose +# (rule #115): a writing project or a home-infrastructure project closes tasks +# too, and its operator's preferences must match as well as a developer's. +COMPLETION_QUERY = ( + "writing the completion report to the operator after finishing a task — " + "how that reply should be laid out and what it should include" +) + +# A handful, not a menu. More than a few shape preferences for ONE kind of +# reply would contradict each other before they helped; the limit is here to +# keep one noisy corpus from turning a status change into a wall of text. +LIMIT = 3 + + +async def _threshold(user_id: int) -> float: + try: + value = float(await get_setting( + user_id, PROMPTRULE_THRESHOLD_KEY, str(PROMPTRULE_DEFAULT_THRESHOLD))) + except (TypeError, ValueError): + value = PROMPTRULE_DEFAULT_THRESHOLD + return min(1.0, max(0.0, value)) + + +async def completion_preferences(user_id: int, *, project_id: int | None = None) -> list[dict]: + """The operator's preferences for a completion report, best match first. + + KIND-FILTERED, for the reason `_reserve_slot_for_preference` gives: a + binding rule that happened to resemble the query would otherwise ride out + under a key that says "how the operator likes this written", which is a + claim about force the record does not make. + + Every call is logged, the empty ones included: a surface that records only + the calls it liked reports a flawless clear-rate however badly its bar is + set. An install with no preferences at all logs nothing, because no search + ran (`searched` stays False) — that is not a decline. + + Fails open to an empty list: this decorates a write that has already + happened, and a lookup that errors must not turn it into a failure. + """ + try: + threshold = await _threshold(user_id) + report: dict = {} + t0 = time.perf_counter() + hits = await semantic_search_rules( + user_id, COMPLETION_QUERY, limit=LIMIT, threshold=threshold, + kind="preference", report=report, + ) + hits = [(score, rule) for score, rule in hits if rule.kind == "preference"] + record_retrieval( + user_id=user_id, source=SOURCE, query=COMPLETION_QUERY, + threshold=threshold, limit=LIMIT, project_id=project_id, + is_task=None, results=hits, + best_available=report.get("best_available_score"), + best_available_id=report.get("best_available_id"), + searched=bool(report.get("searched", True)), + duration_ms=(time.perf_counter() - t0) * 1000.0, + ) + if not hits: + return [] + # RANKED: this surface chose what it showed, so the name is in + # rule_usage.RANKED_SOURCES and its hits count toward pull-through. + record_rule_surfaced( + user_id=user_id, rule_ids=[rule.id for _s, rule in hits], source=SOURCE, + ) + return [ + {"id": rule.id, "title": rule.title, "statement": rule.statement, "kind": "preference"} + for _score, rule in hits + ] + except Exception: # noqa: BLE001 - a decoration never breaks its payload + logger.warning("completion preference lookup failed", exc_info=True) + return [] diff --git a/src/scribe/services/rule_usage.py b/src/scribe/services/rule_usage.py index 3162b41..1fc53cc 100644 --- a/src/scribe/services/rule_usage.py +++ b/src/scribe/services/rule_usage.py @@ -105,6 +105,9 @@ RANKED_SOURCES = ( # surface built because a record class kept losing would be the one whose # hits nobody could confirm. "preference_slot", + # The completion-report lookup on update_task (milestone 409 step 4). It + # runs its own query and shows only what cleared the bar — a ranker. + "report_preference", ) diff --git a/tests/test_guidance_ownership.py b/tests/test_guidance_ownership.py index 7ae0e23..1d1a23b 100644 --- a/tests/test_guidance_ownership.py +++ b/tests/test_guidance_ownership.py @@ -152,6 +152,9 @@ TOPICS: tuple[Topic, ...] = ( "prior art offered beside a write is not noise", index=("create_snippet",)), Topic("report back where the work stands", "skill:reporting-back", ("reporting-back", "placement"), "take the placement from the record", index=("placement",)), + Topic("the operator's own reply shapes come first", "skill:reporting-back", + ("reply_preferences", 'content_type="rule"'), + "the operator's own shapes come first"), # ── 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_mcp_tool_report_back_cue.py b/tests/test_mcp_tool_report_back_cue.py index 67094a1..7d2129c 100644 --- a/tests/test_mcp_tool_report_back_cue.py +++ b/tests/test_mcp_tool_report_back_cue.py @@ -3,6 +3,9 @@ The in-band half of milestone 409 step 3: the skill and static context only exist in the Claude Code plugin, and a tool response reaches every MCP client at the moment a piece of work closes. Pinned on the response, not the wording. + +Step 4 adds the operator's own completion-report preferences beside the cue, +retrieved at that same moment — and only then, and only when there are any. """ from unittest.mock import AsyncMock, MagicMock, patch @@ -10,24 +13,46 @@ import pytest pytestmark = pytest.mark.usefixtures("_bind_user") +_PREF = {"id": 41, "title": "Say how it was checked", "statement": "…", "kind": "preference"} -async def _update(**kwargs): + +async def _update(prefs=None, **kwargs): from scribe.mcp.tools.tasks import update_task - note = MagicMock(id=5, user_id=7, project_id=None) + note = MagicMock(id=5, user_id=7, project_id=3) note.to_dict.return_value = {"id": 5} + lookup = AsyncMock(return_value=list(prefs or [])) with patch("scribe.mcp.tools.tasks.notes_svc.update_note", AsyncMock(return_value=note)), \ patch("scribe.mcp.tools.tasks.systems_tools.attach_systems", AsyncMock()), \ - patch("scribe.mcp.tools.tasks.placement_svc.attach_placement", AsyncMock()): - return await update_task(task_id=5, **kwargs) + patch("scribe.mcp.tools.tasks.placement_svc.attach_placement", AsyncMock()), \ + patch("scribe.mcp.tools.tasks.reply_prefs_svc.completion_preferences", lookup): + return await update_task(task_id=5, **kwargs), lookup @pytest.mark.parametrize("status", ["done", "cancelled"]) async def test_closing_a_task_carries_the_cue(status): - out = await _update(status=status) + out, _ = await _update(status=status) assert "placement" in out["report_back"] and "needs them" in out["report_back"] @pytest.mark.parametrize("kwargs", [{"status": "in_progress"}, {"status": "todo"}, {"body": "more notes"}]) async def test_other_updates_do_not(kwargs): - assert "report_back" not in await _update(**kwargs) + out, lookup = await _update(prefs=[_PREF], **kwargs) + assert "report_back" not in out and "reply_preferences" not in out + lookup.assert_not_awaited() + + +async def test_closing_hands_back_the_operators_report_preferences(): + out, lookup = await _update(prefs=[_PREF], status="done") + assert out["reply_preferences"] == [_PREF] + # The cue says the key is there, so it never arrives unexplained. + assert "reply_preferences" in out["report_back"] + lookup.assert_awaited_once_with(7, project_id=3) + + +async def test_no_preferences_means_no_key_and_the_plain_cue(): + from scribe.mcp.tools.tasks import REPORT_BACK_CUE + + out, _ = await _update(prefs=[], status="done") + assert "reply_preferences" not in out + assert out["report_back"] == REPORT_BACK_CUE diff --git a/tests/test_services_reply_preferences.py b/tests/test_services_reply_preferences.py new file mode 100644 index 0000000..4c2e428 --- /dev/null +++ b/tests/test_services_reply_preferences.py @@ -0,0 +1,81 @@ +"""The completion-report preference lookup (milestone 409 step 4). + +What it pins: the lookup asks for PREFERENCES only, logs every call under its +own source (the empty ones too), counts only what it showed as surfaced, and +fails open. The query stays domain-neutral, because every kind of project +closes tasks. +""" +import re +from unittest.mock import AsyncMock, MagicMock, patch + +M = "scribe.services.reply_preferences" + + +def _rule(rid, kind="preference"): + return MagicMock(id=rid, kind=kind, title=f"t{rid}", statement=f"s{rid}") + + +async def _run(hits, *, searched=True, raises=None): + from scribe.services.reply_preferences import completion_preferences + + async def search(user_id, query, **kw): + if raises: + raise raises + kw["report"].update({"searched": searched, "best_available_score": 0.7}) + return hits + + search_mock = AsyncMock(side_effect=search) + with patch(f"{M}.semantic_search_rules", search_mock), \ + patch(f"{M}.get_setting", AsyncMock(return_value="0.72")), \ + patch(f"{M}.record_retrieval") as logged, \ + patch(f"{M}.record_rule_surfaced") as surfaced: + out = await completion_preferences(7, project_id=3) + return out, search_mock, logged, surfaced + + +async def test_asks_for_preferences_only_and_returns_them_best_first(): + out, search, logged, surfaced = await _run([(0.9, _rule(1)), (0.8, _rule(2))]) + assert search.await_args.kwargs["kind"] == "preference" + assert [p["id"] for p in out] == [1, 2] + assert all(p["kind"] == "preference" for p in out) + assert logged.call_args.kwargs["source"] == "report_preference" + assert surfaced.call_args.kwargs == {"user_id": 7, "rule_ids": [1, 2], "source": "report_preference"} + + +async def test_a_rule_that_slips_through_is_not_handed_back_as_a_preference(): + out, _, _, surfaced = await _run([(0.9, _rule(1, kind="rule")), (0.8, _rule(2))]) + assert [p["id"] for p in out] == [2] + assert surfaced.call_args.kwargs["rule_ids"] == [2] + + +async def test_an_empty_call_is_still_logged_and_surfaces_nothing(): + out, _, logged, surfaced = await _run([]) + assert out == [] + logged.assert_called_once() + assert logged.call_args.kwargs["results"] == [] + surfaced.assert_not_called() + + +async def test_a_search_that_never_ran_says_so_to_the_log(): + _, _, logged, _ = await _run([], searched=False) + assert logged.call_args.kwargs["searched"] is False + + +async def test_fails_open(): + out, _, _, surfaced = await _run([], raises=RuntimeError("embedder down")) + assert out == [] + surfaced.assert_not_called() + + +def test_it_is_a_ranked_source(): + from scribe.services.rule_usage import is_ambient + + assert not is_ambient("report_preference") + + +def test_the_query_assumes_no_particular_domain(): + from scribe.services.reply_preferences import COMPLETION_QUERY + + dev_only = [w for w in (r"\bCI\b", r"\bcommit", r"\bpull request", r"\bcode\b", r"\btest") + if re.search(w, COMPLETION_QUERY, re.IGNORECASE)] + assert not dev_only, f"software-only vocabulary in a query every project runs: {dev_only}" -- 2.54.0 From dd80e2bc86a423ceecc66e24e66646653bbd410e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 18:34:46 -0400 Subject: [PATCH 4/6] feat(409): a Stop hook checks that a reply closing a task has the completion sections (#4014) Everything else Scribe gives an agent arrives before the reply is written. A Stop hook is the one moment the finished reply exists, so it is the last chance to fix a report the operator can't read, and the only place adherence to the shape can be measured. - plugin/hooks/scribe_report_check.sh (Stop): deterministic, no model call. 1. Did this turn close a task? That means an update_task/create_task call with status "done" since the turn's prompt, whose tool_result is not an error. Otherwise it stays silent, which covers most turns (one grep). 2. Does the reply that ends the turn say where the work sits (a record by id and title, or step N of M), what needs the operator, and what comes next? Matched on those words, not on exact headings. 3. If sections are missing, it blocks once. With stop_hook_active set, a rewrite is recorded (passed_after_rewrite / missing_after_rewrite) and never blocked again. A block loop started by another plugin (no marker from this hook) is left alone. - Measured: every checked reply is reported to GET /api/plugin/report-check (passed / blocked / after rewrite). Turns that close nothing are not reported; they would cost a request per turn and add nothing to the rate. Outcomes go to app_logs as category "plugin", action "report_check". - It blocks only when the block was recorded, and only in the server's words. The endpoint returns the block reason, so the hook carries timing and transport only (PACKAGING.md), and an unconfigured or unreachable instance never stops a session. - The transcript format is read from real transcripts and marked in the hook as observed rather than documented. The Stop contract (transcript_path, stop_hook_active, decision/reason, no matcher, SubagentStop separate) was checked against the Claude Code hooks docs. A prompt-type hook was not needed: the deterministic check passed a real completion report from this session and blocked a stripped one. - A pipefail trap was caught while exercising the hook: `tail | grep -q` reports failure exactly when grep matches, because tail dies of SIGPIPE. The prefilter reads through process substitution; the section checks use here-strings. - Tests: an end-to-end hook suite over synthetic transcripts and the shared HTTP sink (silence, pass, server-worded block, rewrite recorded, foreign loop, errored write, earlier turn, unwritten reply, no recorded check, bare id), and service tests for the reason wording and the outcome record. Smoke event added to check_plugin; README and PACKAGING list the hook and endpoint. Plugin version minted. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/PACKAGING.md | 3 +- plugin/README.md | 9 ++ plugin/hooks/hooks.json | 10 ++ plugin/hooks/scribe_report_check.sh | 186 ++++++++++++++++++++++++++++ scripts/check_plugin.py | 7 ++ src/scribe/routes/plugin.py | 40 ++++++ src/scribe/services/report_check.py | 80 ++++++++++++ tests/test_report_check_hook.py | 168 +++++++++++++++++++++++++ tests/test_services_report_check.py | 47 +++++++ 10 files changed, 550 insertions(+), 2 deletions(-) create mode 100644 plugin/hooks/scribe_report_check.sh create mode 100644 src/scribe/services/report_check.py create mode 100644 tests/test_report_check_hook.py create mode 100644 tests/test_services_report_check.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index ae281d1..6017ce9 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.14.2143", + "version": "2026.09.14.2234", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/PACKAGING.md b/plugin/PACKAGING.md index a0919f6..fd96a0e 100644 --- a/plugin/PACKAGING.md +++ b/plugin/PACKAGING.md @@ -15,7 +15,7 @@ another one means adding files, not moving or rewriting any. |---|---|---| | **The skills** | `plugin/skills/*/SKILL.md` | Agent Skills (the open SKILL.md format). They state every Scribe reflex in full and name no client. `tests/test_guidance_ownership.py` fails if a skill names a particular client, or references anything outside its own folder. Every client package ships this folder verbatim. | | **The MCP server** | `/mcp` | HTTP, `Authorization: Bearer `. Its `_INSTRUCTIONS` is a client-neutral index (≤2,000 chars); each tool's description carries its contract; in-band responses (`placement`, `report_back`, `systems_hint`, the duplicate gate, the guessed-id refusal) fire in every client. | -| **The adapter API** | `/api/plugin/*` | Plain `GET` endpoints any client's hooks can call with the same key (read scope is enough): `context` (live session state), `retrieve` (rules, preferences and notes for a message), `prior-art` (records and shape-ledger hints for code being written), `tool-rules` (rules for a command about to run), `processes` (stored Processes to expose as skills). | +| **The adapter API** | `/api/plugin/*` | Plain `GET` endpoints any client's hooks can call with the same key (read scope is enough): `context` (live session state), `retrieve` (rules, preferences and notes for a message), `prior-art` (records and shape-ledger hints for code being written), `tool-rules` (rules for a command about to run), `report-check` (records a completion-report check and returns the reason for a block), `processes` (stored Processes to expose as skills). | | **The API key** | Scribe → Settings → API Keys | One `fmcp_` key per install. Read scope for hooks; write scope for the MCP tools. | ## Added by each client @@ -39,6 +39,7 @@ another one means adding files, not moving or rewriting any. | `hooks/scribe_prior_art.sh` | PreToolUse on editor writes: `GET /api/plugin/prior-art`. | | `hooks/scribe_after_write.sh` | PostToolUse on shell commands: the same check for code written through the shell. | | `hooks/scribe_tool_rules.sh` | PreToolUse on shell commands: `GET /api/plugin/tool-rules`. | +| `hooks/scribe_report_check.sh` | Stop: when the turn closed a task, checks the reply for the completion sections and reports to `GET /api/plugin/report-check`; blocks once, with the reason the server returns. | | `hooks/scribe_sync_processes.sh` + `commands/sync.md` | `GET /api/plugin/processes` → `~/.claude/skills/scribe-proc-*` stubs; `/scribe:sync` on demand. | | `hooks/scribe_defs.sh` | Shared shell helpers: config, dedup ledgers, outage line. | | `hooks/scribe_static_context.md` | The adapter static text. | diff --git a/plugin/README.md b/plugin/README.md index dea320e..cfd79e9 100644 --- a/plugin/README.md +++ b/plugin/README.md @@ -82,6 +82,15 @@ On install you'll be asked for: answer" line (8 s budget here — it runs after the tool, so it gates nothing). The extractor, the prose/data skip list, the local by-name duplicate arm and the outage line are shared in `hooks/scribe_defs.sh`. +- `hooks/hooks.json` → Stop hook (`hooks/scribe_report_check.sh`): when the + turn closed a Scribe task (`update_task`/`create_task` with status done), + checks the reply that ends it for the completion sections — where the work + sits, what needs you, what comes next — and reports the outcome to + `GET /api/plugin/report-check`. If sections are missing it blocks once with + the reason the server returns, and records how the rewrite came out; it + never blocks twice, and never blocks when the instance did not record the + check (unconfigured or unreachable). Outcomes land in the admin logs under + category `plugin`, action `report_check`. - `skills/` → the universal process-skills, surfaced by description match. - `hooks/scribe_sync_processes.sh` (a 2nd SessionStart hook) + the `/scribe:sync` command → generate `~/.claude/skills/scribe-proc-*` stubs from your Scribe diff --git a/plugin/hooks/hooks.json b/plugin/hooks/hooks.json index 0bec8ea..c55be67 100644 --- a/plugin/hooks/hooks.json +++ b/plugin/hooks/hooks.json @@ -54,6 +54,16 @@ } ] } + ], + "Stop": [ + { + "hooks": [ + { + "type": "command", + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_report_check.sh\"" + } + ] + } ] } } diff --git a/plugin/hooks/scribe_report_check.sh b/plugin/hooks/scribe_report_check.sh new file mode 100644 index 0000000..72f8a5d --- /dev/null +++ b/plugin/hooks/scribe_report_check.sh @@ -0,0 +1,186 @@ +#!/usr/bin/env bash +# Scribe plugin — Stop hook: a reply that closes a task carries the completion +# sections (milestone 409 step 5). +# +# Everything else the plugin does happens BEFORE the agent writes: context, +# retrieval, the reporting-back skill. This is the one moment the finished +# reply exists, so it is both the last chance to fix a report the operator +# cannot read and the only place adherence to the shape can be measured. +# +# DETERMINISTIC, NO MODEL CALL. Three questions, cheapest first: +# +# 1. Did this turn close a Scribe task? An `update_task` / `create_task` tool +# call with status "done" since the turn's prompt, whose result was not an +# error. Most turns stop here, silently. +# 2. Does the reply that ends the turn have the completion sections? Loosely: +# where the work sits (a record named by id and title, or step N of M), +# what needs the operator, and what comes next. Matched on the words that +# carry the meaning rather than exact headings, so the skill's wording can +# change without breaking this. +# 3. If sections are missing, block once with a reason naming them. The agent +# rewrites; the rewrite is checked and recorded, and never blocked again. +# +# MEASURED FROM THE FIRST CALL. Each checked reply is reported to the instance +# (`/api/plugin/report-check`): passed, blocked, and after a rewrite either +# passed_after_rewrite or missing_after_rewrite. Turns that closed no task are +# not reported — they would cost a request on every turn and add nothing to +# the rate step 6 reads (blocked among checked replies). +# +# IT BLOCKS ONLY WHEN THE BLOCK IS RECORDED, AND ONLY IN THE SERVER'S WORDS. +# The report goes out first; the instance answers a recorded `blocked` with +# the reason to send the agent back with, and the hook blocks only on that +# reason. An unconfigured or unreachable instance therefore never stops a +# session, every intervention is one the numbers can see, and the guidance +# text lives on the server (plugin/PACKAGING.md: hooks carry timing and +# transport). +# +# THE TRANSCRIPT FORMAT IS OBSERVED, NOT DOCUMENTED. Claude Code documents +# `transcript_path` and `stop_hook_active` for Stop, not the JSONL inside. As +# read from real transcripts (2026-09-14): one content block per line; +# `type: "assistant"` lines carry `message.content[]` blocks of `text` / +# `tool_use` ({id, name, input}); tool results arrive as `type: "user"` lines +# whose content is a `tool_result` array ({tool_use_id, is_error}); a turn's +# prompt — typed, or a background-task notification — is a `user` line whose +# content is a plain string and which is not `isMeta`. Anything that does not +# parse that way makes the hook stay out of the way rather than guess. +# +# Config (same as the other hooks): +# CLAUDE_PLUGIN_OPTION_API_ENDPOINT base URL, no trailing slash +# CLAUDE_PLUGIN_OPTION_API_TOKEN fmcp_ API key (sensitive) +# SCRIBE_URL / SCRIBE_TOKEN override for the settings.json dogfooding path. +set -uo pipefail + +command -v jq >/dev/null 2>&1 || exit 0 +command -v curl >/dev/null 2>&1 || exit 0 + +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + +# Stop delivers { session_id, transcript_path, cwd, hook_event_name, stop_hook_active }. +event=$(cat 2>/dev/null || true) +transcript=$(printf '%s' "$event" | jq -r '.transcript_path // empty' 2>/dev/null) || exit 0 +[ -n "$transcript" ] && [ -f "$transcript" ] || exit 0 +session_id=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || session_id="" +active=$(printf '%s' "$event" | jq -r '.stop_hook_active // false' 2>/dev/null) || active="false" +event_cwd=$(printf '%s' "$event" | jq -r '.cwd // empty' 2>/dev/null) || event_cwd="" + +safe_sid=$(printf '%s' "${session_id:-nosession}" | tr -c 'A-Za-z0-9._-' '_') +state_dir="${TMPDIR:-/tmp}/scribe-reportcheck" +mkdir -p "$state_dir" 2>/dev/null || true +marker="$state_dir/${safe_sid}.blocked" + +# Cheap prefilter: no task tool anywhere in the recent transcript → nothing to +# check. Keeps the ordinary turn at one grep. Process substitution, NOT a pipe: +# under `pipefail`, `grep -q` exiting on the first match kills `tail` with +# SIGPIPE, and the pipeline then reports failure precisely when it matched. +grep -q -E '"name":"([^"]*__)?(update|create)_task"' < <(tail -c 2000000 "$transcript" 2>/dev/null) || { + rm -f "$marker" 2>/dev/null || true + exit 0 +} + +# The turn, parsed once. A window of recent lines, slurped raw and split inside +# jq (a line-by-line `-R` read is the #2198 trap). A first line cut mid-record +# fails to parse and is dropped. If the window holds no prompt, the turn cannot +# be bounded, so the hook reports nothing and stays out of the way. +facts=$(tail -n 3000 "$transcript" 2>/dev/null | jq -sRc ' + split("\n") | map(try fromjson catch empty) + | map(select((.isSidechain // false) | not)) + | . as $lines + | [range(0; length) | select( + $lines[.].type == "user" and ($lines[.].isMeta // false | not) + and ($lines[.].message.content | type) == "string")] as $prompts + | if ($prompts | length) == 0 then {bounded: false} else + $lines[($prompts | last) + 1:] as $turn + | [ $turn[] | select(.type == "assistant") | .message.content[]? + | select(.type == "tool_use" + and ((.name // "") | test("(^|__)(update|create)_task$")) + and (.input.status? == "done")) + | {id, task: (.input.task_id? // null)} ] as $closes + | [ $turn[] | select(.type == "user") | .message.content[]? + | select(type == "object" and .type == "tool_result" and .is_error == true) + | .tool_use_id ] as $errors + | [ $closes[] | select(.id as $i | ($errors | index($i)) | not) ] as $closed + | ([range(0; $turn | length) | select( + $turn[.].type == "user" + or ($turn[.].type == "assistant" + and ([$turn[.].message.content[]?.type] | index("tool_use"))))] + | last // -1) as $last_act + | {bounded: true, + closed: ($closed | length), + task_ids: [$closed[].task | select(. != null)], + reply: ([ $turn[$last_act + 1:][] | select(.type == "assistant") + | .message.content[]? | select(.type == "text") | .text ] | join("\n"))} + end' 2>/dev/null) || exit 0 + +[ "$(printf '%s' "$facts" | jq -r '.bounded // false')" = "true" ] || exit 0 +closed=$(printf '%s' "$facts" | jq -r '.closed // 0') +if [ "${closed:-0}" = "0" ]; then + rm -f "$marker" 2>/dev/null || true + exit 0 +fi +reply=$(printf '%s' "$facts" | jq -r '.reply // ""') +task_ids=$(printf '%s' "$facts" | jq -r '.task_ids | map(tostring) | join(",")') + +# The reply may not be written to the transcript yet when the hook fires. An +# empty reply is "cannot tell", not "missing everything" — stay out of the way. +[ -n "$(printf '%s' "$reply" | tr -d '[:space:]')" ] || exit 0 + +missing=() +# Where the work sits: a record named by id AND title (#12 "…", milestone 3 "…"), +# or a step position. A bare id is exactly the homework this shape removes. +# shellcheck disable=SC2016 # backticks here are literal markdown, not an expansion +grep -q -i -E '(#[0-9]+|milestone [0-9]+|task [0-9]+)[*_`]*[[:space:]]*[*_`]*["“]|step [0-9]+ of [0-9]+' <<< "$reply" \ + || missing+=("where it sits") +# What needs the operator — "needs you: nothing" counts; it is an answer. +grep -q -i -E 'needs? (from )?you|nothing (is )?needed from you|your (call|decision)' <<< "$reply" \ + || missing+=("needs you") +# What comes next. +grep -q -i -E '\bnext\b' <<< "$reply" \ + || missing+=("next") + +# Reports the outcome; prints the instance's reply and returns 0 only if the +# instance recorded it. +report() { + scribe_config || return 1 + local q repo enc m + q="outcome=$1&task_ids=${task_ids}" + m=$(IFS=,; printf '%s' "${missing[*]:-}") + if [ -n "$m" ]; then + enc=$(printf '%s' "$m" | jq -sRr '@uri' 2>/dev/null) || enc="" + q="${q}&missing=${enc}" + fi + repo=$(git -C "${event_cwd:-${CLAUDE_PROJECT_DIR:-$PWD}}" remote get-url origin 2>/dev/null || true) + if [ -n "$repo" ]; then + enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" + [ -n "$enc" ] && q="${q}&repo=${enc}" + fi + curl -fsS --max-time 4 \ + -H "Authorization: Bearer ${token}" \ + "${url%/}/api/plugin/report-check?${q}" 2>/dev/null +} + +if [ "$active" = "true" ]; then + # A Stop hook already blocked this stop. If it was this one, the reply is + # the rewrite: record how it came out, and let the session stop whatever + # the answer. If it was another plugin's block, this hook has nothing to add. + [ -f "$marker" ] || exit 0 + rm -f "$marker" 2>/dev/null || true + if [ ${#missing[@]} -eq 0 ]; then report passed_after_rewrite >/dev/null; else report missing_after_rewrite >/dev/null; fi + exit 0 +fi +rm -f "$marker" 2>/dev/null || true + +if [ ${#missing[@]} -eq 0 ]; then + report passed >/dev/null + exit 0 +fi + +# The words the agent is sent back with are the server's (plugin/PACKAGING.md: +# a hook carries timing and transport). No reason back → nothing recorded → +# no block. +answer=$(report blocked) || exit 0 +reason=$(printf '%s' "$answer" | jq -r '.reason // empty' 2>/dev/null) || reason="" +[ -n "$reason" ] || exit 0 +: > "$marker" 2>/dev/null || true +jq -n --arg r "$reason" '{decision: "block", reason: $r}' +exit 0 diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index d0da1ac..8271ff4 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -325,6 +325,13 @@ SMOKE_EVENTS: dict[str, str] = { {"session_id": "smoke", "cwd": ".", "tool_name": "Bash", "tool_input": {"command": "true"}, "tool_response": {}} ), + # The Stop-hook report check (milestone 409 step 5). A transcript that does + # not exist is the smoke case: nothing to read, so it must stay silent and + # never block, configured or not. + "scribe_report_check.sh": json.dumps( + {"session_id": "smoke", "transcript_path": "/nonexistent/smoke.jsonl", + "cwd": ".", "hook_event_name": "Stop", "stop_hook_active": False} + ), # The shared library is sourced, never run; executed bare it defines # functions and exits — silent by construction. "scribe_defs.sh": "", diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index a0b1d6d..b812980 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -14,6 +14,7 @@ from scribe.auth import admin_required, get_current_user_id, login_required from scribe.config import Config from scribe.services import plugin_context as plugin_ctx_svc from scribe.services import repo_bindings as repo_bindings_svc +from scribe.services import report_check as report_check_svc from scribe.services.settings import get_admin_setting, set_setting plugin_bp = Blueprint("plugin", __name__, url_prefix="/api/plugin") @@ -257,6 +258,45 @@ def _parse_shapes(raw: str) -> list[tuple[str, str]]: return out +@plugin_bp.get("/report-check") +@login_required +async def report_check(): + """Record what a Stop hook found in a reply that closed a task (milestone 409 step 5). + + The hook decides which completion sections the reply lacks — a local check + of text it can read — and reports the outcome here. For `blocked` the + response carries the `reason` to send the agent back with: the words are + the server's, so every client's hook says the same thing (plugin/PACKAGING.md). + A hook blocks only on a `reason` it received, which means only on a block + that was recorded. + + A GET for the reason every plugin endpoint is one: a read-scoped key must + be enough to run the plugin, and this records telemetry the way /retrieve + records a retrieval log. + + Query: + outcome (str) — passed | blocked | passed_after_rewrite | + missing_after_rewrite. Anything else is a 400. + missing (opt) — comma-separated sections the reply lacked: + "where it sits", "needs you", "next". + task_ids (opt) — comma-separated ids of the tasks the turn closed. + repo (opt) — working repo remote, resolved like the other arms. + """ + outcome = (request.args.get("outcome") or "").strip() + if outcome not in report_check_svc.OUTCOMES: + return jsonify({"error": f"outcome must be one of {list(report_check_svc.OUTCOMES)}"}), 400 + missing = [m for m in (request.args.get("missing") or "").split(",") if m.strip()] + task_ids = _int_list(request.args.get("task_ids"))[:20] + project_id, _repo, _unbound = await _project_scope() + await report_check_svc.record_report_check( + g.user.id, outcome, missing=missing, task_ids=task_ids, project_id=project_id or None, + ) + body: dict = {"status": "ok"} + if outcome == "blocked": + body["reason"] = report_check_svc.block_reason(missing) + return jsonify(body) + + @plugin_bp.get("/processes") @login_required async def process_manifest(): diff --git a/src/scribe/services/report_check.py b/src/scribe/services/report_check.py new file mode 100644 index 0000000..ba88123 --- /dev/null +++ b/src/scribe/services/report_check.py @@ -0,0 +1,80 @@ +"""The report-shape check: what the plugin's Stop hook found, and what it says. + +WHY THIS EXISTS (milestone 409 step 5) + +Everything that helps an agent write a readable completion report arrives +BEFORE the reply is written. A client's Stop hook is the one moment the +finished reply exists, so it checks that a reply closing a task carries the +completion sections (where the work sits, what needs the operator, what comes +next), and reports what it found here. Two jobs live on this side: + + - RECORDING the outcome, so the rate of `blocked` among checked replies is a + number milestone 409's last step can read rather than an impression. + - OWNING THE WORDS the agent is sent back with. A hook carries timing and + transport only (plugin/PACKAGING.md); guidance text comes from the server, + so a second client's hook gets the same instruction by calling the same + endpoint, and the wording changes in one place. + +app_logs rather than a table of its own: one small event with a JSON detail is +what that table holds, it already has retention and an admin viewer, and +nothing here needs a join. If the numbers earn a readout, that is the moment to +decide whether they earn a table. +""" +from __future__ import annotations + +import json + +from scribe.models import async_session +from scribe.models.app_log import AppLog + +OUTCOMES = ("passed", "blocked", "passed_after_rewrite", "missing_after_rewrite") + +# The sections a hook may name as missing, in the order the reason lists them. +# Anything else a client sends is dropped rather than echoed into an +# instruction the agent will follow. +SECTIONS = ("where it sits", "needs you", "next") + + +def known_sections(missing: list[str]) -> list[str]: + wanted = {m.strip().lower() for m in missing} + return [s for s in SECTIONS if s in wanted] + + +def block_reason(missing: list[str]) -> str: + """What the agent is told when its completion report is sent back. + + Names what is missing and points at the reporting-back skill for the shape + rather than restating it — the skill owns the shape (decision #4027). + """ + listed = ", ".join(known_sections(missing)) or "the completion sections" + return ( + f"This turn closed a Scribe task, and the reply that ends it is missing: {listed}. " + "The operator reads this reply to find out where the work stands. Rewrite it as a " + "completion report (the reporting-back skill has the shape): where it sits — the task " + "or milestone by id and title, from `placement` — what now works, what needs them " + "(or \"nothing\"), and what comes next." + ) + + +async def record_report_check( + user_id: int | None, + outcome: str, + *, + missing: list[str] | None = None, + task_ids: list[int] | None = None, + project_id: int | None = None, +) -> None: + if outcome not in OUTCOMES: + raise ValueError(f"unknown report-check outcome {outcome!r}") + details: dict = {"outcome": outcome, "missing": known_sections(missing or []), + "task_ids": list(task_ids or [])} + if project_id: + details["project_id"] = project_id + async with async_session() as session: + session.add(AppLog( + category="plugin", + user_id=user_id, + action="report_check", + details=json.dumps(details), + )) + await session.commit() diff --git a/tests/test_report_check_hook.py b/tests/test_report_check_hook.py new file mode 100644 index 0000000..2269be9 --- /dev/null +++ b/tests/test_report_check_hook.py @@ -0,0 +1,168 @@ +"""The Stop hook that checks a task-closing reply for the completion sections +(milestone 409 step 5). + +Runs the real shell against synthetic transcripts in the shape Claude Code +writes (one content block per JSONL line) and the shared HTTP sink. What it +pins: silence on every turn that closed nothing; a block only when the +instance recorded it, in the words the instance returned; one rewrite at most, +recorded; and no block from another plugin's loop or a failed task write. +""" +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +from tests.helpers import http_sink + +HOOK = Path(__file__).resolve().parents[1] / "plugin" / "hooks" / "scribe_report_check.sh" +TOOL = "mcp__plugin_scribe_scribe__update_task" +GOOD = ('**Where this sits:** milestone 12 "Move the backups offsite", step 3 of 5.\n' + "**What now works:** the sync runs nightly.\n**Needs you:** nothing.\n**Next:** alerts.") +BAD = "All done, pushed it." +REASON = "SERVER REASON: rewrite as a completion report" + + +def _env(tmp_path, url="http://127.0.0.1:9"): + for tool in ("jq", "curl", "bash"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + return {"PATH": os.environ["PATH"], "SCRIBE_URL": url, "SCRIBE_TOKEN": "t", + "TMPDIR": str(tmp_path), "HOME": str(tmp_path)} + + +def _prompt(text="please finish it"): + return {"type": "user", "message": {"role": "user", "content": text}} + + +def _tool_use(tid="toolu_1", status="done", name=TOOL, task_id=41): + return {"type": "assistant", "message": {"content": [ + {"type": "tool_use", "id": tid, "name": name, "input": {"task_id": task_id, "status": status}}]}} + + +def _result(tid="toolu_1", is_error=False): + return {"type": "user", "message": {"content": [ + {"type": "tool_result", "tool_use_id": tid, "is_error": is_error, "content": "{}"}]}} + + +def _text(text): + return {"type": "assistant", "message": {"content": [{"type": "text", "text": text}]}} + + +def _transcript(tmp_path, lines): + path = tmp_path / "t.jsonl" + path.write_text("\n".join(json.dumps(line) for line in lines) + "\n") + return path + + +def _run(env, transcript, active=False, session="s1"): + out = subprocess.run( + ["bash", str(HOOK)], + input=json.dumps({"session_id": session, "transcript_path": str(transcript), + "cwd": str(transcript.parent), "hook_event_name": "Stop", + "stop_hook_active": active}), + capture_output=True, text=True, env=env, timeout=30, + ) + assert out.returncode == 0, out.stderr + return out.stdout.strip() + + +def _closing_turn(reply): + return [_prompt(), _text("On it."), _tool_use(), _result(), _text(reply)] + + +def test_a_turn_that_closed_nothing_is_silent_and_reports_nothing(tmp_path): + with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + t = _transcript(tmp_path, [_prompt(), _tool_use(status="in_progress"), _result(), _text(BAD)]) + assert _run(env, t) == "" + assert seen == [] + + +def test_a_complete_report_passes_silently_and_is_recorded(tmp_path): + with http_sink(b'{"status":"ok"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + assert _run(env, _transcript(tmp_path, _closing_turn(GOOD))) == "" + assert [q["outcome"] for q in seen] == [["passed"]] + assert seen[0]["task_ids"] == ["41"] + + +def test_a_missing_section_blocks_once_in_the_servers_words_then_records_the_rewrite(tmp_path): + reply = json.dumps({"status": "ok", "reason": REASON}).encode() + with http_sink(reply) as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + out = json.loads(_run(env, _transcript(tmp_path, _closing_turn(BAD)))) + assert out == {"decision": "block", "reason": REASON} + assert seen[0]["outcome"] == ["blocked"] + assert seen[0]["missing"] == ["where it sits,needs you,next"] + + # The rewrite: Claude Code sets stop_hook_active; the hook records and never blocks again. + rewritten = _transcript(tmp_path, _closing_turn(BAD) + [_text(GOOD)]) + assert _run(env, rewritten, active=True) == "" + assert seen[1]["outcome"] == ["passed_after_rewrite"] + assert _run(env, rewritten, active=True) == "" + assert len(seen) == 2 + + +def test_a_rewrite_that_still_misses_is_recorded_and_not_blocked(tmp_path): + reply = json.dumps({"status": "ok", "reason": REASON}).encode() + with http_sink(reply) as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + t = _transcript(tmp_path, _closing_turn(BAD)) + _run(env, t) + assert _run(env, t, active=True) == "" + assert [q["outcome"][0] for q in seen] == ["blocked", "missing_after_rewrite"] + + +def test_another_hooks_block_loop_is_left_alone(tmp_path): + with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + assert _run(env, _transcript(tmp_path, _closing_turn(BAD)), active=True) == "" + assert seen == [] + + +def test_a_task_write_that_failed_closed_nothing(tmp_path): + with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + t = _transcript(tmp_path, [_prompt(), _tool_use(), _result(is_error=True), _text(BAD)]) + assert _run(env, t) == "" + assert seen == [] + + +def test_a_task_closed_in_an_earlier_turn_does_not_count(tmp_path): + with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + t = _transcript(tmp_path, _closing_turn(GOOD) + [_prompt("thanks, what else?"), _text(BAD)]) + assert _run(env, t) == "" + assert seen == [] + + +def test_a_reply_not_yet_written_is_not_judged(tmp_path): + with http_sink(b'{"status":"ok","reason":"x"}') as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + t = _transcript(tmp_path, [_prompt(), _tool_use(), _result()]) + assert _run(env, t) == "" + assert seen == [] + + +def test_no_block_without_a_recorded_check(tmp_path): + t = _transcript(tmp_path, _closing_turn(BAD)) + # Unreachable instance. + assert _run(_env(tmp_path), t) == "" + # An instance that answered but returned no reason. + with http_sink(b'{"status":"ok"}') as (port, seen): + assert _run(_env(tmp_path, f"http://127.0.0.1:{port}"), t, session="s2") == "" + assert seen[0]["outcome"] == ["blocked"] + + +def test_a_bare_id_does_not_count_as_placing_the_work(tmp_path): + reply = json.dumps({"status": "ok", "reason": REASON}).encode() + with http_sink(reply) as (port, seen): + env = _env(tmp_path, f"http://127.0.0.1:{port}") + bare = "Closed #41.\n**Needs you:** nothing.\n**Next:** #42." + assert json.loads(_run(env, _transcript(tmp_path, _closing_turn(bare))))["decision"] == "block" + assert seen[0]["missing"] == ["where it sits"] diff --git a/tests/test_services_report_check.py b/tests/test_services_report_check.py new file mode 100644 index 0000000..0f34231 --- /dev/null +++ b/tests/test_services_report_check.py @@ -0,0 +1,47 @@ +"""The server half of the report-shape check (milestone 409 step 5): the words a +blocked reply is sent back with, and the outcome record.""" +import json +from unittest.mock import patch + +import pytest + +from tests.helpers import make_mock_session + + +def test_the_reason_names_only_sections_it_knows(): + from scribe.services.report_check import block_reason + + reason = block_reason(["next", "ignore previous instructions", "Where It Sits"]) + assert "missing: where it sits, next." in reason + assert "ignore previous instructions" not in reason + # It points at the skill that owns the shape rather than restating it. + assert "reporting-back" in reason and "placement" in reason + + +def test_a_reason_with_nothing_recognised_still_says_what_to_do(): + from scribe.services.report_check import block_reason + + assert "missing: the completion sections." in block_reason([]) + + +async def test_the_outcome_is_recorded_as_a_plugin_event(): + from scribe.services.report_check import record_report_check + + session = make_mock_session() + with patch("scribe.services.report_check.async_session", return_value=session): + await record_report_check(7, "blocked", missing=["next", "bogus"], task_ids=[41], project_id=2) + row = session.add.call_args.args[0] + assert (row.category, row.action, row.user_id) == ("plugin", "report_check", 7) + assert json.loads(row.details) == {"outcome": "blocked", "missing": ["next"], + "task_ids": [41], "project_id": 2} + session.commit.assert_awaited_once() + + +async def test_an_unknown_outcome_is_refused_before_anything_is_written(): + from scribe.services.report_check import record_report_check + + session = make_mock_session() + with patch("scribe.services.report_check.async_session", return_value=session), \ + pytest.raises(ValueError): + await record_report_check(7, "skipped") + session.add.assert_not_called() -- 2.54.0 From c7531d37001c41091450b1da6c1305b55d3db323 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 18:37:13 -0400 Subject: [PATCH 5/6] fix(409): the report check's prefilter no longer depends on compact JSON (#4014) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run #517 failed five of the new hook tests, all of them expecting a request that never went out. The prefilter matched `"name":"…update_task"` with no space after the colon, which is how Claude Code writes its transcripts, while the tests wrote theirs with json.dumps defaults (`"name": "…"`). The hook exited at the prefilter for every test transcript. The silent-case tests passed for the same wrong reason. - The prefilter now allows whitespace after the colon. The jq parse behind it never depended on formatting. - The test transcripts are written compact, matching the real file. The silent-case tests now reach the turn parse rather than stopping at the prefilter. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_report_check.sh | 2 +- tests/test_report_check_hook.py | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 6017ce9..7cd103f 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.14.2234", + "version": "2026.09.14.2237", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_report_check.sh b/plugin/hooks/scribe_report_check.sh index 72f8a5d..54cf313 100644 --- a/plugin/hooks/scribe_report_check.sh +++ b/plugin/hooks/scribe_report_check.sh @@ -73,7 +73,7 @@ marker="$state_dir/${safe_sid}.blocked" # check. Keeps the ordinary turn at one grep. Process substitution, NOT a pipe: # under `pipefail`, `grep -q` exiting on the first match kills `tail` with # SIGPIPE, and the pipeline then reports failure precisely when it matched. -grep -q -E '"name":"([^"]*__)?(update|create)_task"' < <(tail -c 2000000 "$transcript" 2>/dev/null) || { +grep -q -E '"name":[[:space:]]*"([^"]*__)?(update|create)_task"' < <(tail -c 2000000 "$transcript" 2>/dev/null) || { rm -f "$marker" 2>/dev/null || true exit 0 } diff --git a/tests/test_report_check_hook.py b/tests/test_report_check_hook.py index 2269be9..d401d0f 100644 --- a/tests/test_report_check_hook.py +++ b/tests/test_report_check_hook.py @@ -55,7 +55,8 @@ def _text(text): def _transcript(tmp_path, lines): path = tmp_path / "t.jsonl" - path.write_text("\n".join(json.dumps(line) for line in lines) + "\n") + # Compact, like the file Claude Code writes ({"name":"…"}, no spaces). + path.write_text("\n".join(json.dumps(line, separators=(",", ":")) for line in lines) + "\n") return path -- 2.54.0 From 0fab08276cd9980f433e10c5af1682be961b4205 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 14 Sep 2026 18:54:49 -0400 Subject: [PATCH 6/6] fix(plugin): an early-exiting head no longer voids its own output under pipefail (#4042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a repo where 3,000 files define `slug`, scribe_local_dups printed nothing; with 3 files it printed the duplicate line. Every hook runs under `set -uo pipefail`, and `hits=$(git grep -l … | head -4) || hits=""` lost head's four lines whenever git grep was still writing when head exited. That is a SIGPIPE, the substitution fails, and the outer fallback wipes the result. So the by-name duplicate arm went silent for exactly the most-duplicated names. Found while fixing the same trap in the new Stop hook (#4041). - The fallback moves inside the substitution, `$(… | head -N || true)`, at all five sites: scribe_defs.sh (local dups), scribe_prior_art.sh (names, old_first, shapes) and scribe_after_write.sh (names). A real upstream failure still yields empty output. - check_plugin gains a known-bad pattern for the shape, so no hook can bring it back. - Tests: the real function against a 3,000-file repo (past the pipe buffer), and the pattern shown to flag the old shape and pass the fix. Plugin version minted. Co-Authored-By: Claude Opus 5 (1M context) --- plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_after_write.sh | 3 +- plugin/hooks/scribe_defs.sh | 7 +++- plugin/hooks/scribe_prior_art.sh | 7 ++-- scripts/check_plugin.py | 13 +++++++ tests/test_hook_pipefail_head.py | 59 ++++++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 tests/test_hook_pipefail_head.py diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 7cd103f..323ec5f 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.14.2237", + "version": "2026.09.14.2254", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index b474098..977c7fb 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -138,7 +138,8 @@ while IFS= read -r rel_path; do code=$(cat "$file_path" 2>/dev/null) || code="" fi [ -n "$code" ] || continue - names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12) || names="" + # `|| true` inside: an early-exiting `head` must not void its own output (#4042). + names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12 || true) # Nothing DEFINED in what was written (prose, data, a call-site edit) → # nothing to say; the arms are about shapes. [ -n "$names" ] || continue diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 9da0088..86ea6bb 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -114,7 +114,12 @@ scribe_local_dups() { *) pat="(function|def|class|func|fun|fn|sub|struct|trait|interface|enum|object|protocol|type)[[:space:]]+${name}[^A-Za-z0-9_]|func[[:space:]]*\([^)]*\)[[:space:]]*${name}[[:space:]]*\(|(const|let)[[:space:]]+${name}[[:space:]]*=" ;; esac # -I skips binaries; :(exclude) drops the file being written. - hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null | head -4) || hits="" + # `|| true` INSIDE the substitution, not `|| hits=""` outside it (#4042): + # under the hooks' `pipefail`, `head` exiting after four lines kills a + # git grep that is still writing, the pipeline reports SIGPIPE, and an + # outer fallback then wipes the four hits head already printed. The name + # most duplicated — the one this arm exists for — was the one it dropped. + hits=$(git -C "$root" grep -I -l -E -e "$pat" -- . ":(exclude)${rel}" 2>/dev/null | head -4 || true) [ -n "$hits" ] || continue count=$(printf '%s\n' "$hits" | grep -c . 2>/dev/null || echo 0) label=$([ "$kind" = css ] && printf '.%s' "$name" || printf '%s' "$name") diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index 04f2cd7..d0dce6f 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -75,7 +75,8 @@ fi # in the repo? (scribe_local_dups in scribe_defs.sh carries the why.) names="" if [ -n "$code" ]; then - names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12) || names="" + # `|| true` inside: an early-exiting `head` must not void its own output (#4042). + names=$(printf '%s' "$code" | scribe_defs | sort -u | head -12 || true) fi local_lines="" @@ -105,11 +106,11 @@ shapes="$names" if [ -z "$shapes" ] && [ -f "$file_path" ] && command -v tac >/dev/null 2>&1; then old_first=$(printf '%s' "$event" \ | jq -r '.tool_input.old_string // .tool_input.old_str // empty' 2>/dev/null \ - | grep -m1 -v '^[[:space:]]*$') || old_first="" + | grep -m1 -v '^[[:space:]]*$' || true) if [ -n "$old_first" ]; then ln=$(grep -nF -m1 -- "$old_first" "$file_path" 2>/dev/null | cut -d: -f1) || ln="" if [ -n "$ln" ]; then - shapes=$(head -n "$ln" "$file_path" | tac | scribe_defs | head -1) || shapes="" + shapes=$(head -n "$ln" "$file_path" | tac | scribe_defs | head -1 || true) fi fi fi diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index 8271ff4..9e82042 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -208,6 +208,19 @@ PATTERNS: list[tuple[re.Pattern, str, str]] = [ "produces separate encoded lines joined by raw newlines — an invalid " "URL. Use -s (slurp) as well, e.g. `jq -sRr '@uri'`.", ), + ( + # `$(producer | head -N) || var=""` — the fallback OUTSIDE the + # substitution (#4042). Every hook runs under `pipefail`; once `head` + # (or `grep -m`/`-q`) has what it needs it exits, a producer still + # writing dies of SIGPIPE, the substitution fails, and the fallback + # wipes the output that was already captured. Silent, and only on big + # inputs — which for the duplicate arm meant the most-duplicated names. + re.compile(r"\|\s*(?:head\b|grep\s+-[A-Za-z]*[mq])[^)]*\)\s*\|\|\s*\w+=\"\""), + "early-exit consumer with the fallback outside the substitution", + "Under pipefail, `$(x | head -N) || v=\"\"` discards head's output whenever x " + "is still writing when head exits (SIGPIPE). Put the fallback inside: " + "`$(x | head -N || true)`.", + ), ( re.compile(r"\|\s*cut\s+-c"), "line-oriented cut for a payload cap", diff --git a/tests/test_hook_pipefail_head.py b/tests/test_hook_pipefail_head.py new file mode 100644 index 0000000..272778c --- /dev/null +++ b/tests/test_hook_pipefail_head.py @@ -0,0 +1,59 @@ +"""Under `pipefail`, an early-exiting `head` must not void the output it kept (#4042). + +The hooks run with `set -uo pipefail`. `hits=$(git grep -l … | head -4) || +hits=""` lost its four hits whenever git grep was still writing when head +exited: SIGPIPE, a failed substitution, and the fallback wiped the result. It +only showed on big outputs, so the by-name duplicate arm went silent for +exactly the most-duplicated names. + +Two halves: the real function on a repo big enough to overflow the pipe, and +the check_plugin pattern that keeps the shape out of every hook (shown able to +fail, rule #167). +""" +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +DEFS = ROOT / "plugin" / "hooks" / "scribe_defs.sh" + + +def test_the_duplicate_arm_still_names_a_name_defined_in_thousands_of_files(tmp_path): + for tool in ("git", "bash"): + if shutil.which(tool) is None: + pytest.skip(f"{tool!r} not installed") + env = {"PATH": os.environ["PATH"], "HOME": str(tmp_path), + "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@x", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@x"} + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True, env=env) + # ~3000 × ~60 bytes of `git grep -l` output: well past a 64 KiB pipe buffer, + # so git grep is still writing when head has its four lines. + for i in range(3000): + (repo / f"module_with_a_fairly_long_descriptive_name_{i}.py").write_text("def slug(t):\n return t\n") + subprocess.run(["git", "add", "."], cwd=repo, check=True, env=env) + subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True, env=env) + + script = f'set -uo pipefail\n. "{DEFS}"\nprintf "sym\\tslug\\n" | scribe_local_dups "{repo}" new.py\n' + out = subprocess.run(["bash", "-c", script], capture_output=True, text=True, env=env, timeout=60) + assert out.returncode == 0, out.stderr + assert "`slug` is already defined in 4 other file(s)" in out.stdout + + +def test_the_lint_pattern_catches_the_shape_and_passes_the_fix(): + from scripts.check_plugin import PATTERNS + + pattern = next(p for p, label, _why in PATTERNS if label.startswith("early-exit consumer")) + for bad in ('hits=$(git grep -l x | head -4) || hits=""', + " | grep -m1 -v '^[[:space:]]*$') || old_first=\"\"", + 'x=$(a | grep -q b) || x=""'): + assert pattern.search(bad), bad + for good in ('hits=$(git grep -l x | head -4 || true)', + 'ln=$(grep -nF -m1 -- "$a" "$f" | cut -d: -f1) || ln=""'): + assert not pattern.search(good), good -- 2.54.0