0.8.0: refactoring you can check – pin it, prove it, or ask
0.8.0 is about how the suite changes code: what it checks before, during and after a restructuring, and when it stops. It is not a breaking release. A project on 0.7 has nothing to do before it calls the suite again; what you will notice is listed at the end, in What changes for your project.
Where this release comes from
Most of 0.8.0 started with a look at pstack, a plugin by Lauren Tan for working with agents in a codebase. It is built differently: interactive, one router skill with many playbooks and one-principle skills, where this suite is a chain of decision points driven by your tracker. But many of its ideas about refactoring carry over: pin behaviour before structure moves, measure a change by how much easier the code is to read, say why a change is safe, stop when stuck instead of trying forever.
I went through those ideas, checked each against the suite's skills, and built the ones that fit into the suite's own planning, implementation and review. Most sections below are one of them.
One thing to know before reading on: since 0.7, a run uses your project's own skills for planning and implementing where it has them, and the suite's procedures only as a fallback. What this post describes is what those procedures do. If your project plans and implements with its own skills, you will see less of it.
The same preference should let you use pstack and this suite side by side: pstack's skills for the planning and the change, this suite for finding the work and carrying it to a merge request. I have not tried that combination yet.
A test before anything moves
A refactoring promises that behaviour does not change. That can only be checked against behaviour written down before the change. So a structural ticket now starts with a Pinning Test: a test that holds what the code does today and passes before any structure moves. The plan names it, the implementation commits it first, the review checks that it came first, and the merge request reports that it passed before the change. Where tests that reach the code already exist, those are the pin.
The idea is Michael Feathers' characterization test, from Working Effectively with Legacy Code: a test that records what the code actually does, not what it should do (overview on Wikipedia).
A type check or a linter is no pin. Where a pin cannot be written at a reasonable cost, the run does not restructure unchecked: it asks you.
For a restructuring that reaches over several steps and produces something comparable, such as text, a file or data, the pin can be an equivalence script. It runs the old and the new code on your project's own sample inputs (fixtures, example files, inputs of your tests, never production data) and compares the outputs. The merge request says on how many inputs they agreed.
A tooling ticket gets no new test: there a tool changes the code, and your existing test suite is what notices a difference.
Easier to follow, or not at all
A change can be correct and still make nothing better. 0.8.0 adds a measure for that: reader load, the effort it takes to follow the code. The plan names the friction the ticket is about; the review checks each part of the change against it. A part that lowers reader load nowhere is not reworked, it is taken off. When nothing is left, the run ends without a merge request and leaves a comment on the ticket saying why.
Reader load follows Artem Zakirullin's Cognitive load is what matters: part of the effort comes from the problem itself and stays; the part that comes from how the code is built, such as layers to trace and state to keep in mind, is the part a refactoring can lower.
Behind this is a split of the suite's guidance into rules and principles. Rules are few and hold without exception: a refactoring keeps behaviour, a test pins it first, work happens on its own branch. Principles help with calls that depend on the code, and the plan names each principle that changed a decision. Two of them:
- An API that something outside the project depends on is replaced piece by piece, old and new side by side. An internal one is replaced in one step: all callers move and the old API is deleted, so no compatibility layer stays behind. When the suite cannot tell, it assumes someone outside depends on it.
- Dead code is removed before reshaping, and every rename is searched for across strings, docs and configuration, not only where a tool finds it.
Why the change is safe
The pin covers what a user of the code sees. A rename, a move or a deletion can still break something it never reaches: a name in a string, a configuration file, a container entry, stored data. Where a change touches such a name, the merge request now states the one fact the change is safe because of, and how far that was proved: read in the source, shown that the bad case cannot occur, run, or reproduced in the application. What could not be proved is written as unproved, so you know where to look. Where such a fact touches security, permissions, a migration or concurrency and stays unproved, the run asks before it goes on.
Merge requests a person can review
0.7 made it possible to deliver one ticket in several merge requests. 0.8 uses that for tooling tickets: the plan first estimates the size of the change, with the tool's dry run or a count of files. Where one merge request would be too much to review, the plan is cut into parts at a seam of kind or risk: rewrites that only restate the code on one side, rules that change what it does on the other. Each part is worth merging on its own, and the next run picks up the first part not yet merged. There is no fixed file count, because the same count can be easy or hard to review.
What the review finds, and what a bot says
The suite's own review now sorts its findings:
- fix: the work goes back for it. The default.
- note: right, but beyond what the plan covers. It is named in the merge request and left, so the change stays the size you agreed to.
- dismiss: not a finding after all, with a concrete reason. Dismissals are listed in the closing report, so you can overrule one.
A finding against the plan, or a change in what the application does, is never dismissed.
The same care applies to comments on an existing merge request. When a run picks up a ticket whose merge request has review comments, it no longer fixes them all:
- A bot's comment is checked first, by running the test or command it cites where that is cheap. Then it is fixed, dismissed with a reason, or asked about. Anything about security, data, permissions, migrations or concurrency is always asked.
- A person's comment is fixed or answered, never dismissed.
Each handled comment gets a short reply. Resolving threads stays with the reviewer.
Stopping when stuck
A run now stops when the same failing check or review finding survives two fix attempts. No count of rounds decides it, only that the run is not getting further. The branch stays local and is not pushed; the ticket gets an open question in plain words saying what was tried and where it fails. No check is deleted, loosened or ignored to turn it green.
A closing report that starts with you
The run's closing report now opens with how many things wait for you, or that none does. For each, it says what it recommends and what happens if you do not answer. And where a question about behaviour, output or timing can be settled by running something harmless, the design now runs it instead of asking you.
Smaller changes
- A PHPStan baseline ticket's merge request may now add a new baseline
entry, as long as the baseline gets shorter overall; it names the entry.
A finding to ignore for good goes into
phpstan.neononly after you allow it.
What changes for your project
Nothing you have to do. What you will notice:
- Merge requests say more: the pin and that it passed before the change, where the code became easier to follow, the review's notes, and for renames and moves why the change is safe.
- Large tooling changes arrive in parts, each its own merge request.
- Some runs end without a merge request: when the review finds that no part of the change makes the code easier to follow, or when a fix does not hold. The ticket then carries a comment or an open question.
- Review comments get replies, and bot comments may be dismissed with a reason instead of being fixed.
The FAQ explains the reasoning behind most of these changes, for example why a structural ticket starts with a test and why the suite doesn't just do what a review bot asks.
Changelog 0.8.0
The upstream changelog entries for 0.8.0, verbatim. Also on GitHub: release 0.8.0.
Show changelog entries
- A structural refactoring ticket now starts with a Pinning Test — a test that holds today's behaviour and passes before any restructuring — which the plan names, the review checks, and the merge request reports; where none can be written at a reasonable cost, the run asks instead of restructuring unchecked.
- For a restructuring that reaches over several steps behind one edge, the plan can pin the behaviour with a script that runs the old and the new code on the project's own sample inputs and compares the outputs; the merge request says on how many inputs they agreed.
- A structural refactoring now has to make the code easier to follow: the review takes off any change that lowers the reader's effort nowhere, and the run ends without a merge request when none is left. Judgement aids — reader load, the Strangler Fig, the technique vocabulary — now live in their own principles reference, apart from the few binding rules.
- Plans now tell apart an API someone outside depends on (replaced piece by piece, old and new side by side) from an internal one (all callers moved and the old API deleted in one step), remove dead code before reshaping, and every rename is searched for across strings, docs and configuration.
- When a restructuring renames, moves or deletes something that can be referred to from outside the tested interface, the merge request now states the one fact the change is safe because of and how far it was proved — or that it is not proved.
- A plan for a tooling ticket now estimates how big its merge request would be and, where a person could not review it, is cut into parts at a seam of kind or risk, each worth merging alone.
- The suite's own review now sorts its findings into fix, note and dismiss: what the plan cannot fix is named in the merge request, a dismissal carries its reason in the closing report, and a finding against the plan is never dismissed.
- When a run works a ticket that already has a merge request, it now sorts the review comments instead of fixing all of them: a bot's comment is checked and then fixed, dismissed with a reason, or asked about (security, data and permissions are always asked); a person's comment is fixed or answered, never dismissed; each handled comment gets a short reply, and threads are left for the reviewer to resolve.
- A run now stops when the same failing check or review finding survives two fix attempts: the branch stays local, the ticket gets an open question in plain words, and no check is deleted, loosened or ignored to get green.
- The design now settles a question about behaviour, output or timing by running something harmless instead of asking you, and a run's closing report opens with how many things wait for you and says what is recommended and what happens if you do not answer.
- A PHPStan baseline ticket's merge request may now add a new baseline entry as long as the baseline gets shorter overall, and names it; a finding to ignore for good, or a baseline that would not get shorter, becomes an ignore rule in
phpstan.neononly after the human allows it. - Contributors are told to turn a rule that is broken twice into a check, and the validator now checks that README and docs cite no ADR or PR numbers and no scratch files.