PR review and merge preparation

Reviewing

• Re-derive the root cause from the code. Do not trust the PR title, the description or the author's branch analysis. Verify every claim yourself. Use small probe scripts when reasoning alone leaves doubt.
• Read the whole discussion before judging: the linked issue, the PR comments, the reviews and the inline comments. Compare each reviewer request with the current diff. Flag requests that were ignored or only partly applied.
• Challenge every added public or protected method, property, option, parameter, Twig function, Stimulus value and class. Write the use case with the API that already exists, and run it. Accept the addition only when that attempt fails, or only succeeds through internals the package does not promise to keep. An alternative that runs is not automatically a refusal: weigh whether a user would find it, what it forces them to get right by hand, and what it reports when it fails. Saving a few calls is not a reason: public API is carried for a whole major version, and a shortcut added today constrains every later change. Apply the same test to the API you add yourself during merge prep, and run it before making the pull request ready, not after.
• Report that attempt whichever way it went. When it failed, the report is what justifies the addition, and it is what makes the decision survive the discussion. When it succeeded, close with the working snippet rather than the refusal alone, so the author leaves with a solution instead of a no.
• Give the same scrutiny to changed behavior of existing public API. It is riskier than an addition, because nobody opts into it: establish what depends on the current behavior before accepting the change. A diff that edits existing fixtures, expectations or snapshots to make them pass is announcing such a change; treat that as the signal rather than as housekeeping.
• Revert-verify every new test: put the changed source back in its base state, keep the tests, and confirm they fail with the expected error, not another one. A test that also passes on base protects existing behavior; it does not prove the fix. Be clear about the role of each test.
• Undo a revert-verify without destroying the working tree. git checkout <base> -- <paths> writes the base version to the index as well, so restoring with git checkout -- <paths> reads back from that index and silently discards every fix made during review. Commit or amend first and restore with git checkout HEAD -- <paths>, or copy the files aside and put them back by hand. Confirm with git diff --stat before trusting the next test run.
• Use three-state verification to judge a rework: base (bug present), PR as submitted (what it really does), final (fixed). Run the same checks in the three states. The pattern of failures across the states is the proof.
• Validate the test harness before trusting it: run it on the known-broken state and watch it fail. A check that cannot tell "read the right thing" from "fell back to a default" proves nothing. Give each outcome its own marker.
• Check the branch target. Bugfixes, features and deprecations go to 3.x. 2.x only receives security fixes. Security fixes follow the security-triage skill. Retarget a pull request opened against 2.x for anything else.
• A config tree must not depend on anything external. Reject a bundle Configuration class or configure() method that reads container parameters (kernel.debug), env vars, another extension's raw config or machine state (extension_loaded(), class_exists()) into node defaults, normalization or validation: the same tree must come out whatever the environment or the machine, because config:dump-reference and the generated config reference are built from it. Such a default belongs in the extension, behind a defaultNull() node whose info() names the fallback, or behind a static '%kernel.debug%' default that the extension resolves.
• List the edge cases and run them. Do not accept plausible reasoning when a probe can answer: nullable, union and interface types, empty input, recursion, encodings, interaction with sibling state.
• Check every symbol the change borrows from another package against that package's declared version constraint, in composer.json and in assets/package.json (peerDependencies). A symbol added in the current release cannot be satisfied by the oldest version the constraint allows, so the call site needs an existence guard, or the test belongs in the package that owns the code. The lowest-dependency CI job is only the symptom: the same code is broken at runtime for anyone on that older version.
• Test composition: rebase onto the current base tip and rerun. When sibling PRs touch the same area, also run the combined result.
• Account for every failing CI job: reproduce it when possible and classify it as caused by the PR or pre-existing. A red job nobody can explain is a finding. When comparing against a baseline run, check its date and not only its conclusion: a branch tip that saw no push for hours keeps a stale green run, while CI installs current dependencies on every run, which makes an unrelated change look like the regressor.
• Before blaming the PR for a failure, reproduce the failure on the base branch. Local environments have pre-existing failures.
• Check documentation duties in both directions. A feature needs its CHANGELOG.md entry and its documentation in src/<Package>/doc/*.rst, in the same PR; a deprecation also needs its upgrade note. A plain bugfix carries no CHANGELOG entry: the CHANGELOG lists features, deprecations, backward-compatibility breaks and bugs big enough that someone upgrading needs to know. Check that an entry sits under the topmost, unreleased version heading of the package's CHANGELOG.md. Contributors routinely file it under a version that already shipped, because that was the branch they opened against.
• Test at the layer where the defect lives. Assertions on intermediate state (service wiring, generated config, the HTML a snapshot pins) cannot catch a bug that only appears in a later phase (compiled container, the browser, a real Stimulus controller). When the bug lives there, the test must go there, which for JS behavior can mean a Playwright test.

Batch review

• To review many PRs, run one subagent per PR, each in its own git worktree. Parallel runs must never share mutable state.
• Put each worktree in .claude/worktrees/pr<number> (listed in .git/info/exclude, so git status stays clean). Create it detached, then run gh pr checkout <number> inside it: that creates a local branch tracking the contributor's fork, so the push the maintainer runs at the end goes to the right place. Never fetch the head with git fetch upstream pull/<number>/head:..., which leaves the branch with no push remote.
• Give each worktree its own installs (pnpm install at the root, composer update in each touched package). Never link the main checkout's vendor/ or node_modules/ into a worktree: an installer that rewrites files in place propagates through the link and breaks every checkout at once.
• Never let agents touch the main checkout; treat it as owned by the user, along with the branches it has checked out.
• Give every agent the same brief: the full single-PR standard above plus the batch-wide checks (branch target, composition with sibling PRs), returning a structured report with verdict, root cause, branch conclusion, and findings with evidence.
• When an agent stalls or dies, resume it with a message restating where it was and which constraints apply; do not restart the review from scratch.
• Remove each worktree with git worktree remove once its PR is merged or abandoned.

Making it merge-ready

• Preserve the contributor's authorship and ship one commit. Squash the PR into a single commit authored by the contributor: amend into their commit, or rebuild it with git commit -C <their commit>, which keeps their author. 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 handing the branch over, 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, minimum PHP, test style, docblock metadata or attributes, tool versions). Rebuild tests that depend on things the target branch lacks. When a fix lands on 2.x and 3.x needs a different shape, record the known merge-up resolution in the pull request description, under a Merge-up to 3.x: 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, a dist/ built from the old sources. The test run decides, not the rebase. Rebuild dist/ after every rebase that touches assets/src/.
• Rebase onto the current base tip (git fetch upstream && git rebase upstream/<base>) and confirm the parent commit equals it. Check again just before handing the branch over: active repositories move. Run rebases and merges non-interactively (GIT_EDITOR=true).
• A clean local style run is necessary, not sufficient. fabbot 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 package before calling it done; never run every package at once. PHP suites run per package: cd src/<Package> && composer update && php vendor/bin/phpunit, and the same from src/<Package>/src/Bridge/<Bridge> for a bridge. JS runs per package too: cd src/<Package>/assets && pnpm run test:unit (and pnpm run test:browser when the change affects browser behavior). After any change under assets/src/, run pnpm run build in that package and commit dist/; CI fails when dist/ does not match a fresh build. If an assertion depends on the installed dependency version, use feature detection instead of assuming one version.
• Retarget before the push, 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 small change cannot explain. Rerunning does not help, since the event is unchanged.

• The maintainer pushes; never run git push and never merge the pull request. Hand the branch back as exactly two lines:

  cd .claude/worktrees/pr<number>
  git push --force

• Update the PR's metadata to match what it has become: title and package prefix ([LiveComponent]), header table, description, labels and base. On a PR that sat, the header table can claim a feature the review turned into a fix, the description keeps wording the review already corrected, and the prefix can name the wrong package. Leave exactly one Status: label, and none the diff no longer justifies.
• Editing the PR, posting a review and commenting are outward actions published under the maintainer's account. Show the text and ask before the first one, unless the maintainer already asked for it.
• Write the description for someone who never saw the discussion: the problem in one paragraph, the fix in one paragraph, and a before/after table for a behavior change. Cut what the diff already shows. The documentation itself ships in the same PR, under src/<Package>/doc/.
• 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. Add a new entry at the end of the version's list, or next to the entries it relates to. 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.
• Keep link definitions and their uses in sync when editing .rst docs: DOCtor-RST fails CI on a link definition nothing references.
• Apply the house rules to inherited content too: when amending someone else's commit, remove what the rules forbid instead of keeping it. An attribution trailer already in the contributor's commit is theirs: leave it unless the maintainer asks to remove it. Scan the added diff lines for violations before handing the branch over.
• 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 package last.
• New test* methods follow their branch: no return type on 2.x, where fabbot rejects one; : void on 3.x, where php-cs-fixer's void_return rule adds it and fabbot's check is disabled.
• No em-dashes, no Co-Authored-By trailers, no credit to AI tools in what you write: 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.