diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 794ecf9..2034969 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "0.1.45", + "version": "0.1.46", "author": { "name": "Bryan Van Deusen" }, "mcpServers": { "scribe": { diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index cf5959f..52dcfd4 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -104,10 +104,7 @@ while IFS=$'\t' read -r path sha; do done <<< "$current" [ -n "$changed" ] || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +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) repo_q="" if [ -n "$repo" ]; then diff --git a/plugin/hooks/scribe_autoinject.sh b/plugin/hooks/scribe_autoinject.sh index 599f30e..3197b06 100755 --- a/plugin/hooks/scribe_autoinject.sh +++ b/plugin/hooks/scribe_autoinject.sh @@ -23,6 +23,9 @@ # note is injected at most once per session. Passed back as exclude_ids. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 command -v curl >/dev/null 2>&1 || exit 0 @@ -35,13 +38,8 @@ event_cwd=$(printf '%s' "$event" | jq -r '.cwd // empty' 2>/dev/null) || event_c # Nothing to retrieve against. [ -n "$prompt" ] || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded ${...} placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac # Unconfigured install → silent (auto-inject is pure enrichment). -[ -n "$url" ] && [ -n "$token" ] || exit 0 +scribe_config || exit 0 # Cap the query length — a giant prompt makes a giant URL for no extra signal. # `head -c`, not `cut -c1-2000`: cut is line-oriented and caps EACH LINE, so a diff --git a/plugin/hooks/scribe_defs.sh b/plugin/hooks/scribe_defs.sh index 67b4a32..93a2b39 100644 --- a/plugin/hooks/scribe_defs.sh +++ b/plugin/hooks/scribe_defs.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # shellcheck shell=bash -# Scribe plugin — the pieces the two write-path hooks share (#2901). +# Scribe plugin — the pieces the hooks share (#2901, #2278). # # scribe_prior_art.sh fires BEFORE a Write/Edit tool call; scribe_after_write.sh # fires AFTER a Bash tool call and diffs the working tree, so code written by @@ -14,6 +14,8 @@ # scribe_unreached STATE SID SECS REL the "Scribe didn't answer" line, once # per outage (#2932) — or nothing, if said lately # scribe_reached STATE SID the server answered: the next outage speaks again +# scribe_config sets `url` + `token` from the env, returns 0 +# only if BOTH are usable (#2278) # # Sourced, not executed: `. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh"`. @@ -128,6 +130,34 @@ scribe_local_dups() { # the time it last spoke; within ten minutes of that it stays quiet, and a # successful call clears it so the next outage announces itself afresh. # Unconfigured installs never reach this: no URL/token means no call was owed. +# Where every hook gets its endpoint and credential. Four lines, and each of +# the five hooks carried its own copy until #2278 — which is exactly the +# missing-sibling shape: the `${...}` guard below is a correctness detail a +# sixth hook would have forgotten, and nothing would have failed loudly. +# +# Sets `url` and `token` as globals rather than echoing them: a token must not +# pass through a subshell's output, where it could land in a log or an `xtrace` +# line. Returns 0 only when both are usable, so a caller can either bail +# (`scribe_config || exit 0`) or carry on degraded — the session-context hook +# still owes its static floor when Scribe is unconfigured. +# Declared here, not just assigned inside the function: `scribe_defs.sh` owns +# these two names, and a sourcing hook should have them defined the moment it +# sources — before any code path that might reference them. It also lets +# the linter see the assignment, which it cannot follow into a function in +# another file without -x (SC2154). +url="" +token="" + +scribe_config() { + url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} + token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} + # An unexpanded `${...}` placeholder arriving as a literal would be sent as a + # garbage Bearer token and 401. Treat it as unset. + case "$url" in *'${'*) url="" ;; esac + case "$token" in *'${'*) token="" ;; esac + [ -n "$url" ] && [ -n "$token" ] +} + _SCRIBE_UNREACHED_QUIET=600 scribe_unreached() { diff --git a/plugin/hooks/scribe_prior_art.sh b/plugin/hooks/scribe_prior_art.sh index a19d1b0..2a19aa6 100755 --- a/plugin/hooks/scribe_prior_art.sh +++ b/plugin/hooks/scribe_prior_art.sh @@ -121,11 +121,7 @@ if [ -n "$shapes" ]; then [ -n "$enc" ] && shapes_q="&shapes=${enc}" fi -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded ${...} placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +scribe_config || : # sets url/token; unconfigured is handled just below # Unconfigured install → the recorded-prior-art arms are skipped, but the local # arm above already ran and may have something to say. if [ -z "$url" ] || [ -z "$token" ]; then diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 77b5a17..04aad2a 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -39,6 +39,9 @@ # allowed to fail quietly; see the #2198 comment at the status block below. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 # needed to emit the JSON envelope safely # `CDPATH= cd` is deliberate, not a typo'd assignment: it runs this one `cd` @@ -87,13 +90,9 @@ if [ -f "$manifest" ]; then fi # --- Tier 2: dynamic rules + active-project context (best-effort) --- -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} - -# Guard against an unexpanded `${...}` placeholder reaching us as a literal — it -# would otherwise be sent as a garbage Bearer token and 401. Treat as unset. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac +# Unconfigured is NOT a failure here: tier 1's static floor is still owed, +# so this records the answer rather than acting on it. +scribe_config || : dyn="" status="" diff --git a/plugin/hooks/scribe_sync_processes.sh b/plugin/hooks/scribe_sync_processes.sh index 6422e45..8e0d0dc 100755 --- a/plugin/hooks/scribe_sync_processes.sh +++ b/plugin/hooks/scribe_sync_processes.sh @@ -23,15 +23,13 @@ # #2198), with SCRIBE_URL / SCRIBE_TOKEN as the override. set -uo pipefail +# shellcheck source=plugin/hooks/scribe_defs.sh +. "$(dirname "${BASH_SOURCE[0]}")/scribe_defs.sh" + command -v jq >/dev/null 2>&1 || exit 0 command -v curl >/dev/null 2>&1 || exit 0 -url=${SCRIBE_URL:-${CLAUDE_PLUGIN_OPTION_API_ENDPOINT:-}} -token=${SCRIBE_TOKEN:-${CLAUDE_PLUGIN_OPTION_API_TOKEN:-}} -# Guard against an unexpanded `${...}` placeholder arriving as a literal. -case "$url" in *'${'*) url="" ;; esac -case "$token" in *'${'*) token="" ;; esac -[ -n "$url" ] && [ -n "$token" ] || exit 0 +scribe_config || exit 0 body=$(curl -fsS --max-time 8 \ -H "Authorization: Bearer ${token}" \ diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index c4bdcfd..159eaa2 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -159,7 +159,12 @@ def check_shellcheck() -> None: return for script in hook_scripts(): proc = subprocess.run( - [exe, "--severity=warning", "--shell=bash", str(script)], + # -x FOLLOWS `# shellcheck source=` directives into the sourced + # file. Without it the shared helpers in scribe_defs.sh are + # invisible, so every variable they set reads as unassigned + # (SC2154) and every bug inside them goes unlinted at the call + # site — which is the opposite of what sharing them was for. + [exe, "--severity=warning", "--shell=bash", "-x", str(script)], capture_output=True, text=True, ) rel = script.relative_to(ROOT) diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index 2ad837e..e3994b6 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -15,13 +15,17 @@ from scribe.services import trash as trash_svc from scribe.services.note_usage import record_pulled -async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: +async def list_processes( + q: str = "", tag: str = "", limit: int = 50, offset: int = 0, +) -> dict: """List stored processes (reusable saved prompts). Args: q: Free-text search across title + body (optional). tag: Filter to a single tag (optional). limit: Max results (1-100). + offset: Skip this many before returning — page past the cap. + `total` is the unpaged count, so it says whether more remains. Returns {"processes": [{id, title, tags, preview}], "total": int}. An entry marked `shared: true` with an `owner` is another person's procedure — treat @@ -34,7 +38,8 @@ async def list_processes(q: str = "", tag: str = "", limit: int = 50) -> dict: uid = current_user_id() items, total = await knowledge_svc.query_knowledge( user_id=uid, note_type="process", tags=[tag] if tag else [], - sort="modified", q=q or None, limit=max(1, min(limit, 100)), offset=0, + sort="modified", q=q or None, limit=max(1, min(limit, 100)), + offset=max(0, offset), ) labelled = await access_svc.label_shared_items(uid, items) procs = [{"id": it["id"], "title": it["title"], "tags": it.get("tags", []), diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index f591eeb..ef61aec 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -20,7 +20,8 @@ from scribe.services import systems as systems_svc async def list_snippets( - q: str = "", tag: str = "", limit: int = 50, project_id: int = 0, + q: str = "", tag: str = "", limit: int = 50, offset: int = 0, + project_id: int = 0, repo: str = "", path: str = "", symbol: str = "", verification: str = "", ) -> dict: """List recorded snippets — the project's pattern library. @@ -41,6 +42,9 @@ async def list_snippets( well as wording, so describe what you need the code to DO. tag: Filter to a single tag, e.g. a language like "python" (optional). limit: Max results (1-100). + offset: Skip this many before returning — page through a corpus + larger than one call. `total` is the unpaged count, so + offset+limit against it says whether more remains. project_id: Narrow to one project. 0 (default) searches every project — usually what you want, since a helper you need here may well have been written somewhere else. @@ -81,6 +85,7 @@ async def list_snippets( uid = current_user_id() items, total = await snippets_svc.list_snippets( uid, q=q or None, tag=tag, limit=max(1, min(limit, 100)), + offset=max(0, offset), project_id=project_id or None, repo=repo, path=path, symbol=symbol, verification=verification, ) diff --git a/tests/test_mcp_list_family.py b/tests/test_mcp_list_family.py new file mode 100644 index 0000000..352843b --- /dev/null +++ b/tests/test_mcp_list_family.py @@ -0,0 +1,114 @@ +"""Cross-family contracts for the `list_*` MCP tools (#2278, shape 4). + +Per-tool tests cover what each list tool does. Nothing covered what the FAMILY +owes its callers, which is where the missing-sibling shape hides: a capability +added to one member and not its neighbour changes no return value, so no +behavioural test can see it. Source inspection can. + +WHAT THIS DELIBERATELY DOES NOT ASSERT. The 19 `list_*` tools are genuinely +heterogeneous — 8 take `project_id`, 6 take `limit`, and six take no arguments +at all (`list_projects`, `list_trash`, `list_rulebooks`, `list_design_systems`, +`list_repo_bindings`, `list_starter_role_groups`). Requiring a common parameter +across them would be inventing a convention the API does not have, which the +DRY process's over-DRY guard (§5) warns against by name: a wrong abstraction is +worse than the duplication. So this file asserts ONE contract, the one that is +a real promise rather than a shape coincidence. + +THE CONTRACT: a `limit` without an `offset` is a truncation with no +continuation. The caller is told there are 250 results and handed 50, with no +way to ask for the rest. Both tools that had this were capped over a service +that already accepted an offset — `snippets_svc.list_snippets(offset=0)` was +simply not exposed, and `list_processes` passed a hardcoded `offset=0` into +`query_knowledge`. The capability existed one layer down in both cases; only +the door was missing. + +As in `test_mcp_auth`, the CANDIDATES are derived and the DECISION is explicit. +Deriving the exemption too would make the contract follow a naming convention, +so any future `list_*` could opt itself out by accident. +""" +import ast +import pathlib + +TOOLS_DIR = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" / "mcp" / "tools" + +# `limit` here caps a RANKED top-N, not a page into a corpus, so there is no +# "rest" to ask for — the 51st most-used tag is not what the caller wanted and +# an offset into that ordering answers no question. Anything added here needs a +# reason of that kind, not "it isn't paged yet". +_DELIBERATELY_UNPAGED = { + "list_tags", # most-used tags by count, over a bounded vocabulary +} + + +def _list_tools() -> dict[str, set[str]]: + """{tool name: parameter names} for every `list_*` in the tools package.""" + out: dict[str, set[str]] = {} + for path in sorted(TOOLS_DIR.glob("*.py")): + if path.name == "__init__.py": + continue + for node in ast.parse(path.read_text()).body: + if ( + isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) + and node.name.startswith("list_") + ): + out[node.name] = { + a.arg for a in node.args.args + } | {a.arg for a in node.args.kwonlyargs} + return out + + +def test_the_tools_package_is_where_we_think_it_is(): + """If this fails the sweep below is silently checking nothing.""" + tools = _list_tools() + assert len(tools) >= 15, f"found only {len(tools)} list tools — did the package move?" + + +def test_every_capped_list_tool_can_be_paged(): + """A `limit` promises a cap; without an `offset` it also imposes a ceiling.""" + tools = _list_tools() + capped = {name for name, args in tools.items() if "limit" in args} + assert capped, "no list tool takes a limit — the sweep is not finding signatures" + + unpageable = sorted( + name for name in capped + if "offset" not in tools[name] and name not in _DELIBERATELY_UNPAGED + ) + assert not unpageable, ( + f"these list tools cap their results with no way to page past the cap: " + f"{unpageable}. Each hands the caller a `total` it cannot reach. Add an " + f"`offset` (check the service first — it usually already takes one), or " + f"add the tool to _DELIBERATELY_UNPAGED with a reason saying why there " + f"is no 'rest' to ask for." + ) + + +def test_the_unpaged_exemptions_still_exist(): + """A stale exemption is an exemption for nothing, and it hides the next + tool that inherits the name. Same reverse check `test_mcp_auth` runs on + its allow-lists.""" + tools = _list_tools() + missing = sorted(_DELIBERATELY_UNPAGED - set(tools)) + assert not missing, ( + f"_DELIBERATELY_UNPAGED names tools that no longer exist: {missing}. " + f"Renamed or deleted — drop them from the set." + ) + still_capped = sorted( + name for name in _DELIBERATELY_UNPAGED + if name in tools and "limit" not in tools[name] + ) + assert not still_capped, ( + f"these are exempted from paging but no longer take a `limit` at all, " + f"so the exemption is moot: {still_capped}." + ) + + +def test_offset_never_appears_without_limit(): + """The inverse, and it is a real bug rather than a style point: an offset + with no cap pages through an unbounded result set, so page 2 of an + ever-growing list silently returns everything after the skip.""" + tools = _list_tools() + bad = sorted( + name for name, args in tools.items() + if "offset" in args and "limit" not in args + ) + assert not bad, f"these take an offset but no limit: {bad}"