refactor(plugin+tests): the last two parallel-family gaps — pageable list tools, one config preamble (#2278)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 27s
CI & Build / TypeScript typecheck (push) Successful in 36s
CI & Build / integration (push) Successful in 1m34s
CI & Build / Python tests (push) Successful in 2m21s
CI & Build / Build & push image (push) Successful in 25s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 27s
CI & Build / TypeScript typecheck (push) Successful in 36s
CI & Build / integration (push) Successful in 1m34s
CI & Build / Python tests (push) Successful in 2m21s
CI & Build / Build & push image (push) Successful in 25s
DRY pass 3's remainder. Both halves start from enumeration, because the task's
candidate list was hypotheses and the process requires counting before
proposing — and counting changed the answer twice.
## The list_* family: a limit with no offset
Enumerated all 19 `list_*` MCP tools first. They are genuinely heterogeneous —
8 take `project_id`, 6 take `limit`, six take no arguments at all — so a
common-parameter guard would invent a convention the API does not have, which
is the over-DRY trap (§5). One contract IS real: a `limit` without an `offset`
is a truncation with no continuation. The caller is told there are 250 results,
handed 50, and given no way to ask for the rest.
Two tools had it, and both 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; only the door was missing — the
missing-sibling shape exactly. Both now expose it.
`tests/test_mcp_list_family.py` guards it, with `list_tags` exempted for a
stated reason (a ranked top-N over a bounded vocabulary has no "rest" to page
into). Candidates derived, decision explicit, same design as test_mcp_auth —
plus the reverse checks: a stale exemption, and an offset with no limit, which
would page through an unbounded result set. Verified non-vacuous by running the
sweep against the pre-fix tree, where it fails naming both tools.
## The verb pairs: no finding, which is the finding
`preview`/`apply` and `dry_run`/`commit` do not exist anywhere in the 102
tools — those were guesses about a shape Scribe never adopted. `count_*` does
not exist either. Of the create/delete stems only `project_rule` lacks a
`delete_X`, and deliberately: a project rule IS a rule, `delete_rule` removes
it, and the docstring says so. `force` sits on 6 of 7 duplicate-gated creates;
the exception is `create_system`, whose gate is an exact normalized-NAME match
rather than a semantic near-match — forcing it would split one area's records
across two piles, which its own message explains. No guard added: it would
need a seven-entry exemption list to defend against a hypothetical. Recorded
on the leave-alone list instead, which the process asks for by name.
## The hook config preamble
Not 3 of 6 hooks as recorded — all FIVE carried their own copy, and of four
lines rather than two. The extra two are a guard treating an unexpanded
`${...}` placeholder as unset, so it is never sent as a garbage Bearer token:
precisely the correctness detail a sixth hook would omit with nothing failing
loudly. Now `scribe_config` in scribe_defs.sh, which also declares the two
names it owns. It sets globals rather than echoing, so a token never passes
through a subshell's output where xtrace or a log could catch it, and returns
a status so a caller can bail (`|| exit 0`) or continue degraded — the
session-context hook still owes its static floor when Scribe is unconfigured.
`check_plugin.py` now runs shellcheck with `-x`. Without it the shared helpers
were invisible: every variable they set read as unassigned and every bug inside
them went unlinted at the call site, which is the opposite of what sharing them
was for. All twelve fail-open scenarios still pass, and all five hooks were
probed live against the instance — prior_art and after_write both still name
canon, autoinject returns context, session_context serves 11k chars of rules,
sync_processes stays silent. Plugin 0.1.46 (#2209).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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": {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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=""
|
||||
|
||||
@@ -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}" \
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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", []),
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
@@ -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}"
|
||||
Reference in New Issue
Block a user