From ea972ac3f7f3675c52c89a4deef7f3492986b2f6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 1 Sep 2026 18:28:26 -0400 Subject: [PATCH] refactor(plugin): one definition of what ships, and the exclusion that makes the version check mean something (#3326) Milestone 334 step 2. The set of files that reach a plugin install lived in two hand-kept copies -- SHIPPED in check_plugin.py and the workflow's paths: filter -- with a comment asking a human to keep them in step. That is the shape #3127 section 3 warns about, and both copies had drifted. The load-bearing change is the exclusion. The version check reads "did shipped content change against the base?", and plugin.json lives INSIDE plugin/ -- so bumping the version is itself a change to the set, which then reads as the change that justifies the bump. Every bump passed, no bump could ever fail, and the check proved nothing while looking green. manifest_differs_beyond_version compares parsed objects with `version` dropped from both sides. One field, never the whole file: plugin.json also carries description, mcpServers and userConfig, all of which reach an install, and excluding the file wholesale would let a userConfig-only edit compute an unchanged version and never refresh -- #2209 again with a narrower trigger. Unreadable input answers "changed", because a spurious bump costs one cache refresh while a missed one is the fix reaching the repo and stopping there. shipped_content_changed returns None, not False, when the diff fails. #2663 is why: a read that failed inside a broad except reported the same zero as an empty window, and every counter read zero for weeks. Two dead trigger paths removed, both found by writing the guard rather than by review. fable-mcp/** outlived its directory by three months (deleted in 91bafb6, 2026-05-27) and assets/** named a path that never existed at all. A paths: entry matching nothing never fires, so neither ever failed anything. Their two orphaned bump scripts go with them -- a third manual-bump mechanism, wired into no settings file. DERIVERS is section 3's (deriver -> artifacts whose identity it decides) table. The membership test is "can changing this file change what the artifact says about itself?", not "is it copied in" -- a deriver is never in the COPY list. A checker is not a deriver, which is why check_plugin.py is absent from it; step 3's mint script adds its own row. The workflow's paths: filter is YAML and cannot import Python, so "one definition" is held by drift tests rather than an import. Said plainly in the test module, because it is the honest shape rather than the ideal one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DN4zBVFWhBST9YqjCfQmPb --- .forgejo/workflows/ci.yml | 2 - scripts/bump_fable_mcp_version.sh | 17 -- scripts/check_plugin.py | 141 +++++++++++++++-- scripts/pre_commit_fable_mcp.sh | 39 ----- tests/test_plugin_shipped_set.py | 250 ++++++++++++++++++++++++++++++ 5 files changed, 377 insertions(+), 72 deletions(-) delete mode 100755 scripts/bump_fable_mcp_version.sh delete mode 100755 scripts/pre_commit_fable_mcp.sh create mode 100644 tests/test_plugin_shipped_set.py diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 267a6e7..3d11996 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -46,8 +46,6 @@ on: - "alembic/**" - "alembic.ini" - "Dockerfile" - - "assets/**" - - "fable-mcp/**" # The plugin ships straight from this repo — installs fetch it via # .claude-plugin/marketplace.json, NOT from the image. So a push here is # the release, with no build step in between. Omitting these paths meant diff --git a/scripts/bump_fable_mcp_version.sh b/scripts/bump_fable_mcp_version.sh deleted file mode 100755 index 16b658c..0000000 --- a/scripts/bump_fable_mcp_version.sh +++ /dev/null @@ -1,17 +0,0 @@ -#!/usr/bin/env bash -# Bump the patch segment of fable-mcp/pyproject.toml version and stage the file. -# Usage: called automatically by the Claude Code pre-commit hook, or manually. -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -FILE="$REPO_ROOT/fable-mcp/pyproject.toml" - -current=$(grep '^version = ' "$FILE" | sed 's/version = "\(.*\)"/\1/') -major=$(echo "$current" | cut -d. -f1) -minor=$(echo "$current" | cut -d. -f2) -patch=$(echo "$current" | cut -d. -f3) -new_version="$major.$minor.$((patch + 1))" - -sed -i "s/^version = \"$current\"/version = \"$new_version\"/" "$FILE" -git -C "$REPO_ROOT" add "$FILE" -echo "fable-mcp: $current → $new_version" diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index 159eaa2..dceaf70 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -50,9 +50,45 @@ PLUGIN_DIR = ROOT / "plugin" HOOKS_DIR = PLUGIN_DIR / "hooks" MANIFEST = PLUGIN_DIR / ".claude-plugin" / "plugin.json" -# Paths whose contents reach an install. Keep in step with the workflow's -# `paths:` filter — a path that ships but isn't checked here is the gap again. -SHIPPED = ("plugin", ".claude-plugin") +# ── What ships, and what decides what it says about itself ───────────────── +# +# ONE definition (#3127 §3, milestone 334 step 2). It has TWO consumers that +# need different granularities, and conflating them is the bug: +# +# the workflow's `paths:` trigger whole paths should CI run at all? +# the version check paths MINUS should the version +# the manifest have moved? +# `version` +# +# The second one is why this is not just a tuple of paths. `plugin.json` lives +# INSIDE `plugin/`, so a version bump is itself a change to the shipped set — +# and a check that reads the set naively then treats the bump as its own +# justification. Any bump passes, no bump fails, and it has proved nothing. +# `shipped_content_changed` below is the exclusion-aware reader. +# +# The exclusion is that ONE FIELD, never the whole file: `plugin.json` also +# carries description, mcpServers and userConfig, all of which reach an +# install and all of which matter. Excluding the file wholesale would mean a +# userConfig-only edit computes an unchanged version and never refreshes — +# #2209 again with a narrower trigger. +SHIPPED_PATHS = ("plugin", ".claude-plugin") + +# Files that decide what a published artifact SAYS ABOUT ITSELF — kept as a +# table so the next artifact is a one-line addition rather than a third +# bespoke guard (#3127 §3). The membership test is NOT "is this copied into +# the artifact?" but "can changing this file change the published bytes, or +# what the artifact says about itself?" — FC learned that twice in four days +# (#3156, #3202), and a deriver is never in the COPY list. +# +# Note what is absent: a CHECKER does not belong here. Whatever validates a +# version decides whether the lane goes red, not what any artifact reports, +# so `check_plugin.py` itself is not a deriver — the plugin's mint script +# (milestone 334 step 3) will be, and adds its own row. +DERIVERS: dict[str, tuple[str, ...]] = { + # The "Generate image tags and version" step computes the server image's + # name, ordering key and channel (#3298). + ".forgejo/workflows/ci.yml": ("server-image",), +} failures: list[str] = [] @@ -391,23 +427,95 @@ def _git(*args: str) -> tuple[int, str]: return proc.returncode, (proc.stdout or proc.stderr).strip() -def manifest_version(ref: str | None = None) -> str | None: - """The manifest version at `ref`, or in the working tree when ref is None.""" +def manifest_text(ref: str | None = None) -> str | None: + """The manifest's RAW TEXT at `ref`, or in the working tree when ref is None. + + Split out from `manifest_version` because the exclusion below needs every + field except one, not the one field. + """ if ref is None: try: - return json.loads(MANIFEST.read_text()).get("version") - except Exception: + return MANIFEST.read_text() + except OSError: return None rel = MANIFEST.relative_to(ROOT).as_posix() code, out = _git("show", f"{ref}:{rel}") - if code != 0: + return out if code == 0 else None + + +def manifest_version(ref: str | None = None) -> str | None: + """The manifest version at `ref`, or in the working tree when ref is None.""" + text = manifest_text(ref) + if text is None: return None try: - return json.loads(out).get("version") + return json.loads(text).get("version") except Exception: return None +# Distinct from None, which is a legitimate "this manifest does not exist". +_UNREADABLE = object() + + +def manifest_differs_beyond_version(a: str | None, b: str | None) -> bool: + """Do two `plugin.json` texts differ in anything OTHER than `version`? + + THE exclusion, and it is kept pure — no git, no filesystem — because this + is the half worth testing hard and it needs no repository to exercise. + + Compares PARSED objects rather than text, so reformatting, key reordering + and whitespace do not read as content changes. `version` is dropped from + both sides; everything else counts, which is what keeps a userConfig-only + or mcpServers-only edit demanding a new version. + + Unreadable input answers True. The conservative direction is "demand a new + version": a spurious bump costs one cache refresh, while a missed one is + #2209 — the fix reaches the repo and stops there. + """ + def without_version(text: str | None): + if text is None: + return None + try: + data = json.loads(text) + except Exception: + return _UNREADABLE + if not isinstance(data, dict): + return _UNREADABLE + return {k: v for k, v in data.items() if k != "version"} + + left, right = without_version(a), without_version(b) + if left is _UNREADABLE or right is _UNREADABLE: + return True + return left != right + + +def shipped_content_changed(base: str) -> tuple[bool | None, list[str]]: + """Has anything that REACHES AN INSTALL changed against `base`? + + Returns `(changed, paths)`. `changed` is **None** when the question could + not be answered — a caller must never read that as "no", which is the + distinction #2663 cost weeks of zeroed telemetry to learn. + + The manifest is special-cased, not excluded: if it is the ONLY thing that + moved and the only difference is `version`, nothing that reaches an + install has changed. Any other manifest field, or any other file, counts. + """ + code, out = _git("diff", "--name-only", base, "--", *SHIPPED_PATHS) + if code != 0: + return None, [] + paths = [p for p in out.splitlines() if p.strip()] + if not paths: + return False, [] + + rel_manifest = MANIFEST.relative_to(ROOT).as_posix() + if paths == [rel_manifest]: + return manifest_differs_beyond_version( + manifest_text(), manifest_text(base) + ), paths + return True, paths + + def check_version_bump(base: str = "origin/main") -> None: """If shipped plugin content differs from `base`, the version must too. @@ -416,6 +524,11 @@ def check_version_bump(base: str = "origin/main") -> None: actually matters is that whatever reaches an install carries a version the installer can tell apart from the one already cached. One bump per batch, which is also how a human would do it. + + Reads the set through `shipped_content_changed`, so a commit whose ONLY + change is the version field does not count as content moving. Without that + the check is circular — the bump edits a file inside `plugin/`, which then + reads as the change that justifies the bump. """ code, _ = _git("rev-parse", "--verify", base) if code != 0: @@ -429,11 +542,11 @@ def check_version_bump(base: str = "origin/main") -> None: ) return - code, changed = _git("diff", "--name-only", base, "--", *SHIPPED) - if code != 0: - fail(f"git diff against {base} failed: {changed}") + changed, paths = shipped_content_changed(base) + if changed is None: + fail(f"git diff against {base} failed, so the version check could not run") return - if not changed.strip(): + if not changed: ok(f"no shipped plugin changes against {base} — version bump not required") return @@ -445,7 +558,7 @@ def check_version_bump(base: str = "origin/main") -> None: ok(f"no manifest on {base} — treating as a new plugin (version {here})") return if here == there: - files = "\n ".join(changed.splitlines()) + files = "\n ".join(paths) fail( f"plugin content changed but the manifest version is still {here}.\n" f" The installer compares versions to decide whether to refresh " diff --git a/scripts/pre_commit_fable_mcp.sh b/scripts/pre_commit_fable_mcp.sh deleted file mode 100755 index 8dbd54c..0000000 --- a/scripts/pre_commit_fable_mcp.sh +++ /dev/null @@ -1,39 +0,0 @@ -#!/usr/bin/env bash -# Claude Code PreToolUse hook for Bash. -# Reads the tool input JSON from stdin; if the command is a git commit -# and fable-mcp files (other than pyproject.toml) are staged, bumps -# the fable-mcp patch version before the commit proceeds. -# -# Exits 0 always so it never blocks the commit. - -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" - -input=$(cat) -command=$(echo "$input" | python3 -c " -import sys, json -data = json.load(sys.stdin) -# Claude Code sends {tool_input: {command: ...}} -ti = data.get('tool_input', data) -print(ti.get('command', '')) -" 2>/dev/null || echo "") - -# Only act on git commit commands -if ! echo "$command" | grep -qE "git commit"; then - exit 0 -fi - -cd "$REPO_ROOT" - -# Check if fable-mcp files other than pyproject.toml are staged -fable_staged=$(git diff --cached --name-only 2>/dev/null \ - | grep "^fable-mcp/" \ - | grep -v "^fable-mcp/pyproject.toml$" \ - || true) - -if [ -n "$fable_staged" ]; then - bash "$REPO_ROOT/scripts/bump_fable_mcp_version.sh" -fi - -exit 0 diff --git a/tests/test_plugin_shipped_set.py b/tests/test_plugin_shipped_set.py new file mode 100644 index 0000000..0a8f46e --- /dev/null +++ b/tests/test_plugin_shipped_set.py @@ -0,0 +1,250 @@ +"""One definition of what SHIPS in the plugin, and the drift guards on it. + +WHAT THIS IS ABOUT (#3127 §3, milestone 334 step 2). Scribe publishes two +artifacts. `plugin/` is not in the Docker image — installs fetch it from this +repo through `.claude-plugin/marketplace.json`, so **a push IS the release**, +with no build step in between. That makes "which files reach an install?" a +question with real consequences, and it has been answered wrong twice: + + - #2198 — `plugin/**` was in no `paths:` filter, so four broken hooks + reached live installs having triggered no CI at all. + - #2209 — the fix for that shipped and still could not reach an install, + because the manifest version had not moved. + +The set lives in `scripts/check_plugin.py`. Its second consumer is the +workflow's `paths:` trigger, which is YAML and cannot import Python — so the +"one definition" is held together by the drift tests here rather than by an +import. That is the honest shape, and it is why these tests exist at all. + +The exclusion tests are the load-bearing half. Without the manifest-`version` +exclusion the version check is CIRCULAR: bumping the version edits a file +inside `plugin/`, which then reads as the content change that justifies the +bump. Every bump passes, no bump ever fails, and the check has proved nothing +while looking green. +""" +import json +import pathlib +import re + +import pytest + +from scripts.check_plugin import ( + DERIVERS, + SHIPPED_PATHS, + manifest_differs_beyond_version, +) + +ROOT = pathlib.Path(__file__).resolve().parents[1] +CI = ROOT / ".forgejo/workflows/ci.yml" + + +def trigger_paths() -> list[str]: + """The `paths:` list under the workflow's push trigger. + + Parsed with a regex rather than a YAML library, matching what + test_version_endpoint.py already does with this file — the alternative is + adding PyYAML as a dependency for one assertion. Raises rather than + returning empty: a silent no-op here would defeat the point of the file. + """ + text = CI.read_text() + block = re.search(r"^ paths:\n((?:(?: [-#].*)?\n)+)", text, re.M) + if block is None: + raise AssertionError("could not find the push trigger's `paths:` block") + found = re.findall(r'^ - "([^"]+)"', block.group(1), re.M) + if not found: + raise AssertionError("the `paths:` block parsed to zero entries") + return found + + +# ── The set itself ───────────────────────────────────────────────────────── + + +def test_every_shipped_path_exists(): + """A set naming something that isn't there is not a definition of anything.""" + for path in SHIPPED_PATHS: + assert (ROOT / path).exists(), f"SHIPPED_PATHS names {path}, which does not exist" + + +def test_every_shipped_path_triggers_ci(): + """#2198's exact hole, stated as an assertion. + + Directional on purpose: the trigger is a superset (it also fires on + `src/**`, `tests/**` and friends). What must never happen is a path that + reaches an install and fires no lane. + """ + triggers = trigger_paths() + for path in SHIPPED_PATHS: + covered = any(t == path or t.startswith(f"{path}/") for t in triggers) + assert covered, ( + f"{path} ships to installs but no `paths:` entry covers it — " + f"changes there would reach a live install having run no CI (#2198)" + ) + + +def test_the_checker_itself_triggers_ci(): + """Changing the checks must re-run them. + + Not a member of the shipped set — a checker decides whether the lane goes + red, not what any artifact reports — but a change to it that runs no lane + is the same silence by a different route. + """ + assert "scripts/check_plugin.py" in trigger_paths() + + +def test_no_trigger_path_names_something_that_does_not_exist(): + """The guard that catches scaffolding outliving its subsystem. + + `fable-mcp/**` sat in this list for three months after the directory was + deleted (commit 91bafb6, 2026-05-27), and `assets/**` named a path that + never existed at all. Neither ever failed anything — a `paths:` entry + matching nothing simply never fires — which is precisely why a list kept + by hand drifts and nobody finds out. + """ + missing = [ + entry for entry in trigger_paths() + if not (ROOT / re.sub(r"/\*\*$", "", entry)).exists() + ] + assert not missing, ( + f"`paths:` names {missing}, which do not exist in the repo. A trigger " + f"that matches nothing is silent, so it survives every review." + ) + + +def test_every_deriver_exists(): + """§3's table, kept honest. + + The point of the table is that the next artifact is a one-line addition + (milestone 334 step 3 adds the plugin's mint script). A row pointing at a + file that has moved would make the table read as complete when it is not. + """ + for path, artifacts in DERIVERS.items(): + assert (ROOT / path).exists(), f"DERIVERS names {path}, which does not exist" + assert artifacts, f"DERIVERS[{path}] names no artifact" + + +# ── The exclusion — the half that makes the version check mean anything ──── + + +def manifest(**fields) -> str: + base = { + "name": "scribe", + "description": "d", + "version": "0.1.48", + "mcpServers": {"scribe": {"type": "http", "url": "${user_config.api_endpoint}/mcp"}}, + "userConfig": {"api_endpoint": {"type": "string"}}, + } + base.update(fields) + return json.dumps(base) + + +def test_a_version_only_change_is_NOT_a_content_change(): + """THE assertion. Without it the version check is self-satisfying: the + bump edits `plugin.json`, which lives inside `plugin/`, so the bump is its + own justification and every bump passes.""" + assert manifest_differs_beyond_version( + manifest(version="2026.09.01.0512"), manifest(version="0.1.48") + ) is False + + +def test_an_identical_manifest_is_not_a_change(): + assert manifest_differs_beyond_version(manifest(), manifest()) is False + + +@pytest.mark.parametrize("field,value", [ + ("userConfig", {"api_endpoint": {"type": "string", "title": "changed"}}), + ("mcpServers", {"scribe": {"type": "http", "url": "elsewhere"}}), + ("description", "a different description"), + ("name", "renamed"), +]) +def test_every_OTHER_manifest_field_still_demands_a_new_version(field, value): + """Why the exclusion is one FIELD and never the whole file. + + `plugin.json` carries description, mcpServers and userConfig alongside the + version, and all of them reach an install. Excluding the file wholesale + would mean a userConfig-only edit computes an unchanged version and never + refreshes — #2209 again, with a narrower trigger and the same silence. + """ + assert manifest_differs_beyond_version(manifest(**{field: value}), manifest()) is True + + +def test_reformatting_is_not_a_content_change(): + """Parsed objects, not text. Whitespace and key order are not content, and + a check that treated them as such would demand a version for a re-indent.""" + data = json.loads(manifest()) + reordered = {k: data[k] for k in reversed(list(data))} + assert manifest_differs_beyond_version( + json.dumps(reordered, indent=4), json.dumps(data, separators=(",", ":")) + ) is False + + +@pytest.mark.parametrize("bad", ["", "{not json", "[]", '"a string"', "null"]) +def test_unreadable_input_demands_a_new_version(bad): + """The conservative direction, chosen deliberately. + + A spurious bump costs one cache refresh. A missed one is #2209 — the fix + reaches the repo and stops there, and the only detector is a human saying + "I don't think it updated." + """ + assert manifest_differs_beyond_version(bad, manifest()) is True + assert manifest_differs_beyond_version(manifest(), bad) is True + + +def test_a_manifest_appearing_or_vanishing_is_a_change(): + """None means the file is absent at that ref — a real difference, and not + the same thing as unreadable.""" + assert manifest_differs_beyond_version(None, manifest()) is True + assert manifest_differs_beyond_version(manifest(), None) is True + + +# ── The reader that joins the exclusion to git ───────────────────────────── + + +def test_shipped_content_changed_reports_a_version_only_commit_as_unchanged(monkeypatch): + """End to end through the git seam, with git stubbed. + + The unit above proves the comparison; this proves it is actually WIRED to + the path that `check_version_bump` reads. A correct helper nobody calls + would leave the circular check exactly as it was. + """ + from scripts import check_plugin + + monkeypatch.setattr( + check_plugin, "_git", + lambda *a: (0, "plugin/.claude-plugin/plugin.json"), + ) + monkeypatch.setattr( + check_plugin, "manifest_text", + lambda ref=None: manifest(version="2026.09.01.0512" if ref is None else "0.1.48"), + ) + changed, paths = check_plugin.shipped_content_changed("origin/main") + assert changed is False + assert paths == ["plugin/.claude-plugin/plugin.json"] + + +def test_shipped_content_changed_reports_a_hook_edit_as_changed(monkeypatch): + """The guard against an exclusion that swallowed everything — a check that + can never fire is indistinguishable from one that is broken.""" + from scripts import check_plugin + + monkeypatch.setattr( + check_plugin, "_git", + lambda *a: (0, "plugin/hooks/scribe_session_context.sh"), + ) + changed, paths = check_plugin.shipped_content_changed("origin/main") + assert changed is True + assert paths == ["plugin/hooks/scribe_session_context.sh"] + + +def test_a_failed_diff_is_None_and_never_False(monkeypatch): + """Could-not-tell and nothing-changed must not collapse into one value. + + #2663 is the precedent: a read that failed inside a broad except reported + the same zero as a genuinely empty window, and every counter read zero for + weeks with nothing to distinguish the two. + """ + from scripts import check_plugin + + monkeypatch.setattr(check_plugin, "_git", lambda *a: (128, "fatal: bad revision")) + changed, paths = check_plugin.shipped_content_changed("origin/main") + assert changed is None + assert paths == []