diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index e67ef7b..2b8533f 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.15.1921", + "version": "2026.09.16.1232", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/hooks.json b/plugin/hooks/hooks.json index c55be67..ff1d27b 100644 --- a/plugin/hooks/hooks.json +++ b/plugin/hooks/hooks.json @@ -55,6 +55,16 @@ ] } ], + "PreCompact": [ + { + "hooks": [ + { + "type": "command", + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/scribe_precompact_preserve.sh\"" + } + ] + } + ], "Stop": [ { "hooks": [ diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index 977c7fb..1510983 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -105,12 +105,9 @@ done <<< "$current" [ -n "$changed" ] || exit 0 scribe_config || : # sets url/token; the call below is guarded on them -repo=$(git -C "$repo_root" remote get-url origin 2>/dev/null || true) +scope=$(scribe_scope_query "$repo_root") repo_q="" -if [ -n "$repo" ]; then - enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" - [ -n "$enc" ] && repo_q="&repo=${enc}" -fi +[ -n "$scope" ] && repo_q="&${scope}" # The dedup channels are the PRE-write hook's files, on purpose (see header). state_dir="${TMPDIR:-/tmp}/scribe-priorart" diff --git a/plugin/hooks/scribe_autoinject.sh b/plugin/hooks/scribe_autoinject.sh index c32d324..956d8a2 100755 --- a/plugin/hooks/scribe_autoinject.sh +++ b/plugin/hooks/scribe_autoinject.sh @@ -65,14 +65,11 @@ q=$(printf '%s' "$prompt" | head -c 2000) # worth retrieving against were exactly the ones silently dropped. -s slurps. q_enc=$(printf '%s' "$q" | jq -sRr '@uri' 2>/dev/null) || exit 0 -# Resolve the working repo's remote so the server can scope to the bound project. +# Scope to this directory's project — a `.scribe` marker, else the git remote. repo_dir=${event_cwd:-${CLAUDE_PROJECT_DIR:-$PWD}} -repo=$(git -C "$repo_dir" remote get-url origin 2>/dev/null || true) +scope=$(scribe_scope_query "$repo_dir") repo_q="" -if [ -n "$repo" ]; then - enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" - [ -n "$enc" ] && repo_q="&repo=${enc}" -fi +[ -n "$scope" ] && repo_q="&${scope}" # Per-session dedup: ids already injected this session are skipped. state_dir="${TMPDIR:-/tmp}/scribe-autoinject" diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 86ea6bb..4d75438 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -19,6 +19,11 @@ # scribe_rules_live FILE live rule ids from the exclusion ledger, # comma-joined; entries age out (#3751) # scribe_rules_append FILE stdin ids -> the ledger, timestamped +# scribe_scope_query DIR `project_id=N` or `repo=` for DIR — the +# project-scope key EVERY hook sends (#4085). +# Helpers: scribe_marker_file, scribe_url_host, +# scribe_marker_read (idwhy-not), +# scribe_marker_project (the id alone) # # Sourced, not executed: `. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh"`. @@ -283,3 +288,124 @@ scribe_rules_append() { now=$(date +%s 2>/dev/null) || now=0 awk -v ts="$now" 'NF { print $1 "\t" ts }' >> "$f" 2>/dev/null || true } + +# --------------------------------------------------------------------------- +# WHICH PROJECT IS THIS DIRECTORY'S? (#4085) +# +# Every hook here scopes its request to a project, and until this existed all +# six did it the same single way: `git remote get-url origin`, resolved +# server-side through the repo bindings. That works well inside a bound repo +# and not at all outside one — a session in a plain directory got no project +# scope from ANY hook, silently, because the one key the whole chain turns on +# only exists in a git repo. +# +# The second key is a `.scribe` file in the directory (or above it, the way +# git finds its root), naming the project the work belongs to. It is a +# POINTER, not a copy: an id and enough to check the id means what it says. +# Nothing Scribe should be holding goes in it. +# +# {"instance": "https://scribe.example.com", "project_id": 2, +# "project": "FabledScribe"} +# +# instance WHICH Scribe the id belongs to, and the reason this file is +# not just a number. A project id means nothing on its own: id 2 +# is a different project on every instance, so a marker that +# travels — a copied directory, a shared machine, a repo someone +# else clones — would silently scope a session to the wrong +# project. Compared HOST-ONLY against the configured endpoint, so +# http/https and a trailing slash don't cause a false mismatch. +# A mismatch drops the id: no project beats the wrong project. +# project_id the pointer itself. Required. +# project a human label. NOTHING READS IT. It is there so the file +# answers "what is this?" when opened, and so a stale one is +# visible rather than inert. +# +# A bare integer is also accepted (`echo 2 > .scribe`), because it is what a +# person writes by hand and it parses as JSON already. It skips the instance +# check by having nothing to check — deliberate, and the reason the written +# form carries `instance`. +# +# THE MARKER WINS over a git remote. Someone put the file there on purpose; +# a remote is just where the code happens to be pushed. That also makes the +# marker the way to override a binding for one directory. + +# Nearest `.scribe` at or above DIR. Walks up to the filesystem root, capped so +# a pathological path cannot spin. +scribe_marker_file() { + local dir="$1" depth=0 + [ -n "$dir" ] || return 0 + while [ "$depth" -lt 40 ]; do + [ -f "$dir/.scribe" ] && { printf '%s' "$dir/.scribe"; return 0; } + case "$dir" in ""|"/") return 0 ;; esac + dir=$(dirname -- "$dir" 2>/dev/null) || return 0 + depth=$((depth + 1)) + done + return 0 +} + +# Host of a URL, lowercased, port kept. "" for empty input. +scribe_url_host() { + printf '%s' "${1:-}" \ + | sed -e 's#^[A-Za-z][A-Za-z0-9+.-]*://##' -e 's#^[^/@]*@##' -e 's#[/?].*$##' \ + | tr 'A-Z' 'a-z' +} + +# Read a marker file: prints "IDREASON", at most one of them non-empty. +# +# "7\t" use project 7 +# "\tnames no …" a file is there and deliberately NOT used; say why +# "\t" no marker file at all — the ordinary case, say nothing +# +# One line rather than an id plus a global, because every caller reads this +# through `$( )` and a global set inside a command substitution dies with the +# subshell. The caller would then read an unset variable, which under the +# `set -u` these hooks all run with aborts the hook and costs the whole +# session's context — a failure far larger than the message it was fetching. +scribe_marker_read() { + local f="$1" id inst want + [ -n "$f" ] && [ -f "$f" ] || { printf '\t'; return 0; } + command -v jq >/dev/null 2>&1 || { printf '\t'; return 0; } + # A bare integer is valid JSON, so one filter reads both forms. + id=$(jq -r 'if type=="number" then (.|floor|tostring) + elif type=="object" then (.project_id // empty | tostring) + else empty end' "$f" 2>/dev/null) || id="" + case "$id" in ''|*[!0-9]*) id="" ;; esac + if [ -z "$id" ] || [ "$id" = "0" ]; then + printf '\tnames no project_id' + return 0 + fi + inst=$(jq -r 'if type=="object" then (.instance // empty) else empty end' "$f" 2>/dev/null) || inst="" + if [ -n "$inst" ]; then + want=$(scribe_url_host "${url:-}") + inst=$(scribe_url_host "$inst") + if [ -n "$want" ] && [ "$inst" != "$want" ]; then + printf '\tpoints at %s, but this session is configured for %s' "$inst" "$want" + return 0 + fi + fi + printf '%s\t' "$id" +} + +# Just the id a marker names, or "" — the half scribe_scope_query needs. +scribe_marker_project() { + scribe_marker_read "$1" | cut -f1 +} + +# The query args identifying DIR's project — `project_id=N` or `repo=` — +# with NO leading `?` or `&`, so each caller keeps its own separator. Empty +# when neither key is available. Requires `url` to be set (scribe_config) for +# the marker's instance check; without it a marker is still honoured, since an +# unconfigured hook is not going to send the request anyway. +scribe_scope_query() { + local dir="$1" id repo enc + id=$(scribe_marker_project "$(scribe_marker_file "$dir")") + if [ -n "$id" ]; then + printf 'project_id=%s' "$id" + return 0 + fi + repo=$(git -C "$dir" remote get-url origin 2>/dev/null || true) + [ -n "$repo" ] || return 0 + command -v jq >/dev/null 2>&1 || return 0 + enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" + [ -n "$enc" ] && printf 'repo=%s' "$enc" +} diff --git a/plugin/hooks/scribe_precompact_preserve.sh b/plugin/hooks/scribe_precompact_preserve.sh new file mode 100644 index 0000000..bb5536d --- /dev/null +++ b/plugin/hooks/scribe_precompact_preserve.sh @@ -0,0 +1,74 @@ +#!/usr/bin/env bash +# Scribe plugin — PreCompact: tell the summarizer what must survive (#3680). +# +# WHAT THIS HOOK ACTUALLY DOES, and why it is not what the design first assumed. +# +# Spike #3680 asked whether a PreCompact hook can reach the model, and note +# #3679 assumed the channel would be `hookSpecificOutput.additionalContext` +# plus a block. Both halves of that are wrong, read out of the installed build +# (Claude Code 2.1.273): +# +# * `additionalContext` is NEVER read on PreCompact. The hook-output schema +# has no PreCompact variant at all; the field is honoured for SessionStart, +# SubagentStart, Stop and friends, and silently dropped here. +# +# * A PreCompact hook's STDOUT becomes the compaction's custom instructions. +# The handler collects every hook that exited 0 with non-empty stdout and +# returns it as `newCustomInstructions`, which is merged with whatever the +# operator typed after `/compact` and passed into the prompt that writes +# the summary. Every path does this — manual, auto and partial compaction. +# +# So there IS a model-reaching channel, and it is better than the one designed: +# we do not interrupt the compaction, we steer the summary it produces. The +# summary is what the next turn reads, so text that survives into it survives +# the compaction. +# +# WHAT NOT TO DO HERE: never block. A PreCompact block (exit 2, or +# `{"decision":"block"}`) does not pause for the model and cannot tell it +# anything — the compaction is SKIPPED, a warning goes to the operator's +# screen, and the session continues uncompacted toward its context limit with +# no summary at all. That failure is silent from the model's side, which is +# exactly the outcome #3680 existed to avoid shipping. This hook exits 0 on +# every path. +# +# SCOPE: in a subagent the handler discards hook stdout and keeps only a block, +# so this steers the main session's compaction and nothing else. That is the +# one we care about — a subagent's summary does not outlive it. +# +# NO NETWORK, NO CONFIG. What must be preserved is already in the conversation +# being summarized; this hook's job is to say which parts of it are load-bearing +# so the summarizer keeps them literally instead of compressing them away. +# Naming the in-flight task ids from the instance would need a server round-trip +# inside the compaction path, which is a separate, measurable question. +# +# PROOF THAT IT FIRED: on a manual `/compact` the handler shows the operator +# `PreCompact [] completed successfully: `, so the hook's own +# text is the receipt. (Auto-compaction suppresses that notification.) +set -uo pipefail + +# Drain the event so the caller never sees a broken pipe. Nothing in it changes +# what we emit: the instruction is the same whether the operator typed +# `/compact` or the session hit its limit, and it composes with any custom +# instructions they gave, which are merged ahead of ours. +cat >/dev/null 2>&1 || true + +cat <<'EOF' +Preserve the following literally in the summary — copied through, not +paraphrased or counted: + +- Every Scribe record the conversation refers to, by id AND title: tasks, + issues, milestones, notes, rules, systems. A bare "#4061" is not enough; a + record whose name is lost has to be looked up again before it can be used. +- Which task is in progress and what its status was last set to, plus the + milestone it sits under. +- Work that was done but NOT yet recorded in Scribe — an edit with no work-log, + a fix not filed as an issue, a decision not written down. Carry these over as + outstanding; they exist nowhere else once this conversation is summarized. +- Any rule or preference that was retrieved and still governs the work, by id + and title. +- Anything the operator asked for that has not been done yet, in their words. + +Everything else here can be recovered from the repository or from Scribe. These +cannot: they are this session's only copy. +EOF +exit 0 diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index d0dce6f..db2081f 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -150,13 +150,10 @@ q=$(printf '%s' "$code" | head -c 1200) path_enc=$(printf '%s' "$rel_path" | jq -sRr '@uri' 2>/dev/null) || exit 0 code_enc=$(printf '%s' "$q" | jq -sRr '@uri' 2>/dev/null) || code_enc="" -# Resolve the working repo's remote so the server can scope to the bound project. -repo=$(git -C "$lookup_dir" remote get-url origin 2>/dev/null || true) +# Scope to this directory's project — a `.scribe` marker, else the git remote. +scope=$(scribe_scope_query "$lookup_dir") repo_q="" -if [ -n "$repo" ]; then - enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" - [ -n "$enc" ] && repo_q="&repo=${enc}" -fi +[ -n "$scope" ] && repo_q="&${scope}" # Per-session dedup, in its own file rather than sharing auto-inject's. Each # surface shows a given snippet at most once per session, but they don't silence diff --git a/plugin/hooks/scribe_report_check.sh b/plugin/hooks/scribe_report_check.sh index 54cf313..1b79045 100644 --- a/plugin/hooks/scribe_report_check.sh +++ b/plugin/hooks/scribe_report_check.sh @@ -149,11 +149,8 @@ report() { 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 + scope=$(scribe_scope_query "${event_cwd:-${CLAUDE_PROJECT_DIR:-$PWD}}") + [ -n "$scope" ] && q="${q}&${scope}" curl -fsS --max-time 4 \ -H "Authorization: Bearer ${token}" \ "${url%/}/api/plugin/report-check?${q}" 2>/dev/null diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index b486800..85aa216 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -24,9 +24,15 @@ # fires after a compaction (SessionStart input `source` == "compact"), when # earlier turns have just been summarized and in-flight state is most at risk. # On that source we lead with a banner telling the model to reload project + -# in-flight tasks from Scribe. (PreCompact is the wrong tool here — a host hook -# can't make the model flush, and can't know the in-flight task ids; the durable -# path is record-as-you-go + this post-compaction reload.) +# in-flight tasks from Scribe. This stays the durable path: record-as-you-go +# plus a post-compaction reload from the instance. +# +# The other half arrived with #3680. A host hook still cannot make the model +# flush before a compaction — but scribe_precompact_preserve.sh steers the +# SUMMARY, because a PreCompact hook's stdout becomes the compaction's custom +# instructions. The two are complementary, and neither replaces the other: that +# hook decides what survives into the summary, this one reloads from the record +# once the summary lands. # # IMPORTANT: do NOT pass config via `${user_config.*}` substitution in a # shell-form hooks.json command — Claude Code rejects that outright (splicing a @@ -145,14 +151,19 @@ scribe_config || : dyn="" status="" if [ -n "$url" ] && [ -n "$token" ] && command -v curl >/dev/null 2>&1; then - # Resolve the working repo's remote so the server can map it to a project. + # Which project is this directory's? A `.scribe` marker first, then the git + # remote (#4085). scribe_scope_query answers that, and is shared with the + # other five hooks so a directory scopes the same way everywhere; the pieces + # are re-read here only to explain what happened when nothing resolved. repo_dir=${CLAUDE_PROJECT_DIR:-$PWD} + marker=$(scribe_marker_file "$repo_dir") + marker_read=$(scribe_marker_read "$marker") + marker_id=${marker_read%%$'\t'*} + marker_why=${marker_read#*$'\t'} repo=$(git -C "$repo_dir" remote get-url origin 2>/dev/null || true) + scope=$(scribe_scope_query "$repo_dir") q="" - if [ -n "$repo" ]; then - enc=$(printf '%s' "$repo" | jq -sRr '@uri' 2>/dev/null) || enc="" - [ -n "$enc" ] && q="?repo=${enc}" - fi + [ -n "$scope" ] && q="?${scope}" body=$(curl -fsS --max-time 8 \ -H "Authorization: Bearer ${token}" \ "${url%/}/api/plugin/context${q}" 2>/dev/null) || body="" @@ -178,6 +189,29 @@ fi [ -n "$dyn" ] && append "$dyn" [ -n "$status" ] && append "$status" +# --- Nothing resolved: say WHICH nothing, and what would fix it (#4085) --- +# +# The server can say "no project is bound to this working directory", and +# until now that was the whole answer. It is the same sentence for a directory +# that is not a repo, a repo whose remote nobody bound, and a marker file +# naming a project this account cannot read — three different problems with +# three different fixes, and no way to tell them apart from inside the session. +# +# The marker is the adapter's convention, so the adapter explains it: the +# server reports whether a project resolved, and this hook — which knows what +# it sent and why — turns that into the sentence the operator can act on. The +# repo case is left to the server's existing "bind this repo" hint. +if [ -n "$dyn" ] && [ -z "$(printf '%s' "$body" | jq -r '.project.id // empty' 2>/dev/null)" ]; then + host=$(scribe_url_host "$url") + if [ -n "$marker_why" ]; then + append "> ⚠️ Scribe: the marker file \`${marker}\` ${marker_why}, so no project context was loaded. Fix the file, or ignore it and bind this directory another way." + elif [ -n "$marker_id" ]; then + append "> ⚠️ Scribe: \`${marker}\` names project ${marker_id}, which this account cannot read on ${host} — it may belong to a different Scribe instance, or the project may have been deleted. Check with \`list_projects()\` and correct the file." + elif [ -z "$repo" ]; then + append "> ℹ️ Scribe: this directory is not a git repository and has no \`.scribe\` marker, so no project context was loaded — every hook this session is unscoped. If this work belongs to a Scribe project, call \`list_projects()\` and write the marker: \`{\"instance\": \"${url%/}\", \"project_id\": , \"project\": \"\"}\` in \`${repo_dir}/.scribe\`. Future sessions here load that project on their own." + fi +fi + # Compaction re-grounding: lead with a reload banner when this fire is a compact. if [ "$source" = "compact" ]; then prepend "> ⟳ This session was just COMPACTED — earlier turns are now a summary, so in-flight detail may be lost. Any rules that had been retrieved went into that summary with everything else, so treat yourself as holding none: before the next consequential act, ask again with \`search(content_type=\"rule\")\` rather than trusting a half-remembered one. Re-run \`enter_project()\` for the active project, check its recent milestones and open tasks, and reconcile what you are mid-way through against what Scribe records. Scribe is the record." diff --git a/plugin/hooks/scribe_tool_rules.sh b/plugin/hooks/scribe_tool_rules.sh index 16a1bd4..3262460 100644 --- a/plugin/hooks/scribe_tool_rules.sh +++ b/plugin/hooks/scribe_tool_rules.sh @@ -63,11 +63,8 @@ tool_enc=$(printf '%s' "$tool_name" | jq -sRr '@uri' 2>/dev/null) || exit 0 repo_q="" lookup_dir=${event_cwd:-${CLAUDE_PROJECT_DIR:-$PWD}} -repo_remote=$(git -C "$lookup_dir" remote get-url origin 2>/dev/null || true) -if [ -n "$repo_remote" ]; then - repo_enc=$(printf '%s' "$repo_remote" | jq -sRr '@uri' 2>/dev/null) || repo_enc="" - [ -n "$repo_enc" ] && repo_q="&repo=${repo_enc}" -fi +scope=$(scribe_scope_query "$lookup_dir") +[ -n "$scope" ] && repo_q="&${scope}" # THE SHARED SESSION LEDGER, and the thing most worth getting right here. # diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 3ada364..972ee1a 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -13,10 +13,23 @@ asked for. ## Do this first (every session) -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's goal, the milestones and open tasks worked on most recently, its -Systems and the titles of its own rules in one shot. +If the working directory maps to a Scribe project, call `enter_project(id)` — +it returns the project's goal, the milestones and open tasks worked on most +recently, its Systems and the titles of its own rules in one shot. + +**A directory does not have to be a git repo to have a project.** A repo is +bound by its remote (`list_repo_bindings` shows the bindings). Anything else — +a notes folder, a server's config directory, a scratch directory — is bound by +a `.scribe` file naming the project: + + {"instance": "https://scribe.example.com", "project_id": 2, "project": "Homelab"} + +`instance` is what makes the id trustworthy. An id means nothing on its own — +it is a different project on every Scribe — so a marker that has travelled to +another instance is ignored rather than followed to the wrong project. A bare +`2` also works when writing the file by hand. When work plainly belongs to a +project and the directory names none, offer to write the marker; +`list_projects` has the id. 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, diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 4fd6943..5476d33 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -44,7 +44,10 @@ async def get_milestone(milestone_id: int) -> dict: rules surface again on recall. Returns: milestone (incl. body), progress, steps (its tasks ordered by - status then update), and applicable_rules / project_rules. + status then update), and applicable_rules / project_rules. Each step says + what it is and where it stands, not its whole body — read a step in full + with get_task(id). A plan with forty long steps is otherwise too big to + arrive inline, and the design is in the milestone body. """ uid = current_user_id() milestone = await milestones_svc.get_milestone(uid, milestone_id) @@ -61,7 +64,7 @@ async def get_milestone(milestone_id: int) -> dict: out.update(progress) return { "milestone": out, - "steps": [t.to_dict() for t in steps], + "steps": [notes_svc.brief_row(t, {milestone.id: milestone.title}) for t in steps], **rulebooks_svc.rules_payload(applicable, user_id=uid, source="get_milestone"), } diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index 51b3cd4..d13b490 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -44,6 +44,9 @@ async def list_notes( Prefer `search` for a pure lookup — it returns scores and reaches everything readable; this adds the lifecycle filters on top. + Each row says what the note is — id, title, description, type, project, + tags, updated_at — and not what it says: read one in full with get_note(id). + Args: project_id: Scope to one project. PASS THE ACTIVE PROJECT'S ID whenever a project is in scope so you list that project's notes, not every @@ -60,7 +63,7 @@ async def list_notes( limit=max(1, min(limit, 100)), offset=max(0, offset), ) - return {"notes": [n.to_dict() for n in rows], "total": total} + return {"notes": [notes_svc.brief_row(n) for n in rows], "total": total} diff --git a/src/scribe/mcp/tools/systems.py b/src/scribe/mcp/tools/systems.py index df0050e..bd8784b 100644 --- a/src/scribe/mcp/tools/systems.py +++ b/src/scribe/mcp/tools/systems.py @@ -14,6 +14,7 @@ from __future__ import annotations from scribe.mcp._context import current_user_id from scribe.services import canonical_systems as canonical_systems_svc +from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc from scribe.services import systems as systems_svc @@ -267,16 +268,18 @@ async def get_system(system_id: int) -> dict: """Fetch a System plus the records associated with it. Returns the system, plus its associated records split into `issues`, - `tasks` (work/plan), and `notes`. + `tasks` (work/plan), and `notes` — each saying what the record is and where + it sits, not what it says (open one with get_note / get_task). """ uid = current_user_id() system = await systems_svc.get_system(uid, system_id) if system is None: raise ValueError(f"system {system_id} not found") records = await systems_svc.list_records_for_system(uid, system_id) + titles = await milestones_svc.titles_for({r.milestone_id for r in records}) issues, tasks, notes = [], [], [] for r in records: - d = r.to_dict() + d = notes_svc.brief_row(r, titles) if r.status is None: notes.append(d) elif r.task_kind == "issue": @@ -339,12 +342,16 @@ async def list_system_records( kind: filter by task_kind — 'issue', 'work', 'spike' (or the retired 'plan'). Omit for all. open_only: limit to tasks not done/cancelled (e.g. open issues only). + + Each record says what it is and where it sits, not what it says — open one + with get_note / get_task / get_snippet. """ uid = current_user_id() rows = await systems_svc.list_records_for_system( uid, system_id, kind=kind or None, open_only=open_only, ) - return {"records": [r.to_dict() for r in rows]} + titles = await milestones_svc.titles_for({r.milestone_id for r in rows}) + return {"records": [notes_svc.brief_row(r, titles) for r in rows]} async def delete_system(system_id: int) -> dict: diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index d3fcbb7..093dfce 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -22,6 +22,7 @@ from scribe.mcp._context import current_user_id from scribe.mcp.tools import systems as systems_tools from scribe.services import access as access_svc from scribe.services import dedup as dedup_svc +from scribe.services import milestones as milestones_svc from scribe.services import notes as notes_svc # Imported by NAME, not reached through notes_svc: minted_kind is pure # validation, not a service call, and a test that stubs the service module to @@ -58,7 +59,10 @@ async def list_tasks( kind: Filter by task kind — 'work', 'issue', 'spike' (or the retired 'plan'). Omit (empty) for all kinds. - Results are ordered by last-updated descending. + Results are ordered by last-updated descending. Each row says what the task + is and where it sits — id, title, status, kind, priority, milestone (id and + title), tags, updated_at, plus description, parent and due date when set — + and not what it says: read one in full with get_task(id). """ uid = current_user_id() rows, total = await notes_svc.list_notes( @@ -70,7 +74,8 @@ async def list_tasks( limit=max(1, min(limit, 100)), offset=max(0, offset), ) - return {"tasks": [n.to_dict() for n in rows], "total": total} + titles = await milestones_svc.titles_for({n.milestone_id for n in rows}) + return {"tasks": [notes_svc.brief_row(n, titles) for n in rows], "total": total} async def get_task(task_id: int) -> dict: diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index b812980..ee9e6e6 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -64,8 +64,11 @@ async def session_context(): resolves it to the bound project (see services/repo_bindings); an unbound repo yields a "bind this repo" hint instead. This is how the active project is determined — the plugin never pins a project id. - project_id (optional int) — explicit override, mainly for manual/ad-hoc - curl testing; takes precedence over `repo` when set. + project_id (optional int) — the project named directly, and the only + key a caller outside a git repo has (#4085): the plugin's hooks + send it when a `.scribe` marker file names a project. Takes + precedence over `repo`. Access-checked like any other read — an id + this account cannot read loads no project rather than failing. """ project_id, _repo, unbound_repo = await _project_scope() result = await plugin_ctx_svc.build_session_context( diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index 0e99a8e..cada46c 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -72,6 +72,26 @@ async def get_milestone(user_id: int, milestone_id: int) -> Milestone | None: return result.scalars().first() +async def titles_for(milestone_ids: set[int]) -> dict[int, str]: + """{id: title} for the given milestones, for rows that name where a record sits. + + No ownership filter, deliberately: the callers are listings whose rows the + caller could already read, and a milestone title is part of "where does + this record sit". Filtering here would blank the placement of a shared + task in someone else's plan while still showing the task. + """ + ids = {i for i in milestone_ids if i} + if not ids: + return {} + async with async_session() as session: + rows = (await session.execute( + select(Milestone.id, Milestone.title).where( + Milestone.id.in_(ids), Milestone.deleted_at.is_(None), + ) + )).all() + return {mid: title for mid, title in rows} + + async def get_milestone_in_project(project_id: int, milestone_id: int) -> Milestone | None: """Fetch a milestone by id within a project, without a user_id ownership check. Callers must verify project access separately before using this.""" diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index 31d1c8e..758e963 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -7,6 +7,7 @@ from sqlalchemy import func, or_, select, text from scribe.models import async_session from scribe.models.note import Note, TaskKind, TaskPriority, TaskStatus +from scribe.models.base import iso logger = logging.getLogger(__name__) @@ -1057,3 +1058,40 @@ async def get_note_for_user( async with async_session() as session: note = await session.get(Note, note_id) return (note, perm) if note else None + + +# What a LISTING row carries (#4061): what the record is and where it sits, +# never what it says. list_tasks once returned every row's to_dict(), body +# included, and a project's todo list came to 93-165k characters, past what an +# MCP client accepts inline, so the list arrived as a file to page through. The +# same failure #4045 fixed for enter_project, one call over. get_task / get_note +# read a record in full; a list is for choosing which one to open. +# +# A field most rows leave empty (a one-line description, a parent, a due date) +# is attached only when set: a hundred rows of `null` are a hundred chances to +# learn to skip the key (#2483), and the bytes are the thing being cut. +def brief_row(note: Note, milestone_titles: dict[int, str] | None = None) -> dict: + row = { + "id": note.id, + "title": note.title, + "note_type": note.note_type or "note", + "project_id": note.project_id, + "tags": note.tags or [], + "updated_at": iso(note.updated_at), + } + if note.description: + row["description"] = note.description + if note.is_task: + row.update({ + "status": note.status, + "task_kind": note.task_kind, + "priority": note.priority, + "milestone_id": note.milestone_id, + }) + if milestone_titles is not None and note.milestone_id: + row["milestone_title"] = milestone_titles.get(note.milestone_id) + if note.parent_id: + row["parent_id"] = note.parent_id + if note.due_date: + row["due_date"] = iso(note.due_date) + return row diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 63e82a6..acea2bb 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -2171,7 +2171,10 @@ async def build_session_context( Args: user_id: the operator. project_id: the resolved active project (0 = none). The endpoint - resolves this from the working repo's remote, not from config. + resolves this from the working repo's remote, or from a `.scribe` + marker file naming the project directly — the only key a session + outside a git repo has (#4085). A non-zero id that does not + resolve is reported rather than silently dropped. unbound_repo: when the hook sent a repo remote that maps to no project, its normalized key — triggers a one-line "bind this repo" hint so the binding is self-healing. @@ -2241,18 +2244,34 @@ async def build_session_context( f"`get_design_system({design['id']})` → " f"`resolved_guidance`.", ] - elif unbound_repo: - lines += [ - "", - "## Repository not yet bound", - f"This repo (`{unbound_repo}`) isn't mapped to a Scribe project, so " - "no project context was loaded. Bind it once with " - f'`bind_repo(repo_url="{unbound_repo}", project_id=<id>)` ' - "(call `list_projects` to find the id) and future sessions here will " - "auto-load that project's context.", - ] - else: - lines += ["", "No Scribe project is bound to this working directory."] + # Nothing loaded — say which nothing (#4085). This used to hang off the + # `if project_id:` above as an `elif`, which meant an id that was SENT and + # did not resolve produced no message at all: the outer branch was taken, + # the inner one was not, and the caller got a context that simply omitted + # the project it had asked for. That is the one case worth being loudest + # about, because the caller is holding a pointer it believes in. + if project_dict is None: + if project_id: + lines += [ + "", + f"## Project {project_id} could not be loaded", + f"This session asked for project {project_id}, but this account " + "cannot read it — the id may belong to a different Scribe " + "instance, or the project may have been deleted. " + "`list_projects` shows what is readable here.", + ] + elif unbound_repo: + lines += [ + "", + "## Repository not yet bound", + f"This repo (`{unbound_repo}`) isn't mapped to a Scribe project, so " + "no project context was loaded. Bind it once with " + f'`bind_repo(repo_url="{unbound_repo}", project_id=<id>)` ' + "(call `list_projects` to find the id) and future sessions here will " + "auto-load that project's context.", + ] + else: + lines += ["", "No Scribe project is bound to this working directory."] context = "\n".join(line for line in lines if line is not None) if len(context) > _MAX_CHARS: diff --git a/tests/test_list_rows_brief.py b/tests/test_list_rows_brief.py new file mode 100644 index 0000000..faab13e --- /dev/null +++ b/tests/test_list_rows_brief.py @@ -0,0 +1,93 @@ +"""List tools return rows that say what a record is, never what it says (#4061). + +list_tasks once returned every row's to_dict(), body included. A project's todo +list came to 93-165k characters, past what an MCP client accepts as a tool +result, so the list arrived as a file to page through — the failure #4045 fixed +for enter_project, one call over. A list is for choosing which record to open; +get_task / get_note read one in full. +""" +import json +from datetime import datetime, timezone +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.mcp.tools.notes import list_notes +from scribe.mcp.tools.systems import list_system_records +from scribe.mcp.tools.tasks import list_tasks +from scribe.services.notes import brief_row + +pytestmark = pytest.mark.usefixtures("_bind_user") + +STEP_PLAN = "A step plan paragraph long enough to matter. " * 110 # ~5k chars +WHEN = datetime(2026, 9, 15, tzinfo=timezone.utc) + + +def _row(nid: int, *, is_task: bool = True, milestone_id: int | None = 415) -> SimpleNamespace: + """A real-shaped Note: every attribute brief_row or to_dict could read.""" + return SimpleNamespace( + id=nid, title=f"Step {nid} — something worth doing", body=STEP_PLAN, + description=None, note_type="note", project_id=2, tags=["retrieval"], + updated_at=WHEN, created_at=WHEN, is_task=is_task, + status="todo" if is_task else None, task_kind="work", priority="high", + milestone_id=milestone_id if is_task else None, parent_id=None, due_date=None, + ) + + +def test_a_task_row_names_what_it_is_and_where_it_sits_but_not_its_body(): + row = brief_row(_row(7), {415: "An existing plan is found"}) + assert "body" not in row + assert row["milestone_title"] == "An existing plan is found" + assert {"id", "title", "status", "task_kind", "priority", "milestone_id", + "tags", "updated_at", "project_id"} <= set(row) + # Empty optional fields are left off rather than sent as null (#2483). + assert not {"description", "parent_id", "due_date"} & set(row) + assert row["updated_at"] == WHEN.isoformat() + + +def test_a_note_row_carries_no_task_fields(): + row = brief_row(_row(8, is_task=False)) + assert "body" not in row and "status" not in row and "milestone_title" not in row + + +def test_a_task_outside_any_milestone_says_so_rather_than_inventing_a_title(): + row = brief_row(_row(9, milestone_id=None), {}) + assert row["milestone_id"] is None and "milestone_title" not in row + + +@pytest.mark.asyncio +async def test_a_full_page_of_long_tasks_fits_inline(): + """The ceiling. 100 rows (list_tasks' own limit cap) of ~5k-character step + plans: as to_dict() rows this was over half a million characters.""" + rows = [_row(i) for i in range(100)] + with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", + AsyncMock(return_value=(rows, 100))), \ + patch("scribe.mcp.tools.tasks.milestones_svc.titles_for", + AsyncMock(return_value={415: "An existing plan is found"})) as titles: + out = await list_tasks(project_id=2, limit=100) + size = len(json.dumps(out)) + assert size < 40_000, f"list_tasks page is {size} characters" + assert titles.await_args.args[0] == {415} + assert out["tasks"][0]["milestone_title"] == "An existing plan is found" + + +@pytest.mark.asyncio +async def test_list_notes_rows_leave_the_body_to_get_note(): + rows = [_row(i, is_task=False) for i in range(3)] + with patch("scribe.mcp.tools.notes.notes_svc.list_notes", + AsyncMock(return_value=(rows, 3))): + out = await list_notes(project_id=2) + assert all("body" not in r for r in out["notes"]) + + +@pytest.mark.asyncio +async def test_list_system_records_rows_leave_the_body_to_the_getters(): + rows = [_row(1), _row(2, is_task=False)] + with patch("scribe.mcp.tools.systems.systems_svc.list_records_for_system", + AsyncMock(return_value=rows)), \ + patch("scribe.mcp.tools.systems.milestones_svc.titles_for", + AsyncMock(return_value={415: "Plan"})): + out = await list_system_records(system_id=3) + assert [r["id"] for r in out["records"]] == [1, 2] + assert all("body" not in r for r in out["records"]) diff --git a/tests/test_mcp_tool_milestones.py b/tests/test_mcp_tool_milestones.py index 9390761..19a4b07 100644 --- a/tests/test_mcp_tool_milestones.py +++ b/tests/test_mcp_tool_milestones.py @@ -6,7 +6,7 @@ import pytest from scribe.mcp.tools.milestones import ( list_milestones, get_milestone, create_milestone, update_milestone, ) -from tests.helpers import fake_milestone +from tests.helpers import fake_milestone, fake_task pytestmark = pytest.mark.usefixtures("_bind_user") @@ -111,8 +111,7 @@ async def test_update_milestone_sends_body(): @pytest.mark.asyncio async def test_get_milestone_returns_body_steps_and_rules(): m = fake_milestone(id=5, project_id=3, body="## Goal") - step = MagicMock() - step.to_dict.return_value = {"id": 9, "title": "step 1", "status": "todo"} + step = fake_task(id=9, title="step 1", milestone_id=5, body="a long step body") applicable = {"rules": [{"id": 1, "title": "r"}], "truncated": False, "project_rules": [{"id": 3, "title": "own"}]} with patch("scribe.mcp.tools.milestones.milestones_svc.get_milestone", @@ -126,7 +125,10 @@ async def test_get_milestone_returns_body_steps_and_rules(): out = await get_milestone(milestone_id=5) assert out["milestone"]["body"] == "## Goal" assert out["milestone"]["total"] == 1 - assert out["steps"] == [{"id": 9, "title": "step 1", "status": "todo"}] + assert [(s["id"], s["title"], s["status"]) for s in out["steps"]] == [(9, "step 1", "todo")] + # A step's body is get_task's job (#4061); its placement is not. + assert "body" not in out["steps"][0] + assert out["steps"][0]["milestone_title"] == m.title assert out["applicable_rules"] == [{"id": 1, "title": "r"}] diff --git a/tests/test_mcp_tool_systems.py b/tests/test_mcp_tool_systems.py index 9432553..6f8c79e 100644 --- a/tests/test_mcp_tool_systems.py +++ b/tests/test_mcp_tool_systems.py @@ -2,7 +2,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from tests.helpers import fake_note, fake_system +from tests.helpers import fake_note, fake_system, fake_task # The name assessment both doors run before minting (milestone 307). A test @@ -87,9 +87,9 @@ async def test_create_system_duplicate_names_the_existing_one_and_creates_nothin @pytest.mark.asyncio async def test_get_system_splits_records_by_kind(): - issue = MagicMock(); issue.to_dict.return_value = {"id": 10}; issue.task_kind = "issue"; issue.status = "todo" - work = MagicMock(); work.to_dict.return_value = {"id": 11}; work.task_kind = "work"; work.status = "todo" - note = MagicMock(); note.to_dict.return_value = {"id": 12}; note.task_kind = "work"; note.status = None + issue = fake_task(id=10, task_kind="issue", milestone_id=None) + work = fake_task(id=11, milestone_id=None) + note = fake_note(id=12, status=None, milestone_id=None) with patch("scribe.mcp.tools.systems.current_user_id", return_value=1), \ patch("scribe.mcp.tools.systems.systems_svc") as svc: svc.get_system = AsyncMock(return_value=fake_system(id=3)) diff --git a/tests/test_mcp_tool_tasks.py b/tests/test_mcp_tool_tasks.py index 47db67a..bcbc8d0 100644 --- a/tests/test_mcp_tool_tasks.py +++ b/tests/test_mcp_tool_tasks.py @@ -16,7 +16,7 @@ pytestmark = pytest.mark.usefixtures("_bind_user") @pytest.mark.asyncio async def test_list_tasks_passes_is_task_true_and_repackages(): - rows = [fake_task(id=1), fake_task(id=2)] + rows = [fake_task(id=1, milestone_id=None), fake_task(id=2, milestone_id=None)] mock = AsyncMock(return_value=(rows, 2)) with patch("scribe.mcp.tools.tasks.notes_svc.list_notes", mock): out = await list_tasks() diff --git a/tests/test_precompact_hook.py b/tests/test_precompact_hook.py new file mode 100644 index 0000000..f07f77d --- /dev/null +++ b/tests/test_precompact_hook.py @@ -0,0 +1,138 @@ +"""The PreCompact hook steers the summary and never blocks the compaction (#3680). + +The mechanism this pins was read out of the installed Claude Code build, not +out of the documentation, which describes a different one. Three facts decide +whether the hook works at all, and all three are properties of what the shell +writes rather than of anything Scribe runs: + + * **Exit 0.** The handler keeps a hook's stdout only when it `succeeded`, + which is `status === 0`. A non-zero exit sends the same bytes down the + failure branch instead, where they become a line on the operator's screen + and reach the model not at all. + + * **Plain text on stdout.** That text is returned as `newCustomInstructions` + and merged into the prompt that writes the summary. JSON is not unwrapped + for this event — the hook-output schema has no PreCompact variant — so a + JSON envelope would be spliced into the summarizer's instructions verbatim, + braces and all. + + * **Never blocked.** `exit 2` or `{"decision": "block"}` makes the handler + SKIP the compaction. The model is never told; the session simply runs on + uncompacted toward its context limit with no summary. That is strictly + worse than having no hook, and it is the outcome the spike existed to keep + out of the plugin — so it is pinned here rather than left to review. + +The fourth test is the one that catches a rewrite drifting back toward the +original design: a future edit that reaches for `additionalContext` would look +correct beside every other hook in this directory and would inject nothing. +""" +from __future__ import annotations + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +HOOK = ROOT / "plugin" / "hooks" / "scribe_precompact_preserve.sh" +HOOKS_JSON = ROOT / "plugin" / "hooks" / "hooks.json" + +EVENT = {"session_id": "s1", "transcript_path": "/tmp/t.jsonl", "cwd": "/repo", + "hook_event_name": "PreCompact", "trigger": "manual", + "custom_instructions": None} + + +def _run(event: dict) -> subprocess.CompletedProcess: + if shutil.which("bash") is None: + pytest.skip("bash not installed") + return subprocess.run(["bash", str(HOOK)], input=json.dumps(event), + capture_output=True, text=True, timeout=30) + + +def _code() -> str: + """The script with its comments and its heredoc removed. + + The static checks below are about what the shell DOES. Run over the whole + file they also read the header — which quotes `{"decision":"block"}` in the + course of explaining why this hook must never emit one — and the emitted + instructions, which use the word "decision" in a sentence. Both are the + file doing its job, and neither is code. + """ + lines, in_heredoc = [], False + for line in HOOK.read_text().splitlines(): + if in_heredoc: + in_heredoc = line.strip() != "EOF" + continue + if line.lstrip().startswith("cat <<'EOF'"): + in_heredoc = True + continue + if not line.lstrip().startswith("#"): + lines.append(line) + return "\n".join(lines) + + +def test_the_comment_stripper_still_leaves_the_shell_behind(): + """Guard on the three checks below it. `_code()` returning nothing would + make every one of them pass while checking an empty string — the same + circularity the plugin version check has to defend against.""" + code = _code() + assert "set -uo pipefail" in code and "exit 0" in code + # It really did strip: both of the words the checks look for are present + # in the file, and neither is in the code. + assert "decision" in HOOK.read_text() + + +@pytest.mark.parametrize("trigger", ["manual", "auto"]) +def test_it_exits_zero_with_instructions_on_stdout(trigger): + """Exit 0 plus non-empty stdout is the entire contract for reaching the + summarizer; either half missing and the hook is decoration.""" + out = _run({**EVENT, "trigger": trigger}) + assert out.returncode == 0, out.stderr + assert out.stdout.strip(), "empty stdout is dropped by the handler" + + +def test_the_instructions_name_what_has_to_survive(): + """The summary is the next turn's only copy of these, so the hook says so + in the words a summarizer can act on.""" + said = _run(EVENT).stdout.lower() + assert "id" in said and "title" in said + for anchor in ("in progress", "scribe", "not yet recorded"): + assert anchor in said, f"the instruction no longer mentions {anchor!r}" + + +def test_it_emits_text_and_not_a_json_envelope(): + """Every other hook here answers in JSON. This one must not: for PreCompact + the envelope is not unwrapped, it is pasted into the summarizer's prompt.""" + assert not _run(EVENT).stdout.lstrip().startswith("{") + + +def test_it_never_blocks_the_compaction(): + """A block skips compaction silently from the model's side. Nothing in the + script may produce one — not an exit code, not a decision.""" + code = _code() + assert "decision" not in code, "a block decision would skip the compaction" + assert "exit 2" not in code + # A truncated or absent event must not turn into a non-zero exit either. + for event in ("", "not json", "{}"): + out = subprocess.run(["bash", str(HOOK)], input=event, + capture_output=True, text=True, timeout=30) + assert out.returncode == 0, f"{event!r} → {out.returncode}: {out.stderr}" + + +def test_additional_context_is_not_how_this_event_works(): + """SessionStart's channel, which does not exist on PreCompact. A rewrite + that reaches for it would read as consistent with the other hooks and + inject nothing at all.""" + assert "additionalContext" not in _code() + + +def test_the_plugin_registers_it_on_precompact(): + entries = json.loads(HOOKS_JSON.read_text())["hooks"]["PreCompact"] + commands = [h["command"] for e in entries for h in e["hooks"]] + assert any(HOOK.name in c for c in commands) + assert all(h["type"] == "command" for e in entries for h in e["hooks"]), ( + "PreCompact accepts command hooks only — a prompt or agent hook is " + "rejected at registration" + ) diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index 952b29b..d589587 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -735,14 +735,27 @@ def test_the_hook_and_the_route_agree_on_every_parameter_name(): import re hook = Path("plugin/hooks/scribe_tool_rules.sh").read_text() + defs = Path("plugin/hooks/scribe_defs.sh").read_text() route = Path("src/scribe/routes/plugin.py").read_text() handler = route.split("async def pre_tool_rules")[1].split("\n@plugin_bp")[0] sent = set(re.findall(r"[?&]([a-z_]+)=", hook)) - assert sent == {"tool", "command", "repo", "exclude_rule_ids"}, sent + assert sent == {"tool", "command", "exclude_rule_ids"}, sent - # `repo` is read by the shared _project_scope() helper, not inline. + # The project-scope key is the shared helper's to choose (#4085): the hook + # splices in `scribe_scope_query`'s output, which is `repo=` inside a git + # repo and `project_id=` where a `.scribe` marker names the project. Both + # halves are one contract with the route, so both are pinned. + assert "scribe_scope_query" in hook, "hook no longer asks for a project scope" + assert set(re.findall(r"printf '([a-z_]+)=", defs)) == {"repo", "project_id"} + + # Both scope keys are read by the shared _project_scope() helper, not inline. assert "_project_scope()" in handler + scope = route.split("async def _project_scope")[1].split("\n@plugin_bp")[0] + for arg in ("project_id", "repo"): + assert f'request.args.get("{arg}"' in scope, ( + f"the hooks can send {arg!r} and the route never reads it" + ) for arg in ("tool", "command", "exclude_rule_ids"): assert f'request.args.get("{arg}")' in handler, ( f"the hook sends {arg!r} and the route never reads it" diff --git a/tests/test_scribe_marker.py b/tests/test_scribe_marker.py new file mode 100644 index 0000000..7af04f3 --- /dev/null +++ b/tests/test_scribe_marker.py @@ -0,0 +1,150 @@ +"""`.scribe` — the second key a directory can be scoped by (#4085). + +Every hook in `plugin/hooks/` scopes its request to a project, and until this +existed all six turned on one key: `git remote get-url origin`. That key does +not exist outside a git repo, so a session in a plain directory was unscoped +everywhere at once — silently, because a missing remote is indistinguishable +from a remote nobody bound. + +`scribe_scope_query` is the shared answer, so these run the real shell rather +than a reimplementation of it. What they pin: + + * the marker is found from a subdirectory, the way git finds its root; + * a bare integer works, because that is what a person writes by hand; + * the marker BEATS a git remote, since someone put the file there on purpose; + * an `instance` naming a different Scribe is refused — the case the field + exists for. A project id means nothing on its own: id 2 is a different + project on every instance, so a marker that travels would otherwise scope + the session to the wrong project, confidently and without a word. + +That last one is the reason the file is JSON and not a number, so it is tested +from both directions: the matching host is honoured, the mismatched one is not. +""" +from __future__ import annotations + +import json +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" +INSTANCE = "https://scribe.example.com" + + +def _sh(script: str, cwd: Path, env_extra: dict | None = None) -> str: + for tool in ("bash", "jq"): + if shutil.which(tool) is None: + pytest.skip(f"hook runtime tool {tool!r} not installed") + env = {"PATH": os.environ["PATH"], "HOME": str(cwd)} + env.update(env_extra or {}) + body = f'set -uo pipefail\n. "{DEFS}"\nurl="{INSTANCE}"\n{script}\n' + out = subprocess.run(["bash", "-c", body], cwd=cwd, capture_output=True, + text=True, env=env, timeout=30) + assert out.returncode == 0, out.stderr + return out.stdout.strip() + + +def _marker(d: Path, **fields) -> None: + (d / ".scribe").write_text(json.dumps(fields)) + + +def _git_repo(d: Path, remote: str) -> None: + env = {"PATH": os.environ["PATH"], "HOME": str(d)} + subprocess.run(["git", "init", "-q"], cwd=d, check=True, env=env) + subprocess.run(["git", "remote", "add", "origin", remote], cwd=d, check=True, env=env) + + +def test_a_bare_integer_is_a_valid_marker(tmp_path): + """What someone writes by hand. It parses as JSON already, so one filter + reads it and the written form alike.""" + (tmp_path / ".scribe").write_text("7\n") + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=7" + + +def test_the_marker_is_found_from_a_subdirectory(tmp_path): + _marker(tmp_path, instance=INSTANCE, project_id=2, project="FabledScribe") + deep = tmp_path / "src" / "scribe" / "services" + deep.mkdir(parents=True) + assert _sh(f'scribe_scope_query "{deep}"', tmp_path) == "project_id=2" + + +def test_a_matching_instance_is_honoured(tmp_path): + """Host-only comparison, so http/https and a trailing slash are not a + mismatch — only a genuinely different Scribe is.""" + _marker(tmp_path, instance="http://scribe.example.com/", project_id=2) + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=2" + + +def test_a_marker_for_another_instance_is_refused_with_a_reason(tmp_path): + """The case the `instance` field exists for. No project beats the wrong + project, and the reason is carried so the session can say which it was.""" + _marker(tmp_path, instance="https://someone-elses.example.org", project_id=2) + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "" + said = _sh(f'scribe_marker_read "{tmp_path}/.scribe"', tmp_path) + assert "someone-elses.example.org" in said and "scribe.example.com" in said + + +def test_the_marker_beats_a_git_remote(tmp_path): + """Someone put the file there deliberately; a remote is only where the + code happens to be pushed. This is also how a directory overrides its + binding.""" + _git_repo(tmp_path, "git@git.example.com:someone/thing.git") + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path).startswith("repo=") + _marker(tmp_path, instance=INSTANCE, project_id=2) + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "project_id=2" + + +def test_without_a_marker_the_git_remote_still_answers(tmp_path): + """The path every existing install is on — it must not have moved.""" + _git_repo(tmp_path, "git@git.example.com:someone/thing.git") + got = _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) + assert got.startswith("repo=") and "git.example.com" in got.replace("%2F", "/") + + +def test_a_plain_directory_with_no_marker_scopes_to_nothing(tmp_path): + """Not an error — the caller sends no scope and the server says so.""" + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "" + + +@pytest.mark.parametrize("body", ["", "not json at all", "{}", '{"project_id": 0}', + '{"project_id": "../../etc"}', "[1,2,3]"]) +def test_an_unusable_marker_is_refused_rather_than_sent(tmp_path, body): + """A marker is operator-written and can say anything. Nothing that is not + a positive integer may reach the query string.""" + (tmp_path / ".scribe").write_text(body) + assert _sh(f'scribe_scope_query "{tmp_path}"', tmp_path) == "" + + +def test_the_reason_survives_the_command_substitution_that_fetches_it(tmp_path): + """Regression on a bug this nearly shipped with. + + The reason used to be a global the function set, which every caller reads + through `$( )` — a subshell, so the assignment died with it and the caller + saw an UNSET variable. The hooks all run `set -u`, where reading one aborts + the script: a directory with a misaddressed marker would have cost the + session its entire SessionStart context, to fetch a warning about a file. + + So the reason comes back through stdout with the id, and this asserts on + the shape the caller actually uses: read in a subshell, split, and used + under `set -u` without the shell dying. + """ + _marker(tmp_path, instance="https://elsewhere.example.org", project_id=2) + got = _sh( + f'read_out=$(scribe_marker_read "$(scribe_marker_file "{tmp_path}")")\n' + 'printf "id=[%s] why=[%s]" "${read_out%%$\'\\t\'*}" "${read_out#*$\'\\t\'}"', + tmp_path, + ) + assert got.startswith("id=[]") + assert "elsewhere.example.org" in got + + +def test_the_walk_up_terminates_at_the_root(tmp_path): + """No marker anywhere above: the loop must end rather than spin. tmp_path + is several levels down from /, so this really does walk.""" + deep = tmp_path / "a" / "b" / "c" + deep.mkdir(parents=True) + assert _sh(f'scribe_scope_query "{deep}"', tmp_path) == "" diff --git a/tests/test_services_plugin_context.py b/tests/test_services_plugin_context.py index 33bc3d4..fff5f62 100644 --- a/tests/test_services_plugin_context.py +++ b/tests/test_services_plugin_context.py @@ -215,6 +215,31 @@ async def test_build_session_context_unbound_repo_emits_bind_hint(): assert "## Active project" not in ctx +@pytest.mark.asyncio +async def test_a_project_id_that_does_not_resolve_is_reported_not_dropped(): + """A `.scribe` marker names a project directly (#4085), so for the first + time a caller arrives holding a pointer it BELIEVES in. If the id is for + another instance, or names a deleted project, the session has to be told — + it cannot infer it from an absence. + + This used to render nothing whatsoever: the branch hung off `if project_id` + as an `elif`, so an id that was sent and failed took the outer arm, found + no project, and fell out of the block having said nothing at all. + """ + from scribe.services.plugin_context import build_session_context + with patch("scribe.services.plugin_context.projects_svc.get_project", + AsyncMock(return_value=None)): + out = await build_session_context(user_id=7, project_id=41) + + ctx = out["context"] + assert out["project"] is None + assert "## Project 41 could not be loaded" in ctx + assert "list_projects" in ctx + # Not mistaken for the repo case, which has a different remedy. + assert "## Repository not yet bound" not in ctx + assert "No Scribe project is bound" not in ctx + + @pytest.mark.asyncio async def test_build_process_manifest_renders_stub_specs(): items = [ diff --git a/tests/test_write_path_trigger.py b/tests/test_write_path_trigger.py index d6dd323..84af7cd 100644 --- a/tests/test_write_path_trigger.py +++ b/tests/test_write_path_trigger.py @@ -931,10 +931,16 @@ def test_route_reads_every_arg_the_hook_sends(): assert f'request.args.get("{arg}"' in src, f"route ignores {arg}" hook = HOOK.read_text() - for arg in ("path=", "code=", "repo=", "exclude_ids=", "exclude_sync_ids=", - "shapes="): + for arg in ("path=", "code=", "exclude_ids=", "exclude_sync_ids=", "shapes="): assert arg in hook, f"hook never sends {arg}" + # The project-scope key is no longer spelled in the hook: since #4085 it is + # whichever of `repo=` / `project_id=` scribe_scope_query picks, so the hook + # splices in its output and the helper is the other half of the contract. + assert "scribe_scope_query" in hook, "hook no longer asks for a project scope" + defs = (HOOK.parent / "scribe_defs.sh").read_text() + assert set(re.findall(r"printf '([a-z_]+)=", defs)) == {"repo", "project_id"} + def test_route_resolves_repo_to_a_project_not_to_a_location_filter(): """A snippet's `repo` is a label the operator typed; the hook sends a git