fix(checkpoints): the two that never ran, and the scripts nobody tested

.github/workflows/tests.yml

@@ -1,8 +1,8 @@
 name: Tests
 
-# The repo's other workflows are thin callers of reusables; none of them ran
-# anything under tests/, so the test files were never executed on a PR. A test
-# nothing runs is not a gate.
+# Thin caller for the skill-repo-skill reusable. This workflow used to inline
+# the whole runner; the reusable now carries it, so every skill repo runs its
+# tests the same way and a fix reaches all of them at once.
 
 on:
   push:
@@ -13,46 +13,6 @@ permissions: {}
 
 jobs:
   tests:
-    name: Test suite
-    runs-on: ubuntu-latest
+    uses: netresearch/skill-repo-skill/.github/workflows/tests.yml@main
     permissions:
       contents: read
-    steps:
-      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
-        with:
-          # Nothing after checkout talks to the remote; leaving the token in
-          # .git/config only exposes it to later steps and artifacts.
-          persist-credentials: false
-      # Every loop below asserts it matched something. With nullglob a moved or
-      # renamed tests/ directory makes the glob expand to nothing and the job
-      # goes green having run zero tests — the failure this workflow exists to
-      # prevent, wearing a passing check.
-      - name: Shell tests
-        run: |
-          shopt -s nullglob globstar
-          files=(tests/**/*.sh)
-          if [ ${#files[@]} -eq 0 ]; then
-            echo "::error::no shell tests matched tests/**/*.sh — did the suite move?"
-            exit 1
-          fi
-          for t in "${files[@]}"; do
-            echo "::group::$t"
-            bash "$t"
-            echo "::endgroup::"
-          done
-      # The Python tests here are standalone scripts with a main() and a
-      # non-zero exit on failure ("Run: python3 tests/<file>"), not pytest
-      # modules — pytest collects nothing from them and exits 5.
-      - name: Python tests
-        run: |
-          shopt -s nullglob globstar
-          files=(tests/**/*.py)
-          if [ ${#files[@]} -eq 0 ]; then
-            echo "::error::no python tests matched tests/**/*.py — did the suite move?"
-            exit 1
-          fi
-          for t in "${files[@]}"; do
-            echo "::group::$t"
-            python3 "$t"
-            echo "::endgroup::"
-          done

skills/git-workflow/checkpoints.yaml

@@ -128,16 +128,19 @@ mechanical:
       see references/git-hooks-setup.md 'CaptainHook + git worktrees'
 
   # === UNRELEASED COMMITS ===
-  - id: GW-15
-    type: command
-    pattern: 'tag=$(git describe --tags --abbrev=0 2>/dev/null); [ -z "$tag" ] || test "$(git rev-list ${tag}..HEAD --count)" -le 20'
-    severity: warning
-    desc: "Main branch should not accumulate >20 unreleased commits since last tag"
+  # GW-15 was removed rather than fixed: counting commits since the last tag
+  # needs a rev-range (`<tag>..HEAD`) and command substitution, and the runner's
+  # allowlist rejects both — `..` as path traversal, `$(` as command chaining.
+  # It never ran. The rule itself is intact and now lives where a full shell is
+  # available, in scripts/verify-git-workflow.sh ("Unreleased Commits").
 
   # === INTERMEDIATE PLANNING ARTIFACTS ===
+  # `! … | grep -q .` rather than `test -z "$(…)"`: same verdict, no command
+  # substitution. The runner strips a leading `!` before checking the base
+  # command, so the negated pipeline is allowlist-conform.
   - id: GW-16
     type: command
-    pattern: 'test -z "$(git ls-files -- docs/superpowers/ claudedocs/ docs/working/ 2>/dev/null)"'
+    pattern: '! git ls-files -- docs/superpowers/ claudedocs/ docs/working/ | grep -q .'
     severity: warning
     desc: >-
       Intermediate planning artifacts (superpowers specs/plans, claudedocs,
@@ -190,6 +193,7 @@ llm_reviews:
     severity: warning
     desc: "Recent commits should follow conventional commit format"
 
+  # mechanical-counterpart: GW-17
   - id: GW-21
     domain: git-workflow
     prompt: |
@@ -353,6 +357,8 @@ llm_reviews:
       directory, no `.bare/` sibling) — the convention does not apply.
 
   # === STAGING BRANCH COMPOSER.LOCK MERGE STRATEGY ===
+  # mechanical-counterpart: none (the commands gather the staging lockfile; the
+  # judgement — which merge strategy applies — is the checkpoint)
   - id: GW-30
     domain: git-workflow
     severity: warning

skills/git-workflow/scripts/verify-git-workflow.sh

@@ -205,6 +205,23 @@ if [[ -f "package.json" ]]; then
     fi
 fi
 
+# Unreleased commits since the last tag. This was checkpoint GW-15, which the
+# runner's allowlist rejected outright — a rev-range needs `<tag>..HEAD` and the
+# count needs command substitution, and `..` and `$(` are both refused. It never
+# ran once. Here a full shell is available, so the rule survives intact.
+LAST_TAG=$(git describe --tags --abbrev=0 2>/dev/null || true)
+if [[ -n "$LAST_TAG" ]]; then
+    UNRELEASED=$(git rev-list "${LAST_TAG}..HEAD" --count 2>/dev/null || echo 0)
+    if [[ "$UNRELEASED" -le 20 ]]; then
+        echo "✅ $UNRELEASED commit(s) since $LAST_TAG"
+    else
+        echo "⚠️  $UNRELEASED commits since $LAST_TAG — cut a release"
+        WARNINGS=$((WARNINGS + 1))
+    fi
+else
+    echo "ℹ️  No tags yet"
+fi
+
 # Check current branch
 echo ""
 echo "=== Current State ==="
@@ -222,17 +239,23 @@ fi
 # Check if up to date with remote
 if git remote | grep -q "origin" 2>/dev/null; then
     git fetch origin --quiet 2>/dev/null || true
-    LOCAL=$(git rev-parse "$CURRENT_BRANCH" 2>/dev/null)
-    REMOTE=$(git rev-parse "origin/$CURRENT_BRANCH" 2>/dev/null) || true
-
-    if [[ -n "$REMOTE" ]]; then
+    # --verify --quiet, because plain `git rev-parse origin/<branch>` echoes the
+    # ref NAME back on stdout when it does not resolve. The literal string then
+    # passed the -n test, the rev-list below failed on it, and `set -e` killed
+    # the script three sections early — silently, on every unpushed branch.
+    LOCAL=$(git rev-parse --verify --quiet "$CURRENT_BRANCH" || true)
+    REMOTE=$(git rev-parse --verify --quiet "origin/$CURRENT_BRANCH" || true)
+
+    if [[ -n "$LOCAL" && -n "$REMOTE" ]]; then
         if [[ "$LOCAL" == "$REMOTE" ]]; then
             echo "✅ Up to date with origin/$CURRENT_BRANCH"
         else
-            BEHIND=$(git rev-list --count "$LOCAL..$REMOTE" 2>/dev/null)
-            AHEAD=$(git rev-list --count "$REMOTE..$LOCAL" 2>/dev/null)
+            BEHIND=$(git rev-list --count "$LOCAL..$REMOTE" 2>/dev/null || echo "?")
+            AHEAD=$(git rev-list --count "$REMOTE..$LOCAL" 2>/dev/null || echo "?")
             echo "ℹ️  Branch is $AHEAD ahead, $BEHIND behind origin/$CURRENT_BRANCH"
         fi
+    elif [[ -z "$REMOTE" ]]; then
+        echo "ℹ️  Branch not pushed to origin yet"
     fi
 fi
 

tests/test_checkpoint_patterns.sh

@@ -0,0 +1,84 @@
+#!/usr/bin/env bash
+# tests/test_checkpoint_patterns.sh — every `type: command` checkpoint must be
+# executable by the assessment runner.
+#
+# The runner (automated-assessment run-checkpoints.sh) refuses a pattern that
+# chains commands, and reads YAML line by line so a block scalar arrives as the
+# literal `|-`. Neither produces a visible failure: the checkpoint is simply
+# skipped, and the assessment report says nothing about it. GW-15 and GW-16 sat
+# in this file rejected for as long as they existed, and GW-17 was written the
+# same way on the day it was added.
+#
+# The rule is mirrored here rather than imported: automated-assessment is not a
+# dependency of this repo, and a test that needs an absent checkout is a test
+# that does not run.
+
+set -uo pipefail
+
+HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHECKPOINTS="$(cd "$HERE/.." && pwd)/skills/git-workflow/checkpoints.yaml"
+
+fail=0
+report() { echo "  FAIL $1"; fail=1; }
+
+# Mirrored from is_safe_eval_command's allowed_cmds.
+ALLOWED="grep egrep fgrep find test wc jq yq python3 python composer php \
+phpstan phpcs phpcbf rector phpunit node npm cat head tail ls stat file diff \
+sort uniq git make go sed awk tr cut xargs for if while case until [ set \
+printf echo true false gh"
+
+echo "checkpoints.yaml: command patterns"
+
+# id<TAB>pattern for every mechanical entry whose type is command
+while IFS=$'\t' read -r id pat; do
+    [ -n "$id" ] || continue
+
+    case "$pat" in
+        "|"|"|-"|">"|">-"|"")
+            report "$id: pattern is a multi-line YAML scalar — the runner receives '${pat:-<empty>}'"
+            continue
+            ;;
+    esac
+
+    if grep -qE '[;`]|&&|\|\||\$\(' <<<"$pat"; then
+        report "$id: pattern contains a command-chaining metacharacter (; && || \` \$()) — the runner rejects it"
+        continue
+    fi
+    if grep -qF '..' <<<"$pat"; then
+        report "$id: pattern contains '..' — the runner rejects it as path traversal"
+        continue
+    fi
+
+    # The runner strips a leading `!` before looking at the base command, so a
+    # negated pipeline is judged on the command that follows it.
+    stripped="${pat#!}"
+    read -r base _ <<<"$stripped"
+    if [[ " $ALLOWED " != *" $base "* ]]; then
+        report "$id: base command '$base' is not on the runner's allowlist"
+        continue
+    fi
+
+    echo "  ok   $id: '$base …' is executable by the runner"
+done < <(awk '
+    /^mechanical:/  { sect = 1; next }
+    /^[a-z_]+:/     { sect = 0 }
+    !sect           { next }
+    /^  - id:/ { if (id != "" && type == "command") print id "\t" pat; id = $3; type = ""; pat = ""; next }
+    /^    type:/    { type = $2 }
+    /^    pattern:/ {
+        line = $0
+        sub(/^    pattern:[[:space:]]*/, "", line)
+        if (line ~ /^".*"$/)        { sub(/^"/, "", line); sub(/"$/, "", line) }
+        else if (line ~ /^\x27.*\x27$/) { sub(/^\x27/, "", line); sub(/\x27$/, "", line) }
+        pat = line
+    }
+    END { if (id != "" && type == "command") print id "\t" pat }
+' "$CHECKPOINTS")
+
+echo
+if [ "$fail" -eq 0 ]; then
+    echo "All command patterns are runnable"
+else
+    echo "Some command patterns would never run"
+fi
+exit "$fail"

tests/test_hook_gates.sh

@@ -0,0 +1,138 @@
+#!/usr/bin/env bash
+# tests/test_hook_gates.sh — the three shipped gates that had no test:
+# merge-gate.sh, conflict-marker-gate.py and spec-cleanup-guard.sh.
+#
+# All three are hooks: they decide whether a command runs at all. A hook that
+# fails open on a case it should block is invisible — nothing reports the
+# permission it forgot to withhold — so the cases below are the blocking ones,
+# plus the pass-through cases that must not become false positives.
+#
+# merge-gate is driven with a stubbed `gh`, so no network and no repo.
+
+set -uo pipefail
+
+HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+ROOT="$(cd "$HERE/.." && pwd)"
+SCRIPTS="$ROOT/skills/git-workflow/scripts"
+WORK="$(mktemp -d)"
+trap 'rm -rf "$WORK"' EXIT
+
+export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null
+
+fail=0
+check() { # check <name> <expected> <actual>
+    if [ "$2" = "$3" ]; then
+        echo "  ok   $1"
+    else
+        echo "  FAIL $1: expected '$2', got '$3'"
+        fail=1
+    fi
+}
+
+# ---------------------------------------------------------------- merge-gate
+echo "merge-gate.sh"
+
+STUB_DIR="$WORK/stub"
+mkdir -p "$STUB_DIR"
+# Stub `gh`: the gate makes two different calls and they need different shapes
+# — `gh pr view --json mergeStateStatus,url` first, then `gh api graphql` for
+# the review threads. A single canned payload silently answers the first call
+# with a null mergeStateStatus and an empty url, at which point the gate exits 0
+# and every blocking case looks like a pass.
+cat > "$STUB_DIR/gh" <<'STUB'
+#!/usr/bin/env bash
+for a in "$@"; do
+    case "$a" in
+        graphql) cat "$STUB_GRAPHQL"; exit 0 ;;
+    esac
+done
+cat "$STUB_VIEW"
+STUB
+chmod +x "$STUB_DIR/gh"
+export STUB_VIEW="$STUB_DIR/view.json" STUB_GRAPHQL="$STUB_DIR/graphql.json"
+
+# gate <mergeStateStatus> <unresolved-threads> <command>  → prints the decision
+gate() {
+    printf '{"mergeStateStatus":"%s","url":"https://github.com/netresearch/git-workflow-skill/pull/163"}\n' "$1" > "$STUB_VIEW"
+    local nodes=""
+    [ "$2" -gt 0 ] && nodes='{"isResolved":false}'
+    printf '{"data":{"repository":{"pullRequest":{"reviewThreads":{"pageInfo":{"hasNextPage":false,"endCursor":null},"nodes":[%s]}}}}}\n' "$nodes" > "$STUB_GRAPHQL"
+    printf '{"tool_input":{"command":"%s"}}' "$3" \
+        | PATH="$STUB_DIR:$PATH" bash "$SCRIPTS/merge-gate.sh" 2>&1
+}
+
+out=$(gate CLEAN 0 "gh pr merge 163 --repo netresearch/git-workflow-skill --merge")
+check "a CLEAN pr with no threads is not denied" 0 "$(grep -ci 'deny' <<<"$out")"
+
+out=$(gate BLOCKED 0 "gh pr merge 163 --repo netresearch/git-workflow-skill --merge")
+check "a BLOCKED pr is denied" 1 "$(grep -ci 'deny' <<<"$out")"
+
+out=$(gate UNSTABLE 0 "gh pr merge 163 --repo netresearch/git-workflow-skill --merge")
+check "UNSTABLE is denied too — a red non-required check is still red" 1 "$(grep -ci 'deny' <<<"$out")"
+
+out=$(gate CLEAN 1 "gh pr merge 163 --repo netresearch/git-workflow-skill --merge")
+check "an unresolved thread is denied even when CLEAN" 1 "$(grep -ci 'deny' <<<"$out")"
+
+out=$(gate CLEAN 0 "gh pr view 163 --repo netresearch/git-workflow-skill")
+check "an unrelated gh command passes through" 0 "$(grep -ci 'deny' <<<"$out")"
+
+# ------------------------------------------------------- conflict-marker-gate
+echo "conflict-marker-gate.py"
+
+repo="$WORK/cm"
+mkdir -p "$repo"
+git -C "$repo" init -q -b main
+git -C "$repo" config user.name t; git -C "$repo" config user.email t@e.com
+printf 'clean\n' > "$repo/ok.txt"; git -C "$repo" add ok.txt
+
+marker_gate() { # marker_gate <cwd> <command>
+    printf '{"tool_input":{"command":"%s"}}' "$2" \
+        | ( cd "$1" && python3 "$SCRIPTS/conflict-marker-gate.py" 2>&1 )
+}
+
+out=$(marker_gate "$repo" "git commit -m 'chore: clean'")
+check "a clean staged tree is not denied" 0 "$(grep -ci 'deny' <<<"$out")"
+
+printf '<<<<<<< HEAD\nours\n=======\ntheirs\n>>>>>>> other\n' > "$repo/conflicted.txt"
+git -C "$repo" add conflicted.txt
+out=$(marker_gate "$repo" "git commit -m 'chore: with markers'")
+check "staged conflict markers are denied" 1 "$(grep -ci 'deny' <<<"$out")"
+
+out=$(marker_gate "$repo" "git status")
+check "a non-commit command passes through" 0 "$(grep -ci 'deny' <<<"$out")"
+
+# Fails open outside a repository — a hook that hard-errors would block every
+# command in a non-repo directory.
+out=$(marker_gate "$WORK" "git commit -m x")
+check "outside a repository it fails open" 0 "$(grep -ci 'deny' <<<"$out")"
+
+# ------------------------------------------------------- spec-cleanup-guard
+echo "spec-cleanup-guard.sh"
+
+repo="$WORK/spec"
+mkdir -p "$repo"
+git -C "$repo" init -q -b main
+git -C "$repo" config user.name t; git -C "$repo" config user.email t@e.com
+echo x > "$repo/README.md"; git -C "$repo" add README.md
+git -C "$repo" commit -q -m "chore: seed"
+
+( cd "$repo" && bash "$SCRIPTS/spec-cleanup-guard.sh" >/dev/null 2>&1 )
+check "a clean repo exits 0" 0 "$?"
+
+mkdir -p "$repo/docs/superpowers"
+echo plan > "$repo/docs/superpowers/plan.md"
+( cd "$repo" && bash "$SCRIPTS/spec-cleanup-guard.sh" >/dev/null 2>&1 )
+check "an untracked planning artifact is reported" 1 "$?"
+
+# The script's stated invariant: it never deletes, stages or modifies anything.
+check "the artifact still exists after the run" yes \
+    "$([ -f "$repo/docs/superpowers/plan.md" ] && echo yes || echo no)"
+check "nothing was staged" "" "$(git -C "$repo" diff --cached --name-only)"
+
+echo
+if [ "$fail" -eq 0 ]; then
+    echo "All hook-gate tests passed"
+else
+    echo "Some hook-gate tests FAILED"
+fi
+exit "$fail"