Code Style, Test Quality, and Building Trust

Trust in Firecracker is not granted, it is metered out — one merged PR at a time, by a small AWS team that owns a piece of production infrastructure running your Lambda functions and Fargate tasks. This chapter is the mechanics of earning it: the formatting and static-analysis gates that you must never make a reviewer enforce by hand, the tools/devtool checks that bundle them, the integration-test requirement that turns "it works on my machine" into "it will keep working," and the plain fact that sustained, high-quality contribution plus careful review of others' work is the only currency that buys influence and, eventually, de facto ownership of an area in a single-vendor project.

This is the everyday craft floor beneath everything else in this section. It is the receiving end of PR Quality, it sits directly on top of the licensing discipline in Licensing and the DCO, and it is the last thing you internalize before the capstone makes you do all of it for real.

cd ~/fc-src
# The gates this chapter explains, as the contribution doc describes them (verify wording):
rg -n -i "checkstyle|checkbuild|devtool|clippy|rustfmt|cargo sort|integration test" CONTRIBUTING.md
# The devtool subcommands that drive every gate locally:
tools/devtool --help

The Automated Style Gates: Never Make a Human Enforce Them

A reviewer who has to write "run the formatter" has already spent attention on something a machine should have caught, and you have already signaled that you didn't run the gate yourself. Firecracker enforces style and static analysis mechanically, in CI, with warnings treated as errors. Your job is to make all of it green before anyone looks at the diff. Every one of these is self-service.

The -D warnings flag is the one that surprises people coming from projects where clippy is advisory. In Firecracker it is mandatory: a single clippy warning fails CI exactly like a compile error. A #[allow(...)] to suppress a lint is itself a reviewable decision — you must justify it, not sprinkle it to make the build pass.

The discipline is a fixed sequence — run it every time, in this order, and CI never surprises you:

cd ~/fc-src
tools/devtool fmt                 # 1. cargo fmt + clippy --fix + cargo sort + black/isort/mdformat
git add -A && git commit -s ...   # 2. commit the formatted result (note the -s: DCO sign-off)
tools/devtool checkstyle          # 3. headers + style rules the formatter doesn't auto-fix
tools/devtool checkbuild --all    # 4. build + clippy -D warnings across arches/targets/features

Note: tools/devtool fmt modifies your files (it runs the fixers, not just the checkers). Run it, then git add and commit the result so the formatted code is what you push. A common newbie failure is running only the --check variants, seeing red, and being confused that nothing got fixed — tools/devtool fmt is the one that actually edits.

Tip: CONTRIBUTING.md recommends wiring tools/devtool checkstyle and checkbuild into a git pre-push hook so you physically cannot push a red branch. Pair it with the DCO sign-off habit from licensing-and-dco.md — same muscle memory, two reasons a PR bounces, both eliminated before review.

The devtool Gates: checkstyle and checkbuild --all

Two devtool commands are the local mirror of CI. Internalize what each one protects, because that is what a maintainer is implicitly trusting when they see them green.

The --all in checkbuild --all is load-bearing. Firecracker targets x86_64 and aarch64, builds against musl (the default) and gnu, in debug and release, with various feature flags (e.g. --enable-pci). Code that compiles on your x86_64 musl release build can fail to compile on aarch64, or trip a clippy lint that only fires under a different feature set. checkbuild --all catches the cross-arch and feature-matrix breakage before CI does — and a cross-arch compile failure discovered in CI, after you said it was ready, is a small but real withdrawal from your trust balance.

cd ~/fc-src
# The two commands that must be clean before you open a PR:
tools/devtool checkstyle
tools/devtool checkbuild --all          # slow — but it's what CI runs; run it once before pushing
# Narrow the matrix while iterating, then run --all before the final push:
tools/devtool build --release
cargo clippy --all --all-targets --all-features -- -D warnings

Warning: "It builds for me" means it builds for one point in the matrix. The minimal-device-model philosophy and the aarch64 target mean a non-trivial fraction of breakage is arch- or feature-specific. Run checkbuild --all before you claim a PR is ready, not after a reviewer points at a red CI job.

