From e1626282b1d6c9cef9eeca543d5fb329ee9ebccd Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 21 Aug 2026 09:32:02 -0500 Subject: [PATCH] Add verification and automation review rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of BiggerPockets/biggerpockets#30384 approved a scheduled job that could never do its work, and stated the defect was correct: the job declared issues: read, the /cwa check reads comments on a PR-backed issue, that endpoint also needs pull-requests: read, and the summary recorded the scope as correctly declared. The finding that would have caught it — a swallowed curl -sf failure — was retired as parity with sibling workflows and as self-healing, reasoning that held for the DELETE call it was written about and not for the comments call it was applied to. verification-rules.md governs claims and dismissals: no affirmative correctness claim without a check that could have failed, parity is evidence about regression rather than about correctness, and a retired finding names its call site instead of generalizing. automation-rules.md gives CI and unattended-job diffs the domain block the other four shared rules already give app code: surfaced failures on external calls, declared scopes checked against the endpoint, a fail-closed guard that separates nothing-to-do from could-not-tell, and a success signal that means something. Both Claude arms and the Codex first pass get the automation step, and both arms get the verification rules on the validate step, so the arms stay byte-identical through their shared steps and remain comparable. --- prompts/_shared/automation-rules.md | 26 +++++++++++++++++++++++ prompts/_shared/verification-rules.md | 22 +++++++++++++++++++ prompts/claude-synthesize-thesis-first.md | 11 +++++++--- prompts/claude-synthesize.md | 11 +++++++--- prompts/codex-first-pass.md | 5 ++++- 5 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 prompts/_shared/automation-rules.md create mode 100644 prompts/_shared/verification-rules.md diff --git a/prompts/_shared/automation-rules.md b/prompts/_shared/automation-rules.md new file mode 100644 index 0000000..573d7a1 --- /dev/null +++ b/prompts/_shared/automation-rules.md @@ -0,0 +1,26 @@ +When the diff adds or changes CI configuration, a GitHub Actions workflow, a scheduled +job, or any other unattended automation, review it as production code whose only user is +a log nobody reads. Flag each genuine problem with a file/line reference: +- Every external call must have its failure surfaced. A non-2xx response or a failed + request that is swallowed into an empty string or an ignored exit status turns a broken + job into a green one. `curl -sf ... || true`, an unchecked exit status before a success + log line, and a pipeline without `set -o pipefail` are all this defect. Require that the + status be captured and either fail the step or emit a `::warning::`/`::error::` + annotation naming what could not be reached. +- Check that the token scopes and permissions the job declares actually cover the API + calls it makes, against the endpoint's documented requirements rather than against a + sibling workflow. A too-narrow scope does not fail loudly; it returns 403 and the script + proceeds down whatever branch an empty response selects. GitHub Actions in particular: + a `permissions:` block replaces the default set rather than adding to it, and reading or + writing comments on a pull request needs `pull-requests` even though the endpoint path + says `issues`. +- A guard that fails closed must be able to tell "nothing to do" from "could not tell". A + conservative default is right, but if the safe branch is also what an outage, a + permission error, or an empty response selects, then the automation can no-op forever + while every run reports success. Require that the two cases log differently and that the + error case is visible without reading the log line by line. +- Judge whether the job's success signal means anything. A step that exits 0 on the path + where it did nothing, or that prints a success message before verifying the action took + effect, gives no way to notice the feature is dead. Say what the job should assert, or + what it should count, so that a run which accomplished nothing is distinguishable from a + run with nothing to accomplish. diff --git a/prompts/_shared/verification-rules.md b/prompts/_shared/verification-rules.md new file mode 100644 index 0000000..5be184b --- /dev/null +++ b/prompts/_shared/verification-rules.md @@ -0,0 +1,22 @@ +These rules govern how you report what you checked and how you retire a finding. They +exist because a review that states a wrong conclusion confidently is worse than one that +says nothing: it closes the question for every later reader. +- Do not write that something is correct, valid, sufficient, or safe unless you ran a + check that could have come back the other way. Confirming that a value is *present* is + not confirming that it is *right* — say "declares X" rather than "correctly declares X" + when presence is all you established. When the diff declares a token scope, permission, + credential, environment variable, version constraint, or config key, correctness means + checking it against the thing that consumes it: the API's own documented requirements, + the tool's schema, the reading code. A neighbouring file that declares something similar + is not that check. +- Matching a pattern that already exists in the repository is evidence about whether a + finding is a *regression*, never evidence about whether it is a *bug*. In a file the diff + adds there is no prior behavior to preserve, so parity cannot retire a finding — at most + it widens it. Write "the same problem exists in X", not "therefore it is not a problem", + and let the verdict rest on the impact, not the precedent. +- When you retire a finding — yours, Codex's, or one raised earlier in the thread — name + the exact call site and state the conditions under which your reasoning holds. Reasoning + that a failure is self-healing, tolerable, or cosmetic is a claim about one specific call + under specific conditions, not about a construct. If the same construct appears elsewhere + in the diff, judge each occurrence on its own; a dismissal that silently generalizes is + how a real defect ships with an approval attached. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/claude-synthesize-thesis-first.md index 6f0b71f..fa845d7 100644 --- a/prompts/claude-synthesize-thesis-first.md +++ b/prompts/claude-synthesize-thesis-first.md @@ -34,11 +34,16 @@ Synthesize a single review decision for pull request #{{PR}}. 8. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -9. Validate which of Codex's findings are real (discard false positives), add any genuine +9. When the diff touches CI config, a GitHub Actions workflow, or any unattended job, + review it per these rules: + {{@prompts/_shared/automation-rules.md}} +10. Validate which of Codex's findings are real (discard false positives), add any genuine issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal - acceptance criteria in a sound way rather than flagging it as non-compliant. -10. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one + acceptance criteria in a sound way rather than flagging it as non-compliant. Hold every + claim you make and every finding you retire to these rules: + {{@prompts/_shared/verification-rules.md}} +11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue; otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. diff --git a/prompts/claude-synthesize.md b/prompts/claude-synthesize.md index 32738b6..5703bac 100644 --- a/prompts/claude-synthesize.md +++ b/prompts/claude-synthesize.md @@ -34,11 +34,16 @@ Synthesize a single review decision for pull request #{{PR}}. 8. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -9. Validate which of Codex's findings are real (discard false positives), add any genuine +9. When the diff touches CI config, a GitHub Actions workflow, or any unattended job, + review it per these rules: + {{@prompts/_shared/automation-rules.md}} +10. Validate which of Codex's findings are real (discard false positives), add any genuine issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal - acceptance criteria in a sound way rather than flagging it as non-compliant. -10. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one + acceptance criteria in a sound way rather than flagging it as non-compliant. Hold every + claim you make and every finding you retire to these rules: + {{@prompts/_shared/verification-rules.md}} +11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue; otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. diff --git a/prompts/codex-first-pass.md b/prompts/codex-first-pass.md index b892914..1e7f98b 100644 --- a/prompts/codex-first-pass.md +++ b/prompts/codex-first-pass.md @@ -28,7 +28,10 @@ verbatim and handed to a second reviewer, so do not add conversational preamble. 7. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -8. Report concrete issues — bugs, regressions, security problems, member-privacy +8. When the diff touches CI config, a GitHub Actions workflow, or any unattended job, + review it per these rules: + {{@prompts/_shared/automation-rules.md}} +9. Report concrete issues — bugs, regressions, security problems, member-privacy violations, and genuine misses of the ticket's intent or clear scope creep — each with a file/line reference and a brief rationale. Do not list "doesn't match acceptance criteria" as an issue by itself; only raise it when the deviation harms the intent. If nothing is