PR Review Merge Prep

Making it merge-ready

• Preserve the contributor's authorship: amend into their commit, or rebuild their commits with the same author. Keep a commit structure that has meaning, such as a tests-then-fix pair. Update the commit message to cover what was added; a message must always match its diff.
• Authorship is two fields. Amending keeps the author and rewrites the committer, and the committer is what the forge shows next to the commit and what stays in public history. Never pass a placeholder identity to git commit or git rebase: the repository's configured identity is already the right one. Check with git log -1 --format='%an <%ae> / %cn <%ce>' before every push, because merged history cannot be fixed.
• Fixes added during review follow the same TDD rule as the original bug: failing test first, then the fix.
• Retargeting to another branch is a rewrite, not a copy: express the fix in the target branch's code shape and conventions (available APIs, test style, annotations or attributes, tool versions). Rebuild tests that depend on things the target branch lacks. Record the known merge-up resolution for the higher branch in the pull request description, under a Merge-up to <branch>: paragraph, so the merger does not have to rediscover it.
• When the rework sits in local commits on top of the contributor's, squash before rebasing. Otherwise the rebase replays the original commit first and raises conflicts against content the rework already replaced.
• A conflict-free rebase can still produce broken code: an import the source branch happened to have, a test runner that ignores newer test syntax. The test run decides, not the rebase.
• Rebase onto the current base tip and confirm the parent commit equals it. Check again just before pushing: active repositories move. Run rebases and merges non-interactively (GIT_EDITOR=true).
• A clean local style run is necessary, not sufficient. The hosted check can be a superset of the local tool, and it lints every file the PR touches, including lines the PR did not write, so pre-existing debt in a touched file becomes this PR's failure. Judge the local tool by its exit code: output formats vary, and a grep for its listing can report clean while the tool found issues.
• Read the reports of static analysis and style tools as hints, not as verdicts. They match patterns, they do not know what the code means, and false positives are normal. Never add an annotation, a cast or any other contortion whose only purpose is to silence a tool, and never patch a line the pull request does not touch to make a check pass. Judge each report by what the code does at runtime, and say so when the right answer is to leave the check red.
• Run scoped tests while iterating and the full suite of each touched component before calling it done; never run the whole monorepo at once. Suites are addressed by path: ./phpunit src/Symfony/Component/<Name>, and the same form for src/Symfony/Bridge/<Name> and src/Symfony/Bundle/<Name>, while everything under src/Symfony/Contracts is a single suite. Check static analysis and lowest-dependency jobs when the change can affect them. If an assertion depends on the installed dependency version, use feature detection instead of assuming one version.
• Retarget before pushing, never after. CI reads the base branch from the pull request event at push time, so pushing a branch rebased onto a new base while the PR still points at the old one makes every job diff against the wrong branch: a diff of thousands of files, style checks on code the PR never touched, and a wall of red that a three-file change cannot explain. Rerunning does not help, since the event is unchanged.
• Push to the PR's real head with an explicit HEAD:<head-branch> refspec, since the work happens on a detached HEAD. The fork's repository name can differ from the upstream one; query it.
• Update the PR's metadata to match what it has become: title and component prefix, header table, description, labels, base and milestone. On a PR that sat, the Branch? row names a branch that has since shipped, the description keeps wording the review already corrected, and the component prefix can name the wrong package. Leave exactly one status label, and none the diff no longer justifies.
• Treat the description as the first draft of the documentation, not as a review log. Write it for someone who never saw the discussion: what the feature does, the public API it adds, the options and their defaults, which choice to make when, and the traps. For a feature, end with the points the documentation must carry, so whoever opens the symfony-docs pull request does not have to reconstruct them from the diff.
• Apply corrections instead of suggesting them. The branch is being rewritten anyway, so a review comment asking for a one-line change is a pointless round trip. This covers the contributor's own code comments and test names when they describe something the code does not do.
• Routine merge prep needs no comment at all: a rebase, a changelog fix, a constraint bump and a style fix are all visible in the diff. Comment when the reader has to decide or react.
• Put what changed during review in a new comment instead. Those notes matter to the people following the thread and stop mattering once it merges, while the description outlives it.
• Keep changelog entries to one line each, naming the new public surface and nothing more. Reasoning, comparison tables and usage guidance belong in the documentation. A changelog entry that has to explain itself is a sign the documentation entry is missing.
• Apply the house rules to inherited content too: when amending someone else's commit, remove what the rules forbid instead of keeping it. Scan the added diff lines for violations before pushing.
• Finish on GitHub: post review feedback as a real review with evidence, close replaced issues with a factual explanation, and correct wrong claims left in threads.
• When closing on the merits, state the decision and the reason and stop. Do not invite a fresh pull request, since that reopens the same discussion under a new number. When closing a stale pull request nobody rejected, a short line welcoming a fresh take is the right tone instead.

House rules

• Write code comments sparingly, only where they add value the code cannot express. A test method or a test helper carries no docblock and no comment narrating it: its name and its assertions say what it checks. The comment that stays in a test is the one saying why a fixture looks wrong on purpose. Never reference issues or pull requests in code or tests.
• A prose docblock opens with one line that summarizes, then a blank line, then the details. Before reshaping a comment into that form, ask whether it should exist at all.
• Do not wrap prose at a column. A line ends where a sentence, a paragraph, a list item or a code sample ends, and a sentence stays on one line unless it runs very long, in which case it breaks where a clause ends: a line break has to mean something, and a column count is not a meaning. The same holds for a @param description.
• Use TDD for every fix: failing test first, implementation second, full suite of the touched component last.
• No em-dashes, no Co-Authored-By trailers, no credit to AI tools anywhere: code, commit messages, PR titles and bodies, review comments, issue comments.
• Keep a factual tone in everything published: findings and evidence, no self-promotion, no filler.
• Never apologize for a late or missing review. Thank the author for their patience and stop there. Do not editorialize about the project's failure to review; credit what the author did well instead.
• Comments are published under the maintainer's own account, so write their past review comments in the first person and everyone else's in the third.
• Private methods go at the end of the class, after all public and protected ones. Do not move existing ones in the same patch.
• Do not call a Symfony or third-party method or constructor with PHP named arguments, in a patch you write or in one you review. Parameter names are outside the backward compatibility promise except for the constructors of attribute classes (note 10 of https://symfony.com/doc/current/contributing/code/bc.html), so a renamed parameter breaks such a call in a minor release. Pass the intermediate defaults explicitly to reach a later optional parameter. Named arguments stay fine for attribute constructors and for PHP's own functions, whose parameter names are part of the language.
• Use plain English: common words, short sentences, one idea per sentence. Avoid idioms, cultural references and rare vocabulary. Most readers are not native speakers.

Reporting

• Lead with the verdict. Then list findings ranked by severity, each with its evidence: what was run, on which state, and the exact failure text.
• Separate regressions from pre-existing gaps, and blockers from polish. Say which findings the merger must act on.
• State what was not covered as plainly as what was.
• In a batch, relay each verdict as soon as it lands. When all reports are in, give a summary table: one line per PR with its verdict and target branch.
• For the detailed pass, present one PR at a time and wait for a go-ahead keyword (such as "next") before the following one. This keeps each review open for discussion and lets fixes happen before moving on.