Test Quality: The Part Maintainers Actually Trust

Style gates are table stakes — green formatting earns the right to be read, nothing more. Test quality is where trust is built or destroyed, because a maintainer's deepest question about your PR is "if this breaks in six months, will a test catch it?" On a VMM that runs hostile multi-tenant workloads, a change with a weak or absent test is one the maintainer now worries about forever — and the maintainer mindset is exactly that worry, professionalized.

Firecracker's test story has two layers, and you must know which one your change needs.

Note — the harness is Python. The primary integration harness is pytest in tests/, not raw cargo test. Unit tests are Rust (cargo test), but the suite that proves a feature works end-to-end is Python-driven, building a real binary and driving it over the API socket. New contributors who go looking for a Rust integration test for "boot with two drives" are looking in the wrong language — rg the pytest tree.

cd ~/fc-src
# Run everything the way CI does:
tools/devtool test
# Run just the unit tests, or just a pytest path while iterating:
tools/devtool test -- -k api          # pass pytest args after the --
# Orient in the integration suite — find the test that already covers your area:
ls tests/integration_tests/
rg -n "def test_" tests/integration_tests/functional/ | head

The integration-test requirement

This is the rule the contribution doc states and the maintainers enforce without exception: new functionality must come with integration tests, and a PR must not lower coverage. It is not a suggestion you argue your way out of with "the unit tests cover it." If your change adds an API field, a device behavior, a boot-config path, or a snapshot guarantee, the trusted proof is a pytest that boots a real microVM and observes the new behavior over the real socket — the only test that exercises the same path a user hits.

The properties of a test a maintainer trusts:

Prove your test actually tests

The cheapest way to earn a reviewer's trust is to demonstrate the test fails without the fix — this is the difference between a test that documents behavior and a test that guards it:

cd ~/fc-src
# 1) Stash the production change, keep the test:
git stash push -- src/vmm/src/...        # the fix only
# 2) Run the new test; it MUST fail (proves it guards the behavior):
tools/devtool test -- -k your_new_test
# 3) Restore the fix; it MUST pass:
git stash pop
tools/devtool test -- -k your_new_test

State in the PR that you did this. "Added a test that fails on main and passes with this change" is a sentence that measurably shortens review, because it answers the maintainer's worry before they have to raise it.

Warning — flaky tests. If you hit a non-deterministic failure that isn't yours, do not silently re-run CI to bury it. Reproduce it, file or find the tracking issue, and fix or correctly mark it — never delete or skip it without a tracked reason. A muted-without-tracking flaky test is a hole in the safety net the whole team relies on. The flaky-test workflow is its own Lab 5.4.

CHANGELOG and Scope Discipline

Two habits make you predictable, which is most of what a reviewer wants:

• CHANGELOG entry. Every user-facing PR adds one line to CHANGELOG.md, under the unreleased section, in the right category (Added / Changed / Deprecated / Removed / Fixed). It is not bureaucracy — it is the raw material for the release notes, and a missing entry is an incomplete PR the checks usually flag before a human does.
• One logical change per commit. Firecracker asks that each commit be a single logical change that passes tests on its own, the title ≤72 chars. Do not bundle a refactor, a feature, and a reformat. A focused PR is faster to review, safer to merge, and a better git bisect target later. If you catch yourself writing "and also" in the description, split it.

cd ~/fc-src
# Confirm the CHANGELOG convention and the unreleased section on your branch:
sed -n '1,40p' CHANGELOG.md

Reviewing Others: The Other Half of Trust

Here is the part newcomers miss. At a single-vendor project, the merge button belongs to the AWS team — you will not get it for a long time, if ever (be clear-eyed: see project-governance.md). But influence is not the merge button. Influence is being the person whose review the maintainers read closely because your comments have repeatedly been right, grounded in the code, and easy to act on. An external contributor can build that, and it is the fastest real path to weight in the project.

What careful review of others' PRs builds, that nothing else does:

