What should block a pull request?

A reviewer leaves a suggestion, and the author treats it as a condition for approval. Another reviewer proposes a different approach. After three rounds, everyone has contributed, but nobody is quite sure what still prevents the change from merging.

The problem is not necessarily that anyone is unreasonable. Code review mixes several conversations into the same comment box: whether the change is correct, whether its design is maintainable, whether it follows team conventions, and whether someone would personally write it differently. When those conversations carry the same apparent weight, a useful review becomes a negotiation over an undefined finish line.

That ambiguity matters in interviews too. Asked about a technical disagreement, it is easy to describe how you stayed polite or eventually persuaded a teammate. The more interesting question is how you decided which concern deserved to stop delivery, what evidence changed your position, and when you were willing to approve an approach that was not your first choice.

A blocker needs an explainable reason and a clear way to resolve it. That reason can be a failure, a security boundary, an established standard, or a design cost the team should not accept. It does not have to be a failing test, but it needs more substance than "I prefer the other version."

One pull request, five different conversations

Consider a fictional change that adds an export button to a customer list. The agreed first release exports the approved fields for every matching customer, up to 10,000 records. If the result is larger, the user must refine the filter; the service must enforce that limit rather than relying on the browser. The application already has a shared filter builder and a file writer.

The author adapts the list query into a new export handler, copies its filter conditions, collects the results in memory, and passes them to the writer. The happy-path test uses twelve customers and passes. Reviewers leave these comments:

All five comments might be worth discussing. They are not automatically five reasons to withhold approval. Before changing code, the author and reviewers need to establish the consequence behind each concern.

This is not a rule that delivery always wins. Google's code-review standard asks reviewers to protect code health while allowing meaningful improvements to proceed without requiring perfection. It also distinguishes personal preference from design concerns grounded in engineering principles. The useful balance is to require a defensible change, not necessarily the reviewer's favorite implementation.

Start with the behavior the change promises

The pagination comment is a blocker. An export of all matching customers cannot silently contain only the first hundred. The twelve-record test tells us that the writer works for that fixture; it does not establish that the export contract is met.

A useful comment makes both the defect and the missing evidence visible:

Required before merge: this call uses the list query's default page size, so an export with 101 matching customers omits the final record. Please use an export query that covers the complete permitted result and add a test crossing the page boundary.

The reviewer has not dictated every implementation detail. The author can propose another approach, but any acceptable solution must preserve the required behavior. If the author believed "export" meant the visible page, the disagreement is partly a requirements problem. Resolve that with the relevant product owner rather than asking the code to settle an ambiguous promise.

Correctness and security concerns often deserve this kind of explicit stop: returning another customer's data, logging credentials, changing a public contract without a migration path, or making an irreversible operation unsafe. The reviewer should identify the affected boundary and plausible failure path. The author should not need to guess why a brief "please change this" is urgent.

Missing tests need similar precision. "Needs more tests" leaves an open-ended assignment. "We have no coverage proving the server rejects an export over the agreed limit" identifies an important claim that remains unverified. Test count is not the acceptance criterion; evidence for the relevant behavior is.

A design concern can block without a demonstrated bug

The memory comment is less settled. "It works on my machine" does not answer it, but "streaming is better" does not answer it either.

For this example, useful questions include whether the server really enforces the limit, how large the permitted fields can be, whether serialization creates additional copies, how many exports can run together, and what resource budget the application has. Ten thousand short rows and ten thousand rows containing large free-text fields are different workloads. A row count alone is not a memory budget.

The reviewer could write:

Needs evidence before approval: this materializes the export in memory. I cannot yet see a server-enforced limit or a measurement for the largest permitted result. Can we establish those bounds and check peak memory under the expected concurrency before deciding whether this approach is safe?

That is a concrete request even though the reviewer has not reproduced an outage. The open question is whether the design fits the operating conditions. A measurement, an enforceable bound, or a revised implementation can resolve it. If there is not enough information to evaluate a material risk, explicitly asking for that information is better than declaring either "fine" or "bad architecture."

Other design concerns may be resolved through reasoning rather than a benchmark. Suppose every future export field would require changes in three independent mappings. The reviewer can point to those mappings, explain how they could drift, and ask for one authoritative definition. That is a maintainability issue, even when today's tests pass. The author can propose a smaller way to remove the duplication instead of accepting an entire new framework.

The distinction is between an explainable cost and an alternative taste. "This is too complex" becomes actionable when it identifies the repeated decisions, hidden coupling, or difficult behavior that makes the code costly to change. "I normally use a strategy pattern" is not enough on its own.

Standards should be recognizable before the review

The shared filter builder matters because the application has already chosen one place to express filtering behavior. Copying its conditions into the export introduces another implementation that can drift. A customer excluded from the list could then appear in the export after only one path is updated.

In this case, reusing the shared builder is a reasonable requirement. The reviewer should point to the existing path and the behavior it preserves, rather than saying "this is how we do things" as if familiarity settles every question.

Some standards have smaller consequences but still legitimately apply: a public naming convention, an agreed error contract, or repository formatting rules. Make those expectations accessible and apply them consistently. Where a formatter or linter can enforce a mechanical rule, let it do that work instead of repeatedly negotiating the same detail in comments.

