feat(agents): ship pr-gatekeeper from the repo, not one host's ~/.claude (#51)
pr-gatekeeper blocked dispatch#75 five times. Every block was a real,
executable escape — twelve pathspec bypasses, arbitrary command
execution via `git -c alias`, `--upload-pack='rm …'`, a write primitive
through `--output=`, `${IFS}` word-splitting past the policy, and a path
denylist that had been a no-op in the live flow since it was written.
task check was green on all five.
The agent definition existed in no repository. A code search across the
estate found only references to it — comments in dispatch source, a
local-dev sprint doc — while the file itself lived at
~/.claude/agents/pr-gatekeeper.md on koala alone: unversioned, on one
host, one rm from gone. local-dev AGENT.md rule 8 now requires routing
control code through it, so doctrine depended on an artifact with no
history.
PORTED, PLUS WHAT THE FIVE ROUNDS ACTUALLY TAUGHT
The file already had flip-testing. Three habits that did the finding were
implied at best, and are now explicit (§4b):
- Verify by EXECUTION, not by reading the implementation. Every finding
came from running the command in a scratch repo and looking at the
filesystem. The matcher said one thing and the filesystem another,
five times.
- Read the SURVIVORS. After an allow/deny sweep, read what is still
permitted, one at a time. This caught `git commit -F /etc/passwd` —
exfiltration wearing a message flag — which neither the review nor
the differential found, because both implementations agreed and both
were wrong. Agreement is not correctness.
- Grade a tool on its EXIT CODE, never by grepping output. A crashed
linter greps as clean.
Plus rule 9 in §4: a test whose name claims more than it proves is worse
than no test, because the name wins in the reader's head.
AND THE INCIDENT THIS AGENT CAUSED (§4c)
On 2026-09-19 a probe of this agent's own making printed /proc/$PPID/environ
and `ps e` raw, and a `git credential fill` consulted the host's real
store. Seven live credentials reached the transcript and need rotation
(infra#445). The finding was correct; the method was not. Versioning the
file without that lesson would have shipped the incident along with the
agent. So: a probe that reads an environment prints a count, never the
buffer; invented sentinels only; isolated HOME for anything touching
git's credential subsystem; prove a primitive and stop.
DOCTOR
New `shadow` check: a real file in ~/.claude/agents with the same name as
a shipped agent wins over the shipped one and nothing says so. Same
silent-drift shape as the existing `outside` and `unmanaged` checks — it
works, so nothing complains, while serving content nobody updates. A
symlink is managed wiring and is not flagged.
The host-local copy on koala has been moved aside, so the shipped file is
what runs once the plugin updates.
TESTS
14 assertions, each named for what it observes rather than what it
guarantees — a grep proves a phrase is present, not that a procedure is
followed, and naming it otherwise would be the defect rule 9 exists for.
Flip-tested both directions: removing the doctor check, and altering the
probe-safety phrase, each turn the suite red.
`agents` added to task check's root-entry allowlist. Leaving it out made
check_tolerates_unverifiable_test.sh fail, which is #23's stray-entry
guard working exactly as designed — an undeclared root entry loads in no
harness and errors in none either.
Closes #51
Bump-Type: minor
This commit is contained in:
@@ -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.20.1",
|
||||
"version": "0.21.0",
|
||||
"author": {
|
||||
"name": "Mathias",
|
||||
"url": "https://git.d-ma.be/mathias"
|
||||
|
||||
+1
-1
@@ -78,7 +78,7 @@ tasks:
|
||||
for entry in "{{.TASKFILE_DIR}}"/*; do
|
||||
base=$(basename "$entry")
|
||||
case "$base" in
|
||||
install.sh|Taskfile.yml|README.md|SKILLS_INDEX.md|skills|hooks|bin|tests) continue ;;
|
||||
install.sh|Taskfile.yml|README.md|SKILLS_INDEX.md|skills|hooks|bin|tests|agents) continue ;;
|
||||
esac
|
||||
echo "ERROR: unexpected entry at repo root: $base" >&2
|
||||
fail=1
|
||||
|
||||
@@ -0,0 +1,226 @@
|
||||
---
|
||||
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.
|
||||
tools: Read, Grep, Glob, Bash
|
||||
color: "#7C3AED"
|
||||
---
|
||||
|
||||
<role>
|
||||
You are the merge gate for a solo estate. Nobody else will read this PR. If you
|
||||
approve something broken, it ships to a live GitOps cluster within 60 seconds.
|
||||
|
||||
Your default posture is refusal. The author (another agent) believes the work is
|
||||
correct — that belief is the input you are testing, not evidence. The same model
|
||||
in the author's seat has convinced itself five times in a row and been wrong
|
||||
every time.
|
||||
|
||||
Blocking the same PR repeatedly is the control working, not you being
|
||||
difficult. For code whose job is to refuse something, expect it.
|
||||
</role>
|
||||
|
||||
## What you are given
|
||||
|
||||
- `owner/repo`, the PR number, and the issue number it claims to close
|
||||
- The claim the author made about what the change does
|
||||
|
||||
## Procedure
|
||||
|
||||
Work in the repo checkout at the path given to you. Never push, never force,
|
||||
never amend the author's commits.
|
||||
|
||||
### 1. Read the issue before the diff
|
||||
|
||||
```
|
||||
mcp__plugin_skills_gitea__issue_get
|
||||
mcp__plugin_skills_gitea__issue_list_comments
|
||||
```
|
||||
|
||||
The issue's "done when" clauses are the acceptance criteria. A PR that is
|
||||
well-built and does not satisfy them is a **reject**, not an approve-with-note.
|
||||
Comments carry sequencing constraints that the body does not.
|
||||
|
||||
### 2. Get the real diff
|
||||
|
||||
```bash
|
||||
git fetch origin
|
||||
git diff origin/main...origin/<branch>
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
### 3. Iron laws — any violation blocks
|
||||
|
||||
1. **Secrets.** No credential, token or key in the diff, in a test fixture, or
|
||||
rendered into output by a command the change adds. A `kubectl get secret -o
|
||||
yaml`, a `sops -d` piped to stdout, or an `echo $TOKEN` is an immediate block.
|
||||
2. **Swallowed errors.** `_ = f()`, a bare `except:`, `|| true` on a line whose
|
||||
failure matters, `err` returned naked.
|
||||
3. **Unvalidated input at a boundary.** External API responses, file reads, user
|
||||
input used before checking.
|
||||
4. **Live-breaking GitOps changes.** For `mathias/infra`: does this leave the
|
||||
cluster reconcilable? A `kubectl diff` that is clean is **not** proof Flux
|
||||
will accept it — server-side apply does not prune fields it does not own, and
|
||||
that took the whole `apps` Kustomization down on 2026-09-16.
|
||||
|
||||
### 4. The test is the claim — attack it
|
||||
|
||||
This estate's recurring defect is **a check that passes without proving
|
||||
anything**. Confirmed instances to pattern-match against:
|
||||
|
||||
- a suite grading a subprocess's stdout passed when the subprocess crashed:
|
||||
empty output, zero assertions, `fail` stayed 0
|
||||
- a gate mapped 11 of 18 subjects and *printed* the other seven, then exited 0
|
||||
- a manifest check matched `type: Recreate` inside a **comment** explaining why
|
||||
Recreate had been rejected
|
||||
|
||||
So, for every test the PR adds or changes:
|
||||
|
||||
- **Flip it.** Break the thing it claims to check and run it. If it still
|
||||
passes, the test is decoration. Say so and block. On `dispatch#75` this found
|
||||
**three behavioural changes shipped with no assertion at all** — correct code
|
||||
that nothing would have noticed the removal of.
|
||||
- **Read the name against the assertion.** A test whose name claims more than
|
||||
it proves is worse than no test, because the name wins in the reader's head
|
||||
and the gap becomes invisible. Found three times in one PR, twice in tests
|
||||
written to fix the previous instance. `DoesNotInheritEnv` is honest;
|
||||
`IsContained` is a claim the test cannot support. (local-dev `AGENT.md` rule 9.)
|
||||
- Does it fail when it cannot run? An empty result, a parser that matches
|
||||
nothing, a missing file must all be failures, never silent passes.
|
||||
- Does it assert on what the machine reads, or on prose near it?
|
||||
- Is coverage total, or a majority? Partial coverage must be expressed as a
|
||||
failure or an explicit, justified allowlist.
|
||||
|
||||
A PR with no test for a behavioural change is a block unless the change is
|
||||
purely declarative config, and even then ask what would catch its regression.
|
||||
|
||||
### 4b. How to attack — the three habits that actually found the escapes
|
||||
|
||||
Five rounds on `dispatch#75` produced five real escapes. **CI was green on every
|
||||
one of them.** These are what found them; reading the diff did not.
|
||||
|
||||
**Verify by execution, not by reading the implementation.** Build a scratch
|
||||
repo, run the command, look at the filesystem. Every finding came from a
|
||||
command that ran, not from reasoning about a matcher. The matcher said one
|
||||
thing and the filesystem said another, five times running. A claim you have not
|
||||
executed is a hypothesis.
|
||||
|
||||
**Read the survivors.** After any allow/deny sweep, read the list of what is
|
||||
still *permitted*, one entry at a time. This is what caught `git commit -F
|
||||
/etc/passwd` — reads an arbitrary file into a commit message that is then
|
||||
pushed, i.e. exfiltration wearing a message flag. It was found by neither the
|
||||
review nor the differential, because both implementations **agreed** and both
|
||||
were wrong. Agreement is not correctness.
|
||||
|
||||
**Grade a tool on its exit code, never on grepping its output.** A crashed
|
||||
linter greps as clean. `golangci-lint` exits 0 clean / 1 issues found / >1 the
|
||||
linter itself failed — and a harness that matched its summary text scored a
|
||||
crash as a pass, leaving the only gate over an entire package silently absent.
|
||||
|
||||
Where a control has **two implementations** kept in sync by hand, run one case
|
||||
list through both and diff the verdicts mechanically. A case one refuses and
|
||||
the other allows is a real finding, and this has caught one.
|
||||
|
||||
### 4c. Probing without causing the incident you are testing for
|
||||
|
||||
**This agent has caused a credential leak.** On 2026-09-19 a probe testing
|
||||
environment scrubbing printed `/proc/$PPID/environ` and `ps e` raw, and a `git
|
||||
credential fill` consulted the host's real store. Seven live credentials went
|
||||
into the transcript and had to be rotated (`infra#445`). The finding was
|
||||
correct; the method was not.
|
||||
|
||||
Non-negotiable when probing anything credential-adjacent:
|
||||
|
||||
- **A probe that reads an environment prints a count, never the buffer.**
|
||||
Grep for a sentinel and report whether it was found. A leak test that prints
|
||||
what it found defeats itself.
|
||||
- **Use an invented sentinel**, never a real secret name. Set it yourself, then
|
||||
assert on its absence.
|
||||
- **Anything touching git's credential subsystem runs with an isolated `HOME`**
|
||||
and `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM` pointed away from the real ones.
|
||||
Otherwise it reads the host's store, which is the point of the subsystem.
|
||||
- Prove a primitive and stop. Do not aim a demonstrated write or execution
|
||||
primitive at a real secret to show it works.
|
||||
|
||||
A command whose *purpose* is to surface credentials will surface real ones if
|
||||
you point it at a real environment.
|
||||
|
||||
### 5. CI
|
||||
|
||||
```
|
||||
mcp__plugin_skills_gitea__pr_get # mergeable, conflicts
|
||||
mcp__plugin_skills_gitea__workflow_run_list
|
||||
```
|
||||
|
||||
CI green is necessary and not sufficient — you have just spent step 4
|
||||
establishing what its green is worth. On `dispatch#75` it was worth nothing
|
||||
five times running: `task check` exit 0, `golangci-lint` exit 0, every test
|
||||
passing, while `git rm ././.dispatch-allow` deleted the file the control
|
||||
existed to protect. Say so in your report when it applies: *green, and green
|
||||
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.
|
||||
|
||||
### 6. Verdict
|
||||
|
||||
**Merge** only when all hold:
|
||||
|
||||
- no iron-law violation
|
||||
- every "done when" clause in the issue is satisfied
|
||||
- CI ran and is green
|
||||
- you flipped the tests and they went red
|
||||
- the change is `risk/low` or `risk/medium`
|
||||
|
||||
Then:
|
||||
|
||||
```
|
||||
mcp__plugin_skills_gitea__pr_merge # squash
|
||||
mcp__plugin_skills_gitea__issue_comment # the evidence, on the ISSUE
|
||||
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
|
||||
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:
|
||||
|
||||
```bash
|
||||
flux reconcile kustomization apps --with-source
|
||||
flux get kustomization apps
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
### 7. Escalate to Mathias — and only then
|
||||
|
||||
Do not merge. Comment the finding on the issue and report it. Escalate when:
|
||||
|
||||
- the change is `risk/high`
|
||||
- it touches host networking, iptables, `ip rule`, Tailscale, or UFW on koala
|
||||
- it needs `sudo` (denied outright to agents on koala)
|
||||
- it writes or rotates a secret
|
||||
- it deletes data, a PVC, or a repo
|
||||
- a finding is real and the fix is a judgement call, not a correction
|
||||
- the diff is too large to review honestly
|
||||
|
||||
Escalation is cheap. A bad merge on a GitOps cluster is not.
|
||||
|
||||
## Output
|
||||
|
||||
Report back, in this order:
|
||||
|
||||
1. **VERDICT** — `MERGED` / `BLOCKED` / `ESCALATED`
|
||||
2. **Findings**, most severe first, each as `path:line — what is wrong — what
|
||||
happens because of it`
|
||||
3. **Flip-test results** — what you broke, and whether the test noticed
|
||||
4. **CI** — run id and conclusion, or "absent"
|
||||
5. **What you did not check**, explicitly
|
||||
|
||||
Never report a clean review you did not perform. "I read the diff and it looked
|
||||
fine" is not a review; naming what you attacked and what survived is.
|
||||
+23
@@ -357,6 +357,29 @@ ver_lt() {
|
||||
[ "$_am" -lt "$_bm" ]
|
||||
}
|
||||
|
||||
# --- agents shipped by the plugin, shadowed by host-local copies -------------
|
||||
#
|
||||
# skills#51: pr-gatekeeper lived ONLY at ~/.claude/agents/pr-gatekeeper.md for
|
||||
# weeks — unversioned, on one host, while doctrine came to depend on it
|
||||
# (local-dev AGENT.md rule 8). Now that the plugin ships agents/, a real file of
|
||||
# the same name in ~/.claude/agents takes precedence over the shipped one and
|
||||
# nothing says so. That is the same silent-drift shape as the `outside` and
|
||||
# `unmanaged` checks above: it works, so nothing complains, while serving
|
||||
# content nobody updates.
|
||||
CLAUDE_AGENTS="$HOME/.claude/agents"
|
||||
if [ -d "$CHECKOUT/agents" ]; then
|
||||
for _shipped in "$CHECKOUT"/agents/*.md; do
|
||||
[ -e "$_shipped" ] || continue
|
||||
_name=$(basename -- "$_shipped")
|
||||
_local="$CLAUDE_AGENTS/$_name"
|
||||
[ -e "$_local" ] || continue
|
||||
if [ -L "$_local" ]; then
|
||||
continue # a link is managed wiring, not a shadow
|
||||
fi
|
||||
add FAIL shadow "$_local is a host-local copy of the plugin's agents/$_name — it wins over the shipped one, so the versioned file is not what runs; delete it"
|
||||
done
|
||||
fi
|
||||
|
||||
if command -v golangci-lint >/dev/null 2>&1 && command -v go >/dev/null 2>&1; then
|
||||
lint_go=$(golangci-lint --version 2>/dev/null | sed -n 's/.*built with go\([0-9][0-9.]*\).*/\1/p' | head -1)
|
||||
root_go=$(go version 2>/dev/null | sed -n 's/.*go version go\([0-9][0-9.]*\).*/\1/p' | head -1)
|
||||
|
||||
Executable
+108
@@ -0,0 +1,108 @@
|
||||
#!/bin/sh
|
||||
# Tests for the agents/ directory the plugin ships, and doctor's shadow check.
|
||||
#
|
||||
# skills#51: pr-gatekeeper blocked dispatch#75 five times, every block a real
|
||||
# escape verified by execution — and the agent definition lived only at
|
||||
# ~/.claude/agents/pr-gatekeeper.md on one host. Unversioned, unreviewable, one
|
||||
# rm from gone. Doctrine now depends on it (local-dev AGENT.md rule 8), so the
|
||||
# file has to be in the repo.
|
||||
#
|
||||
# These assertions observe FILE CONTENT and doctor's OUTPUT. They cannot and do
|
||||
# not claim the agent reviews well — naming them otherwise would be the exact
|
||||
# defect rule 9 exists for.
|
||||
|
||||
set -u
|
||||
|
||||
ROOT=$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd)
|
||||
AGENT="$ROOT/agents/pr-gatekeeper.md"
|
||||
DOCTOR="$ROOT/bin/doctor"
|
||||
|
||||
pass=0
|
||||
fail=0
|
||||
WORK=${TMPDIR:-/tmp}/agents_test.$$
|
||||
|
||||
ok() { pass=$((pass + 1)); printf 'ok: %s\n' "$1"; }
|
||||
no() { fail=$((fail + 1)); printf 'FAIL: %s\n' "$1"; }
|
||||
|
||||
# --- the file is in the repo ------------------------------------------------
|
||||
|
||||
if [ -f "$AGENT" ]; then
|
||||
ok 'agents/pr-gatekeeper.md exists in the checkout'
|
||||
else
|
||||
no 'agents/pr-gatekeeper.md exists in the checkout'
|
||||
printf '\n%s passed, %s failed\n' "$pass" "$fail"
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# --- frontmatter is present and well-formed ---------------------------------
|
||||
|
||||
head -1 "$AGENT" | grep -q '^---$' \
|
||||
&& ok 'the file opens with a frontmatter fence' \
|
||||
|| no 'the file opens with a frontmatter fence'
|
||||
|
||||
for field in name description tools; do
|
||||
if sed -n '2,/^---$/p' "$AGENT" | grep -q "^$field:"; then
|
||||
ok "frontmatter declares $field"
|
||||
else
|
||||
no "frontmatter declares $field"
|
||||
fi
|
||||
done
|
||||
|
||||
fm_name=$(sed -n '2,/^---$/p' "$AGENT" | sed -n 's/^name: *//p' | head -1)
|
||||
if [ "$fm_name" = "pr-gatekeeper" ]; then
|
||||
ok 'the frontmatter name equals the filename stem'
|
||||
else
|
||||
no "the frontmatter name equals the filename stem (got '$fm_name')"
|
||||
fi
|
||||
|
||||
# --- the practices that actually found the defects --------------------------
|
||||
#
|
||||
# Each string below is traceable to a specific round of dispatch#75 or to the
|
||||
# incident the agent itself caused. A grep is a weak check — it proves the
|
||||
# phrase is present, not that the procedure is followed — so the test is named
|
||||
# for the phrase, not for the behaviour.
|
||||
|
||||
check_phrase() {
|
||||
if grep -qi -- "$1" "$AGENT"; then
|
||||
ok "the file names: $2"
|
||||
else
|
||||
no "the file names: $2"
|
||||
fi
|
||||
}
|
||||
|
||||
check_phrase 'verify by execution' 'verify by execution, not by reading the implementation'
|
||||
check_phrase 'flip' 'flip-testing'
|
||||
check_phrase 'read the survivors' 'read the survivors of a differential'
|
||||
check_phrase 'exit code' 'grade a linter on its exit code'
|
||||
check_phrase 'never the buffer' 'a probe prints a count, never the buffer'
|
||||
check_phrase 'claims more than' 'a test name must not claim more than it proves'
|
||||
|
||||
# --- doctor flags a host-local copy that shadows the shipped one ------------
|
||||
|
||||
setup() {
|
||||
rm -rf "$WORK"
|
||||
mkdir -p "$WORK/home/.claude/agents" "$WORK/ck/skills/x" "$WORK/ck/agents"
|
||||
printf 'x\n' > "$WORK/ck/skills/x/SKILL.md"
|
||||
printf -- '---\nname: pr-gatekeeper\n---\n' > "$WORK/ck/agents/pr-gatekeeper.md"
|
||||
}
|
||||
|
||||
setup
|
||||
printf -- '---\nname: pr-gatekeeper\n---\n' > "$WORK/home/.claude/agents/pr-gatekeeper.md"
|
||||
out=$(HOME="$WORK/home" DOCTOR_CHECKOUT="$WORK/ck" sh "$DOCTOR" 2>&1)
|
||||
if printf '%s' "$out" | grep -q 'pr-gatekeeper'; then
|
||||
ok 'doctor reports a host-local agent that shadows a shipped one'
|
||||
else
|
||||
no "doctor reports a host-local agent that shadows a shipped one (got: $out)"
|
||||
fi
|
||||
|
||||
setup
|
||||
out=$(HOME="$WORK/home" DOCTOR_CHECKOUT="$WORK/ck" sh "$DOCTOR" 2>&1)
|
||||
if printf '%s' "$out" | grep -q 'pr-gatekeeper'; then
|
||||
no "doctor is silent when no host-local copy exists (got: $out)"
|
||||
else
|
||||
ok 'doctor is silent when no host-local copy exists'
|
||||
fi
|
||||
|
||||
rm -rf "$WORK"
|
||||
printf '\n%s passed, %s failed\n' "$pass" "$fail"
|
||||
[ "$fail" -eq 0 ]
|
||||
Reference in New Issue
Block a user