Responding to Maintainer Feedback

You opened a clean, scoped, signed-off PR with an integration test and an attack-surface note (PR quality). Now a maintainer leaves eight comments. How you respond over the next several rounds determines whether this merges in days or dies in the queue. The review is not an obstacle between you and a merge — it is the contribution process, and at Firecracker it is the process that produces the two approvals every PR needs. Handling it well is a skill the maintainers grade as carefully as your code.

This chapter is how to iterate the Firecracker way: amend and force-push (not pile-on commits), address every comment, know when to push back and when to concede, and survive the specific reviews Firecracker maintainers give over and over — "this adds attack surface," "this belongs upstream in rust-vmm," "this needs a test." It pairs with Level 2 Lab 4: Review a PR, which puts you on the other side of the table.

Iterate by Amend + Force-Push, Not Pile-On Commits

Firecracker values a clean, bisectable history where each commit builds and passes on its own (PR quality). That requirement does not pause during review. So when you address feedback, you do not add a "fix review comments" commit on top — you fold the fix into the commit it belongs to and force-push the rebased branch.

cd ~/fc-src
# Make the fix in your working tree, then fold it into the right commit.
# If it belongs to the tip commit:
git commit --amend -s --no-edit           # keep the DCO sign-off!
# If it belongs to an earlier commit, fixup + autosquash:
git add -p
git commit --fixup=<sha> -s
git rebase -i --autosquash origin/main     # squashes the fixup into <sha>
# Push the rewritten branch:
git push --force-with-lease

Warning: Always --force-with-lease, never a bare --force. --force-with-lease refuses to overwrite if someone else (or a bot) pushed to your branch since you last fetched — it protects you from clobbering a maintainer's commit. And always keep the DCO Signed-off-by line through amends (git commit --amend -s); the DCO bot re-checks every push.

Why this matters to the reviewer: a history of fix, fix again, address comments, fix lint commits is un-bisectable and forces the reviewer to mentally re-assemble the real change. A clean force-pushed history lets the second approver read the PR as if it were right the first time. GitHub preserves the review threads across a force-push, so you lose nothing.

Address Every Comment — Visibly

A review comment is a request that needs a response. Leaving any comment unaddressed — silently ignored — stalls the PR, because a maintainer will not approve a PR with open questions, and the second maintainer will not approve over an unresolved thread from the first. For each comment, do one of three things, and say which:

Never let a thread go silent. "Done," "Fixed in the latest push," or "I disagree, here's why" all keep the conversation moving; nothing kills a PR faster than the author disappearing for two weeks. If you need time, say so: "Looking into the snapshot-compat concern, will push by Friday" buys you the time and keeps the maintainer engaged.

When to Push Back, and How

Conceding to every comment is not the goal — sometimes the reviewer is wrong, or is missing context you have. A maintainer would rather you push back with evidence than silently implement something you believe is incorrect. But push back correctly:

• With evidence, not opinion. "I think it's fine" loses. "Here's a test showing the old behavior is preserved," "the benchmark shows no regression — numbers attached," "docs/X specifies this ordering," or "the PR that introduced this (#NNNN) explicitly chose Y for reason Z" wins. The archaeology from Design via GitHub is your ammunition.
• On the technical merits, never the person. "This approach has a problem: <X>" not "you're wrong."
• Concede fast when you're wrong. The moment the evidence goes against you, say so and implement their suggestion. A graceful concession builds credibility; digging in on a losing position burns it. Maintainers remember both.

The asymmetry to internalize: pushing back well, occasionally, on the merits marks you as a peer who thinks. Pushing back on everything, or on tone, marks you as someone to avoid. Pick the hills worth dying on, bring evidence, and let the rest go.

The Three Reviews You Will Get Over and Over

Firecracker's nature — security-critical, minimal, built on rust-vmm — produces three recurring review verdicts. Recognize them and you can often pre-empt them in your PR description.

"This adds attack surface"

The most important review at Firecracker, and the one with the highest bar. A new device, a new syscall in a seccomp filter, a new code path reachable from the guest, a new external input — each enlarges what a malicious guest can attack. The maintainer is doing their core job. Your options:

• Show it doesn't. If your change adds no syscall, no device, no guest-reachable path, say so explicitly and point at the diff that proves it. Often the perceived surface isn't real.
• Shrink it. Can the feature be done without the new syscall? Behind a flag that's off by default? With a narrower seccomp rule (an args filter that allows only the specific argument values, not the bare syscall)?
• Justify it, with the cost stated. If the surface is genuinely needed, name exactly what it adds and argue the benefit is worth it to the threat model. "It adds madvise(MADV_DONTNEED) to the vmm filter, scoped to this argument, for the balloon device" is a conversation; "it needs a few syscalls" is a rejection.

# Make the seccomp delta visible and minimal:
rg -n "MADV_|madvise|syscall" resources/seccomp/
git diff origin/main...HEAD -- resources/seccomp/

"This belongs upstream in rust-vmm"

Firecracker shares low-level crates with the broader ecosystem (Cloud Hypervisor and others) through rust-vmm — kvm-ioctls, vm-memory, linux-loader, vm-superio, event-manager, virtio-queue, and more. If your change is generic — a fix or feature in KVM plumbing, memory abstractions, the serial device, the virtqueue ring logic — a maintainer may say it belongs upstream, where every VMM benefits and the logic is maintained once. This is not a brush-off; it's correct stewardship. When it happens:

