PR Quality and Preparation

A Firecracker maintainer reviewing your pull request is making a risk decision under unusual stakes: this code will run as privileged host process underneath a multi-tenant cloud, where the guest is untrusted. Will merging it make Firecracker better without introducing a regression, an attack-surface increase, a compatibility break, or a maintenance burden? Everything you do before you click "Create pull request" either lowers that risk or raises it. And because Firecracker needs two maintainer approvals to merge, you are not persuading one reviewer — you are clearing a bar two busy people must both agree you've met.

A high-quality PR is not the one with the cleverest code. It is the one that is easy and safe to say yes to, twice. This chapter is the checklist such a PR meets. It maps to Level 2 Lab 2: Prepare a PR and to the repo's own CONTRIBUTING.md and CHANGELOG.md.

What a Maintainer-Ready Firecracker PR Looks Like

Eight properties, in rough priority order:

1. Focused scope — one logical change. Not "fix bug + reformat + bump a dep + add a feature."
2. One logical change per commit, each commit green — every commit builds and passes tests on its own, so the history is bisectable.
3. DCO sign-off on every commit (git commit -s) — enforced by the DCO bot. There is no CLA.
4. A CHANGELOG.md entry under [Unreleased], in the right category, for any user-visible change.
5. Integration tests (pytest in tests/) for new behavior, plus unit tests; don't lower coverage.
6. A green local gate — tools/devtool checkstyle and tools/devtool checkbuild --all pass, clippy clean at -D warnings, before you push.
7. A clear description linking the issue — what changed, why, how tested, and the attack-surface impact.
8. No scope creep and no new surface you didn't justify — no incidental device, syscall, or dependency the change didn't strictly need.

The rest of the chapter is each of these.

Focused Scope and One Change Per Commit

A reviewer can hold one change in their head; they cannot hold five. A PR that fixes a bug and reformats a module and renames a field is three reviews wearing one hat, and with two required approvals it is six approvals' worth of risk — it will sit. Litmus test before you open it:

cd ~/fc-src
git diff --stat origin/main...HEAD

If the stat lists files you cannot justify as part of the one thing, split them out. Found a second bug while fixing the first? File a separate issue, open a separate PR. "Drive-by" cleanups feel generous; to a reviewer they are noise that hides the real change and expands the blast radius — which, in security-critical host code, is the opposite of what you want.

Firecracker goes further than many projects: each commit should build and pass tests on its own. The history must be bisectable, because when a regression is found months later, a maintainer will git bisect to the commit, not the PR. Structure your branch so each commit is a coherent, green step:

# Each commit should independently pass:
git rebase -i origin/main      # squash WIP, reorder so every commit is self-contained
# Verify a mid-branch commit builds (interactive-bisect-style sanity check):
git stash; git checkout <mid-commit-sha>; tools/devtool build; git checkout -; git stash pop

Note: Interactive rebase (-i) is how you clean history before pushing. After review starts, you will instead amend and force-push — covered in responding to feedback.

Integration Tests: The Non-Negotiable

A PR that changes behavior without a test that would have failed before your change is not ready, full stop. And for Firecracker, "a test" usually means a pytest integration test in tests/, not only a cargo test unit test. New functionality is proven end-to-end: boot a real microVM, exercise the behavior over the API, assert the result.

# Find where a behavior is already tested, and add alongside it:
ls tests/integration_tests/
rg -n "def test_" tests/integration_tests/functional/test_api.py | head

Write the failing test first, watch it fail, then make it pass. A test that passes on both the old and new code proves nothing. Do not lower unit-test coverage — CONTRIBUTING.md calls this out explicitly, and a coverage drop is a near-automatic review comment.

DCO Sign-Off (Not a CLA)

Firecracker requires a Developer Certificate of Origin sign-off on every commit. There is no CLA. The DCO bot fails the PR if any commit lacks a Signed-off-by: line matching your Git identity.

git config user.name  "Your Real Name"
git config user.email "you@example.com"
git commit -s -m "block: fix off-by-one in descriptor length validation"

git commit -s appends:

Signed-off-by: Your Real Name <you@example.com>

Forgot it on commits you already made? Fix the whole branch in one shot:

git rebase --signoff origin/main      # add Signed-off-by to every commit since main
git push --force-with-lease            # force-pushing your own PR branch is normal here

The CHANGELOG Entry

Every user-visible change adds an entry to CHANGELOG.md under the [Unreleased] heading, in the correct category. Forgetting it is a common reason CI or a reviewer bounces the PR.

sed -n '1,40p' ~/fc-src/CHANGELOG.md

The categories follow Keep-a-Changelog (Added / Changed / Deprecated / Removed / Fixed / Security). Write the line for a user reading release notes, not for the reviewer, and link the PR or issue. Anything that changes the API, the snapshot format, defaults, or behavior the user can observe needs an entry; a purely internal refactor or test-only change may not — check recent merged PRs for the convention on your branch.

