.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

scripts/bump_fable_mcp_version.sh

@@ -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"

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 "

scripts/pre_commit_fable_mcp.sh

@@ -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

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