Step 9: Engaging on GitHub Through Review

The PR is open and CI is green. Now the part you cannot rush: review. Firecracker needs two maintainer approvals before one of them merges, and the maintainers are a single AWS team with a high bar and finite review time. Most capstone PRs go through two or three rounds before the second approval lands. This step is about running that cycle well — responding to every comment, iterating with amend + force-push the way Firecracker expects, keeping CI green on every revision, and waiting with patience instead of nagging.

How you behave in the PR conversation is, for a maintainer, a preview of what it is like to have you as a colleague. A contributor who addresses comments crisply and without defensiveness gets their next PR reviewed faster. A contributor who argues every nit or goes silent for two weeks does not.

Goal

Drive the PR from "open" to "two approvals and merged" (or to merged-quality if the merge slips past your window): every review comment addressed in code or with a substantive reply, CI green after every push, the PR description kept accurate, and docs updated where the change warrants it.

The Firecracker Review Mechanic: Amend + Force-Push

Firecracker's CONTRIBUTING is explicit: when you address review comments, amend the existing commits and force-push the branch — do not pile "address review" commits on top. The history that merges should be the clean one-logical-change history you built in Step 8, not a trail of fixups. This is different from projects that prefer fixup commits during review; follow this project's convention.

# Make the requested change, then fold it into the right existing commit.
git add src/vmm/src/resources.rs
git commit --amend -s            # if it belongs in the last commit (keep the sign-off!)
# Or, to amend an earlier commit, rebase interactively and 'edit' that commit:
git rebase -i upstream/main      # mark the target commit 'edit', amend, continue

git push --force-with-lease      # NEVER a bare --force; --force-with-lease is safe

Warning: Always force-push with --force-with-lease, never a bare --force. --force-with-lease refuses to overwrite if the remote moved (someone pushed a suggestion, or another machine of yours did) — it prevents you silently clobbering work. And keep the DCO sign-off through every amend (-s); a rebase that drops a Signed-off-by: turns the DCO check red and costs a round.

After a force-push, leave a short comment summarizing what changed since the last review ("Rebased on main; addressed the bounds-check comment by validating the descriptor length before use; added a negative-control unit test"). Reviewers come back to a moved branch and a one-line changelog of the push respects their time.

Address Every Comment — in Code or in Words

The rule is simple: no comment goes unanswered. For each one, you do exactly one of two things:

1. Make the change, then reply to the thread pointing at the commit/line that addresses it ("Done in <abbrev-sha>: now returns the typed error at the boundary"). Then resolve the thread.
2. Make the technical case for not changing it — calmly, with evidence, and a genuine question if you're unsure ("I kept it at the builder because X already validates Y upstream; happy to move it if you'd prefer the check at the API edge — which do you read as the right boundary?"). Let the reviewer respond before resolving.

What you never do is silently ignore a comment, resolve a thread without addressing it, or argue from preference ("but my way is fine"). A nit you disagree with on taste — take it; it is cheaper than the argument and it is the reviewer's house. Save your disagreements for the ones that matter technically, and make those with data.

Tip: When a reviewer asks the same question twice across two PRs, that's a signal your description or comments aren't carrying the reasoning. Fix it in the artifact (the PR body, an in-code comment, the CHANGELOG wording) so it's answered before it's asked next time.

Keep CI Green on Every Revision

Each force-push reruns the Buildkite matrix. A reviewer who comes back to a red check on your latest push reads it as "not ready" and moves on — you lose the round. After every push:

gh pr checks --repo firecracker-microvm/firecracker <pr-number> --watch

If a check goes red, reproduce and fix it locally (checkstyle, checkbuild --all, the relevant test subset — all from Step 8) before asking for another look. Pushing speculative "maybe this fixes CI" commits across the matrix is the failure signal the capstone index warns about. The gates all have a local equivalent; use them.

