252 lines
11 KiB
TOML
252 lines
11 KiB
TOML
# formulas/review-pr.toml — PR review formula
|
|
#
|
|
# Defines the review agent's judgment: understand the change, assess quality,
|
|
# decide verdict, write structured output. The bash orchestrator
|
|
# (review/review-pr.sh) handles session lifecycle, metadata fetching,
|
|
# API posting, and cleanup.
|
|
#
|
|
# The orchestrator injects PR context (diff, metadata, previous review)
|
|
# alongside this formula. Claude follows the steps in a single session.
|
|
|
|
name = "review-pr"
|
|
description = "AI-powered PR review: understand change, assess quality, decide verdict"
|
|
version = 1
|
|
model = "sonnet"
|
|
|
|
[context]
|
|
files = ["AGENTS.md"]
|
|
|
|
[[steps]]
|
|
id = "review"
|
|
title = "Review the PR and write structured output"
|
|
description = """
|
|
You have full repo access — you are in a checkout of the PR branch.
|
|
Use this to verify claims, check existing code, and understand context
|
|
before flagging issues. Read files rather than guessing.
|
|
|
|
## 1. Understand the change
|
|
|
|
Read the diff and PR description injected by the orchestrator.
|
|
What is this PR doing and why? Identify the scope:
|
|
- Contracts, frontend, backend, docs, infra, formulas, mixed?
|
|
- New feature, bug fix, refactor, config change?
|
|
|
|
## 2. CI relevance
|
|
|
|
If CI has not passed, decide whether CI matters for this PR.
|
|
Non-code changes (docs, formulas, TOML config, markdown-only) do NOT
|
|
need CI — note this in your review rather than blocking. The orchestrator
|
|
already skips the CI gate for non-code PRs, but you should also mention
|
|
CI relevance in the review body when it applies.
|
|
|
|
## 3. Review checklist
|
|
|
|
Adapt based on scope. Check for:
|
|
|
|
- **Bugs & logic errors**: off-by-one, nil/null dereference, missing edge cases,
|
|
wrong return type, incorrect conditionals
|
|
- **Security**: command injection, unquoted bash variables, path traversal, XSS,
|
|
secret leakage in logs or comments
|
|
- **Imports & dependencies**: broken imports, undefined variables, missing deps
|
|
- **Architecture**: verify patterns match AGENTS.md and project conventions
|
|
- **Bash specifics** (for .sh files): ShellCheck compliance, set -euo pipefail,
|
|
proper quoting, error handling with || true where appropriate, no echo of secrets
|
|
- **Dead code**: unused variables, unreachable branches, leftover debug prints
|
|
- **Claim verification**: if docs/README/AGENTS.md changed, verify claims
|
|
against actual code — read the files to confirm
|
|
|
|
Do NOT flag:
|
|
- Style preferences with no correctness impact
|
|
- Missing tests unless the change is clearly untested and risky
|
|
- Things that look wrong but actually work — verify by reading the code first
|
|
- Files that were truncated from the diff (the orchestrator notes truncation)
|
|
|
|
## 3b. Architecture and documentation consistency
|
|
|
|
For each BEHAVIORAL change in the diff (not pure bug fixes or formatting):
|
|
|
|
1. Identify what behavior changed (e.g., scheduling mechanism, auth flow,
|
|
container lifecycle, secret handling)
|
|
2. Search AGENTS.md for claims about that behavior:
|
|
grep -n '<keyword>' AGENTS.md
|
|
Also check docs/ and any per-directory AGENTS.md files.
|
|
3. Search for Architecture Decision references (AD-001 through AD-006):
|
|
grep -n 'AD-0' AGENTS.md
|
|
Read each AD and check if the PR's changes contradict it.
|
|
4. If the PR changes behavior described in AGENTS.md or contradicts an AD
|
|
but does NOT update the documentation in the same PR:
|
|
REQUEST_CHANGES — require the documentation update in the same PR.
|
|
|
|
This check is SKIPPED for pure bug fixes where the intended behavior is
|
|
unchanged (the code was wrong, not the documentation).
|
|
|
|
## 4. Vault item quality (conditional)
|
|
|
|
If the PR adds or modifies vault item files (`vault/pending/*.md` in the ops repo), apply these
|
|
additional checks. These criteria apply ON TOP of the normal review —
|
|
a vault PR must also pass the standard checklist above.
|
|
|
|
### Required sections
|
|
|
|
Every vault item must contain all five of these sections with non-empty
|
|
content:
|
|
- **What** — what is needed
|
|
- **Why** — what this unblocks and why it matters now
|
|
- **Unblocks** — specific issue numbers (e.g. #123)
|
|
- **Human Action** — specific steps the human should take
|
|
- **Factory Will Then** — what happens after approval
|
|
|
|
If any section is missing or empty, REQUEST_CHANGES. Quote the missing
|
|
section names in your review.
|
|
|
|
### Human Action must be specific
|
|
|
|
The Human Action section must contain concrete, actionable steps — not
|
|
open-ended decisions or vague asks. The filing agent must have done
|
|
the research before escalating to the human.
|
|
|
|
PASS examples:
|
|
- "Go to github.com/organizations/new, create org named disinto"
|
|
- "Add SMTP_HOST=mail.example.com to .env.enc via sops"
|
|
- "Run: ssh root@prod apt install redis-server"
|
|
|
|
FAIL examples:
|
|
- "Decide the approach for #466"
|
|
- "Make a decision about branding"
|
|
- "Figure out which provider to use"
|
|
|
|
If the human action is vague or pushes decision-making to the human,
|
|
REQUEST_CHANGES. Explain that the filing agent must research and
|
|
propose a specific action.
|
|
|
|
### Dedup check
|
|
|
|
Check whether `$OPS_REPO_ROOT/vault/pending/`, `$OPS_REPO_ROOT/vault/approved/`, or `$OPS_REPO_ROOT/vault/fired/`
|
|
already contains a similar item (same resource, same ask). List the
|
|
vault directories in the ops repo to inspect existing items. If a duplicate or
|
|
near-duplicate exists, REQUEST_CHANGES and reference the existing item.
|
|
|
|
## 5. External action detection (token separation)
|
|
|
|
Agents must NEVER execute external actions directly. Any action that touches
|
|
an external system (publish, deploy, post, push to external registry, API
|
|
calls to third-party services) MUST go through vault dispatch — i.e., the
|
|
agent files a vault item (`$OPS_REPO_ROOT/vault/pending/*.json`) and the runner
|
|
container executes it with injected secrets.
|
|
|
|
Scan the diff for these patterns:
|
|
|
|
- **Direct publish commands**: `clawhub publish`, `npm publish`,
|
|
`cargo publish`, `docker push`, `gh release create`, or similar
|
|
- **Direct deploy commands**: `ssh ... deploy`, `rsync` to external hosts,
|
|
`kubectl apply`, `helm install`, cloud CLI deploy commands
|
|
- **Direct external API calls**: `curl`/`wget` to external services with
|
|
auth tokens, posting to social media APIs, sending emails
|
|
- **Token usage**: Direct references to `GITHUB_TOKEN`, `CLAWHUB_TOKEN`,
|
|
or other vault-only secrets in agent code (outside of vault/ directory)
|
|
|
|
If ANY of these patterns appear in agent code (scripts in `dev/`, `action/`,
|
|
`planner/`, `gardener/`, `supervisor/`, `predictor/`, `review/`, `formulas/`,
|
|
`lib/`) WITHOUT routing through vault dispatch (file a vault PR on ops repo — see #73-#77), **REQUEST_CHANGES**.
|
|
|
|
Explain that external actions must use vault dispatch per AD-006. The agent
|
|
should file a vault item instead of executing directly.
|
|
|
|
**Exceptions** (do NOT flag these):
|
|
- Code inside `vault/` — the vault system itself is allowed to handle secrets
|
|
- References in comments or documentation explaining the architecture
|
|
- `bin/disinto` setup commands that manage `.env.vault.enc` and the `run` subcommand
|
|
- Local operations (git push to forge, forge API calls with `FORGE_TOKEN`)
|
|
|
|
## 6. Re-review (if previous review is provided)
|
|
|
|
If the orchestrator injected a previous review and incremental diff:
|
|
1. For each finding in the previous review, check if it was addressed
|
|
2. Report status: fixed / not_fixed / partial with explanation
|
|
3. Check for new issues introduced by the fix commits
|
|
4. Be fair — if feedback was addressed well, acknowledge it
|
|
|
|
Focus on the incremental diff for finding-by-finding status, but use the
|
|
full diff to check overall correctness.
|
|
|
|
## 7. Follow-up issues (pre-existing tech debt)
|
|
|
|
If you discover pre-existing issues (NOT introduced by this PR), create
|
|
tech-debt issues via API so they are tracked separately:
|
|
|
|
# Look up tech-debt label ID (create if missing):
|
|
TECH_DEBT_ID=$(curl -sf -H "Authorization: token $FORGE_TOKEN" \
|
|
"$FORGE_API/labels" | jq -r '.[] | select(.name=="tech-debt") | .id')
|
|
|
|
if [ -z "$TECH_DEBT_ID" ]; then
|
|
TECH_DEBT_ID=$(curl -sf -X POST \
|
|
-H "Authorization: token $FORGE_TOKEN" \
|
|
-H "Content-Type: application/json" \
|
|
"$FORGE_API/labels" \
|
|
-d '{"name":"tech-debt","color":"#6B7280","description":"Pre-existing tech debt flagged by AI review"}' | jq -r '.id')
|
|
fi
|
|
|
|
# Check for duplicate before creating:
|
|
EXISTING=$(curl -sf -H "Authorization: token $FORGE_TOKEN" \
|
|
"$FORGE_API/issues?state=open&labels=tech-debt&limit=50" | \
|
|
jq --arg t "TITLE" '[.[] | select(.title == $t)] | length')
|
|
|
|
# Create only if no duplicate:
|
|
curl -sf -X POST -H "Authorization: token $FORGE_TOKEN" \
|
|
-H "Content-Type: application/json" "$FORGE_API/issues" \
|
|
-d '{"title":"...","body":"Flagged by AI reviewer in PR #NNN.\n\n## Problem\n...\n\n---\n*Auto-created from AI review*","labels":[TECH_DEBT_ID]}'
|
|
|
|
File a tech-debt issue for every finding rated **medium** or higher that
|
|
is pre-existing (not introduced by this PR). Also file for **low** findings
|
|
that represent correctness risks (dead code that masks bugs, misleading
|
|
documentation, unguarded variables under set -u).
|
|
|
|
Do NOT file for: style preferences, naming opinions, missing comments,
|
|
or speculative improvements with no concrete failure mode.
|
|
|
|
When in doubt, file. A closed-as-wontfix tech-debt issue costs nothing;
|
|
an unfiled bug costs a future debugging session.
|
|
|
|
## 8. Verdict
|
|
|
|
Choose one:
|
|
- **APPROVE**: Change is correct, complete, and follows conventions
|
|
- **REQUEST_CHANGES**: Has real issues that must be fixed before merge
|
|
- **DISCUSS**: Unclear intent, design question, or needs human conversation
|
|
|
|
Bias toward APPROVE for small, correct changes. Use REQUEST_CHANGES only
|
|
for actual problems (bugs, security issues, broken functionality, missing
|
|
required behavior). Use DISCUSS sparingly.
|
|
|
|
Note: The bias toward APPROVE applies to code correctness and style decisions.
|
|
It does NOT apply to documentation consistency (step 3b) or tech-debt filing
|
|
(step 7) — those are separate concerns that should be handled regardless of
|
|
the change's correctness.
|
|
|
|
## 9. Output
|
|
|
|
Write a single JSON object to the file path from REVIEW_OUTPUT_FILE.
|
|
Use jq to ensure proper JSON escaping of the markdown content:
|
|
|
|
jq -n \
|
|
--arg verdict "APPROVE" \
|
|
--arg verdict_reason "one-line explanation" \
|
|
--arg review_markdown "### Section\\n- **severity** \\`location\\`: description" \
|
|
'{verdict: $verdict, verdict_reason: $verdict_reason, review_markdown: $review_markdown}' \
|
|
> "$REVIEW_OUTPUT_FILE"
|
|
|
|
The review_markdown field must contain the complete, formatted review in
|
|
markdown. Use ### headings for sections. Use this format for findings:
|
|
- **severity** `file:line`: description
|
|
|
|
For a re-review, structure the markdown as:
|
|
### Previous Findings
|
|
- finding summary → FIXED / NOT FIXED / PARTIAL: explanation
|
|
### New Issues (if any)
|
|
- **severity** `location`: description
|
|
|
|
After writing the JSON file, signal completion:
|
|
echo "PHASE:done" > "$PHASE_FILE"
|
|
|
|
Then STOP and wait. The orchestrator will post your review to the forge.
|
|
"""
|