• Figure out whether the logic is Firecracker-specific (stays here) or generic (goes upstream). The rust-vmm section and what-is-rust-vmm explain the boundary.
• If it's generic, open the PR against the relevant rust-vmm crate, then bump Firecracker's dependency to consume it. Yes, that's slower and crosses two repos. It's also how the ecosystem stays coherent, and doing it without being told marks you as someone who understands the project's place.

# Is the code you're touching Firecracker's own, or a thin wrapper over a rust-vmm crate?
rg -n "kvm-ioctls|vm-memory|vm-superio|virtio-queue|event-manager|linux-loader" Cargo.toml

"This needs a test"

Covered in PR quality, but it recurs in review because contributors underestimate it. The fix is always the same: add the integration test (pytest in tests/) that boots a microVM and asserts the new behavior, and make sure it fails before your change and passes after. "I tested it manually" is not an answer a Firecracker maintainer accepts for a behavioral change — the suite has to encode it so a future refactor can't silently break it.

The ≥2-Approval Reality and Persistence Across Rounds

Firecracker requires two maintainer approvals before a maintainer merges. Plan for what that means:

flowchart LR
    A[Open PR<br/>green, scoped, tested] --> B[Reviewer 1<br/>leaves comments]
    B --> C[Amend + force-push<br/>address every comment]
    C --> D{Reviewer 1<br/>approves?}
    D -->|not yet| B
    D -->|yes| E[Reviewer 2<br/>fresh eyes, new comments]
    E --> F[Amend + force-push again]
    F --> G{Two approvals<br/>+ CI green?}
    G -->|no| E
    G -->|yes| H[A maintainer merges]

Practical consequences:

• Expect more than one round, and a second reviewer with fresh concerns. The second approver was not in the first conversation; they may re-raise something or spot something new. This is not back-tracking; it's the second pair of eyes working as intended. Address their comments with the same care.
• Keep CI green at all times. A red CI after a force-push stalls everything — neither approver acts on a broken PR. Re-run tools/devtool checkstyle && checkbuild --all and the relevant tests before every push.
• Be patient between rounds. Days or weeks can pass; the team is small and supports production. A single polite ping after a reasonable interval is fine. Daily "any update?" is noise that does not move you up the queue (see community interaction).
• Don't go stale. The most common way a good PR dies is the author losing momentum after round two. Persistence through the rounds is itself a signal maintainers weigh when they think about who to trust with more (maintainership).

Worked Example: A Two-Round Review

Round 1, the maintainer comments: (a) "this adds madvise to the seccomp filter — can the rule be scoped to MADV_DONTNEED?", (b) "needs an integration test," (c) "nit: this unwrap() can panic on a malformed guest input — return an error." You respond:

# (a) narrow the seccomp rule to the specific arg, not the bare syscall:
$EDITOR resources/seccomp/x86_64.json     # add an args filter: madvise where advice == MADV_DONTNEED
# (c) replace the unwrap with a propagated error:
$EDITOR src/vmm/src/devices/virtio/balloon/...
# (b) add the proving integration test:
$EDITOR tests/integration_tests/functional/test_balloon.py
tools/devtool test -- tests/integration_tests/functional/test_balloon.py   # fails on main, passes here
tools/devtool checkstyle && tools/devtool checkbuild --all
# fold each fix into the commit it belongs to, keep sign-off, force-push:
git add -p && git commit --fixup=<sha> -s && git rebase -i --autosquash origin/main
git push --force-with-lease

You reply to each thread: "Scoped the rule to MADV_DONTNEED — diff in <sha>," "Added test_balloon_* that fails on main and passes here," "Good catch, now returns BalloonError instead of panicking." Round 2, the second approver asks one question about snapshot compatibility of the balloon state; you answer with evidence (the persist format is unchanged — here's the round-trip test), they approve, the first approver re-approves the force-push, a maintainer merges. Four days, two rounds, two approvals, no drama. That is the target.

Anti-Patterns That Kill PRs in Review

Validation: Prove You Understand This

1. Show the exact commands to fold a review fix into an earlier commit and force-push safely, keeping the DCO sign-off.
2. For a comment you believe is wrong, write a push-back reply that uses evidence (a test, a doc, or the introducing PR) rather than opinion.
3. Explain the three recurring Firecracker reviews ("adds attack surface," "belongs in rust-vmm," "needs a test") and how you'd pre-empt each in your PR description.
4. Given "this adds attack surface," list three concrete ways to respond (show it doesn't / shrink it / justify with the cost stated), with a seccomp-rule-scoping example.
5. Explain why the second approver may raise new concerns and how you should treat them.
6. Describe how you keep CI green and the author engaged across multiple review rounds without adding noise to the thread.

You have absorbed this chapter when a wall of review comments reads to you as a path to merge, not an attack — and when your force-pushed history makes the second approver's job trivial. The next chapter — Compatibility, Stability, Performance — is the lens behind the hardest reviews you'll get, the ones about what you're allowed to break.