chore(release): plugin.json version 0.22.1

Skip-Release: true

.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "skills",
   "description": "Mathias's engineering skills library, plus the hooks and MCP servers that make it enforceable: TDD, clean code, debugging, specs, discovery, CI, and secret hygiene.",
-  "version": "0.21.0",
+  "version": "0.22.1",
   "author": {
     "name": "Mathias",
     "url": "https://git.d-ma.be/mathias"

README.md

@@ -29,7 +29,7 @@ descriptions and "use when" triggers.
 /plugin install skills@mathias
 ```
 
-The plugin ships skills, the `secret-guard` + `pre-task-ritual` hooks, and the
+The plugin ships skills, the `secret-guard`, `mcp-destructive-guard` and `pre-task-ritual` hooks, and the
 brain/gitea/infra MCP servers as one versioned unit. Skills are namespaced
 (`skills:tdd`). Updates land when `.claude-plugin/plugin.json`'s `version`
 moves — CI bumps it on every release.

agents/pr-gatekeeper.md

@@ -1,6 +1,6 @@
 ---
 name: pr-gatekeeper
-description: Adversarially reviews one open Gitea PR, verifies its CI verdict, then merges and closes — or escalates. Use for risk/medium work that went through a PR instead of straight to main. Spawned with a repo, PR number and the issue it closes.
+description: Adversarially reviews one change and returns a blocking verdict. Two modes — an open Gitea PR (verify CI, then merge and close, or escalate), or a staged/unpushed diff in a GitOps repo where a branch cannot be reconciled, in which case it reviews before the push and records the verdict on the issue without merging. Use for risk/medium work. Spawned with a repo, the issue it closes, and either a PR number or a checkout path.
 tools: Read, Grep, Glob, Bash
 color: "#7C3AED"
 ---
@@ -20,9 +20,27 @@ difficult. For code whose job is to refuse something, expect it.
 
 ## What you are given
 
+One of two modes. Everything after step 2 is identical — the review standard does
+not change with the plumbing.
+
+**PR mode** (the original):
 - `owner/repo`, the PR number, and the issue number it claims to close
 - The claim the author made about what the change does
 
+**Pre-push mode** (infra#470): there is no PR, because there cannot be one.
+- `owner/repo`, the path to the checkout, and the issue number the work claims to close
+- The claim the author made about what the change does
+
+In a repo whose deployment watches one branch — `mathias/infra`, where Flux
+watches `main` — a branch is invisible to the thing that applies it. A
+`risk/medium` change to `alerts.yaml` or `litellm-config.yaml` can be linted on a
+branch but never *reconciled* there, so a PR would review a state that was never
+deployed. The agreed lane is therefore that such work stays on `main` and gets a
+recorded adversarial review pass **before the push**, with you as the reviewer.
+
+If that lane resolves to "the author was careful", it has replaced nothing. You are
+the difference between the rule and a sentence about the rule.
+
 ## Procedure
 
 Work in the repo checkout at the path given to you. Never push, never force,
@@ -41,11 +59,27 @@ Comments carry sequencing constraints that the body does not.
 
 ### 2. Get the real diff
 
+**PR mode:**
+
 ```bash
 git fetch origin
 git diff origin/main...origin/<branch>
 ```
 
+**Pre-push mode** — the change is staged, or committed and unpushed:
+
+```bash
+git status --porcelain          # know what is staged vs merely present
+git diff --staged               # staged but not committed
+git diff origin/main...HEAD     # committed locally, not yet pushed
+```
+
+Run all three. Which one holds the change is itself information: work that is
+neither staged nor committed is not a diff under review, it is a working tree, and
+you should say so rather than review it. Untracked files do not appear in either
+diff — `git status --porcelain` is how you notice a new file the author forgot to
+stage, and a control that exists only in an unstaged file is not going to ship.
+
 Read every hunk. Do not skim. If the diff is too large to review properly, say
 so and escalate rather than producing confident coverage of a fraction of it.
 
@@ -162,9 +196,24 @@ covers none of these findings.*
 CI **red or absent** is an automatic block: an agent-opened PR with zero runs
 is a known platform failure mode in this estate, not a pass.
 
+**Pre-push mode: CI cannot have run**, because nothing has been pushed. The only
+signal is the author's local gate, and the absence of CI is not a finding against
+the change — it is a fact about the mode. Two consequences:
+
+- Do not block for "CI absent" here. That rule exists for a *pushed* branch with no
+  runs, which is a platform failure; an unpushed diff having no runs is arithmetic.
+- Verify the local gate the way you would verify any claim: run it yourself. Check
+  the **exit code**, not filtered output — `task check; echo $?` — because a grep
+  over a gate's output has hidden a failing test in this estate before. "The author
+  says it is green" is the belief you are testing.
+
+So in pre-push mode your report carries strictly less external evidence than in PR
+mode, and must say so. The verdict is "nothing I could find blocks this push", never
+"CI confirms this".
+
 ### 6. Verdict
 
-**Merge** only when all hold:
+**PR mode — merge** only when all hold:
 
 - no iron-law violation
 - every "done when" clause in the issue is satisfied
@@ -181,12 +230,31 @@ mcp__plugin_skills_gitea__issue_close
 mcp__plugin_skills_gitea__branch_delete
 ```
 
-Put your evidence on the **issue**, not only the PR thread. In Gitea a merge
+**Pre-push mode — clear to push** only when all hold:
-auto-closes the gated issue and review comments live on a thread separate from
-the issue's — so a finding recorded only on the PR is a finding nobody reading
-the issue will ever see.
 
-For `mathias/infra`, after merging:
+- no iron-law violation
+- every "done when" clause in the issue is satisfied
+- you ran the local gate yourself and checked its exit code
+- you flipped the tests and they went red
+- the change is `risk/low` or `risk/medium`
+
+**There is nothing to merge and no PR to close, so do not merge, do not close the
+issue, and do not touch a branch.** Your entire output is a comment:
+
+```
+mcp__plugin_skills_gitea__issue_comment   # the verdict, on the ISSUE
+```
+
+**The verdict goes in the issue.** That is not a stylistic preference — it is what
+makes the lane a *recorded* review pass rather than a conversation. A verdict that
+exists only in a chat transcript is indistinguishable from no review having
+happened, and the rule that created this mode says exactly that: no recorded
+verdict means the lane was not used.
+
+Head the comment so it can be found by grep, state which mode you ran in, and say
+plainly that CI had not run at the time you reviewed. The author pushes; you do not.
+
+For `mathias/infra`, after the author has pushed:
 
 ```bash
 flux reconcile kustomization apps --with-source
@@ -197,6 +265,11 @@ If it does not go `Ready: True`, **revert immediately** rather than
 forward-fixing, and report. A blocked `apps` Kustomization stops every app in
 the cluster, not just the changed one.
 
+Put your evidence on the **issue**, not only the PR thread. In Gitea a merge
+auto-closes the gated issue and review comments live on a thread separate from
+the issue's — so a finding recorded only on the PR is a finding nobody reading
+the issue will ever see.
+
 ### 7. Escalate to Mathias — and only then
 
 Do not merge. Comment the finding on the issue and report it. Escalate when:

hooks/hooks.json

@@ -33,6 +33,16 @@
             "timeout": 5
           }
         ]
+      },
+      {
+        "matcher": "mcp__.*",
+        "hooks": [
+          {
+            "type": "command",
+            "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/mcp-destructive-guard.sh\"",
+            "timeout": 5
+          }
+        ]
       }
     ]
   }

hooks/mcp-destructive-guard.sh

@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# PreToolUse hook (mcp__.* matcher) — makes a human approve irreversible MCP
+# verbs instead of letting the model run them unattended.
+#
+# Why (infra#508, infra ADR-0027): gitea-mcp authenticates by PAT pass-through,
+# so an agent holds the full scope of the caller's PAT. repo_delete's own guard
+# (confirm == repo) is filled in by the model on both sides: it stops typos,
+# not an injected or mistaken agent. The infra-mcp README claimed client-side
+# deny lists covered this; on 2026-10-01 none existed in any settings file.
+#
+# Policy: match gitea-mcp's verb spellings on ANY server name. The same tool is
+# reachable as mcp__plugin_skills_gitea__*, mcp__claude_ai_gitea__* or under
+# whatever name a host registers it with, so keying on the server name would
+# let a rename bypass the gate. It does NOT recognise other servers' spellings
+# of the same operation (e.g. a third-party `delete_repo`).
+#
+# file_write_branch is gated when it would land on main/master (on infra that
+# is a Flux deploy within ~60s — the same effect as pr_merge) or empty a file
+# (the same effect as file_delete). Writes to other branches pass.
+#
+# Emits "ask" or nothing; never "allow" (a hook allow would override the
+# user's own deny/ask rules). Always exits 0: Claude Code ignores a hook's
+# JSON on a non-zero exit. Anything it cannot parse — including a host
+# without jq — fails closed to "ask". The ask JSON is a literal, so the
+# fail-closed path needs no tool that might be missing.
+#
+# Scope limit, recorded in infra ADR-0027: this gates MCP calls only. A
+# session with the PAT in its environment can still reach the Gitea REST API
+# through a shell.
+
+set -u
+
+DESTRUCTIVE='repo_delete|file_delete|branch_delete|pr_merge|tbd_ship|repo_update|repo_mirror_push|workflow_run_trigger|tag_create|release_create'
+
+ask() {
+  # $1 must contain only [A-Za-z0-9 _.:()'/-] — it is spliced into JSON verbatim.
+  printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"%s"}}\n' "$1"
+  exit 0
+}
+
+INPUT="$(cat)"
+
+command -v jq >/dev/null 2>&1 \
+  || ask "mcp-destructive-guard: jq is not installed, so this MCP call cannot be checked. Approve only if you expected it."
+
+if ! TOOL_NAME="$(printf '%s' "$INPUT" | jq -er '.tool_name | strings | select(length > 0)' 2>/dev/null)"; then
+  ask "mcp-destructive-guard: could not read a tool name from this call, so it cannot be shown safe. Approve only if you expected it."
+fi
+
+case "$TOOL_NAME" in
+  mcp__*__*) ;;
+  *) exit 0 ;;
+esac
+
+verb="${TOOL_NAME##*__}"
+safe_verb="$(printf '%s' "$verb" | tr -cd 'A-Za-z0-9_-')"
+
+if printf '%s' "$verb" | grep -Eqix "($DESTRUCTIVE)"; then
+  ask "Irreversible MCP verb '$safe_verb' (infra ADR-0027): needs your approval. The model filled in every argument, including any confirm field."
+fi
+
+if printf '%s' "$verb" | grep -Eqix 'file_write_branch'; then
+  # Cleared only when there is exactly ONE key that case-folds to "branch" and
+  # one to "content": gitea-mcp decodes with Go encoding/json, which matches
+  # keys case-insensitively and lets the last one win, so {"branch":"feat/x",
+  # "Branch":"main"} landed on main while a lowercase-only read saw feat/x.
+  # The branch is cleared by SHAPE (allowlist), not by a blocklist of names:
+  # "refs/heads/main" created a second ref that wedged API writes to main.
+  verdict="$(printf '%s' "$INPUT" | jq -r '
+      .tool_input as $i
+      | if ($i | type) != "object" then "ask" else
+          ([$i | to_entries[] | select(.key | ascii_downcase == "branch")]) as $b
+          | ([$i | to_entries[] | select(.key | ascii_downcase == "content")]) as $c
+          | if ($b | length) != 1 or ($c | length) != 1 then "ask"
+            elif ($b[0].key != "branch") or ($c[0].key != "content") then "ask"
+            elif ($b[0].value | type) != "string" or ($c[0].value | type) != "string" then "ask"
+            elif ($b[0].value | test("^[A-Za-z0-9][A-Za-z0-9._/-]*$") | not) then "ask"
+            elif ($b[0].value | test("\\.\\.|^refs/|/$")) then "ask"
+            elif ($b[0].value | ascii_downcase | . == "main" or . == "master") then "ask"
+            elif ($c[0].value | test("^\\s*$")) then "ask"
+            else "pass" end
+        end' 2>/dev/null || echo ask)"
+  [ "$verdict" = "pass" ] \
+    || ask "file_write_branch to main/master, an unusual branch name, or emptying a file (infra ADR-0027): needs your approval."
+fi
+exit 0

tests/agents_test.sh

@@ -103,6 +103,62 @@ else
   ok 'doctor is silent when no host-local copy exists'
 fi
 
+# --- the pre-push mode the GitOps review lane depends on (infra#470) ---------
+#
+# infra#470 resolved a doctrine conflict: root rule 5 wants a PR for risk/medium+,
+# and mathias/infra forbids branches because Flux watches main and a branch can be
+# linted but never reconciled. The agreed lane is that such work stays on main and
+# instead gets a RECORDED ADVERSARIAL REVIEW PASS before the push.
+#
+# That lane names this agent as its reviewer. If the definition only knows how to
+# review an open PR, the lane resolves to "be careful" — which is what rule 5 was
+# written to replace. These assertions observe FILE CONTENT only: that the mode is
+# documented, that it does not merge, and that the verdict has a stated home. They
+# do NOT claim the agent reviews a staged diff well (rule 9).
+
+if grep -qi 'git diff --staged\|git diff --cached' "$AGENT"; then
+  ok 'the definition names a command that reads a staged diff, not only a branch diff'
+else
+  no 'the definition has no staged-diff command, so the infra#470 lane has no reviewer'
+fi
+
+if grep -qi 'pre-push' "$AGENT"; then
+  ok 'the definition names the pre-push mode'
+else
+  no 'the definition does not name a pre-push mode (infra#470)'
+fi
+
+# The verdict has to land somewhere durable, or "recorded review pass" is untrue.
+# The lane specifies the issue, deliberately not a PR thread — there is no PR.
+if grep -qi 'verdict in the issue\|verdict goes in the issue' "$AGENT"; then
+  ok 'the definition states the verdict goes in the issue'
+else
+  no 'the definition does not say where a pre-push verdict is recorded'
+fi
+
+# In pre-push mode there is nothing to merge and no PR to close. An agent that
+# reaches for pr_merge there would be acting on a PR that does not exist.
+#
+# SCOPED to the pre-push section on purpose. An unscoped grep for "do not merge"
+# passes on line 202 of the current file, which is the ESCALATION step talking
+# about a PR — a pass for entirely the wrong reason, and the kind of vacuous
+# assertion rule 9 is about. Caught by checking what the green was matching.
+PREPUSH=$(awk '/[Pp]re-push/{f=1} f&&/^## [^#]/&&!/[Pp]re-push/{f=0} f' "$AGENT")
+if printf '%s' "$PREPUSH" | grep -qi 'nothing to merge\|do not merge\|no PR to merge'; then
+  ok 'the pre-push section itself rules out merging'
+else
+  no 'the pre-push section does not rule out merging (an unscoped grep would pass here wrongly)'
+fi
+
+# CI cannot have run on an unpushed diff, so the CI step's meaning changes. A
+# definition that still treats "CI green" as available would grade on a signal
+# that does not exist yet.
+if grep -qi 'CI has not run\|no CI run yet\|CI cannot have run' "$AGENT"; then
+  ok 'the definition acknowledges CI has not run on an unpushed diff'
+else
+  no 'the definition does not address CI being absent pre-push'
+fi
+
 rm -rf "$WORK"
 printf '\n%s passed, %s failed\n' "$pass" "$fail"
 [ "$fail" -eq 0 ]

tests/mcp_destructive_guard_test.sh

@@ -0,0 +1,118 @@
+#!/bin/sh
+# Tests for hooks/mcp-destructive-guard.sh — the PreToolUse gate that makes a
+# human approve irreversible MCP verbs (infra#508, infra ADR-0027).
+#
+# Contract: reads PreToolUse hook JSON on stdin, ALWAYS exits 0, and answers
+# in hookSpecificOutput.permissionDecision. "ask" = a human must approve;
+# empty stdout ("NONE") = no opinion, normal permission rules apply. The guard
+# never emits "allow": an allow from a hook would bypass the user's own rules.
+#
+# Cases marked (gatekeeper) are escapes found by the skills#52 review: each
+# passed the first version of this suite.
+
+set -u
+
+ROOT=$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd)
+GUARD="$ROOT/hooks/mcp-destructive-guard.sh"
+
+pass=0
+fail=0
+
+# run_guard <json> [PATH] — invoked exactly as hooks.json does: `bash <path>`.
+# Prints "<decision> rc=<exit>", decision NONE for an empty answer.
+run_guard() {
+  if [ $# -ge 2 ]; then
+    _out=$(printf '%s' "$1" | PATH="$2" "$BASH_BIN" "$GUARD"); _rc=$?
+  else
+    _out=$(printf '%s' "$1" | bash "$GUARD"); _rc=$?
+  fi
+  if [ -z "$_out" ]; then _d=NONE
+  else _d=$(printf '%s' "$_out" | jq -r '.hookSpecificOutput.permissionDecision // "NONE"' 2>/dev/null || echo BROKEN)
+  fi
+  printf '%s rc=%s' "$_d" "$_rc"
+}
+BASH_BIN=$(command -v bash)
+
+check() {
+  _want=$1 _json=$2 _label=$3
+  _got=$(run_guard "$_json")
+  if [ "$_got" = "$_want rc=0" ]; then
+    pass=$((pass + 1)); printf '  ok   %s\n' "$_label"
+  else
+    fail=$((fail + 1)); printf '  FAIL %s\n       want=%s rc=0 got=%s\n       in=%s\n' "$_label" "$_want" "$_got" "$_json"
+  fi
+}
+tool() { printf '{"tool_name":"%s","tool_input":{"owner":"mathias","repo":"x"}}' "$1"; }
+
+echo "mcp-destructive-guard tests"
+
+for v in repo_delete file_delete branch_delete pr_merge tbd_ship repo_update repo_mirror_push \
+         workflow_run_trigger tag_create release_create; do
+  check ask "$(tool "mcp__plugin_skills_gitea__$v")" "plugin gitea $v asks"
+done
+
+# The verb is matched on any server name: connector, bare, or renamed.
+check ask "$(tool mcp__claude_ai_gitea__repo_delete)" "connector namespace asks"
+check ask "$(tool mcp__gitea__repo_delete)"           "bare server name asks"
+check ask "$(tool mcp__git__repo_delete)"             "renamed server still asks"
+check ask "$(tool mcp__plugin_skills_gitea__REPO_DELETE)" "uppercase verb asks"
+# The verb is the LAST segment (gatekeeper: parsing the second segment stayed green).
+check ask  "$(tool mcp__a__b__repo_delete)" "verb taken from last segment"
+check NONE "$(tool mcp__repo_delete__issue_get)" "server named like a verb does not trigger"
+
+# file_write_branch: content landing on main, or emptying a file, is the same
+# effect as the gated pr_merge / file_delete (gatekeeper).
+fwb() { printf '{"tool_name":"mcp__plugin_skills_gitea__file_write_branch","tool_input":{"branch":"%s","content":"%s"}}' "$1" "$2"; }
+check ask  "$(fwb main x)"     "file_write_branch to main asks"
+check ask  "$(fwb master x)"   "file_write_branch to master asks"
+check ask  "$(fwb feat/y '')"  "file_write_branch emptying a file asks"
+check ask  '{"tool_name":"mcp__plugin_skills_gitea__file_write_branch","tool_input":{"content":"x"}}' "file_write_branch without branch asks"
+check NONE "$(fwb feat/y x)"   "file_write_branch to a feature branch passes"
+
+# Go's encoding/json matches keys case-insensitively, last one wins: gitea-mcp
+# wrote to main from {"branch":"feat/y8","Branch":"main"} while the hook read
+# only `branch` (gatekeeper round 2, verified against a real Gitea). So the
+# guard clears a write only when exactly one key case-folds to branch/content.
+FWB=mcp__plugin_skills_gitea__file_write_branch
+check ask  "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y8\",\"Branch\":\"main\",\"content\":\"x\"}}" "case-variant Branch key asks"
+check ask  "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y\",\"content\":\"x\",\"Content\":\"\"}}" "case-variant Content key asks"
+check ask  "{\"tool_name\":\"$FWB\",\"tool_input\":{\"BRANCH\":\"feat/y\",\"content\":\"x\"}}" "only an uppercase BRANCH key asks"
+check ask  "$(fwb Main x)"  "file_write_branch to Main asks"
+check ask  "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y\"}}" "missing content key asks"
+check ask  "$(fwb feat/y ' ')" "whitespace-only content asks"
+check ask  "{\"tool_name\":\"$FWB\",\"tool_input\":\"branch=feat/y\"}" "non-object tool_input asks"
+# Branch names are cleared by shape, not by blocklist: refs/heads/main created a
+# second ref that wedged every API write to main (gatekeeper side finding).
+check ask  "$(fwb refs/heads/main x)" "refs/ prefixed branch asks"
+check ask  "$(fwb ' main' x)"         "branch with whitespace asks"
+check ask  "$(fwb '../main' x)"       "branch with .. asks"
+check NONE "$(fwb release/v1.2_x x)"  "ordinary branch name passes"
+
+# Reversible / read verbs pass through.
+for v in issue_close issue_create file_read repo_list pr_create; do
+  check NONE "$(tool "mcp__plugin_skills_gitea__$v")" "$v passes through"
+done
+check NONE "$(tool mcp__plugin_skills_gitea__repo_delete_dry_run_list)" "suffix near-miss passes"
+check NONE "$(tool Bash)" "non-MCP tool passes"
+check NONE "$(tool repo_delete)" "non-MCP tool named like a verb passes"
+
+# Malformed input fails CLOSED (gatekeeper: non-string tool_name got NONE).
+check ask 'not json'                        "non-JSON asks"
+check ask '{}'                              "missing tool_name asks"
+check ask '{"tool_name":null}'              "null tool_name asks"
+check ask '{"tool_name":""}'                "empty tool_name asks"
+check ask '{"tool_name":5}'                 "number tool_name asks"
+check ask '{"tool_name":["mcp__x__repo_delete"]}' "array tool_name asks"
+check ask '{"tool_name":{"a":1}}'           "object tool_name asks"
+
+# Without jq on PATH the guard must still answer ask, not stay silent
+# (gatekeeper: the fail-closed path itself needed jq).
+nojq=$(mktemp -d); trap 'rm -rf "$nojq"' EXIT
+for b in bash cat grep tr sed; do ln -s "$(command -v $b)" "$nojq/$b"; done
+_got=$(run_guard "$(tool mcp__plugin_skills_gitea__repo_delete)" "$nojq")
+if [ "$_got" = "ask rc=0" ]; then pass=$((pass+1)); echo "  ok   no jq on PATH still asks"; else fail=$((fail+1)); echo "  FAIL no jq on PATH: got=$_got"; fi
+_got=$(run_guard "$(tool mcp__plugin_skills_gitea__issue_get)" "$nojq")
+if [ "$_got" = "ask rc=0" ]; then pass=$((pass+1)); echo "  ok   no jq on PATH asks even for a read verb (cannot parse = cannot clear)"; else fail=$((fail+1)); echo "  FAIL no jq read verb: got=$_got"; fi
+
+printf '\n%d passed, %d failed\n' "$pass" "$fail"
+[ "$fail" -eq 0 ]