• It proves you understand an area deeply enough to catch what a maintainer would catch — the same judgment they need before they trust your changes to that area.
• It reduces the maintainers' load, and load reduction is what a small team values most.
• It makes you a steady technical presence, not a drive-by contributor — and de facto area ownership is granted to the people the team has watched be right, repeatedly, on one subsystem.

# Find PRs in an area you know and review them substantively (not "LGTM"):
gh pr list --repo firecracker-microvm/firecracker --search "virtio" --state open
gh pr view <N> --repo firecracker-microvm/firecracker --comments

A good review reads the code, runs the branch if it's runnable, asks the compatibility and attack-surface questions a maintainer asks (maintainer-mindset.md), and is specific and respectful. Ten of those on one subsystem do more for your standing than thirty scattered typo fixes.

How Trust Compounds Into Influence

The mechanism at a single-vendor project is the same deposits-and-withdrawals ledger as anywhere — the ceiling is just different, and the path runs through influence and area trust rather than a public committer ladder.

flowchart LR
    A["Green gates + integration tests + focused PRs"] --> B["Reviewer spends less effort per PR"]
    B --> C["Faster merges, benefit of the doubt on design calls"]
    C --> D["You review others well, repeatedly, in one area"]
    D --> E["Maintainers trust your judgment on that subsystem"]
    E --> F["De facto area ownership: your input is sought before changes land"]

Each clean, tested, focused PR is a deposit. Each revert, each "please run checkbuild --all," each abandoned PR mid-review, each flaky test left untracked — a withdrawal. The team grants the things you want (fast reviews, design weight, being looped in on your area) on the balance, measured in months to years, not on any single transaction. There is no foundation that will fast-track you; there is only the record.

Trust-Building Checklist

Run this on every PR before you ask for review:

• tools/devtool fmt run and the formatted result committed.
• tools/devtool checkstyle clean (rustfmt, headers, cargo sort, style rules).
• tools/devtool checkbuild --all clean — compiles and clippy -D warnings across both arches, debug/release, all features, musl/gnu.
• tools/devtool test green (unit + pytest integration).
• New functionality has an integration test in tests/; coverage not lowered.
• The new test fails without the production change (you verified this and said so).
• No bare sleeps in new tests; conditions polled with a bounded retry.
• Real assertions on real effects — not "the API returned 204."
• Snapshot/API compatibility considered and tested where the change touches those surfaces.
• CHANGELOG.md entry added under the correct section.
• DCO sign-off on every commit (git commit -s); licensing checklist from licensing-and-dco.md clean.
• PR is one focused logical change; the title reads as the squash commit message (≤72 chars).
• Issue linked; the why explained for the reviewer.

If every box is checked, you have removed every reason a maintainer could send the PR back before reading the logic — which, at a project where review bandwidth is the scarcest resource, is exactly how you earn the bandwidth to have the logic taken seriously.

Prove You Understand This

1. Which tools/devtool command bundles the cross-arch, debug/release, all-features build and clippy check, and why does running it before pushing matter more in Firecracker than in a single-arch project?
2. Clippy is run with -D warnings. What does that change about how you must treat a clippy warning, and what is the reviewable cost of a #[allow(...)]?
3. The primary integration harness is not cargo test. What is it, where does it live, and what class of change requires a test in it?
4. Show, with commands, how you prove a new integration test actually fails without your production change — and why saying so in the PR shortens review.
5. At a single-vendor project you will likely never hold the merge button. Explain, in terms of deposits and area trust, how careful review of others' PRs converts into real influence anyway.
6. Why is a flaky test left untracked a withdrawal from your trust balance even though it isn't a functional bug in your change?

This closes the Release, Review & Governance section. Take all of it — the gates, the integration-test requirement, the licensing discipline, the review habits, the trust ledger — into the capstone: one real issue carried from reproduction through a reviewed, tested, DCO-signed PR to a merge, every gate in this chapter green the first time. That is the whole machine, end to end, and where you prove this section was not just reading.