mathiasandClaude Opus 5
0dcf5cc136
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>
165 lines
6.5 KiB
Bash
Executable File
165 lines
6.5 KiB
Bash
Executable File
#!/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
|
|
|
|
# --- 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 ]
|