The GitHub PR Review Process

This chapter is the mechanics of getting code merged: the pull-request template, the DCO bot, the CI gates (style, clippy -D warnings, build, the pytest integration suite, Kani), the ≥2-maintainer- approval rule, who is actually allowed to press merge, and how review rounds work when a reviewer asks for changes. It is the most operationally important chapter in this section, because it is the process every external contribution passes through, every time, with no exceptions for cleverness or seniority.

The governing reality, stated once so the rest makes sense: Firecracker uses review-then-merge. Nothing reaches main without passing CI and collecting at least two approvals from maintainers, after which a maintainer — not you — merges it. There is no commit-then-review, no "push to a branch and we'll clean it up later." The bar is the same for a one-line typo fix and a new subsystem; what differs is how long the review takes and how hard the questions are.

cd ~/fc-src
# The authoritative process doc. Read it fully; this chapter explains the why.
sed -n '1,200p' CONTRIBUTING.md
# The exact approval/merge wording (verify on your branch — it has tightened over time):
rg -n -i "approv|two maintainer|2 maintainer|merge|review|sign-off|signed-off" CONTRIBUTING.md

The PR Template: Pre-Filled With What Reviewers Will Ask

When you open a PR, GitHub pre-populates it from the repository's pull-request template. That template is not boilerplate — it is the reviewers' checklist, surfaced to you before they see your diff. Every box you leave unticked is a question a reviewer now has to ask, which costs a round-trip and signals you didn't do the work.

cd ~/fc-src
# Find the actual template (location varies — verify on your branch):
find . -iname "pull_request_template*" -o -path "*.github*" -iname "*pull_request*" 2>/dev/null
cat .github/PULL_REQUEST_TEMPLATE.md 2>/dev/null || \
  cat .github/pull_request_template.md 2>/dev/null

The template encodes the project's standards. Expect it to ask you to confirm (verify exact items on your branch — they evolve):

Fill it in honestly and completely. A PR that arrives with the template gutted ("N/A" on everything) is a PR that gets read with suspicion.

The DCO Bot: The First Automated Gate

Firecracker requires a Developer Certificate of Origin sign-off on every commit — not a CLA. The Signed-off-by: trailer is added by git commit -s. A bot checks every commit in the PR and blocks the merge if any commit is missing the trailer or if the email in the trailer doesn't match the commit author. The legal substance is in licensing-and-dco.md; the mechanics you need here:

# Sign off as you commit:
git commit -s -m "vmm: validate balloon size against guest memory"

# Forgot to sign off the last commit? Amend it:
git commit --amend -s --no-edit
git push --force-with-lease

# Sign off a whole branch of commits you already made (rebase, re-signing each):
git rebase --signoff main
git push --force-with-lease

Warning: The sign-off email must match your commit-author email. A green DCO check with a mismatched or anonymized email is a common, avoidable cause of a blocked PR. Set git config user.email to the address you intend to sign with before you start.

The DCO bot is binary and impersonal: it is either green or it blocks merge. No maintainer can wave it through. Get it green before you ask for human review — a red DCO check tells a maintainer the PR isn't ready and they move on to the next one.

The CI Gates: What Must Be Green Before a Human Spends Time

Firecracker's CI (runs on Buildkite and/or GitHub Actions — verify which on your branch) enforces a set of gates that mirror, almost exactly, the local tools/devtool checks. The point of the correspondence is that you can and must make CI green locally first: there is no excuse for a red style or clippy check on a PR, because the same command runs on your machine in seconds.

cd ~/fc-src
# Find what CI actually runs (don't trust this list — read the pipeline on your branch):
find .buildkite .github -type f 2>/dev/null
rg -n -i "devtool|clippy|checkstyle|checkbuild|pytest|kani|cargo test|fmt" \
   .buildkite .github 2>/dev/null | head -40

The clippy gate deserves emphasis because it surprises engineers from more lenient codebases: clippy runs with -D warnings, so warnings are hard errors. A let _ = ... you left in, a needless clone, a redundant closure — any of these fails CI. Run it locally and fix every lint before you push.

cd ~/fc-src
# The exact clippy invocation CI uses (verify flags on your branch):
cargo clippy --all --all-targets --all-features -- -D warnings
# The one-command local pre-flight that approximates the whole gate set:
tools/devtool fmt
tools/devtool checkstyle
tools/devtool checkbuild --all
tools/devtool test

Tip: CONTRIBUTING.md recommends wiring tools/devtool checkstyle and checkbuild as a git pre-commit/pre-push hook. Do it. The cost is one-time; the payoff is never pushing a PR that's red on something you could have caught in three seconds. The full gate discipline is in code-style-trust.md.

The ≥2-Maintainer-Approval Rule and Who Can Merge

Once CI and the DCO bot are green, your PR needs at least two approving reviews from maintainers, and then a maintainer merges it. Decompose that, because each clause is load-bearing:

