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 == []