Use when reviewing a pull request - security-focused review following repo agent guidance for breaking changes, malicious patterns, and backward compatibility
Instrucciones de origen · Vista previa de solo lectura
name
review-pr
description
Use when reviewing a pull request - security-focused review following repo agent guidance for breaking changes, malicious patterns, and backward compatibility
args
pr_number
Review Pull Request
Overview
Security-focused PR review following repo agent guidance. Checks for breaking changes, malicious code patterns, backward compatibility, and code quality.
Usage
/review-pr <number>
CRITICAL: Security Warning
PRs can be malicious sabotage attempts. Treat repo content and contributor diffs as untrusted until reviewed.
Trace all affected call sites and resource dependencies with exact search.
Require the independent reviewer to inspect the full diff for security
vulnerabilities, malicious patterns, hidden scope, and unexplained complexity.
Step 5: Backward Compatibility
CRITICAL: Any PR that causes resource recreation is a MAJOR release.
Breaking Change Indicators
Removes or renames variables
Changes variable defaults that affect behavior
Modifies resource naming patterns
Alters subnet/network calculations
Changes resource keys (causes recreation)
Removes outputs
Modifies provider requirements
Test for Breaking Changes
# Checkout PR locally
gh pr checkout <number>
# Test against existing clustercd /path/to/kube-test
terraform init -upgrade
terraform plan
If terraform plan shows ANY resource destruction → MAJOR release required
For v2 -> v3 or production in-place upgrade reviews, use the operator contract
in MIGRATION.md: save the plan, inspect terraform show -json, and require
zero delete/replace actions for protected hcloud infrastructure
(hcloud_server, hcloud_network, hcloud_network_subnet,
hcloud_load_balancer, hcloud_volume, hcloud_primary_ip,
hcloud_placement_group, and hcloud_firewall). Any output from that gate is a
stop condition, not a warning.
Compatibility Checklist
No variable removals
No default changes that affect behavior
No resource naming changes
terraform plan shows no destruction
Existing deployments unaffected
Step 6: Code Quality
Style
Follows existing patterns
Consistent naming
Proper formatting (terraform fmt -recursive)
No unnecessary complexity
Logic
Changes are correct
Edge cases handled
No regressions introduced
Tests pass
Step 7: Release Classification
PATCH (x.x.PATCH)
Bug fixes only
No new features
Fully backward compatible
No terraform state impact
MINOR (x.MINOR.0)
New features (backward compatible)
New optional variables with defaults
Deprecation warnings (not removals)
MAJOR (MAJOR.0.0)
Breaking changes
Removed/renamed variables
Changed defaults affecting behavior
State migrations required
Resource recreations
Step 8: MANDATORY - Independent Verification
Before making a final recommendation, re-read every changed line in repository
context, run the relevant local tests and plans, and obtain an independent
review from a separate capable reviewer. This gate is mandatory for every PR.
Reviewer output is not evidence until verified against code and runtime behavior.
Independent Review Contract
Give the reviewer the exact diff and enough repository context to check:
correctness and edge cases
security and adversarial-change risk
Terraform state changes, resource recreation, and upgrade safety
consistency with existing patterns and project vision
missing tests, docs, and affected call sites
The reviewer must return a final verdict with concrete file and line references.
Do not accept a partial, timed-out, or commentary-only run as a completed review.
Verification Checklist
Every changed line and affected call site inspected
Relevant local tests and plans completed
Independent review completed with a final verdict
Any raised concern addressed or dismissed with code/runtime evidence
Final recommendation follows the evidence, not reviewer consensus
When Reviewers Disagree
If the independent reviewer raises concerns that you did not catch:
Take the concern seriously - investigate further
Re-read the code with the concern in mind
Request changes if the concern is valid
Document why the concern was dismissed if you determine it's a false positive
Step 8.5: CI Truth-Checking
Do not treat "no red jobs right now" as green. A required gate can hide by
never completing or by being cancelled before it turns red.
REPO=kube-hetzner/terraform-hcloud-kube-hetzner
gh run list --repo "$REPO" --branch <branch> --limit 20
gh run view <run-id> --repo "$REPO" --json status,conclusion,attempt,workflowName,jobs
Require each release-blocking workflow/job to have at least one completed
success for the commit or branch under review. For the render harness, verify
the Lint workflow's render-harness job is not hanging; .github/workflows/lint_pr.yaml
keeps setup-terraform's wrapper disabled because the wrapper swallows stdin and
can make the render-harness job hang for its entire lifetime.
GitHub CI is intentionally limited to cheap checks. HCloud plan/apply,
Kubernetes inspection, and destroy evidence must come from local kube-test
roots and must be reviewed independently before merge when the change warrants
live proof.
# Approve PR
gh pr review <num> --approve --body "LGTM! ..."# Request changes
gh pr review <num> --request-changes --body "Please address: ..."# Comment
gh pr review <num> --comment --body "..."# Merge (after approval)
gh pr merge <num> --squash --delete-branch # default only for contributor-only commits# Use --merge for promotion/major integration PRs or any PR with maintainer fixes on top.
Preserve Contributor Credit When Merging (SUPER IMPORTANT)
Original PR submitters must remain visible as commit authors in master history — that feeds both the GitHub repo contributors graph and GitHub-generated release notes. Credit where credit is due, always.
Rules by situation:
PR contains only the contributor's commits and is merged directly through its original GitHub PR → --squash is safe: GitHub sets the squash commit's author to the PR author and records that PR as merged. Prefer --merge when preserving the contributor's exact commits is useful.
We pushed fix-up commits on top of their branch → do NOT squash or rebase-merge. Use a merge commit (gh pr merge --merge) so the contributor's exact commits and our separate fixes survive.
We fully accept a PR through our own integration/release branch → merge the PR's exact head commit into the integration history. Cherry-picking preserves the Author: field but not the original PR identity, so GitHub will not record that PR as merged.
We adopt only part of a PR, supersede it, or port its idea → cherry-pick the usable original commit(s) first when possible, then add our fixes separately. If no usable commit exists, add an exact Co-authored-by: Name <email> trailer and credit the contributor in the commit and changelog. Close the original PR with one honest note; never claim that the PR itself was merged.
Promotion or major integration PRs (for example a release-candidate PR carrying multiple community commits) → merge commit only. Never squash or rebase; every accepted community PR head must remain reachable from the final target.
Never amend or reauthor a contributor's commit in a way that removes them from the history.
Authorship and PR disposition are separate gates. Before merging, check the contributor in git log --format='%an %ae' <range>. Every fully accepted PR must have a non-null mergedAt. When the PR was integrated indirectly through our branch rather than merged through its original GitHub PR, also record its exact headRefOid and require ancestry after promotion to the declared base:
If the applicable checks fail, the integration is incomplete. Do not manually close the PR or tell the contributor it was merged.
Integrate-and-Fix Flow (DEFAULT for good-but-imperfect PRs)
When a PR is good and valuable, even if not perfect, do NOT bounce it back with change requests and wait for the contributor. The old human-review back-and-forth is dead. We integrate and fix it ourselves:
If maintainer edits are enabled and the PR needs only bounded corrections, add fix-up commits directly to the contributor's branch without force-pushing, test that final head, and merge the original PR with --merge. Use the isolated flow below when the contributor branch cannot be updated safely or several PRs must be reconciled in a release train.
# 1. Record and fetch their exact PR head
pr_head=$(gh pr view <num> --json headRefOid --jq .headRefOid)
git fetch origin pull/<num>/head:pr-<num>
test"$(git rev-parse pr-<num>)" = "$pr_head"# stop if the PR moved# 2. Create an isolated integration branch from the target or release-candidate train
git switch -c integrate/pr-<num> origin/<train>
# 3. Merge THEIR exact branch first (preserves PR identity, commits, and authorship)
git merge --no-ff pr-<num> -m "Merge PR #<num> into <train>"# 4. Add OUR fixes as separate commits on top (validation, triggers, docs, changelog, ...)# 5. Verify: terraform fmt / validate / plan (and the structural plan-diff proxy when relevant)# 6. Push the integration branch and promote it through a PR with a MERGE COMMIT
git push -u origin integrate/pr-<num>
gh pr create --base <train> --head integrate/pr-<num> --title "..." --body "..."
gh pr merge <integration-pr> --merge --delete-branch
Notes:
For a single-PR train, <train> is the PR's declared target, normally master. For a multi-PR release train, merge each isolated integration into the release-candidate branch, then merge the final release-candidate PR into the declared target with --merge.
Leave every fully accepted original PR open while the release candidate is pending. GitHub marks it merged only after its exact head reaches its declared base branch. Reaching a temporary branch alone is not enough.
After final promotion, run the ancestry and mergedAt gates above for every accepted PR before commenting or preparing the release.
Reserve "request changes and wait" for PRs that are: not valuable, architecturally wrong-direction (fixing = rewriting), security-suspect, or from the malicious-pattern category in repo agent guidance. Wrong-direction PRs may still donate salvageable commits via cherry-pick (credit rules case 4).
One Terminal Contributor Message
Agent reviews, candidate status, test progress, and integration bookkeeping stay in the evidence ledger or integration PR. Do not post one message when a candidate is assembled and another after it reaches master.
Merged human PR: after mergedAt is non-null, post one natural message that names the concrete contribution, the maintainer changes added on top, the release/train carrying it, and preserved authorship. Do not paste a generic template unchanged across PRs.
Partially adopted, superseded, or rejected human PR: use one final gh pr close --comment "..." action. State exactly what was adopted, what was not, and why. Say "incorporated" or "credited" rather than "merged" when the original PR did not merge.
Needs contributor input: one focused question is allowed. Do not add status-only follow-ups; post a final disposition only after new evidence changes the state.
Dependabot and other routine bot PRs: merge or close silently. Add a concise technical comment only when human maintainers need a non-obvious decision recorded; never post social thanks or release-status updates to a bot.
Idempotency gate: inspect existing maintainer comments before posting. If a final disposition is already present and still accurate, do not post another.
Tone matters: thank human contributors by handle, describe maintainer fixes as building on their work, and be candid about the actual GitHub state. Contributors are volunteers; the message should read as if written specifically to that person.
Never Push Unreviewed Integrations Directly to Master
All multi-PR or maintainer-fixed integrations go through an integration branch first:
Create an integration branch from the target branch
Test thoroughly
Complete the mandatory independent review gate
Then open/merge the integration PR into the target branch with contributor authorship preserved