An undocumented preference held by one reviewer is not automatically a team standard. If the author follows an existing pattern and the reviewer wants to change that pattern across the repository, the proposed change deserves an explicit team decision. A PR can expose the need for a standard without becoming the place where one person invents it retroactively.

Consistency also has limits. An established approach that creates a real security or correctness problem should not be copied merely to make the new code resemble the old code. Identify the exception, explain it, and agree on its scope.

Useful suggestions do not all need to be conditions

In a small export function, rows may be perfectly clear. exportRows might be a slight improvement, but it need not stop the change. The reviewer can label it optional and allow the author to keep the existing name.

In a different function containing imported rows, rejected rows, and export rows, the same suggestion may prevent a real misunderstanding. Naming is not automatically trivial; its importance depends on what a reader could reasonably get wrong.

Google's guidance on review comments recommends making required changes distinguishable from suggestions and informational feedback. Choose wording that suits the team. A small prefix such as "required," "optional," or "question" can help, but the explanation still matters. Calling a preference a blocker does not make it one, and calling a serious concern a question should not make it disappear.

A reviewer who approves while leaving optional feedback should say so explicitly. The author can then consider the improvement without trying to infer whether an unresolved thread means the change is still unacceptable. Approval means the current approach meets the agreed bar, not that there is nothing left to learn or improve.

Separate the immediate risk from the larger solution

The background export service could be a good proposal. It could also add queues, job state, cancellation, cleanup, and operational responsibilities that this bounded first release does not need.

The author should not dismiss it just because it increases the work. The reviewer should not require it simply because it would handle a larger future system. First ask what problem makes background processing necessary now. Perhaps the permitted export exceeds request timeouts, the operation cannot fit the service's resource budget, or the product requires workloads beyond the agreed limit. Those findings could justify blocking the synchronous approach.

If the immediate design is defensible within the approved limits, the team can keep that implementation and consider a background service separately. If it is not defensible, safe options include changing the implementation, reducing the supported scope with product agreement, or postponing the capability. Quietly returning fewer records is not a scope reduction; it is a broken contract.

An issue for later does not make a current release blocker harmless. When follow-up work genuinely can wait, record the concern, the owner, and what would bring the decision back: a new requirement, a measured resource threshold, or a scheduled design discussion. A ticket labeled "technical debt" without context is unlikely to preserve the reasoning.

The author has responsibilities too

Clear review feedback is only half of the process. An author should make the change reviewable: explain the intended behavior, important limits, relevant tests, and any trade-off that is not obvious from the diff. A reviewer can still challenge those claims, but does not have to reconstruct the entire decision from implementation details.

When a comment arrives, ask what it requires if that is unclear. Do not silently implement three contradictory suggestions and hope the last version satisfies everyone. Equally, do not respond to every concern with "out of scope." A local correction that makes the feature safe belongs in the current change even if it was missing from the initial plan.

A useful reply might be:

I agree that the resource bound needs to be established before merge. I'll enforce the limit on the server and add the relevant measurements. If those support the current implementation, can we keep the background-service proposal as a separate decision? If they do not, I'll revisit the approach.

This acknowledges the concern without treating the proposed solution as the only possible answer. It also commits the author to changing course if the evidence does not support the original design.

When an explanation reveals that the code is unnecessarily hard to understand, improve the code rather than leaving all the clarity in a review thread. Important constraints should survive in an appropriate place: tests, code structure, a concise comment, or documentation that future maintainers can find.

End the review with a merge decision

After discussion, our fictional PR has a much clearer finish line:

Before approval, the export must include the complete permitted result, reuse the shared filter behavior, and enforce the agreed limit on the server. Tests must cover the pagination boundary and limit behavior, and resource measurements must support the in-memory approach. The variable rename is optional. A background service is not required unless those checks show that the current design cannot meet its constraints.

When that evidence is available and the requirements are met, the reviewer can approve without waiting for the optional rename or a new export platform. If the checks reveal another material problem, explain the new finding and update the decision. A finish line should be clear, not immune to relevant evidence.

If two reviewers still demand incompatible approaches, continuing the comment thread indefinitely is unlikely to help. Put the competing concerns together, compare them against the agreed constraints, and have a short discussion with the appropriate maintainer or technical lead. Record the resulting decision on the PR. The goal is accountable resolution, not finding someone senior enough to overrule a teammate without examining the issue.

Turn a real review into interview evidence

For interview preparation, choose a review you actually participated in and reconstruct the decision, not just the disagreement. What did the change promise? What could go wrong? Which part of the feedback did you accept, and which part did you question? What evidence resolved the issue, and what remained outside the PR?

The DrillDisagree productively in code review drill helps turn that experience into a clear answer. Use your own details, including a concern you initially underestimated or an alternative you eventually accepted. "We merged safely after resolving these risks" is more informative than "I convinced the reviewer."

If the difficult part was deciding what could wait under a deadline, continue with DrillBalance quality and delivery pressure. If the review exposed a recurring interpersonal problem, use DrillGive difficult feedback to a teammate to prepare the private, specific conversation rather than turning a PR thread into feedback about someone's personality.

For your next review, try writing one closing sentence that names what still prevents approval and how it can be resolved. If you cannot write that sentence, the review may need a clearer decision before it needs another revision.

Human editorial direction

Aporeon Guides are written and reviewed to help developers prepare practical answers and decisions, not to reproduce documentation or manufacture search traffic. Read the editorial standards.