• Two, not one. A single approval is not enough. This is a deliberate redundancy: Firecracker is security-critical production infrastructure, and two sets of maintainer eyes on every change is the floor. For changes touching the threat model, the bar is effectively higher — the right specific maintainers (e.g. whoever owns the device or the snapshot subsystem) need to be among the two.
• Maintainers, not just anyone. Reviews from non-maintainer contributors are valuable and welcome — they help the maintainers and they build your reputation as a reviewer (code-style-trust.md) — but they don't count toward the two required approvals. Only people on MAINTAINERS.md provide a binding approval.
• A maintainer presses merge, not you. Even with two approvals, you don't merge your own PR. A maintainer does, typically as a clean, signed-off commit history. This is the concrete face of single-vendor governance: the merge button belongs to the AWS team (project-governance.md).

cd ~/fc-src
# Who can give a binding approval (verify the current roster):
sed -n '1,80p' MAINTAINERS.md
# Study a recently merged non-trivial PR end to end — the reviews, the rounds, the merge:
gh pr list --state merged --limit 10
gh pr view <NUMBER> --comments

How Review Rounds Work: Amend, Force-Push, Repeat

Firecracker review is iterative and history-conscious. Reviewers comment; you address; the cycle repeats until two maintainers approve. The mechanical convention is clean commit history — one logical change per commit, each commit independently passing tests, each signed off. That shapes how you respond to feedback:

sequenceDiagram
    participant You
    participant Bot as DCO/CI bots
    participant M1 as Maintainer 1
    participant M2 as Maintainer 2
    You->>Bot: open PR (signed-off, CHANGELOG, tests, template filled)
    Bot-->>You: DCO green, CI green (or fix and re-push)
    M1->>You: review: change request + questions
    You->>You: amend/rebase commits to address; keep history clean
    You->>Bot: force-push (--force-with-lease)
    Bot-->>You: CI re-runs, green
    M1-->>You: approve (1/2)
    M2->>You: second review (often the subsystem owner)
    You->>You: address remaining comments, re-push
    M2-->>You: approve (2/2)
    M2->>You: maintainer merges

The practical conventions:

• Amend and rebase, then force-push with --force-with-lease. Because the project values a clean, bisectable history, you typically fold review fixes into the relevant commit rather than piling on "address review" commits. --force-with-lease (never bare --force) protects against clobbering someone else's push.
• Re-sign after a rebase. git rebase --signoff main preserves the DCO trailers; check the DCO bot stays green after any history rewrite.
• Reply to every thread. Resolve what you fixed; for anything you didn't change, say why, with reasoning. Going silent on a comment stalls the PR — the reviewer won't re-approve a thread they think you ignored. This is the receiving end of responding-to-feedback.md.
• Keep each commit green. Because each commit is expected to pass tests on its own (it makes git bisect and reverts clean), don't leave a "WIP, fixes in next commit" state in the final history.

Note: Force-pushing during review is expected here, not rude — it's how the clean-history convention is honored. What's rude is force-pushing in a way that loses a reviewer's place with no explanation. Leave a short comment summarizing what you changed since their last look.

Reviewer Expectations: What the Two Maintainers Are Looking For

When you eventually review others' PRs (which is how you build the reputation that earns you the benefit of the doubt on your own — code-style-trust.md), and to understand what your reviewers are doing, internalize what a Firecracker maintainer reads for, roughly in priority order:

Notice the ordering. Style is last, not because it's optional (CI made it mandatory) but because it's automatable; the scarce human judgment goes to attack surface, compatibility, and correctness. A PR that's beautifully formatted but expands the attack surface for marginal benefit is in far more trouble than a slightly-rough PR that's obviously safe and well-tested. Optimize for what the human reviewer spends their attention on.

What a Mergeable PR Looks Like (the checklist)

Run this before you request review — every box removes a reason a maintainer could bounce the PR before engaging with its logic:

• Linked to an issue / agreed design (communication-channels.md).
• DCO Signed-off-by on every commit; email matches author; DCO bot green.
• tools/devtool fmt run; tools/devtool checkstyle and checkbuild --all green locally.
• cargo clippy ... -- -D warnings clean — zero warnings.
• tools/devtool test passes; new functionality has integration tests in tests/.
• The new test fails without the production change (you verified this).
• CHANGELOG.md entry added under the correct heading.
• Compatibility impact (API / snapshot / config) stated in the PR description.
• PR template filled honestly; one focused logical change; clean commit history.

If every box is checked, the only thing left for the two maintainers to do is the judgment that actually requires a human — which is exactly how you earn a fast, serious review.

Prove You Understand This

1. State the merge rule in one sentence: how many approvals, from whom, and who presses merge?
2. Your PR's DCO check is red on one of three commits. Give the exact commands to fix it without losing your work.
3. Name the CI gates and the single local command-set that approximates all of them. Which gate fails on a warning, and why does that surprise people?
4. Why does a non-maintainer's approving review not count toward the required two — and why is giving such reviews still worth your time?
5. Walk through a review round: a maintainer requests changes; describe exactly how you respond, including the git mechanics and the DCO implication.
6. List the reviewer's priorities in order. Why is style last despite being mandatory?

Next: The Maintainer Mindset: Compatibility & Risk — what the two reviewers are really worried about when they read your diff.