From 88c54399fe679c3827e81e604a86c31fbfb81385 Mon Sep 17 00:00:00 2001
From: Mathias <mthbqv@gmail.com>
Date: Sun, 23 Aug 2026 14:34:32 +0200
Subject: [PATCH] feat(doctor): fail loud on skills drift, wired to
 SessionStart
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Every failure this fleet actually hit was silent. 243 dangling symlinks were
invisible to git because **/.claude/skills/ is in the global gitignore. A
checkout 20 skills behind still resolved, so nothing complained. A loose
secret-hygiene.md loaded in no harness and errored in none. Two machines had
the library missing entirely. Exit status is the signal that was absent.

bin/doctor reports, with a non-zero exit:
  - dangling links, anywhere under the harness dirs
  - links that RESOLVE but land outside the active checkout — the dangerous
    case, because it works while serving stale content
  - skill-count mismatch between a harness and the checkout
  - stray entries where <name>/SKILL.md was expected
  - leftover per-skill Claude symlinks (plugin skills do not override
    same-named user skills, so leftovers mean everything loads twice)
  - divergence between the plugin copy and the copy the other harnesses link to

Wired as a SessionStart hook with --quiet, so a healthy host prints nothing and
a drifting one says so at the top of the next session. That beats a timer:
no launchd/systemd wiring, and it reaches every host that installs the plugin.
27ms on koala, no network, no git fetch.

Two design points found by the tests, both worth keeping:
  - The canonical checkout is whatever ~/.agents/skills resolves to, NOT the
    copy doctor runs from. As a plugin hook $0 lives in the plugin cache, which
    is legitimately a second copy; judging harness links against it flagged
    every one of them as "outside".
  - "cd X/.." resolves ".." lexically against the link path and lands back in
    ~/.agents. Resolve the link first, then take the parent.

tests/doctor_test.sh drives seven fixtures through $HOME and $DOCTOR_CHECKOUT,
so no test touches the real host. Each asserts both the non-zero exit and that
the report names the fault — an exit-code-only assertion would pass while the
message said nothing useful.

Bump-Type: minor

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BeAp5LTscnz2W7Ubt4eJ6m
---
 bin/doctor           | 185 +++++++++++++++++++++++++++++++++++++++++++
 hooks/hooks.json     |  12 +++
 tests/doctor_test.sh | 124 +++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100755 bin/doctor
 create mode 100644 tests/doctor_test.sh

