mathias/skills
1
0
Fork 0
Code Issues 17 Pull Requests Actions Packages Projects Releases Wiki Activity

feat(pr-gatekeeper): review a staged diff pre-push, for GitOps repos with no PR

Browse Source 
infra#470 resolved a doctrine conflict: root rule 5 wants a PR for risk/medium+,
and mathias/infra forbids branches because Flux watches main — a branch there can
be linted but never reconciled, so a PR reviews a state that was never deployed.
The agreed lane is that such work stays on main and gets a recorded adversarial
review pass BEFORE the push.

That lane names this agent as its reviewer, and the definition only knew how to
review an open PR. A lane whose reviewer cannot see the diff resolves to "the
author was careful", which is what rule 5 was written to replace.

ONE AGENT, TWO MODES — NOT A SECOND FILE

Only three of twelve sections were PR-specific: the diff command, the CI step, and
the verdict. Everything else — the iron laws, "the test is the claim", the three
attack habits, probing without causing the incident, escalation — is diff-agnostic.
Forking a near-duplicate 226-line definition would give the estate two
implementations of one review standard, and they would drift; the same argument
tests/litellm-validate_test.sh makes about its own consolidation.

WHAT DIFFERS, AND WHY

- Diff: three commands, run all of them. Which one holds the change is itself
  information — work that is neither staged nor committed is a working tree, not a
  diff under review. Untracked files appear in neither diff, so `git status
  --porcelain` is how a control that exists only in an unstaged file gets noticed.
- CI: it CANNOT have run, nothing is pushed. So "CI absent" must not block here —
  that rule is for a pushed branch with no runs, which is a platform failure;
  unpushed with no runs is arithmetic. Instead run the local gate and check its
  EXIT CODE, because a grep over gate output has hidden a failing test in this
  estate before.
- Verdict: nothing to merge, no PR to close, no branch to delete. The entire output
  is a comment on the issue. That is what makes the lane RECORDED rather than a
  conversation — a verdict living only in a chat transcript is indistinguishable
  from no review having happened.

The report must state that it carries strictly less external evidence than PR mode:
"nothing I could find blocks this push", never "CI confirms this".

TDD, and one vacuous assertion caught

Five assertions written first, all red. One passed spuriously — an unscoped grep
for "do not merge" matched line 202, the ESCALATION step talking about a PR. A pass
for entirely the wrong reason, which is what rule 9 is about. Now scoped to the
pre-push section, and the comment records why, so nobody re-loosens it.

agents_test 19/19. Repo check exit 0, 45 skills, manifest matches tag.

Refs mathias/infra#470

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
mathias
2026-09-30 10:31:31 +02:00
co-authored by Claude Opus 5
parent c25175e4b3
commit 0dcf5cc136
2 changed files with 136 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
agents/pr-gatekeeper.md
+80 -7
View File
@@ -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
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.
**Pre-push mode — clear to push** only when all hold:
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:
tests/agents_test.sh
+56
View File
@@ -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 ]