### Fixed
- Fixed an off-by-one in virtio-block descriptor length validation that could reject valid requests (#NNNN).
### Changed
- `mem_file_path` on snapshot load is deprecated in favor of `mem_backend` (#NNNN).

A Changed, Deprecated, Removed, or Security entry is also a signal to you: you are touching a compatibility surface, and the compatibility chapter applies before you go further.

The Local Gate: checkstyle, checkbuild, clippy

Run the gate locally before pushing. Discovering a clippy or formatting failure in CI after a long build, when you could have caught it in seconds, wastes a review cycle and signals you didn't run the checks the contributing guide tells you to.

cd ~/fc-src
# 1. Auto-format and auto-fix (cargo fmt + clippy --fix + cargo sort + python/markdown formatters).
tools/devtool fmt

# 2. The style gate (formatting, license headers, lints, etc.).
tools/devtool checkstyle

# 3. Build everything, all targets, all features — and run clippy as warnings-as-errors.
tools/devtool checkbuild --all

# Clippy is -D warnings: a single warning fails the build. This is what runs under the hood:
#   cargo clippy --all --all-targets --all-features -- -D warnings

CONTRIBUTING.md recommends wiring these as git hooks so you cannot push a branch that fails them. Do it once:

# Example: a pre-push hook that runs the style + build gate.
printf '#!/bin/sh\ntools/devtool checkstyle && tools/devtool checkbuild --all\n' \
  > .git/hooks/pre-push && chmod +x .git/hooks/pre-push

The Description and the Attack-Surface Note

A reviewer reads the description first and decides whether to invest in the diff. A strong Firecracker PR description has:

• What and why, in plain prose — the problem and your approach. Link the design discussion if there is one (design via GitHub).
• The linked issue — Closes #NNNN (auto-closes on merge) or Related to #NNNN. A non-trivial PR with no linked issue invites "was this discussed?"
• Testing — exactly how you verified it: which tools/devtool test invocations, which manual microVM boot/curl.
• Attack-surface / compatibility impact — state it yourself. "This adds no new syscall to the seccomp filter, no new device, and no API field." Or, honestly: "This adds one madvise flag to the vmm filter; the new rule is <here>." Volunteering this is the single highest-signal thing in a Firecracker description — it tells the maintainer you think like one, and it answers their first question before they ask it.

# Fill the template the repo provides; don't delete it.
cat ~/fc-src/.github/PULL_REQUEST_TEMPLATE.md 2>/dev/null || rg -l -i "pull.request" ~/fc-src/.github/

A Before/After Example

The same fix, prepared two ways.

Before — the PR that sits unmerged:

Title: fixes and cleanup
Branch: 1 commit, 14 files changed, +812 −critical
- Fixes the block descriptor bug
- Also reformats devices/virtio/net/ (unrelated)
- Bumps a rust-vmm dependency "while I was here"
- Adds a debug eprintln! in the run loop
- No test
- No CHANGELOG entry
- No Signed-off-by (DCO bot red)
- Description: "fixed it"

A reviewer sees a 14-file diff, an unrelated dependency bump (a supply-chain and compatibility concern), stray debug output, no test, no sign-off, and no idea what "it" is. This is two reviewers' worth of work to even understand. It will be asked to split, and likely go stale.

After — the PR that merges:

Title: block: fix off-by-one in descriptor length validation
Branch: 1 commit, signed off, builds + passes on its own
- src/vmm/src/devices/virtio/block/...  (the 6-line fix)
- tests/integration_tests/functional/test_block.py  (a test that fails before, passes after)
- CHANGELOG.md  (one line under ### Fixed, links #NNNN)
Description:
  Closes #NNNN. A request with descriptor length == queue max was rejected as
  too large; off-by-one in the bound check. Fix uses <= instead of <.
  Testing: tools/devtool test -- tests/.../test_block.py (new test fails on main,
  passes here); booted a microVM and confirmed the previously-rejected I/O succeeds.
  Attack surface: no new syscalls, no new device surface, no API change.

Two small files of change, a proving test, a CHANGELOG line, a signed-off single commit, and an explicit attack-surface note. A maintainer can understand, verify, and approve this in one sitting — and so can the second.

PR Readiness Checklist

Run this before you open the PR. Every unchecked box is a reason it will sit.

Common Reasons Firecracker PRs Sit Unmerged

Validation: Prove You Understand This

1. Run git diff --stat origin/main...HEAD on a branch and justify every file as part of one logical change — or split it.
2. Restructure a branch so each commit independently builds and passes; verify one mid-branch commit.
3. Add a CHANGELOG.md entry for a hypothetical fix in the correct category, phrased for a user, with the PR link.
4. Show the commands to (a) sign off every commit on an existing branch and (b) safely force-push.
5. Name the integration-test tier and the tools/devtool test invocation for: an API validation change, a virtio-block change, and a seccomp change.
6. Write a PR description for a real or hypothetical change including Closes #NNNN, the testing section, and an explicit attack-surface note.

When every box on the readiness checklist is green on the first push and your description answers the attack-surface question before it's asked, you have made it easy to say yes — twice. The next chapter — Responding to Maintainer Feedback — is what happens after two maintainers start reading it.