Keep the PR description accurate as the diff changes. If review moved the fix site or changed the approach, update the "Changes"/"Reason" sections so the top of the PR still describes what's actually there. A stale description forces every new reviewer to reconstruct the change from the diff.

The ≥2-Approval Reality, and Patience

You need two maintainer approvals, and one maintainer merges. That is a deliberate quality bar for privileged host code, not a formality — and it means:

• The first approval is not the finish line. A second maintainer reads it fresh, sometimes raising new points. Expect it; don't treat the first ✅ as done.
• Review latency is real and outside your control. The maintainers triage a large issue/PR flow; a clean small PR still waits days to a couple of weeks for two sets of eyes. This is normal. The capstone index told you to plan the write-up and your next issue search to fill this wait — do exactly that.
• Nagging is counter-productive. A single polite "gentle ping" after a genuine stall (a week-plus of silence with green CI) is fine. Repeated pings, @-mentions of the whole team, or "any update?" every two days erodes the goodwill you're building. Maintainers remember both kinds of contributor.

# Check review state without pinging anyone.
gh pr view <pr-number> --repo firecracker-microvm/firecracker \
  --json reviewDecision,reviews,statusCheckRollup

If the PR truly stalls with no response for a couple of weeks despite green CI and addressed comments, a single courteous nudge — or a mention in the community channels — is appropriate. Lead with respect for their time.

Update Docs When the Change Warrants It

A fix that changes user-visible behavior, an API contract, a default, or an operational procedure needs a docs change in the same PR. Firecracker's docs live in the repo (docs/, SPECIFICATION.md, the swagger src/firecracker/swagger/firecracker.yaml, CHANGELOG.md), so there is no separate docs repo to chase — which means there is also no excuse to skip it.

# Does anything in docs/ or the swagger describe the behavior you changed?
rg -n "vcpu_count|machine-config" docs/ src/firecracker/swagger/firecracker.yaml SPECIFICATION.md

When the change needs no docs, state that explicitly in the PR ("the swagger spec already documents vcpu_count >= 1; this only enforces it, so no docs change is needed"). A reviewer would rather see that judgment than wonder whether you forgot. If your only change is the enforcement of an already-documented contract, the swagger may already be correct — confirm it and say so.

Deliverable for Step 9

• Every review comment addressed: changed in code (with a thread reply pointing at the commit) or answered with a substantive technical reply.
• Iteration via amend + --force-with-lease, DCO sign-off preserved on every commit, with a one-line summary comment after each force-push.
• CI green across the matrix after every push; failures reproduced and fixed locally, not pushed speculatively.
• The PR description kept accurate as the diff evolved.
• Docs/swagger/SPECIFICATION updated where the change warrants it, or an explicit "no docs change needed" with the reason.
• Two maintainer approvals obtained (or merged-quality reached and the PR parked on review latency), reviewers thanked.

Rubric Hooks

This is the Communication dimension (10 pts): every comment addressed, amend+force-push hygiene, --force-with-lease, CI green on every revision, and the patience the ≥2-approval model demands. Docs accuracy also feeds PR craft. A contributor who resolves threads with pushed commits, keeps CI green, and waits gracefully scores high; one who argues nits, force-pushes without summarizing, breaks CI on a later push, or nags scores low. See the evaluation rubric and responding to feedback.

Validation / Self-check

Before advancing to Step 10:

1. No review comment is unanswered; each is resolved by a pushed change or a substantive reply.
2. You iterated by amending commits and force-pushing with --force-with-lease, preserving the DCO sign-off on every commit, and summarized each push.
3. CI is green on the latest revision, not only the first; any red check was reproduced and fixed locally.
4. The PR description still accurately describes the current diff.
5. You updated docs/swagger/SPECIFICATION where the change warranted it, or stated why none was needed.
6. You waited out the ≥2-approval cycle without nagging, and thanked your reviewers.

Then go to Step 10: The Engineering Write-Up.