diff --git a/bin/doctor b/bin/doctor
new file mode 100755
index 0000000..ceab5df
--- /dev/null
+++ b/bin/doctor
@@ -0,0 +1,185 @@
+#!/bin/sh
+# doctor — report skills-library drift on this host.
+#
+# Written because every failure this fleet actually hit was SILENT: 243
+# dangling symlinks hidden from git by the **/.claude/skills/ ignore rule, a
+# checkout that still resolved but was 20 skills behind, a loose .md that
+# loaded in no harness, and two machines where the library was simply absent.
+# None of it errored. Exit status here is the signal that was missing.
+#
+#   doctor            human report, non-zero exit on drift
+#   doctor --quiet    print only when something is wrong (SessionStart hook)
+#   doctor --json     machine-readable findings
+#
+# POSIX sh on purpose: it runs on macOS and Linux, before any toolchain is
+# guaranteed present, and inside a session-start hook where latency shows.
+# It does no network I/O and never shells out to git for a fetch.
+#
+# Test seams: $HOME drives harness paths, $DOCTOR_CHECKOUT names the checkout.
+
+set -u
+
+QUIET=0
+JSON=0
+for arg in "$@"; do
+  case "$arg" in
+    --quiet) QUIET=1 ;;
+    --json)  JSON=1 ;;
+    -h|--help)
+      sed -n '2,20p' "$0" | sed 's/^# \{0,1\}//'
+      exit 0 ;;
+    *) printf 'doctor: unknown option %s\n' "$arg" >&2; exit 2 ;;
+  esac
+done
+
+SELF_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
+SELF_CHECKOUT=$(CDPATH= cd -- "$SELF_DIR/.." && pwd)
+
+# The canonical checkout is whatever ~/.agents/skills resolves to — NOT the
+# copy doctor happens to be running from. When invoked as a plugin hook, $0
+# lives in the plugin cache, which is legitimately a second copy: judging the
+# harness links against it would flag every one of them as "outside".
+if [ -n "${DOCTOR_CHECKOUT:-}" ]; then
+  CHECKOUT="$DOCTOR_CHECKOUT"
+elif [ -d "$HOME/.agents/skills" ]; then
+  # Resolve the link FIRST, then take the parent. "cd X/.." resolves ".."
+  # lexically against the link path, which lands back in ~/.agents.
+  _resolved=$(CDPATH= cd -- "$HOME/.agents/skills" 2>/dev/null && pwd -P)
+  CHECKOUT=$(dirname -- "$_resolved")
+else
+  CHECKOUT="$SELF_CHECKOUT"
+fi
+SKILLS_DIR="$CHECKOUT/skills"
+
+findings=0
+report=""
+
+# add <severity> <check> <detail>
+add() {
+  findings=$((findings + 1))
+  if [ "$JSON" -eq 1 ]; then
+    report="${report}{\"severity\":\"$1\",\"check\":\"$2\",\"detail\":\"$(printf '%s' "$3" | sed 's/"/\\"/g')\"},"
+  else
+    report="${report}$1 $2: $3
+"
+  fi
+}
+
+# count_skills <dir> — directories containing a SKILL.md, one level down.
+count_skills() {
+  [ -d "$1" ] || { printf '0'; return; }
+  find "$1"/ -mindepth 2 -maxdepth 2 -name SKILL.md 2>/dev/null | wc -l | tr -d ' '
+}
+
+# --- the checkout itself ----------------------------------------------------
+if [ ! -d "$SKILLS_DIR" ]; then
+  add FAIL checkout "no skills/ directory at $CHECKOUT"
+  # Nothing downstream can be judged without it.
+  printf 'FAIL checkout: no skills/ directory at %s\n' "$CHECKOUT" >&2
+  exit 1
+fi
+
+EXPECTED=$(count_skills "$SKILLS_DIR")
+[ "$EXPECTED" -gt 0 ] || add FAIL checkout "skills/ holds no SKILL.md at all"
+
+# --- stray entries in the checkout -----------------------------------------
+# A loose foo.md where foo/SKILL.md was meant loads in NO harness and errors
+# in none either. That is exactly how secret-hygiene.md sat dead for months.
+for entry in "$SKILLS_DIR"/*; do
+  [ -e "$entry" ] || continue
+  name=$(basename "$entry")
+  if [ ! -d "$entry" ]; then
+    add FAIL stray "$SKILLS_DIR/$name is not a directory — it loads nowhere"
+  elif [ ! -f "$entry/SKILL.md" ]; then
+    add FAIL stray "$SKILLS_DIR/$name has no SKILL.md"
+  fi
+done
+
+# --- harness wiring ---------------------------------------------------------
+# Canonical location first; Pi reads it directly and the others link to it.
+for dir in \
+  "$HOME/.agents/skills" \
+  "$HOME/.config/crush/skills" \
+  "$HOME/.config/opencode/skills"
+do
+  [ -L "$dir" ] || [ -d "$dir" ] || continue
+
+  if [ -L "$dir" ] && [ ! -e "$dir" ]; then
+    add FAIL dangling "$dir -> $(readlink "$dir") (target does not exist)"
+    continue
+  fi
+
+  # Resolve to a physical path and confirm it lands inside THIS checkout.
+  # A link that resolves to a different checkout is the dangerous case: it
+  # works, so nothing complains, while serving stale content.
+  resolved=$(CDPATH= cd -- "$dir" 2>/dev/null && pwd -P)
+  canonical=$(CDPATH= cd -- "$SKILLS_DIR" 2>/dev/null && pwd -P)
+  if [ -n "$resolved" ] && [ "$resolved" != "$canonical" ]; then
+    add FAIL outside "$dir resolves to $resolved, outside the active checkout $canonical"
+    continue
+  fi
+
+  got=$(count_skills "$dir")
+  if [ "$got" != "$EXPECTED" ]; then
+    add FAIL count "$dir sees $got skills, checkout has $EXPECTED"
+  fi
+done
+
+# --- dangling links anywhere under the harness dirs -------------------------
+for dir in "$HOME/.agents/skills" "$HOME/.config/crush/skills" \
+           "$HOME/.config/opencode/skills" "$HOME/.claude/skills"
+do
+  [ -d "$dir" ] || continue
+  # Plain glob, not find|while: a while loop on the right of a pipe runs in a
+  # subshell, so add() would increment a counter that dies with it and doctor
+  # would exit 0 having found faults.
+  for link in "$dir"/*; do
+    [ -L "$link" ] || continue
+    [ -e "$link" ] && continue
+    add FAIL dangling "$link -> $(readlink "$link") (target does not exist)"
+  done
+done
+
+# --- Claude Code: plugin vs leftover per-skill symlinks ---------------------
+# Plugin skills do NOT override same-named user skills — both stay visible. So
+# leftover links from the pre-plugin era mean every skill appears twice.
+CLAUDE_SKILLS="$HOME/.claude/skills"
+if [ -d "$CLAUDE_SKILLS" ]; then
+  legacy=$(find "$CLAUDE_SKILLS"/ -maxdepth 1 -type l 2>/dev/null | wc -l | tr -d ' ')
+  if [ "$legacy" -gt 0 ]; then
+    add FAIL legacy "$CLAUDE_SKILLS holds $legacy per-skill symlink(s) from the pre-plugin wiring — remove them or every skill loads twice"
+  fi
+fi
+
+# Two delivery mechanisms means two copies that can drift apart: the plugin
+# (Claude Code) and the checkout the other harnesses link to. Same host,
+# different skill sets, no error — compare them.
+# Skipped when DOCTOR_CHECKOUT is set: that scopes doctor to an explicit
+# target, so comparing it against whatever copy the script lives in is
+# meaningless rather than informative.
+if [ -z "${DOCTOR_CHECKOUT:-}" ] && [ -f "$SELF_DIR/../.claude-plugin/plugin.json" ] \
+   && [ "$SELF_CHECKOUT" != "$CHECKOUT" ]; then
+  plugin_count=$(count_skills "$SELF_CHECKOUT/skills")
+  if [ "$plugin_count" != "$EXPECTED" ]; then
+    add FAIL drift "Claude Code plugin serves $plugin_count skills, the other harnesses see $EXPECTED — the two copies have diverged"
+  fi
+fi
+
+INSTALLED="$HOME/.claude/plugins/installed_plugins.json"
+if [ -f "$INSTALLED" ]; then
+  grep -q '"skills@mathias"' "$INSTALLED" 2>/dev/null \
+    || add WARN plugin "skills@mathias is not installed for Claude Code — /plugin marketplace add https://git.d-ma.be/mathias/skills.git"
+fi
+
+# --- output -----------------------------------------------------------------
+if [ "$JSON" -eq 1 ]; then
+  printf '{"checkout":"%s","expected":%s,"findings":[%s]}\n' \
+    "$CHECKOUT" "$EXPECTED" "${report%,}"
+elif [ "$findings" -gt 0 ]; then
+  printf 'skills doctor — %s finding(s), checkout %s\n\n%s\n' \
+    "$findings" "$CHECKOUT" "$report"
+elif [ "$QUIET" -eq 0 ]; then
+  printf 'skills doctor: clean — %s skills, checkout %s\n' "$EXPECTED" "$CHECKOUT"
+fi
+
+[ "$findings" -eq 0 ]
diff --git a/hooks/hooks.json b/hooks/hooks.json
index b9cf8e2..8db511f 100644
--- a/hooks/hooks.json
+++ b/hooks/hooks.json
@@ -1,5 +1,17 @@
 {
   "hooks": {
+    "SessionStart": [
+      {
+        "hooks": [
+          {
+            "type": "command",
+            "command": "sh \"${CLAUDE_PLUGIN_ROOT}/bin/doctor\" --quiet",
+            "timeout": 5,
+            "statusMessage": "Checking skills wiring..."
+          }
+        ]
+      }
+    ],
     "UserPromptSubmit": [
       {
         "hooks": [
diff --git a/tests/doctor_test.sh b/tests/doctor_test.sh
new file mode 100644
index 0000000..f18eb8d
--- /dev/null
+++ b/tests/doctor_test.sh
@@ -0,0 +1,124 @@
+#!/bin/sh
+# Tests for bin/doctor.
+#
+# Every case here is a real state this fleet was found in on 2026-08-23, and
+# every one of them was SILENT — nothing errored, git status was clean, the
+# harness simply ran with the wrong skills or none at all. The point of doctor
+# is that none of these can be silent again, so each fixture asserts both a
+# non-zero exit AND that the report names the fault.
+#
+# Fixtures drive doctor through $HOME and $DOCTOR_CHECKOUT, so no test touches
+# the real host.
+
+set -u
+
+ROOT=$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd)
+DOCTOR="$ROOT/bin/doctor"
+
+pass=0
+fail=0
+WORK=${TMPDIR:-/tmp}/doctor_test.$$
+
+if [ ! -f "$DOCTOR" ]; then
+  printf 'FAIL: %s does not exist\n' "$DOCTOR"
+  exit 1
+fi
+
+cleanup() { rm -rf "$WORK"; }
+trap cleanup EXIT INT TERM
+
+# new_fixture <name> — builds $WORK/<name>/{ck,home} with a 2-skill checkout
+# and an empty fake HOME. Echoes the fixture root.
+new_fixture() {
+  _f="$WORK/$1"
+  mkdir -p "$_f/ck/skills/alpha" "$_f/ck/skills/beta" "$_f/home"
+  printf -- '---\nname: alpha\ndescription: a\n---\n' > "$_f/ck/skills/alpha/SKILL.md"
+  printf -- '---\nname: beta\ndescription: b\n---\n' > "$_f/ck/skills/beta/SKILL.md"
+  printf '%s' "$_f"
+}
+
+# wire_clean <fixture> — the correct wiring: one canonical link, harnesses
+# pointing at it.
+wire_clean() {
+  mkdir -p "$1/home/.agents" "$1/home/.config/crush" "$1/home/.config/opencode"
+  ln -sfn "$1/ck/skills" "$1/home/.agents/skills"
+  ln -sfn "$1/home/.agents/skills" "$1/home/.config/crush/skills"
+  ln -sfn "$1/home/.agents/skills" "$1/home/.config/opencode/skills"
+}
+
+# assert_fails <fixture> <substring> <label>
+# Note: doctor must be invoked directly here, not via a helper called in a
+# command substitution — $? from inside $( ) belongs to the subshell and the
+# exit status, which is the whole point of these assertions, would be lost.
+assert_fails() {
+  _out=$(HOME="$1/home" DOCTOR_CHECKOUT="$1/ck" sh "$DOCTOR" 2>&1); rc=$?
+  if [ "$rc" -eq 0 ]; then
+    fail=$((fail + 1))
+    printf '  FAIL %s\n       doctor exited 0; drift went undetected\n' "$3"
+  elif ! printf '%s' "$_out" | grep -qi -- "$2"; then
+    fail=$((fail + 1))
+    printf '  FAIL %s\n       exited %s but report never mentions "%s"\n       got: %s\n' "$3" "$rc" "$2" "$_out"
+  else
+    pass=$((pass + 1))
+    printf '  ok   %s\n' "$3"
+  fi
+}
+
+assert_clean() {
+  _out=$(HOME="$1/home" DOCTOR_CHECKOUT="$1/ck" sh "$DOCTOR" 2>&1); rc=$?
+  if [ "$rc" -eq 0 ]; then
+    pass=$((pass + 1)); printf '  ok   %s\n' "$2"
+  else
+    fail=$((fail + 1)); printf '  FAIL %s\n       exited %s on a healthy fixture\n       got: %s\n' "$2" "$rc" "$_out"
+  fi
+}
+
+# --- 1. healthy host stays silent -------------------------------------------
+f=$(new_fixture clean); wire_clean "$f"
+assert_clean "$f" 'clean fixture exits 0'
+
+# --- 2. dangling link (the 243-link case, gitignored and invisible) ----------
+f=$(new_fixture dangling); wire_clean "$f"
+ln -sfn "$f/ck/skills-that-moved" "$f/home/.agents/skills"
+assert_fails "$f" 'dangling' 'dangling canonical link is reported'
+
+# --- 3. resolves, but to a DIFFERENT checkout (the silent-stale case) --------
+# Worse than dangling: it works, so nothing complains, while serving content
+# from a checkout that is months behind.
+f=$(new_fixture stale)
+mkdir -p "$f/other/skills/alpha" "$f/other/skills/beta"
+printf -- '---\nname: alpha\ndescription: a\n---\n' > "$f/other/skills/alpha/SKILL.md"
+printf -- '---\nname: beta\ndescription: b\n---\n' > "$f/other/skills/beta/SKILL.md"
+wire_clean "$f"
+ln -sfn "$f/other/skills" "$f/home/.agents/skills"
+assert_fails "$f" 'outside' 'link into a foreign checkout is reported'
+
+# --- 4. count mismatch (a harness seeing fewer skills than the checkout) -----
+f=$(new_fixture count)
+mkdir -p "$f/home/.config/crush/skills/alpha"
+printf -- '---\nname: alpha\ndescription: a\n---\n' > "$f/home/.config/crush/skills/alpha/SKILL.md"
+mkdir -p "$f/home/.agents"
+ln -sfn "$f/ck/skills" "$f/home/.agents/skills"
+assert_fails "$f" 'count' 'skill-count mismatch is reported'
+
+# --- 5. stray entry (the loose secret-hygiene.md that loaded nowhere) --------
+f=$(new_fixture stray); wire_clean "$f"
+printf 'orphan\n' > "$f/ck/skills/loose-note.md"
+assert_fails "$f" 'stray' 'stray non-directory entry is reported'
+
+# --- 6. legacy Claude symlinks alongside the plugin (duplicate skills) -------
+f=$(new_fixture dupe); wire_clean "$f"
+mkdir -p "$f/home/.claude/skills"
+ln -sfn "$f/ck/skills/alpha" "$f/home/.claude/skills/alpha"
+assert_fails "$f" 'legacy' 'leftover per-skill Claude symlinks are reported'
+
+# --- 7. dangling link inside a harness dir (not the canonical link itself) --
+# Exercises the per-entry scan rather than the canonical-link check: this is
+# the shape the 243 dead links actually had.
+f=$(new_fixture ghost); wire_clean "$f"
+mkdir -p "$f/home/.claude/skills"
+ln -sfn "$f/ck/skills/vanished" "$f/home/.claude/skills/vanished"
+assert_fails "$f" 'dangling' 'dangling link inside a harness dir is reported'
+
+printf '\ndoctor: %d passed, %d failed\n' "$pass" "$fail"
+[ "$fail" -eq 0 ]