Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
55b2e811d1 | ||
|
|
0dcf5cc136 |
@@ -1,7 +1,7 @@
|
|||||||
{
|
{
|
||||||
"name": "skills",
|
"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.",
|
"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.0",
|
||||||
"author": {
|
"author": {
|
||||||
"name": "Mathias",
|
"name": "Mathias",
|
||||||
"url": "https://git.d-ma.be/mathias"
|
"url": "https://git.d-ma.be/mathias"
|
||||||
|
|||||||
+80
-7
@@ -1,6 +1,6 @@
|
|||||||
---
|
---
|
||||||
name: pr-gatekeeper
|
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
|
tools: Read, Grep, Glob, Bash
|
||||||
color: "#7C3AED"
|
color: "#7C3AED"
|
||||||
---
|
---
|
||||||
@@ -20,9 +20,27 @@ difficult. For code whose job is to refuse something, expect it.
|
|||||||
|
|
||||||
## What you are given
|
## 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
|
- `owner/repo`, the PR number, and the issue number it claims to close
|
||||||
- The claim the author made about what the change does
|
- 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
|
## Procedure
|
||||||
|
|
||||||
Work in the repo checkout at the path given to you. Never push, never force,
|
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
|
### 2. Get the real diff
|
||||||
|
|
||||||
|
**PR mode:**
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
git fetch origin
|
git fetch origin
|
||||||
git diff origin/main...origin/<branch>
|
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
|
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.
|
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
|
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.
|
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
|
### 6. Verdict
|
||||||
|
|
||||||
**Merge** only when all hold:
|
**PR mode — merge** only when all hold:
|
||||||
|
|
||||||
- no iron-law violation
|
- no iron-law violation
|
||||||
- every "done when" clause in the issue is satisfied
|
- 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
|
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
|
```bash
|
||||||
flux reconcile kustomization apps --with-source
|
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
|
forward-fixing, and report. A blocked `apps` Kustomization stops every app in
|
||||||
the cluster, not just the changed one.
|
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
|
### 7. Escalate to Mathias — and only then
|
||||||
|
|
||||||
Do not merge. Comment the finding on the issue and report it. Escalate when:
|
Do not merge. Comment the finding on the issue and report it. Escalate when:
|
||||||
|
|||||||
@@ -103,6 +103,62 @@ else
|
|||||||
ok 'doctor is silent when no host-local copy exists'
|
ok 'doctor is silent when no host-local copy exists'
|
||||||
fi
|
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"
|
rm -rf "$WORK"
|
||||||
printf '\n%s passed, %s failed\n' "$pass" "$fail"
|
printf '\n%s passed, %s failed\n' "$pass" "$fail"
|
||||||
[ "$fail" -eq 0 ]
|
[ "$fail" -eq 0 ]
|
||||||
|
|||||||
Reference in New Issue
Block a user