bvandeusenandClaude Opus 5.5
4c150cbc2f
Twenty-eight test files described the code they guard by what it did before a fix. Each is now the property and the failure it prevents, in the present tense: the backfill snapshot, the per-dial calibration query, the rule scope, the lesson trigger filter, the subshell global, the trash owner scope, the version readout fallback, and others. A docstring line that narrated the backup suite's own history is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
114 lines
5.0 KiB
Python
114 lines
5.0 KiB
Python
"""Cross-family contracts for the `list_*` MCP tools.
|
|
|
|
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. The usual cause is a tool capped over a service that
|
|
already accepts an offset — the service's `offset` simply not exposed, or a
|
|
hardcoded `offset=0` passed into `query_knowledge`. The capability exists one
|
|
layer down; only the door is 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}"
|