TYPO3 Core Patch Review
Skill: typo3-core-patch-review
Review a TYPO3 core patch — your own before you push it, or somebody else's patch set — and say what is wrong, missing or not ready, in priority order: the diff, its tests, the changelog entry, the commit message, the issue reference and the target branch.
Markdown source#
---
name: typo3-core-patch-review
description: 'Review a TYPO3 core patch — your own before you push it, or somebody else''s patch set — and say what is wrong, missing or not ready, in priority order: the diff, its tests, the changelog entry, the commit message, the issue reference and the target branch.'
compatibility: Needs the typo3-dev-companion MCP server, which owns every lookup this workflow routes to and publishes this skill together with the references/base.md it opens on. Install it from github.com/TYPO3/dev-companion and run typo3-dev-companion install in the project. A copy taken out of that repository's skills directory alone has neither the tools nor that base file.
---
# TYPO3 Core Patch Review
Review one patch against the checkout it sits in, and report in priority order.
Keep this skill as routing and review method; the contribution rules, the suites
and the commit-message rules are lookups, and a copy of them here is one that
cannot be corrected.
## Establish the patch, then the rules it is judged by
1. Work through [references/base.md](references/base.md). It fixes the order
every task here starts in, and a review is where that order decides the
result: a rule fetched after the diff has been read confirms a reading
instead of testing it.
2. Read [references/checklist.md](references/checklist.md) for the review
surfaces, what a finding owes, what a dropped candidate owes, and the
severity rubric.
3. Establish the patch itself. The server does not read your working tree, and
what it needs from you is exactly what a patch is: **the changed paths**,
**the branch it targets**, **the commit message**, and **the issue it
names**. One reading of the diff produces all four, and every lookup below
takes one of them as its argument. A review that has not established the
target branch is reviewing against the wrong conventions and cannot tell.
The changed paths are the argument, not the subject. Pass them to
`typo3_hint_lookup` for the conventions of the subsystem the patch is in, one
call per subsystem, before forming a view of whether the code is right.
## What the project already says about this patch
The message names two things the diff does not contain — the issue it resolves
and the change it is — and both are read before the code is read a second time.
- `typo3_forge_lookup` with the issue number. What the change is *for* belongs
to the issue, and a review that takes it from the commit message has the
author's account of the problem and of the solution. It is also where a series
says it is one: an issue calling itself a part tells you the patch is not
meant to stand alone, which decides every finding about what is missing from
it.
- `typo3_gerrit_lookup` with the `Change-Id` the message carries. It answers
whether the change is on the review server, at which patch set and with which
commit, against which branch and in what state — and what the review so far
is: the votes on it and the comments left on it. A comment somebody left on an
earlier patch set and nobody answered is a finding of its own, and it is the
one this review would otherwise make a second time. What "unanswered" means is
yours to read: the flag on a thread and the reply under it are two facts and
both come back.
**Both arguments come out of the commit message, and that is what makes them
safe.** `Resolves:` is the Forge issue, `Change-Id:` is the change, and the
`Change-Id` still names it after an amend. A number carried in from elsewhere
does not fail: asked for under the other's name both lookups answer, with a real
change and a real issue belonging to neither this patch nor each other. So the
check is the subject — what comes back carries the subject of the commit under
review, or the number was wrong rather than the patch.
A patch that is not pushed yet has no change, and an answer of nothing is a
result: say so rather than leaving the surface silent. Where the commit in the
checkout and the change on the server differ, name which of the two was read —
reviewing an older patch set than the one that exists is the failure this step
is here for, and the checkout cannot report it. The answer carries the commit
the current patch set is: hold it against `git rev-parse HEAD`, and where the
two differ say which one the findings are about.
Reading is the whole of it. Voting, commenting and uploading stay with the
person doing the review.
## What the patch owes, per finding
Ask the owner of each obligation rather than recalling it:
- `typo3_rule_lookup` for the contribution rules the diff makes relevant — what
a breaking change owes, what a deprecation owes, what belongs in a changelog
entry, and what review readiness means. The sections are named by subject, so
ask in the words of a subject rather than in a sentence about the patch. **Two
subjects at most in one call, and a third is a call of its own.**
`breaking change changelog entry` returns both sections whole, and asking the
two separately returns the same pair twice. A section is kept only where it
carries half of what the query asks for, so every subject added takes coverage
off the sections the others reach: `changelog entry testing review readiness`
returns nothing at all, while `changelog entry` and `review readiness` each
answer whole. Two subjects were measured never to empty a query and a third
regularly does, whether or not the subjects share a document.
- Enumerate what the diff **removes or renames** before asking. A public class,
method, property, constant, TCA field, TypoScript path or Fluid ViewHelper
argument that disappears is the finding class this review exists for, and it
is the one a reading of the new code does not surface — the evidence is in
what is gone, not in what is there.
- `typo3_changelog_lookup` for the precedent, when the patch does something the
core has done before. What an earlier entry required of the same kind of
change is the strongest argument a review can make, and it is also the one
that settles disagreement without an appeal to taste.
**List the kind before you search for words: `type` and `version`, and no
query at all.** What makes an earlier change a precedent is its shape, and a
shape has no vocabulary — the entry that settles a finding regularly shares no
noun with the diff, and two reviews have lost theirs to a query and found it
by hand afterwards. `type` is the obligation the finding is about; `version`
is the line the precedent would sit on, which is a released one the change is
backported to or a major before that. A released line publishes few entries
per type, so the listing is the whole of what the core did of that kind and
its titles are what a precedent is picked out of. A major still collecting
entries holds more of a type than one answer carries, so bound that one by
`tag` as well.
**Ask it in the words the entry is titled in, not in the identifier the diff
removes.** What the enumeration above leaves you holding is a class and a
method name, while a removal is titled after what was removed *about* — the
subsystem, the kind of API — and carries the identifiers in a list inside the
file. So a query naming one of them and coming back empty has established
nothing, and neither has one narrowed to the branch this patch targets: a
precedent is filed under the version it landed in, which is an earlier one by
definition. Where the listing and the words both miss, the precedent is still
there and the checkout is what holds it — `Documentation/Changelog`, which
this server does not read and you do. Say which of the two answered.
**What kind of change an entry came out of is two readings rather than one.**
`typo3_forge_lookup` with its issue number says what the issue was filed as,
and the argument a review makes is about the commit keyword instead — that an
earlier bugfix of this kind owed an entry. The two disagree in both
directions, so the keyword is read where it is written:
`git log --diff-filter=A` over the entry's own file names the commit that
added it. The issue behind a security entry is not public, and an unavailable
answer there is not an outage.
- `typo3_documentation_lookup` where the diff changes behaviour a manual states.
The books it searches are published outside the core repository, so what is
owed there is a follow-up rather than part of this patch — which is the
finding rather than the reason to skip it. A review said the wording lived
elsewhere and concluded that no documentation change was owed, for a patch
that makes the documented sentence about `stdWrap.override` false. Whether the
patch owes a manual anything at all is `typo3_rule_lookup` asked for
`documentation`, and a system extension's own `Documentation/` is in the
checkout, where it changes in the patch itself.
- **Sweep the checkout for the call sites before proposing an alternative.** A
recommendation to a core reviewer needs precedent rather than taste, and
whether an idiom is established in the core is precedent this server does not
hold. The base's step after the lookups starts at the class that implements a
behaviour; this question has none, and PHP source as code is outside what this
server reads. The checkout answers it, and the answer is the call sites at
their paths and lines. Say how many there are — one is a coincidence and a
spread across system extensions is a convention. A review that proposes an
alternative and names none has argued from taste.
Every finding names the changed path it is about. A statement about the
subsystem that does not tie to a line in this diff belongs in the issue, not in
a review of a patch.
Where the patch is one of a set, a finding is read against the state at the end
of the set before it is reported. What a later patch in the same set removes is
not a defect of the set, and establishing that is a reading of that patch rather
than of what a message promises about it. It is still reported where each patch
has to stand on its own, and it names the later one that settles it.
## Verification is the project's own, and it is narrowed by the diff
`typo3_test_run_guide` with the changed paths returns the suites that can fail
on this change and the targeted invocation for each. That is the verification a
review proposes: the narrowest applicable suite first, the broader ones named
after it.
The core's suites are not among the commands `typo3_project_describe` declares —
that answer is about the repository's own composer scripts, and the test runner
is a script rather than one of them. Take the commands from
`typo3_test_run_guide` and `typo3_script_lookup`, never from memory and never
from the host's own PHP: a check run outside the project's runner is evidence
about your machine.
A review may run what cannot change the code, and says what it ran and what it
printed. **It then writes out, by name, the suites on that list it did not
run.** Leaving them out is what makes four green suites read as a finished
verification, and that is the claim a review is least able to support. "The
tests would presumably still pass" is not a review sentence, and an unnamed
suite is the same sentence with the words taken out.
**A scratch probe is one of the things it may run.** Add a temporary fixture
column, a model property or a test of your own, run a targeted suite against it,
read what it prints, and put the tree back — `git checkout --` on what you
touched, then `git status` to confirm it is clean. The patch under review is not
edited, which is the boundary that matters; a probe writes files and restores
them, and the restoration is verified rather than assumed. This is what turns
"this would presumably throw" into a pasted error, and dropped-candidate
findings are where it earns most: what disproves a path is what makes it
impossible, and a probe is often the only thing that can.
**A diff that changes what the frontend renders is a class of patch where
reading is not evidence at all.** TypoScript defaults, TypoScript declared in an
`ext_localconf.php`, anything below `lib.parseFunc`: the diff says what it sets
and nothing about what comes out, and where no test covers the constellation the
suites stay green on either side of it. What settles it is a throwaway
functional test that renders one snippet and prints what came out, and building
one is `typo3_rule_lookup` with `documentId="core/testing/proving-a-rendering"`
— which cObj renders the snippet, which operator form takes markup that spans
lines, and how the output is got out of a test that would otherwise print
nothing.
## Commit shape and target branch
`typo3_commit_message_guide` with `workflow="core"`, the message and the change
type says whether the message is submittable. Without that argument it checks
the message as a repository of its own and asks for no Forge issue. Read its
answer against the diff rather than on its own: the subject that describes the
wrong action, the missing issue reference, the marker a breaking change needs
and this one does not carry.
The branch the patch targets decides which conventions apply and which findings
matter, so a patch whose target is stated and whose diff does not fit it is a
finding of its own.
## Report
Order by what stops the patch, and say why each one stops it:
1. what blocks the patch from being submitted at all;
2. what a reviewer would send it back for;
3. what is worth changing and would not block it;
4. what was checked and is correct — briefly, so a silent surface and a verified
one are not read alike;
5. what was raised while reading and dropped, with what dropped it.
Close on the checklist's surfaces with each one marked assessed, unassessed or
not applicable to this diff. A review that reports only findings cannot be told
apart from one that looked at less.
**The report is markdown the reader can copy, and the answer is where it goes.**
Everything this section asks for is what makes it long, and length is what makes
the form matter: a review is carried into the change, into the issue or into a
chat, and rendered output is what does not survive being moved. Write it to a
file only where the caller asks for one, at a path outside the checkout under
review — what is modified or untracked beside the patch is a surface this review
reports on.
## Where the review ends and the rework begins
**When you are asked to make the change, invoke `typo3-core-patch-development`
and work from it.** That includes the amend and the push. What asks for it is an
instruction to change the patch — "finish it", "fix it", "amend it", "write the
test" — and it looks like nothing at all from the inside, a sentence in a
conversation in the middle of a session that is going well. A session that
carries on under review rules is holding "it does not change the patch" while
changing the patch, and one did: it edited `ColumnMap.php`, added a fixture
column, wrote a functional test, ran seven suites and amended the commit, all
still inside this skill. Nothing broke and the tree stayed clean, which is why
nothing marked the crossing.
**A remark about a finding's weight is not that instruction.** "That is a reason
to reject it", "I think the tests should show that", "that one blocks it" — each
reaffirms a finding and commissions nothing, so what it asks for is the finding
re-ranked and the review carried on. One session read "I think the tests should
prove it" as the handover and invoked the patch skill, where the reader had
meant that the missing test was reason enough to reject the patch. Where the
sentence could be either, ask which was meant: switching costs a turn under the
wrong skill's rules, and asking costs one sentence.
Until you are asked for the change, the rule above stands whole: a review that
rewrites what it reviews has destroyed the evidence for its own findings. Where
the answer is that the patch needs work, name it and stop.
This skill owns the review of a core patch and the order its findings are
reported in. Reviewing an extension, a sitepackage or a site project belongs to
`typo3-extension-conformance` and its checklist, which reads different surfaces
against different rules.
markdown
---
name: typo3-core-patch-review
description: 'Review a TYPO3 core patch — your own before you push it, or somebody else''s patch set — and say what is wrong, missing or not ready, in priority order: the diff, its tests, the changelog entry, the commit message, the issue reference and the target branch.'
compatibility: Needs the typo3-dev-companion MCP server, which owns every lookup this workflow routes to and publishes this skill together with the references/base.md it opens on. Install it from github.com/TYPO3/dev-companion and run typo3-dev-companion install in the project. A copy taken out of that repository's skills directory alone has neither the tools nor that base file.
---
# TYPO3 Core Patch Review
Review one patch against the checkout it sits in, and report in priority order.
Keep this skill as routing and review method; the contribution rules, the suites
and the commit-message rules are lookups, and a copy of them here is one that
cannot be corrected.
## Establish the patch, then the rules it is judged by
1. Work through [references/base.md](references/base.md). It fixes the order
every task here starts in, and a review is where that order decides the
result: a rule fetched after the diff has been read confirms a reading
instead of testing it.
2. Read [references/checklist.md](references/checklist.md) for the review
surfaces, what a finding owes, what a dropped candidate owes, and the
severity rubric.
3. Establish the patch itself. The server does not read your working tree, and
what it needs from you is exactly what a patch is: **the changed paths**,
**the branch it targets**, **the commit message**, and **the issue it
names**. One reading of the diff produces all four, and every lookup below
takes one of them as its argument. A review that has not established the
target branch is reviewing against the wrong conventions and cannot tell.
The changed paths are the argument, not the subject. Pass them to
`typo3_hint_lookup` for the conventions of the subsystem the patch is in, one
call per subsystem, before forming a view of whether the code is right.
## What the project already says about this patch
The message names two things the diff does not contain — the issue it resolves
and the change it is — and both are read before the code is read a second time.
- `typo3_forge_lookup` with the issue number. What the change is *for* belongs
to the issue, and a review that takes it from the commit message has the
author's account of the problem and of the solution. It is also where a series
says it is one: an issue calling itself a part tells you the patch is not
meant to stand alone, which decides every finding about what is missing from
it.
- `typo3_gerrit_lookup` with the `Change-Id` the message carries. It answers
whether the change is on the review server, at which patch set and with which
commit, against which branch and in what state — and what the review so far
is: the votes on it and the comments left on it. A comment somebody left on an
earlier patch set and nobody answered is a finding of its own, and it is the
one this review would otherwise make a second time. What "unanswered" means is
yours to read: the flag on a thread and the reply under it are two facts and
both come back.
**Both arguments come out of the commit message, and that is what makes them
safe.** `Resolves:` is the Forge issue, `Change-Id:` is the change, and the
`Change-Id` still names it after an amend. A number carried in from elsewhere
does not fail: asked for under the other's name both lookups answer, with a real
change and a real issue belonging to neither this patch nor each other. So the
check is the subject — what comes back carries the subject of the commit under
review, or the number was wrong rather than the patch.
A patch that is not pushed yet has no change, and an answer of nothing is a
result: say so rather than leaving the surface silent. Where the commit in the
checkout and the change on the server differ, name which of the two was read —
reviewing an older patch set than the one that exists is the failure this step
is here for, and the checkout cannot report it. The answer carries the commit
the current patch set is: hold it against `git rev-parse HEAD`, and where the
two differ say which one the findings are about.
Reading is the whole of it. Voting, commenting and uploading stay with the
person doing the review.
## What the patch owes, per finding
Ask the owner of each obligation rather than recalling it:
- `typo3_rule_lookup` for the contribution rules the diff makes relevant — what
a breaking change owes, what a deprecation owes, what belongs in a changelog
entry, and what review readiness means. The sections are named by subject, so
ask in the words of a subject rather than in a sentence about the patch. **Two
subjects at most in one call, and a third is a call of its own.**
`breaking change changelog entry` returns both sections whole, and asking the
two separately returns the same pair twice. A section is kept only where it
carries half of what the query asks for, so every subject added takes coverage
off the sections the others reach: `changelog entry testing review readiness`
returns nothing at all, while `changelog entry` and `review readiness` each
answer whole. Two subjects were measured never to empty a query and a third
regularly does, whether or not the subjects share a document.
- Enumerate what the diff **removes or renames** before asking. A public class,
method, property, constant, TCA field, TypoScript path or Fluid ViewHelper
argument that disappears is the finding class this review exists for, and it
is the one a reading of the new code does not surface — the evidence is in
what is gone, not in what is there.
- `typo3_changelog_lookup` for the precedent, when the patch does something the
core has done before. What an earlier entry required of the same kind of
change is the strongest argument a review can make, and it is also the one
that settles disagreement without an appeal to taste.
**List the kind before you search for words: `type` and `version`, and no
query at all.** What makes an earlier change a precedent is its shape, and a
shape has no vocabulary — the entry that settles a finding regularly shares no
noun with the diff, and two reviews have lost theirs to a query and found it
by hand afterwards. `type` is the obligation the finding is about; `version`
is the line the precedent would sit on, which is a released one the change is
backported to or a major before that. A released line publishes few entries
per type, so the listing is the whole of what the core did of that kind and
its titles are what a precedent is picked out of. A major still collecting
entries holds more of a type than one answer carries, so bound that one by
`tag` as well.
**Ask it in the words the entry is titled in, not in the identifier the diff
removes.** What the enumeration above leaves you holding is a class and a
method name, while a removal is titled after what was removed *about* — the
subsystem, the kind of API — and carries the identifiers in a list inside the
file. So a query naming one of them and coming back empty has established
nothing, and neither has one narrowed to the branch this patch targets: a
precedent is filed under the version it landed in, which is an earlier one by
definition. Where the listing and the words both miss, the precedent is still
there and the checkout is what holds it — `Documentation/Changelog`, which
this server does not read and you do. Say which of the two answered.
**What kind of change an entry came out of is two readings rather than one.**
`typo3_forge_lookup` with its issue number says what the issue was filed as,
and the argument a review makes is about the commit keyword instead — that an
earlier bugfix of this kind owed an entry. The two disagree in both
directions, so the keyword is read where it is written:
`git log --diff-filter=A` over the entry's own file names the commit that
added it. The issue behind a security entry is not public, and an unavailable
answer there is not an outage.
- `typo3_documentation_lookup` where the diff changes behaviour a manual states.
The books it searches are published outside the core repository, so what is
owed there is a follow-up rather than part of this patch — which is the
finding rather than the reason to skip it. A review said the wording lived
elsewhere and concluded that no documentation change was owed, for a patch
that makes the documented sentence about `stdWrap.override` false. Whether the
patch owes a manual anything at all is `typo3_rule_lookup` asked for
`documentation`, and a system extension's own `Documentation/` is in the
checkout, where it changes in the patch itself.
- **Sweep the checkout for the call sites before proposing an alternative.** A
recommendation to a core reviewer needs precedent rather than taste, and
whether an idiom is established in the core is precedent this server does not
hold. The base's step after the lookups starts at the class that implements a
behaviour; this question has none, and PHP source as code is outside what this
server reads. The checkout answers it, and the answer is the call sites at
their paths and lines. Say how many there are — one is a coincidence and a
spread across system extensions is a convention. A review that proposes an
alternative and names none has argued from taste.
Every finding names the changed path it is about. A statement about the
subsystem that does not tie to a line in this diff belongs in the issue, not in
a review of a patch.
Where the patch is one of a set, a finding is read against the state at the end
of the set before it is reported. What a later patch in the same set removes is
not a defect of the set, and establishing that is a reading of that patch rather
than of what a message promises about it. It is still reported where each patch
has to stand on its own, and it names the later one that settles it.
## Verification is the project's own, and it is narrowed by the diff
`typo3_test_run_guide` with the changed paths returns the suites that can fail
on this change and the targeted invocation for each. That is the verification a
review proposes: the narrowest applicable suite first, the broader ones named
after it.
The core's suites are not among the commands `typo3_project_describe` declares —
that answer is about the repository's own composer scripts, and the test runner
is a script rather than one of them. Take the commands from
`typo3_test_run_guide` and `typo3_script_lookup`, never from memory and never
from the host's own PHP: a check run outside the project's runner is evidence
about your machine.
A review may run what cannot change the code, and says what it ran and what it
printed. **It then writes out, by name, the suites on that list it did not
run.** Leaving them out is what makes four green suites read as a finished
verification, and that is the claim a review is least able to support. "The
tests would presumably still pass" is not a review sentence, and an unnamed
suite is the same sentence with the words taken out.
**A scratch probe is one of the things it may run.** Add a temporary fixture
column, a model property or a test of your own, run a targeted suite against it,
read what it prints, and put the tree back — `git checkout --` on what you
touched, then `git status` to confirm it is clean. The patch under review is not
edited, which is the boundary that matters; a probe writes files and restores
them, and the restoration is verified rather than assumed. This is what turns
"this would presumably throw" into a pasted error, and dropped-candidate
findings are where it earns most: what disproves a path is what makes it
impossible, and a probe is often the only thing that can.
**A diff that changes what the frontend renders is a class of patch where
reading is not evidence at all.** TypoScript defaults, TypoScript declared in an
`ext_localconf.php`, anything below `lib.parseFunc`: the diff says what it sets
and nothing about what comes out, and where no test covers the constellation the
suites stay green on either side of it. What settles it is a throwaway
functional test that renders one snippet and prints what came out, and building
one is `typo3_rule_lookup` with `documentId="core/testing/proving-a-rendering"`
— which cObj renders the snippet, which operator form takes markup that spans
lines, and how the output is got out of a test that would otherwise print
nothing.
## Commit shape and target branch
`typo3_commit_message_guide` with `workflow="core"`, the message and the change
type says whether the message is submittable. Without that argument it checks
the message as a repository of its own and asks for no Forge issue. Read its
answer against the diff rather than on its own: the subject that describes the
wrong action, the missing issue reference, the marker a breaking change needs
and this one does not carry.
The branch the patch targets decides which conventions apply and which findings
matter, so a patch whose target is stated and whose diff does not fit it is a
finding of its own.
## Report
Order by what stops the patch, and say why each one stops it:
1. what blocks the patch from being submitted at all;
2. what a reviewer would send it back for;
3. what is worth changing and would not block it;
4. what was checked and is correct — briefly, so a silent surface and a verified
one are not read alike;
5. what was raised while reading and dropped, with what dropped it.
Close on the checklist's surfaces with each one marked assessed, unassessed or
not applicable to this diff. A review that reports only findings cannot be told
apart from one that looked at less.
**The report is markdown the reader can copy, and the answer is where it goes.**
Everything this section asks for is what makes it long, and length is what makes
the form matter: a review is carried into the change, into the issue or into a
chat, and rendered output is what does not survive being moved. Write it to a
file only where the caller asks for one, at a path outside the checkout under
review — what is modified or untracked beside the patch is a surface this review
reports on.
## Where the review ends and the rework begins
**When you are asked to make the change, invoke `typo3-core-patch-development`
and work from it.** That includes the amend and the push. What asks for it is an
instruction to change the patch — "finish it", "fix it", "amend it", "write the
test" — and it looks like nothing at all from the inside, a sentence in a
conversation in the middle of a session that is going well. A session that
carries on under review rules is holding "it does not change the patch" while
changing the patch, and one did: it edited `ColumnMap.php`, added a fixture
column, wrote a functional test, ran seven suites and amended the commit, all
still inside this skill. Nothing broke and the tree stayed clean, which is why
nothing marked the crossing.
**A remark about a finding's weight is not that instruction.** "That is a reason
to reject it", "I think the tests should show that", "that one blocks it" — each
reaffirms a finding and commissions nothing, so what it asks for is the finding
re-ranked and the review carried on. One session read "I think the tests should
prove it" as the handover and invoked the patch skill, where the reader had
meant that the missing test was reason enough to reject the patch. Where the
sentence could be either, ask which was meant: switching costs a turn under the
wrong skill's rules, and asking costs one sentence.
Until you are asked for the change, the rule above stands whole: a review that
rewrites what it reviews has destroyed the evidence for its own findings. Where
the answer is that the patch needs work, name it and stop.
This skill owns the review of a core patch and the order its findings are
reported in. Reviewing an extension, a sitepackage or a site project belongs to
`typo3-extension-conformance` and its checklist, which reads different surfaces
against different rules.
References#
Where every task starts#
# Where every task starts
## Nothing starts until the server answers
A skill is a file the installer left behind: it loads and reads the same whether
the tools behind it are connected or not, and neither side notices. So the first
call below is also the check.
- No `typo3_` tool in this session, or a first call that errors: stop, say this
workflow needs the server and it is not there, and name what came back.
- Do not fall back to general TYPO3 knowledge or start reading the checkout.
That answer carries this workflow's order and confidence and none of its
evidence, and nothing in it says which of the two it is.
- Continue only when asked to after saying so, and repeat it in the answer and
in every finding a lookup would have carried.
## The order
This is the order, and it is an order rather than a list because each step
decides what the next one is worth. A convention fetched after the code has been
read confirms a view instead of testing it. A command recommended before the
project's own are known is a guess that sounds like advice. Where a step below
carries a condition for skipping it, that condition is narrow on purpose: a
prescription that gets skipped teaches the next reader to skip the ones that
matter too.
1. **`typo3_project_describe`** — the installation, its TYPO3 and PHP version,
the extensions that are the project's own, its sites, and the commands this
repository actually declares. Every later answer is filtered by that version,
and a check the repository does not declare is a wrong answer however
sensible it sounds. It ends with the whole procedures this server carries, as
ids: that list is the only place they are named to a client that renders no
resource list, and each one is a `typo3_rule_lookup` with that `documentId`
rather than a search.
2. **`typo3_extension_describe`** for each extension in scope — what it
registers, and what it ships beside that: its manual, its README, its test
layers, its XLF files with the source language each one declares. What it
does *not* ship is answered too, and that is the half no file listing can
give you.
Where step 1 reported none — a core checkout is the case, since it names the
project's own extensions and not TYPO3's — that answer is this step, and
there is nothing to call. Say so. A step passed over in silence cannot be
told from one that was dropped.
3. **`typo3_task_guide`** with a short English task, the paths it touches, the
target version and the change type — the workflow this task belongs to and
the checks that come with it.
Run it in every session, this skill's own tasks included. The brief is built
from the paths as well as the task text, and no skill knows which paths the
caller is holding: a skill that covers the task is not that brief, and
skipping the step costs the hints and the core checks those paths match.
Where the guide's own answer is what named this skill, this is one call for
an answer already in the session. That is the price of a step there is
nothing to decide about.
4. **`typo3_hint_lookup`** for each subsystem in scope, with its concrete paths.
One query per subsystem; a single broad query is not subsystem evidence.
Where step 3 ran with those paths, its answer says whether this step is still
owed. A brief that carried everything the lookup matched says so — "these are
everything typo3_hint_lookup matches for these paths" — and there the call is
made and asking again returns the same hints. One that stopped short says
that instead and names the ids it left, and those are what is owed: fetch
them by id rather than repeating the query. Read the sentence rather than the
populated `hints` key, which is present either way and does not tell the two
apart.
5. **`typo3_changelog_lookup` with `type: deprecation`**, at each major the
package declares, bounded by `tag` and with the query omitted. Those three
are the changelog's own axes, and the extension's vocabulary is not among
them: an entry carries a query only when its title carries every word of it
at once, and the core titled those entries about its own code. Words taken
from what step 2 reported are therefore matched against titles they were
never written in, and the sweep comes back empty however right the query
looks.
Step 2 picks the tags instead. `ext:core`, `ext:frontend`, `ext:form` and the
rest name the system extension a change is **in** — one call for each one the
package requires, renders through or registers into, which is more than its
manifest lists — and `TCA`, `TypoScript`, `Fluid`, `YAML`, `Backend`,
`Frontend` name the surface, one for each kind of file it ships. An extension
key of your own is not among them and matches nothing. Every call also
returns every tag that version and type carry, so the second call onwards is
read off the first rather than guessed at.
Step 2 is what the answers are checked against, which is the other half the
words were doing. Verify each identifier that comes back in the checkout — a
deprecation nothing here calls is not a finding — and carry the
`FullyScanned` / `PartiallyScanned` tag into the answer, because it says
whether the Extension Scanner can find the remaining call sites or whether
that reading is yours. Bounded this way the sweep is still writable before a
file is opened, which is why it is a step of the order: one left to the
reading reaches only what a finding stumbled into, and the deprecation that
decides whether the package survives the next major is not usually the one a
finding walks past.
A changelog records change events, so a pattern nothing has touched for ten
majors has no entry at all. An empty sweep is therefore not an answer about
what still works. "Does this still work in version N" goes to
`typo3_documentation_lookup` at that version — here, and whenever the reading
raises it again.
That is a question for a documented surface — a ViewHelper, a TCA type, a
TypoScript setting. The manual matches page titles and section paths, never
the text of a page, so a PHP identifier has no page to be titled after. It
reaches whatever its own words happen to spell instead. An identifier goes to
`typo3_changelog_lookup` under its own name, which reaches the entries
writing it however the change was titled, and then to the class below. Where
the manual has no page for a surface either, that is a result and not an
answer. Undocumented is not unsupported.
A task that produces no change does not reach this step at all. The property
is what the task produces, and a triage, a reproduction and a review are
illustrations of it rather than the list it is read off. The sweep asks what
a package will have to stop calling, and a task that writes nothing is not
going to call anything. Followed literally in a triage of one issue it costs
one call per declared major per tag and none of them bears on the report.
The exemption ends where the workflow produces a change. A review asked to
make the change is that other workflow, and it starts this order again
holding the files it is about to write.
Skip the sweep only where the change touches no TYPO3 API — a code style
fixer, a CI file, an `.editorconfig`. A deprecation is a statement about API
the package calls, so a change that calls none has nothing for the sweep to
land on and it is empty before it is run. That condition is worth stating
because this is the most expensive step of the order: one call per declared
major per tag. Which side a change falls on is read off the files it touches
and never off the task it started as — one PHP file edited along the way puts
it back among the ordinary ones, and a skip there costs the deprecation no
finding would have walked into.
A report names the step it did not reach, under either exemption. A step
passed over in silence cannot be told from one that was dropped, which is
what step 2 already asks of itself.
**Then** read the checkout. Not before: listing the files first makes everything
after the listing look optional, and the conventions arrive as a footnote to a
verdict that has already formed.
## When the lookups run out
A behaviour question that survives the lookups above is read out of the
installed source rather than guessed at. What answers it is the class that
implements the behaviour and the one it inherits from. That reading is the step
after the lookups, and what it replaces is changing the code until it works.
What it settles is what this installation does and never what TYPO3 supports. So
a finding says the question could not be settled beyond the version installed,
and an answer built on the reading names the version it holds for.
## Two kinds of lookup, and neither stands in for the other
`typo3_backend_module_lookup`, `typo3_icon_lookup`, `typo3_label_lookup`,
`typo3_fluid_namespace_list` and `typo3_configuration_lookup` report what is
registered, what a path resolves to, what a value really is at runtime. They
establish the facts of this installation and they are never a verdict on it.
`typo3_hint_lookup` and `typo3_documentation_lookup` say whether those facts are
right. A subsystem confirmed by its own runtime lookup can still break every
rule that governs it, so it is not established until both were asked.
## A rule is read in both directions
It says what new code should do, and it says what this checkout is already doing
wrong. A file that has settled into the opposite of a rule is a finding, not a
local style to preserve: consistency with a project's own habit establishes
nothing about whether the habit is right.
## What the code is for is evidence, and the repository states it
A mechanism that costs something is not a defect for costing it. Before
reporting one, find what it is there for — the manual, the README, the
changelog, the setting it is driven by, the versions the package declares it
supports — and say so. Where a purpose is documented, what you have is a
trade-off to name with its cost and its alternative, not a defect; where you
cannot find one, the finding says that it could not be established rather than
that none exists. This is the other direction of the rule above, and skipping it
turns a review into a list of everything the author did on purpose.
## What a finding rests on is part of the finding
Three things carry one: a file that was read, at its path and its line; a
command that was run, with what it printed; a mechanism traced into an installed
package. Say which of the three it is. Leaving it unsaid gives a finding read
out of a CI file the weight of one with a verified line, and the reader has no
way to separate them again.
Where one of the project's own commands would settle it, run it.
`typo3_project_describe` marks each command it lists **check**, **change** or
**unknown**, read off the declared body: a check reports and hands the code back
as it was, so even a task told not to change files runs it, and the linter the
repository already declares is the cheapest evidence in it. A change is not run
under that instruction, and an unknown — a test suite, a shell pipeline, a
console command — is named in the answer as evidence that is available rather
than run unasked. What a check prints is not the finding: the configuration that
makes it fail is still what the finding is about, and the run is what takes that
finding from derived to established.
## What this server does not know
It does not read your working tree. Which files changed, which branch you are
on, and whether a path or an identifier still exists there are yours to
establish — then pass the concrete paths back, because that is what turns a
general convention into an answer about this code.
## Query it in English
The knowledge is written in English and matched lexically, so a query in another
language reaches the loanwords the two happen to share and nothing else.
Translate the subject before calling and the answer back afterwards, whatever
language you are speaking with the user.
markdown
# Where every task starts
## Nothing starts until the server answers
A skill is a file the installer left behind: it loads and reads the same whether
the tools behind it are connected or not, and neither side notices. So the first
call below is also the check.
- No `typo3_` tool in this session, or a first call that errors: stop, say this
workflow needs the server and it is not there, and name what came back.
- Do not fall back to general TYPO3 knowledge or start reading the checkout.
That answer carries this workflow's order and confidence and none of its
evidence, and nothing in it says which of the two it is.
- Continue only when asked to after saying so, and repeat it in the answer and
in every finding a lookup would have carried.
## The order
This is the order, and it is an order rather than a list because each step
decides what the next one is worth. A convention fetched after the code has been
read confirms a view instead of testing it. A command recommended before the
project's own are known is a guess that sounds like advice. Where a step below
carries a condition for skipping it, that condition is narrow on purpose: a
prescription that gets skipped teaches the next reader to skip the ones that
matter too.
1. **`typo3_project_describe`** — the installation, its TYPO3 and PHP version,
the extensions that are the project's own, its sites, and the commands this
repository actually declares. Every later answer is filtered by that version,
and a check the repository does not declare is a wrong answer however
sensible it sounds. It ends with the whole procedures this server carries, as
ids: that list is the only place they are named to a client that renders no
resource list, and each one is a `typo3_rule_lookup` with that `documentId`
rather than a search.
2. **`typo3_extension_describe`** for each extension in scope — what it
registers, and what it ships beside that: its manual, its README, its test
layers, its XLF files with the source language each one declares. What it
does *not* ship is answered too, and that is the half no file listing can
give you.
Where step 1 reported none — a core checkout is the case, since it names the
project's own extensions and not TYPO3's — that answer is this step, and
there is nothing to call. Say so. A step passed over in silence cannot be
told from one that was dropped.
3. **`typo3_task_guide`** with a short English task, the paths it touches, the
target version and the change type — the workflow this task belongs to and
the checks that come with it.
Run it in every session, this skill's own tasks included. The brief is built
from the paths as well as the task text, and no skill knows which paths the
caller is holding: a skill that covers the task is not that brief, and
skipping the step costs the hints and the core checks those paths match.
Where the guide's own answer is what named this skill, this is one call for
an answer already in the session. That is the price of a step there is
nothing to decide about.
4. **`typo3_hint_lookup`** for each subsystem in scope, with its concrete paths.
One query per subsystem; a single broad query is not subsystem evidence.
Where step 3 ran with those paths, its answer says whether this step is still
owed. A brief that carried everything the lookup matched says so — "these are
everything typo3_hint_lookup matches for these paths" — and there the call is
made and asking again returns the same hints. One that stopped short says
that instead and names the ids it left, and those are what is owed: fetch
them by id rather than repeating the query. Read the sentence rather than the
populated `hints` key, which is present either way and does not tell the two
apart.
5. **`typo3_changelog_lookup` with `type: deprecation`**, at each major the
package declares, bounded by `tag` and with the query omitted. Those three
are the changelog's own axes, and the extension's vocabulary is not among
them: an entry carries a query only when its title carries every word of it
at once, and the core titled those entries about its own code. Words taken
from what step 2 reported are therefore matched against titles they were
never written in, and the sweep comes back empty however right the query
looks.
Step 2 picks the tags instead. `ext:core`, `ext:frontend`, `ext:form` and the
rest name the system extension a change is **in** — one call for each one the
package requires, renders through or registers into, which is more than its
manifest lists — and `TCA`, `TypoScript`, `Fluid`, `YAML`, `Backend`,
`Frontend` name the surface, one for each kind of file it ships. An extension
key of your own is not among them and matches nothing. Every call also
returns every tag that version and type carry, so the second call onwards is
read off the first rather than guessed at.
Step 2 is what the answers are checked against, which is the other half the
words were doing. Verify each identifier that comes back in the checkout — a
deprecation nothing here calls is not a finding — and carry the
`FullyScanned` / `PartiallyScanned` tag into the answer, because it says
whether the Extension Scanner can find the remaining call sites or whether
that reading is yours. Bounded this way the sweep is still writable before a
file is opened, which is why it is a step of the order: one left to the
reading reaches only what a finding stumbled into, and the deprecation that
decides whether the package survives the next major is not usually the one a
finding walks past.
A changelog records change events, so a pattern nothing has touched for ten
majors has no entry at all. An empty sweep is therefore not an answer about
what still works. "Does this still work in version N" goes to
`typo3_documentation_lookup` at that version — here, and whenever the reading
raises it again.
That is a question for a documented surface — a ViewHelper, a TCA type, a
TypoScript setting. The manual matches page titles and section paths, never
the text of a page, so a PHP identifier has no page to be titled after. It
reaches whatever its own words happen to spell instead. An identifier goes to
`typo3_changelog_lookup` under its own name, which reaches the entries
writing it however the change was titled, and then to the class below. Where
the manual has no page for a surface either, that is a result and not an
answer. Undocumented is not unsupported.
A task that produces no change does not reach this step at all. The property
is what the task produces, and a triage, a reproduction and a review are
illustrations of it rather than the list it is read off. The sweep asks what
a package will have to stop calling, and a task that writes nothing is not
going to call anything. Followed literally in a triage of one issue it costs
one call per declared major per tag and none of them bears on the report.
The exemption ends where the workflow produces a change. A review asked to
make the change is that other workflow, and it starts this order again
holding the files it is about to write.
Skip the sweep only where the change touches no TYPO3 API — a code style
fixer, a CI file, an `.editorconfig`. A deprecation is a statement about API
the package calls, so a change that calls none has nothing for the sweep to
land on and it is empty before it is run. That condition is worth stating
because this is the most expensive step of the order: one call per declared
major per tag. Which side a change falls on is read off the files it touches
and never off the task it started as — one PHP file edited along the way puts
it back among the ordinary ones, and a skip there costs the deprecation no
finding would have walked into.
A report names the step it did not reach, under either exemption. A step
passed over in silence cannot be told from one that was dropped, which is
what step 2 already asks of itself.
**Then** read the checkout. Not before: listing the files first makes everything
after the listing look optional, and the conventions arrive as a footnote to a
verdict that has already formed.
## When the lookups run out
A behaviour question that survives the lookups above is read out of the
installed source rather than guessed at. What answers it is the class that
implements the behaviour and the one it inherits from. That reading is the step
after the lookups, and what it replaces is changing the code until it works.
What it settles is what this installation does and never what TYPO3 supports. So
a finding says the question could not be settled beyond the version installed,
and an answer built on the reading names the version it holds for.
## Two kinds of lookup, and neither stands in for the other
`typo3_backend_module_lookup`, `typo3_icon_lookup`, `typo3_label_lookup`,
`typo3_fluid_namespace_list` and `typo3_configuration_lookup` report what is
registered, what a path resolves to, what a value really is at runtime. They
establish the facts of this installation and they are never a verdict on it.
`typo3_hint_lookup` and `typo3_documentation_lookup` say whether those facts are
right. A subsystem confirmed by its own runtime lookup can still break every
rule that governs it, so it is not established until both were asked.
## A rule is read in both directions
It says what new code should do, and it says what this checkout is already doing
wrong. A file that has settled into the opposite of a rule is a finding, not a
local style to preserve: consistency with a project's own habit establishes
nothing about whether the habit is right.
## What the code is for is evidence, and the repository states it
A mechanism that costs something is not a defect for costing it. Before
reporting one, find what it is there for — the manual, the README, the
changelog, the setting it is driven by, the versions the package declares it
supports — and say so. Where a purpose is documented, what you have is a
trade-off to name with its cost and its alternative, not a defect; where you
cannot find one, the finding says that it could not be established rather than
that none exists. This is the other direction of the rule above, and skipping it
turns a review into a list of everything the author did on purpose.
## What a finding rests on is part of the finding
Three things carry one: a file that was read, at its path and its line; a
command that was run, with what it printed; a mechanism traced into an installed
package. Say which of the three it is. Leaving it unsaid gives a finding read
out of a CI file the weight of one with a verified line, and the reader has no
way to separate them again.
Where one of the project's own commands would settle it, run it.
`typo3_project_describe` marks each command it lists **check**, **change** or
**unknown**, read off the declared body: a check reports and hands the code back
as it was, so even a task told not to change files runs it, and the linter the
repository already declares is the cheapest evidence in it. A change is not run
under that instruction, and an unknown — a test suite, a shell pipeline, a
console command — is named in the answer as evidence that is available rather
than run unasked. What a check prints is not the finding: the configuration that
makes it fail is still what the finding is about, and the run is what takes that
finding from derived to established.
## What this server does not know
It does not read your working tree. Which files changed, which branch you are
on, and whether a path or an identifier still exists there are yours to
establish — then pass the concrete paths back, because that is what turns a
general convention into an answer about this code.
## Query it in English
The knowledge is written in English and matched lexically, so a query in another
language reaches the loanwords the two happen to share and nothing else.
Translate the subject before calling and the answer back afterwards, whatever
language you are speaking with the user.
Core patch review checklist#
# Core patch review checklist
The surfaces below are written down whole before the diff is read a second time,
and each one is answered in the report — assessed, unassessed, or not applicable
to this diff. A surface this patch does not touch costs one line; a surface
nobody looked at reads as clean unless it is named.
A review disposes of a thing in three ways: it reports it, it drops it, or it
declares it clean. All three are claims about a reading the author has to take
on trust, so all three carry what backs them — the file that was opened, the
call site that was followed, the command that was run, the lookup that answered.
Assessed with nothing under it is the cheapest sentence in a review to write and
the one a reader has no way to check, so where the reading did not happen the
word is unassessed.
## Review surfaces
- **Public API.** What the diff removes, renames, or changes the signature of,
and what the contribution rules require for each of those. This is the surface
a reading of the new code does not show, so it is enumerated from the diff's
deletions — and from every signature line its additions touch, because a
parameter added to a public or protected method on a class that is not final
fatals every subclass overriding it and reads in the diff as an addition. No
suite in the checkout can fail on that, so the surface is answered from the
declaration rather than from a green run.
- **Behaviour.** What the patch changes for code that calls it, including the
paths it does not touch: a guard that was unreachable before and is live
after, a value that used to be written and now is not, a shape of output other
code asserts on.
- **Compatibility.** Whether the change fits the branch it targets, and whether
what it does is available on that branch at all.
- **Tests.** What exercises the changed behaviour, and whether the layer is the
one that can fail on it. A change with no coverage is a finding; coverage that
cannot fail on this change is a worse one, because it reads as coverage. The
surface is answered with both halves: what ran, and which of the suites the
guide returned nobody started.
- **Documentation and changelog.** What the diff obliges — the entry, its
directory, its file name, its cross-references — and equally, whether an entry
is owed at all. Demanding one where the rules do not is a review defect. The
manual is the surface's other half and it sits in two places: a system
extension's own `Documentation/` is in this checkout and changes in the patch,
and the books `typo3_documentation_lookup` searches are outside the
repository. A page the diff makes false is a finding wherever it lives —
outside is where the follow-up goes, not a reason none is owed.
- **Commit shape.** Subject, body, issue reference, target branch line, and the
markers the change type requires.
- **Review readiness.** Whether the patch can be understood from the issue and
the message alone, and whether a reviewer can reproduce what it claims to fix.
The issue is read for that, not inferred from the message that names it.
- **The review this patch is already in.** What the issue asks for, whether the
change is on the review server and at which patch set, whether the commit that
patch set is the one checked out, and whether a comment from an earlier one
went unanswered. An unanswered comment is why a change sits unmerged, and none
of this is visible from the checkout.
- **Security.** Where the diff touches authorization, user input, output
escaping, file paths, or a boundary between what a role may and may not do. A
finding here is a value and a sink, and both are established or it is not a
finding.
- **The working tree around the patch.** What is modified or untracked beside
the commit, because it ships with the patch if anybody stages it carelessly
and it is not part of the change.
## What a finding owes
Every finding carries five things, and two of them are the ones reviews skip:
1. the changed path, at its line;
2. what the patch does there;
3. the rule or the behaviour it collides with, from the lookup that owns it;
4. **the consequence** — what breaks, for whom, and when it would be noticed;
5. **whether this patch introduced it** — the line the diff wrote and the line
it only moved past are two different requests to the author.
A finding without the fourth is a preference. A finding whose rule came from
recall rather than from a lookup is a preference with a citation. A finding
without the fifth sends the author to repair something they did not change, and
nothing in the report says it was not meant to.
What the patch did not introduce is reported in those words. It stays in the
review where it blocks submission on its own and goes to the issue tracker
otherwise, because the reading that was asked for is of a change, and a list of
what was already wrong around it is a second review nobody ordered.
Distinguish what was verified from what was reasoned. A behaviour traced into
the installed code, a command that was run with its output, and a reading of the
diff are three different weights, and a report that does not separate them hands
the reader a uniform confidence the review did not have.
## What a dropped candidate owes
A review drops more than it reports, and dropping is the step nothing records.
Each candidate raised while reading and then let go is named with what let it go
— the guard that turned out to be there, the caller that holds it, the rule that
turned out not to apply, the line that was actually read. One sentence each, and
it is what tells the reader that a quiet surface went quiet after the reading
rather than before it.
The two directions are not held to the same bar. Raising a candidate costs a
reading; dropping one costs the author a finding, silently, and nothing
afterwards says it happened. So a candidate is dropped only where something
concretely disproves it, and one that can be neither established nor disproved
is reported as open, with the reading that would settle it named beside it.
Two dismissals go wrong reliably:
- Dropped because a comment, a docblock or an annotation says the code behaves
that way. That is a sentence somebody wrote, not the behaviour — read the
implementation it describes, and where the two disagree the disagreement is
the finding.
- Dropped because it looks unlikely to happen. Unlikely is not disproved. What
disproves a path is what makes it impossible: a guard that cannot be passed or
a caller that cannot exist, at a line.
Before a finding is reported, make the author's case against it — the caller
they know holds the guard, the invariant the subsystem carries, the choice the
commit message already states. What survives that is reported together with what
it survived; what does not is a dropped candidate, with the same evidence
written down.
## Severity
- **Blocks submission** — the patch cannot go up as it is: the message will be
rejected, the change breaks something the rules forbid breaking, or the diff
does not do what it says.
- **Sent back in review** — a reviewer would ask for it: missing coverage, a
missing changelog entry, an unhandled case, a public API obligation not met.
- **Worth changing** — real and not blocking. Say so, and do not spend the
reader's first paragraphs on it.
- **Correct and checked** — kept short and kept in, because it is the only thing
that separates a surface that was read from one that was skipped. It names
what was read, for the same reason a finding names what it collides with.
Rank by what stops the patch first and by consequence second. A cosmetic finding
above a behavioural one costs the review its credibility for the rest of the
list.
Who can reach the path raises a rank and never lowers one. A diff is the weakest
evidence there is about reachability, and a real finding ranked down because it
looked hard to reach is the mistake this rubric cannot recover from: where the
path could not be established, rank on consequence and say so.
markdown
# Core patch review checklist
The surfaces below are written down whole before the diff is read a second time,
and each one is answered in the report — assessed, unassessed, or not applicable
to this diff. A surface this patch does not touch costs one line; a surface
nobody looked at reads as clean unless it is named.
A review disposes of a thing in three ways: it reports it, it drops it, or it
declares it clean. All three are claims about a reading the author has to take
on trust, so all three carry what backs them — the file that was opened, the
call site that was followed, the command that was run, the lookup that answered.
Assessed with nothing under it is the cheapest sentence in a review to write and
the one a reader has no way to check, so where the reading did not happen the
word is unassessed.
## Review surfaces
- **Public API.** What the diff removes, renames, or changes the signature of,
and what the contribution rules require for each of those. This is the surface
a reading of the new code does not show, so it is enumerated from the diff's
deletions — and from every signature line its additions touch, because a
parameter added to a public or protected method on a class that is not final
fatals every subclass overriding it and reads in the diff as an addition. No
suite in the checkout can fail on that, so the surface is answered from the
declaration rather than from a green run.
- **Behaviour.** What the patch changes for code that calls it, including the
paths it does not touch: a guard that was unreachable before and is live
after, a value that used to be written and now is not, a shape of output other
code asserts on.
- **Compatibility.** Whether the change fits the branch it targets, and whether
what it does is available on that branch at all.
- **Tests.** What exercises the changed behaviour, and whether the layer is the
one that can fail on it. A change with no coverage is a finding; coverage that
cannot fail on this change is a worse one, because it reads as coverage. The
surface is answered with both halves: what ran, and which of the suites the
guide returned nobody started.
- **Documentation and changelog.** What the diff obliges — the entry, its
directory, its file name, its cross-references — and equally, whether an entry
is owed at all. Demanding one where the rules do not is a review defect. The
manual is the surface's other half and it sits in two places: a system
extension's own `Documentation/` is in this checkout and changes in the patch,
and the books `typo3_documentation_lookup` searches are outside the
repository. A page the diff makes false is a finding wherever it lives —
outside is where the follow-up goes, not a reason none is owed.
- **Commit shape.** Subject, body, issue reference, target branch line, and the
markers the change type requires.
- **Review readiness.** Whether the patch can be understood from the issue and
the message alone, and whether a reviewer can reproduce what it claims to fix.
The issue is read for that, not inferred from the message that names it.
- **The review this patch is already in.** What the issue asks for, whether the
change is on the review server and at which patch set, whether the commit that
patch set is the one checked out, and whether a comment from an earlier one
went unanswered. An unanswered comment is why a change sits unmerged, and none
of this is visible from the checkout.
- **Security.** Where the diff touches authorization, user input, output
escaping, file paths, or a boundary between what a role may and may not do. A
finding here is a value and a sink, and both are established or it is not a
finding.
- **The working tree around the patch.** What is modified or untracked beside
the commit, because it ships with the patch if anybody stages it carelessly
and it is not part of the change.
## What a finding owes
Every finding carries five things, and two of them are the ones reviews skip:
1. the changed path, at its line;
2. what the patch does there;
3. the rule or the behaviour it collides with, from the lookup that owns it;
4. **the consequence** — what breaks, for whom, and when it would be noticed;
5. **whether this patch introduced it** — the line the diff wrote and the line
it only moved past are two different requests to the author.
A finding without the fourth is a preference. A finding whose rule came from
recall rather than from a lookup is a preference with a citation. A finding
without the fifth sends the author to repair something they did not change, and
nothing in the report says it was not meant to.
What the patch did not introduce is reported in those words. It stays in the
review where it blocks submission on its own and goes to the issue tracker
otherwise, because the reading that was asked for is of a change, and a list of
what was already wrong around it is a second review nobody ordered.
Distinguish what was verified from what was reasoned. A behaviour traced into
the installed code, a command that was run with its output, and a reading of the
diff are three different weights, and a report that does not separate them hands
the reader a uniform confidence the review did not have.
## What a dropped candidate owes
A review drops more than it reports, and dropping is the step nothing records.
Each candidate raised while reading and then let go is named with what let it go
— the guard that turned out to be there, the caller that holds it, the rule that
turned out not to apply, the line that was actually read. One sentence each, and
it is what tells the reader that a quiet surface went quiet after the reading
rather than before it.
The two directions are not held to the same bar. Raising a candidate costs a
reading; dropping one costs the author a finding, silently, and nothing
afterwards says it happened. So a candidate is dropped only where something
concretely disproves it, and one that can be neither established nor disproved
is reported as open, with the reading that would settle it named beside it.
Two dismissals go wrong reliably:
- Dropped because a comment, a docblock or an annotation says the code behaves
that way. That is a sentence somebody wrote, not the behaviour — read the
implementation it describes, and where the two disagree the disagreement is
the finding.
- Dropped because it looks unlikely to happen. Unlikely is not disproved. What
disproves a path is what makes it impossible: a guard that cannot be passed or
a caller that cannot exist, at a line.
Before a finding is reported, make the author's case against it — the caller
they know holds the guard, the invariant the subsystem carries, the choice the
commit message already states. What survives that is reported together with what
it survived; what does not is a dropped candidate, with the same evidence
written down.
## Severity
- **Blocks submission** — the patch cannot go up as it is: the message will be
rejected, the change breaks something the rules forbid breaking, or the diff
does not do what it says.
- **Sent back in review** — a reviewer would ask for it: missing coverage, a
missing changelog entry, an unhandled case, a public API obligation not met.
- **Worth changing** — real and not blocking. Say so, and do not spend the
reader's first paragraphs on it.
- **Correct and checked** — kept short and kept in, because it is the only thing
that separates a surface that was read from one that was skipped. It names
what was read, for the same reason a finding names what it collides with.
Rank by what stops the patch first and by consequence second. A cosmetic finding
above a behavioural one costs the review its credibility for the rest of the
list.
Who can reach the path raises a rank and never lowers one. A diff is the weakest
evidence there is about reachability, and a real finding ranked down because it
looked hard to reach is the mistake this rubric cannot recover from: where the
path could not be established, rank on consequence and say so.