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.

Threat Awareness

• Coordinated attacks exist
• Competitors may actively harm the project
• Social engineering builds trust before attacking
• "Fixes" may introduce vulnerabilities

Workflow

digraph review_flow {
    rankdir=TB;
    node [shape=box];

    fetch [label="1. Fetch PR details"];
    author [label="2. Assess author risk"];
    files [label="3. Analyze changed files"];
    security [label="4. Security review"];
    compat [label="5. Backward compatibility"];
    quality [label="6. Code quality"];
    classify [label="7. Release classification"];
    verify [label="8. MANDATORY: Independent verification", style=bold];
    recommend [label="9. Final Recommendation"];

    fetch -> author;
    author -> files;
    files -> security;
    security -> compat;
    compat -> quality;
    quality -> classify;
    classify -> verify;
    verify -> recommend;
}

Step 1: Fetch PR Details

# Get PR info
gh pr view <number> --repo kube-hetzner/terraform-hcloud-kube-hetzner

# Get diff
gh pr diff <number> --repo kube-hetzner/terraform-hcloud-kube-hetzner

# Get changed files
gh pr view <number> --repo kube-hetzner/terraform-hcloud-kube-hetzner --json files --jq '.files[].path'

# Get diff stats
gh pr view <number> --repo kube-hetzner/terraform-hcloud-kube-hetzner --json additions,deletions

Step 2: Assess Author Risk

# Check account age
gh api users/<username> --jq '.created_at'

# Check prior contributions
gh pr list --author <username> --repo kube-hetzner/terraform-hcloud-kube-hetzner --state all --json number | jq length

Risk Signals

Step 3: Analyze Changed Files

Security-Critical Files (AUTO HIGH RISK)

init.tf              # Cluster initialization, secrets
main.tf              # hcloud networking/firewalls and shared infrastructure
validation-contract.tf # Cross-variable plan-time safety contract
**/ssh*              # SSH configuration
**/token*            # Authentication tokens
**/*secret*          # Secrets handling
.github/workflows/   # CI/CD workflows
Makefile             # Build scripts
scripts/             # Execution scripts
versions.tf          # Provider dependencies
templates/*.yaml.tpl # Rendered manifests/cloud-init
templates/*.sh.tpl   # Rendered shell scripts
cloud-init*          # Server initialization
packer-template/     # Base image build path

Risk by File Count

Risk by Diff Size

Step 4: Security Review

Checklist

• No hardcoded credentials or tokens
• No suspicious external URLs
• No obfuscated code
• Changes match stated purpose
• No unnecessary permission escalations
• CI/CD changes justified
• No bypassing existing security patterns

Red Flags

Deep Security Analysis

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 cluster
cd /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:

1. correctness and edge cases
2. security and adversarial-change risk
3. Terraform state changes, resource recreation, and upgrade safety
4. consistency with existing patterns and project vision
5. 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:

1. Take the concern seriously - investigate further
2. Re-read the code with the concern in mind
3. Request changes if the concern is valid
4. 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.

Output in Final Review

Include a concise verification summary:

### Verification

| Check | Result | Key Finding |
|-------|--------|-------------|
| Maintainer diff review | PASS/FAIL | <summary> |
| Local tests/plans | PASS/FAIL | <summary> |
| Independent reviewer | PASS/FAIL | <summary> |

Step 9: Final Recommendation

PR Review Output Template

## PR Review: #<number>

**Title:** <title>
**Author:** @<username>
**Files:** <count> files changed (+<additions>/-<deletions>)

### Risk Assessment

| Factor | Value | Risk |
|--------|-------|------|
| Author tenure | X months | 🟢/🟡/🔴 |
| Prior contributions | N PRs | 🟢/🟡/🔴 |
| Files changed | N files | 🟢/🟡/🔴 |
| Lines changed | +X/-Y | 🟢/🟡/🔴 |
| Security-critical files | Yes/No | 🟢/🔴 |
| External dependencies | Yes/No | 🟢/🔴 |

**Overall Risk:** 🔴 HIGH / 🟡 MEDIUM / 🟢 LOW

### Security Review

- [ ] No hardcoded credentials
- [ ] No suspicious external URLs
- [ ] No obfuscated code
- [ ] Changes match stated purpose

### Backward Compatibility

- [ ] No breaking changes
- [ ] terraform plan shows no destruction
- [ ] Existing deployments unaffected

### Release Classification

**Type:** PATCH / MINOR / MAJOR
**Reason:** <explanation>

### Verification

| Check | Result | Key Finding |
|-------|--------|-------------|
| Maintainer diff review | PASS/FAIL | <summary> |
| Local tests/plans | PASS/FAIL | <summary> |
| Independent reviewer | PASS/FAIL | <summary> |

### Recommendation

**Action:** APPROVE / REQUEST CHANGES / CLOSE
**Notes:** <specific concerns or required changes>

Quick Commands

# 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:

1. 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.
2. 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.
3. 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.
4. 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.
5. 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.
6. 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:

pr_head=$(gh pr view <num> --json headRefOid --jq .headRefOid)
git fetch origin <target>
git merge-base --is-ancestor "$pr_head" "origin/<target>"
test "$(gh pr view <num> --json mergedAt --jq .mergedAt)" != "null"

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:

1. Create an integration branch from the target branch
2. Test thoroughly
3. Complete the mandatory independent review gate
4. Then open/merge the integration PR into the target branch with contributor authorship preserved