From 51e5c22818952ef89a1c30d3637b47143b70311f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 28 Jul 2026 20:56:15 -0400 Subject: [PATCH 1/4] =?UTF-8?q?ci(plugin):=20gate=20plugin/**=20=E2=80=94?= =?UTF-8?q?=20the=20path=20that=20shipped=20two=20live=20defects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plugin/` is not built into the image; installs fetch it from this repo via .claude-plugin/marketplace.json, so a push IS the release. It was absent from the workflow's `paths:` filter entirely, meaning plugin changes ran no CI at all. Two separate defects reached a live install through that gap: #2198 — all four hook scripts inert (lowercase userConfig env vars, line-oriented `jq -rR`, line-oriented `cut -c`) #2209 — the fix for #2198 couldn't reach an install because the manifest version wasn't bumped, so the installer never refreshed its cache Adds `plugin/**` + `.claude-plugin/**` to `paths:` and a `plugin` job running scripts/check_plugin.py: 1. `bash -n` on every hook. 2. The three known-bad patterns from #2198. Verified by replay against c569cdd^ — all three are caught. Narrow by design; see below. 3. Shipped plugin content differs from origin/main => the manifest version must differ too. Stated against the base branch, not per-commit, so a batch needs one bump rather than one per commit. Replayed against c569cdd: correctly fails. The checker found a real outstanding bug on its first run: the `cut -c1-2000` prompt cap in scribe_autoinject.sh was still line-oriented. Only the prior-art hook's copy got fixed in c569cdd. Now `head -c`. It then failed on this very commit for a missing version bump, which is the third time that rule has mattered and the first time something other than memory enforced it. Manifest bumped to 0.1.20. WHAT THIS DOESN'T COVER, and why. shellcheck is the right tool for check 2 and is NOT in ci-python; nor is jq, which every hook requires and silently bails without — so a "runs and stays silent" smoke test would pass vacuously today and prove nothing. Both need those two packages added to the CI image in the CI-runner repo, which is a separate change to a separate repo (rule #5: the toolchain comes from the image, not from apt-get at job start). Verified against CI-runner's Dockerfile and scripts/install-common.sh rather than assumed (rule #37). `plugin` is not in the build job's `needs`: the plugin doesn't ship in the image, and blocking the build wouldn't un-publish a bad hook — the push already did. A failed job still reddens the run. Closes #2204 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- .forgejo/workflows/ci.yml | 43 ++++++ plugin/.claude-plugin/plugin.json | 2 +- plugin/hooks/scribe_autoinject.sh | 6 +- scripts/check_plugin.py | 235 ++++++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 2 deletions(-) create mode 100755 scripts/check_plugin.py diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 7e2a5ec..a07e6e0 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -48,6 +48,15 @@ on: - "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 + # plugin changes triggered no workflow at all, which is how #2198's three + # broken hooks and then #2209's missing version bump both reached a live + # install. See the `plugin` job below. + - "plugin/**" + - ".claude-plugin/**" + - "scripts/check_plugin.py" - ".forgejo/workflows/ci.yml" # Manual trigger from the Forgejo Actions UI. Useful when an image has # been built but the deployment didn't pick it up, or when re-running @@ -102,6 +111,34 @@ jobs: run: npx vue-tsc --noEmit working-directory: frontend + # Guards the one part of this repo that ships to users without a build step. + # See scripts/check_plugin.py for what it checks and, as importantly, what it + # can't check yet (shellcheck and jq are absent from ci-python). + plugin: + name: Plugin hooks + if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v') + runs-on: python-ci + container: + image: git.fabledsword.com/bvandeusen/ci-python:3.14 + steps: + - uses: actions/checkout@v6 + with: + # The version-bump check diffs shipped plugin content against + # origin/main, so it needs history — a shallow clone can't resolve it + # and the check would fail loudly rather than pass blind. + fetch-depth: 0 + + # On main the comparison is against itself, so only the syntax and + # pattern checks are meaningful there. + - name: Check plugin hooks and manifest + run: | + if [ "${{ github.ref }}" = "refs/heads/main" ]; then + python3 scripts/check_plugin.py --no-version + else + git fetch --no-tags origin main:refs/remotes/origin/main + python3 scripts/check_plugin.py + fi + lint: name: Python lint if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v') @@ -228,6 +265,12 @@ jobs: build: name: Build & push image + # `plugin` is deliberately NOT in needs. The plugin isn't in the image — + # installs fetch it from git — so gating the server image on a hook lint + # would couple two things that don't ship together, and blocking the build + # wouldn't un-publish a bad hook anyway: the push already did that. A failed + # `plugin` job still turns the whole run red, which is the signal that + # matters. needs: [typecheck, lint, test] # Build on dev, main, and v* tag pushes. dev → :dev, main → :latest, # tag → :latest + :; every build also gets an immutable :. diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index ea6040f..4b3cdd2 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -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.19", + "version": "0.1.20", "author": { "name": "Bryan Van Deusen" }, "mcpServers": { "scribe": { diff --git a/plugin/hooks/scribe_autoinject.sh b/plugin/hooks/scribe_autoinject.sh index a182936..599f30e 100755 --- a/plugin/hooks/scribe_autoinject.sh +++ b/plugin/hooks/scribe_autoinject.sh @@ -44,7 +44,11 @@ case "$token" in *'${'*) token="" ;; esac [ -n "$url" ] && [ -n "$token" ] || exit 0 # Cap the query length — a giant prompt makes a giant URL for no extra signal. -q=$(printf '%s' "$prompt" | cut -c1-2000) +# `head -c`, not `cut -c1-2000`: cut is line-oriented and caps EACH LINE, so a +# long multi-line prompt sailed past the budget entirely. Same defect as the +# prior-art hook's code cap; this copy was missed when that one was fixed, and +# scripts/check_plugin.py caught it. +q=$(printf '%s' "$prompt" | head -c 2000) # `-sRr`, not `-rR`: jq -R reads LINE BY LINE, so a multi-line prompt encoded as # several lines joined by raw newlines and the request died. Single-line prompts # worked, which is why this looked healthy — the long, substantial prompts most diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py new file mode 100755 index 0000000..f535c36 --- /dev/null +++ b/scripts/check_plugin.py @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +"""Guards for `plugin/` — the one part of this repo that ships straight to users. + +WHY THIS EXISTS. `plugin/` is not built into the Docker image. Installs fetch it +from this git repo via `.claude-plugin/marketplace.json`, so a push IS the +release for plugin content: no build, no gate, immediately fetchable. Two +separate defects have reached a live install through that path: + + - #2198 — all four hook scripts were inert (wrong env-var case, line-oriented + `jq -rR`, line-oriented `cut -c`). No CI ran, because `plugin/**` wasn't in + the workflow's `paths:` filter at all. + - #2209 — the fix for #2198 shipped to `main` and still couldn't reach an + install, because `plugin.json`'s version wasn't bumped and the installer + compares versions to decide whether to refresh its cache. + +The rule for the second one was already written down and was still missed. A +written rule that depends on being remembered during a long session is not a +control; this is. + +WHAT IT DOESN'T COVER. shellcheck would be the right tool for the pattern +checks below and is NOT in `ci-python` (verified against CI-runner's Dockerfile ++ scripts/install-common.sh, not from memory — rule #37). Neither is `jq`, which +every hook requires and bails without, so a "runs and stays silent" smoke test +would pass vacuously today and prove nothing. Both need the image to grow those +two packages; until then this checks the specific bug classes that actually bit, +which is narrower than shellcheck but not nothing. + +Usage: + python3 scripts/check_plugin.py # all checks + python3 scripts/check_plugin.py --no-version # skip the bump check +""" +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +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") + +failures: list[str] = [] + + +def fail(msg: str) -> None: + failures.append(msg) + print(f"FAIL {msg}") + + +def ok(msg: str) -> None: + print(f"ok {msg}") + + +def hook_scripts() -> list[Path]: + return sorted(HOOKS_DIR.glob("*.sh")) + + +# --- syntax ---------------------------------------------------------------- + +def check_syntax() -> None: + """`bash -n` every hook. Catches nothing subtle, costs nothing, and a syntax + error here means a hook that silently never runs.""" + for script in hook_scripts(): + proc = subprocess.run( + ["bash", "-n", str(script)], capture_output=True, text=True + ) + if proc.returncode != 0: + fail(f"{script.relative_to(ROOT)}: bash -n — {proc.stderr.strip()}") + else: + ok(f"{script.relative_to(ROOT)}: syntax") + + +# --- known-bad patterns ---------------------------------------------------- + +# Each entry: (compiled pattern, short label, why it's wrong). +# These are the exact classes from #2198. They are deliberately specific — a +# broad shell linter belongs in the image, not hand-rolled here. +PATTERNS: list[tuple[re.Pattern, str, str]] = [ + ( + re.compile(r"CLAUDE_PLUGIN_OPTION_[a-z]"), + "lowercase userConfig env var", + "Claude Code exports userConfig to hooks as CLAUDE_PLUGIN_OPTION_ " + "with the key UPPERCASED. The lowercase spelling reads as empty and the " + "hook then does nothing, silently.", + ), + ( + # -R without -s: reads input line by line, so a multi-line payload is + # encoded per line and joined with raw newlines. The class is a-r + t-z + # (i.e. every letter EXCEPT `s`) so `-rR` is caught and `-sRr` is not — + # an earlier a-q spelling silently excluded `r` and missed the real + # defect, which is exactly the flag combination that shipped. + re.compile(r"jq\s+-(?:[a-rt-zA-Z]*R[a-rt-zA-Z]*)\s"), + "line-oriented jq -R", + "jq -R reads input LINE BY LINE. Encoding a multi-line payload that way " + "produces separate encoded lines joined by raw newlines — an invalid " + "URL. Use -s (slurp) as well, e.g. `jq -sRr '@uri'`.", + ), + ( + re.compile(r"\|\s*cut\s+-c"), + "line-oriented cut for a payload cap", + "cut -c truncates EACH LINE, so it does not cap total size. Use " + "`head -c N` to bound a payload.", + ), +] + + +def check_patterns() -> None: + for script in hook_scripts(): + text = script.read_text(encoding="utf-8", errors="replace") + rel = script.relative_to(ROOT) + hits = 0 + for line_no, line in enumerate(text.splitlines(), 1): + # A line that only *documents* the trap is fine — several hooks now + # carry a comment naming the wrong form so the next reader knows. + if line.lstrip().startswith("#"): + continue + for pattern, label, why in PATTERNS: + if pattern.search(line): + hits += 1 + fail(f"{rel}:{line_no}: {label}\n {line.strip()}\n {why}") + if not hits: + ok(f"{rel}: no known-bad patterns") + + +# --- the version bump ------------------------------------------------------ + +def _git(*args: str) -> tuple[int, str]: + proc = subprocess.run( + ["git", *args], capture_output=True, text=True, cwd=ROOT + ) + 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.""" + if ref is None: + try: + return json.loads(MANIFEST.read_text()).get("version") + except Exception: + return None + rel = MANIFEST.relative_to(ROOT).as_posix() + code, out = _git("show", f"{ref}:{rel}") + if code != 0: + return None + try: + return json.loads(out).get("version") + except Exception: + return None + + +def check_version_bump(base: str = "origin/main") -> None: + """If shipped plugin content differs from `base`, the version must too. + + Stated against the BASE BRANCH rather than the last commit on purpose. A + per-commit rule would demand a bump from every commit in a batch; what + 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. + """ + code, _ = _git("rev-parse", "--verify", base) + if code != 0: + # Do NOT pass silently — a check that quietly no-ops is how this class + # of bug survives in the first place. + fail( + f"cannot resolve {base}, so the version-bump check could not run. " + f"Fetch it (checkout needs fetch-depth: 0) or pass --no-version " + f"deliberately." + ) + return + + code, changed = _git("diff", "--name-only", base, "--", *SHIPPED) + if code != 0: + fail(f"git diff against {base} failed: {changed}") + return + if not changed.strip(): + ok(f"no shipped plugin changes against {base} — version bump not required") + return + + here, there = manifest_version(), manifest_version(base) + if here is None: + fail(f"could not read a version from {MANIFEST.relative_to(ROOT)}") + return + if there is None: + ok(f"no manifest on {base} — treating as a new plugin (version {here})") + return + if here == there: + files = "\n ".join(changed.splitlines()) + fail( + f"plugin content changed but the manifest version is still {here}.\n" + f" The installer compares versions to decide whether to refresh " + f"its cache, so an unchanged version means these edits reach the repo " + f"and stop there — the marketplace clone updates, the cache that " + f"actually executes does not (issue #2209).\n" + f" Bump `version` in {MANIFEST.relative_to(ROOT)}.\n" + f" Changed:\n {files}" + ) + else: + ok(f"plugin content changed and version moved {there} -> {here}") + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--no-version", action="store_true", + help="skip the manifest version-bump check") + parser.add_argument("--base", default="origin/main", + help="branch the version bump is measured against") + args = parser.parse_args() + + if not HOOKS_DIR.is_dir(): + print(f"FAIL no hooks directory at {HOOKS_DIR}") + return 1 + + check_syntax() + check_patterns() + if not args.no_version: + check_version_bump(args.base) + + print() + if failures: + print(f"{len(failures)} problem(s) found.") + return 1 + print("All plugin checks passed.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From 1feef179d2fcfc125c79716dcfc5b30431a32b4d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 28 Jul 2026 21:00:14 -0400 Subject: [PATCH 2/4] ci(plugin): drop `with:` from the plugin job's checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 3027: the Plugin hooks job failed at checkout, before the script ran. Adding a `with: fetch-depth: 0` block made actions/checkout@v6 fail to extract on the act_runner — Cannot find module '/var/run/act/actions//dist/index.js' — while every bare `uses: actions/checkout@v6` in the same run succeeded. The runner's action-cache handling is the difference, not git. No depth was needed in the first place. The version check compares two TREES, and a tree diff needs both trees, not a common ancestor. Verified against a real depth-1 clone: after `git fetch --depth=1 origin main:refs/remotes/origin/main`, both `git diff origin/main -- plugin` and `git show origin/main:plugin/.claude-plugin/plugin.json` work. So the explicit fetch already in the step is sufficient, and cheaper than the full history the `with:` block was asking for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- .forgejo/workflows/ci.yml | 17 +++++++++-------- scripts/check_plugin.py | 5 +++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index a07e6e0..e851ac5 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -121,21 +121,22 @@ jobs: container: image: git.fabledsword.com/bvandeusen/ci-python:3.14 steps: + # Bare `uses:`, no `with:` block. Adding one made this action fail to + # extract on the act_runner ("Cannot find module .../dist/index.js") while + # every bare checkout in the same run succeeded — see run 3027. Nothing + # here needs `fetch-depth: 0` anyway: the version check diffs two trees, + # and a tree diff needs both trees, not a common ancestor. A depth-1 fetch + # of main's tip is enough, and cheaper. - uses: actions/checkout@v6 - with: - # The version-bump check diffs shipped plugin content against - # origin/main, so it needs history — a shallow clone can't resolve it - # and the check would fail loudly rather than pass blind. - fetch-depth: 0 - # On main the comparison is against itself, so only the syntax and - # pattern checks are meaningful there. + # On main the comparison would be against itself, so only the syntax and + # pattern checks mean anything there. - name: Check plugin hooks and manifest run: | if [ "${{ github.ref }}" = "refs/heads/main" ]; then python3 scripts/check_plugin.py --no-version else - git fetch --no-tags origin main:refs/remotes/origin/main + git fetch --no-tags --depth=1 origin main:refs/remotes/origin/main python3 scripts/check_plugin.py fi diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index f535c36..c67c92f 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -171,8 +171,9 @@ def check_version_bump(base: str = "origin/main") -> None: # of bug survives in the first place. fail( f"cannot resolve {base}, so the version-bump check could not run. " - f"Fetch it (checkout needs fetch-depth: 0) or pass --no-version " - f"deliberately." + f"Fetch it first — `git fetch --depth=1 origin main:refs/remotes/" + f"origin/main` is enough, since this diffs two trees and needs no " + f"common ancestor — or pass --no-version deliberately." ) return From dd878bc4987eb98c4561ff94a0e7923fb11ddca7 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 28 Jul 2026 22:39:55 -0400 Subject: [PATCH 3/4] ci(plugin): add shellcheck + jq, and the fail-open smoke test they unlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-job installs, not an image change. CI-runner's docs/process.md decision checkpoint is explicit: "If only one project needs the dep, prefer that project installing it per-job in their workflow — at least until a second consumer arrives." Scribe is the only consumer, and shellcheck is not a natural extension of a Python image's purpose. Promotion into ci-python is filed as an issue on CI-runner rather than assumed here — same doc, step 1: the maintainer's call goes in the issue, then the PR. This also corrects something I got wrong earlier in this work: I cited rule #5 as blocking a per-job install. Rule #5 is about language TOOLCHAINS via setup-* actions, not small lint utilities, and CI-runner's own process doc positively recommends per-job installs in exactly this case. jq is load-bearing rather than convenient. Every hook opens with `command -v jq || exit 0`, so without it a "runs and stays silent" smoke test passes while exercising nothing — a green tick proving less than no test at all. That is why the smoke test didn't ship with the first cut. The smoke test pins the fail-open contract: each hook, with no credentials and then against a refused connection, must exit 0. Three must also stay silent; scribe_session_context.sh must NOT, because its static behavioural floor is meant to survive having no credentials and no network — asserting silence there would encode the opposite of the design. Verified it can actually fail, rather than assuming: injected a non-zero exit and separately a stray stdout write, and confirmed each is caught. shellcheck and jq are both optional at runtime — missing either SKIPs its check loudly rather than passing. A check that quietly no-ops is the exact failure mode this file exists to prevent. ci-requirements.md updated: jq + shellcheck recorded under per-job installs (the input CI-runner's maintainer uses for the next promotion decision), plus two stale entries corrected — the sheet claimed four jobs when there are six, and listed `uv` as a per-job install when it has been in the image since the ci-python Dockerfile started pip-installing it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- .forgejo/workflows/ci.yml | 13 ++++ ci-requirements.md | 24 ++++--- scripts/check_plugin.py | 137 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 157 insertions(+), 17 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index e851ac5..fc4986c 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -129,6 +129,19 @@ jobs: # of main's tip is enough, and cheaper. - uses: actions/checkout@v6 + # Per-job, not in the image, per CI-runner's docs/process.md: "If only one + # project needs the dep, prefer that project installing it per-job in + # their workflow — at least until a second consumer arrives." Scribe is + # the only consumer today. Promotion into ci-python is filed as an issue + # on CI-runner rather than assumed here. + # + # jq is not optional for the smoke test: every hook exits at line 1 + # without it, so the check would pass while exercising nothing. + - name: Install shell tooling + run: | + apt-get update -qq + apt-get install -y -qq --no-install-recommends jq shellcheck + # On main the comparison would be against itself, so only the syntax and # pattern checks mean anything there. - name: Check plugin hooks and manifest diff --git a/ci-requirements.md b/ci-requirements.md index 3f7aad2..217dbf4 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -9,8 +9,9 @@ git.fabledsword.com/bvandeusen/ci-python:3.14 ``` -Used by all four jobs in `.forgejo/workflows/ci.yml`: typecheck (Vue/TS), -lint (ruff), test (pytest), build (docker buildx). +Used by all six jobs in `.forgejo/workflows/ci.yml`: typecheck (Vue/TS), +plugin (hook checks), lint (ruff), test (pytest), integration (pytest + +real Postgres), build (docker buildx). ## Image deps used @@ -18,6 +19,8 @@ lint (ruff), test (pytest), build (docker buildx). - node 24 (used for `npm ci` + `vue-tsc` in the typecheck job, and as the frontend builder stage inside the production `Dockerfile`) - ruff (lint job runs `ruff check src/` with zero install overhead) +- uv (test + integration jobs run `uv sync --locked`; installed in the + image since the ci-python Dockerfile started pip-installing it) - docker CLI + buildx (build job pushes the production image to the Forgejo registry) @@ -26,14 +29,15 @@ lint (ruff), test (pytest), build (docker buildx). Anything CI installs at job time that isn't in the image. Promotion candidates if more than one project needs them. -- `uv` — installed inline in the test job (`curl -LsSf - https://astral.sh/uv/install.sh | sh`). **Temporary**: belongs in the - ci-python image so every consumer doesn't re-install on cold start. - Tracked at [CI-Runner](https://git.fabledsword.com/bvandeusen/CI-runner). -- `http-ece` is `--no-build-isolation`-installed before the editable - package install because http-ece doesn't declare `setuptools` as a - build dep and uv creates bare venvs without it. Not promotion-worthy - (one project, one wheel). +- `jq` + `shellcheck` — apt-installed in the **plugin** job, which lints + the four Claude Code hook scripts and runs their fail-open smoke test. + Per `docs/process.md`'s decision checkpoint, single-consumer deps stay + per-job until a second consumer wants them; Scribe is the only one so + far. Both are small (jq ~1 MB, shellcheck ~20 MB) and would be + promotion candidates the moment another project lints shell. + **jq is load-bearing for the smoke test specifically**: every hook + starts with `command -v jq || exit 0`, so without it the test passes + while exercising nothing. ## Notes diff --git a/scripts/check_plugin.py b/scripts/check_plugin.py index c67c92f..9fc42e2 100755 --- a/scripts/check_plugin.py +++ b/scripts/check_plugin.py @@ -17,13 +17,18 @@ The rule for the second one was already written down and was still missed. A written rule that depends on being remembered during a long session is not a control; this is. -WHAT IT DOESN'T COVER. shellcheck would be the right tool for the pattern -checks below and is NOT in `ci-python` (verified against CI-runner's Dockerfile -+ scripts/install-common.sh, not from memory — rule #37). Neither is `jq`, which -every hook requires and bails without, so a "runs and stays silent" smoke test -would pass vacuously today and prove nothing. Both need the image to grow those -two packages; until then this checks the specific bug classes that actually bit, -which is narrower than shellcheck but not nothing. +shellcheck and jq are NOT in `ci-python` (verified against CI-runner's Dockerfile +and scripts/install-common.sh, not from memory — rule #37). CI installs both +per-job, which is what CI-runner's own docs/process.md prescribes for a dep with +a single consumer: "If only one project needs the dep, prefer that project +installing it per-job in their workflow — at least until a second consumer +arrives." Promotion into the image is filed as an issue there rather than +assumed here. + +Both are optional at runtime: without shellcheck the lint step is SKIPPED and +says so, and without jq the smoke test is skipped. A skipped check announces +itself loudly, because a check that quietly no-ops is the failure mode this +whole file exists to prevent. Usage: python3 scripts/check_plugin.py # all checks @@ -33,7 +38,9 @@ from __future__ import annotations import argparse import json +import os import re +import shutil import subprocess import sys from pathlib import Path @@ -59,6 +66,12 @@ def ok(msg: str) -> None: print(f"ok {msg}") +def skip(msg: str) -> None: + # Loud on purpose. A check that quietly does nothing is indistinguishable + # from a check that passed — the exact confusion that let #2198 survive. + print(f"SKIP {msg}") + + def hook_scripts() -> list[Path]: return sorted(HOOKS_DIR.glob("*.sh")) @@ -132,6 +145,114 @@ def check_patterns() -> None: # --- the version bump ------------------------------------------------------ +# --- shellcheck ------------------------------------------------------------ + +def check_shellcheck() -> None: + """Real shell linting, where available. + + The hand-rolled patterns above only know the bugs that already happened. + This is what catches the next one. + """ + exe = shutil.which("shellcheck") + if not exe: + skip("shellcheck not installed — install it to lint the hooks properly") + return + for script in hook_scripts(): + proc = subprocess.run( + [exe, "--severity=warning", "--shell=bash", str(script)], + capture_output=True, text=True, + ) + rel = script.relative_to(ROOT) + if proc.returncode != 0: + fail(f"{rel}: shellcheck\n{proc.stdout.strip()}") + else: + ok(f"{rel}: shellcheck") + + +# --- the fail-open contract ------------------------------------------------ + +# Every hook promises never to break the operator's session: unconfigured or +# unreachable, it exits 0. Three of them additionally promise SILENCE, because +# they are pure enrichment. scribe_session_context.sh is the exception by +# design — it always emits a static behavioural floor that needs no credentials +# and no network, so "silent" would be the wrong assertion for it. +# +# This is the contract that made #2198 invisible for weeks, so it is worth +# pinning: the bug and the healthy no-results case look identical from outside. +# Pinning it does NOT make the failure visible; it makes sure the fail-open +# behaviour is deliberate rather than accidental. +SMOKE_EVENTS: dict[str, str] = { + "scribe_autoinject.sh": json.dumps( + {"session_id": "smoke", "cwd": ".", "prompt": "a multi-line\nprompt\nhere"} + ), + "scribe_prior_art.sh": json.dumps( + {"session_id": "smoke", "cwd": ".", "tool_name": "Edit", + "tool_input": {"file_path": "src/x.py", "new_string": "def f():\n pass\n"}} + ), + "scribe_sync_processes.sh": json.dumps({"source": "startup"}), + "scribe_session_context.sh": json.dumps({"source": "startup"}), +} + +# The one hook that legitimately produces output with no credentials. +STATIC_FLOOR = "scribe_session_context.sh" + + +def _run_hook(script: Path, event: str, env_extra: dict[str, str]) -> subprocess.CompletedProcess: + env = {k: v for k, v in os.environ.items() + if not k.startswith(("SCRIBE_", "CLAUDE_PLUGIN_OPTION_"))} + env.update(env_extra) + return subprocess.run( + ["bash", str(script)], input=event, capture_output=True, text=True, + env=env, timeout=30, + ) + + +def check_fail_open() -> None: + if not shutil.which("jq"): + # Without jq every hook bails at its first line, so this would pass + # while exercising nothing. Say so rather than bank a green tick. + skip("jq not installed — the hooks would exit at line 1, so this " + "check would pass without testing anything") + return + + scenarios = [ + ("unconfigured", {}), + # Connection refused immediately — exercises the unreachable-instance + # path without waiting on a real network timeout. + ("unreachable", {"SCRIBE_URL": "http://127.0.0.1:1", "SCRIBE_TOKEN": "x"}), + ] + for script in hook_scripts(): + rel = script.relative_to(ROOT) + event = SMOKE_EVENTS.get(script.name) + if event is None: + skip(f"{rel}: no smoke event defined") + continue + for label, env_extra in scenarios: + try: + proc = _run_hook(script, event, env_extra) + except subprocess.TimeoutExpired: + fail(f"{rel} [{label}]: hung — a hook must never block a session") + continue + if proc.returncode != 0: + fail(f"{rel} [{label}]: exited {proc.returncode}, must be 0 — " + f"a recall aid may never fail the operator's action") + continue + out = proc.stdout.strip() + if script.name == STATIC_FLOOR: + # Emits its bundled static tier regardless; that floor is the + # whole point of the two-tier design. + if not out: + fail(f"{rel} [{label}]: emitted nothing — the static " + f"behavioural floor must survive having no credentials") + else: + ok(f"{rel} [{label}]: exit 0, static floor present") + elif out: + fail(f"{rel} [{label}]: emitted output with no working instance:\n" + f" {out[:200]}") + else: + ok(f"{rel} [{label}]: exit 0, silent") + + def _git(*args: str) -> tuple[int, str]: proc = subprocess.run( ["git", *args], capture_output=True, text=True, cwd=ROOT @@ -221,6 +342,8 @@ def main() -> int: check_syntax() check_patterns() + check_shellcheck() + check_fail_open() if not args.no_version: check_version_bump(args.base) From b0a0bf8abdf10f62eaf17bf62af75823b8a3084e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 28 Jul 2026 22:43:30 -0400 Subject: [PATCH 4/4] fix(plugin): justify the one shellcheck finding (SC1007, false positive) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First CI run with shellcheck (run 3029) flagged exactly one thing: scribe_session_context.sh:44 SC1007 Remove space after = if trying to assign a value here=$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) `CDPATH= cd` is the deliberate POSIX idiom for running a single command with CDPATH empty — it stops `cd` resolving through the operator's CDPATH and echoing the resolved path into our stdout, which for a hook whose stdout IS its protocol would be a real bug. shellcheck cannot distinguish that from a typo'd `CDPATH=cd`, so this is a false positive. Scoped `# shellcheck disable=SC1007` with the reason above it, matching how CI-runner's own scripts/install-common.sh handles SC2086. One line-scoped disable, no file-level or blanket suppression — a lint you silence broadly stops being a lint. Everything else in that run passed, including the parts that could only run once jq was installed: all four hooks exit 0 and stay silent unconfigured and against a refused connection, with the session-context hook correctly still emitting its static floor. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- plugin/hooks/scribe_session_context.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 9cfec0c..4c6b52c 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -41,6 +41,11 @@ set -uo pipefail 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` +# with CDPATH empty, so an operator whose CDPATH happens to contain a matching +# directory name can't send us somewhere else — and `cd` won't echo the resolved +# path into our output. shellcheck can't tell that idiom from `CDPATH=cd`. +# shellcheck disable=SC1007 here=$(CDPATH= cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) || exit 0 # SessionStart delivers a JSON event on stdin; `source` is startup|resume|compact|clear.