---
title: "TYPO3 Core Patch Review"
description: "Skill: typo3-core-patch-review"
canonical: index.html
navigation-title: "TYPO3 Core Patch Review"
---

<a id="typo3-core-patch-review"></a>

# TYPO3 Core Patch Review

- [Markdown source](#markdown-source)
- [References](#references)
  - [Where every task starts](#where-every-task-starts)
  - [Core patch review checklist](#core-patch-review-checklist)

**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.

<a id="markdown-source"></a>

## Markdown source

```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. A copy of them here is one nobody can
correct.

## Establish the patch, then the rules you judge it 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 you fetch after you read the diff confirms a reading instead
   of a test of 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 changed paths**, **the branch it targets**,
   **the commit message**, and **the issue it names**. Every lookup below takes
   one of them as its argument. A review that has not established the target
   branch reviews against the wrong conventions and cannot tell.
   `typo3_gerrit_lookup` with the `Change-Id` or the change number answers all
   four for a patch set somebody has pushed. So this costs no checkout, and you
   triage a shortlist without a fetch. A patch nobody pushed is the other case,
   and one reading of the diff produces the same four.

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. Do that before you form a view of whether the code is right.

**Where the review needs the patch on disk, invoke `typo3-core-patch-checkout`
for that one change. Say in the report which ref it fetched.** A read of the
hunks needs it, because the answer above carries the paths and not the diff. So
does a suite run against the patch.

The decision which of several changes to read does not. The paths, the size and
the vote state settle that. Afterwards nobody can tell a ref you fetched for a
triage from the branches with the reader's own work. One session pulled in eight
before its user stopped it.

## 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. Read both before you read the code a second time.

- `typo3_forge_lookup` with the issue number. What the change is *for* belongs
  to the issue. 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 that calls itself a part tells you the patch is not meant to
  stand alone. That decides every finding about what it lacks.
- The `typo3_gerrit_lookup` answer step 3 established the patch from also says
  what the review so far is. That is the votes on it and the comments on it. A
  comment somebody left on an earlier patch set and nobody answered is a finding
  of its own. 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. Where the patch reached you as a
  commit rather than a change number, its `Change-Id` is the argument. A commit
  hash out of the checkout reaches the same answer.

**Both arguments come out of the commit message, and that 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 you carry in from elsewhere does not
fail. Asked under the other's name, both lookups answer. They return a real
change and a real issue that belong 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 nobody pushed yet has no change, and an answer of nothing is a result.
Say so rather than leave the surface silent. Where the commit in the checkout
and the change on the server differ, name which of the two you read. A review of
an older patch set than the one that exists is the failure this step exists for.
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. Votes, comments and uploads stay with the person who
does the review.

## What the patch owes, per finding

Ask the owner of each obligation rather than recall it:

- `typo3_rule_lookup` for the contribution rules the diff makes relevant. That
  is what a breaking change owes and what a deprecation owes. It is what belongs
  in a changelog entry, and what review readiness means. The sections have
  subjects for names. 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. The two asked
  apart return the same pair twice.

  The lookup keeps a section only where it carries half of what the query asks
  for. So every subject you add 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. Measured:
  two subjects never empty a query, and a third regularly does, whether or not
  the subjects share a document.
- Enumerate what the diff **removes or renames** before you ask. A public class,
  method, property, constant, TCA field, TypoScript path or Fluid ViewHelper
  argument can disappear. That is the class of finding this review exists for. A
  read of the new code does not surface it. 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. It also 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. Two reviews 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. That is a released line the core backports the change
  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. You pick a
  precedent out of its titles.

  A major that still collects entries holds more of a type than the default
  answer carries, so raise `limit` there.

  **Ask it in the words of the entry's title, not in the identifier the diff
  removes.** The enumeration above leaves you with a class and a method name. A
  removal has a title after what it removed *about*: the subsystem, the kind of
  API. It carries the identifiers in a list inside the file.

  So a query naming one of them and coming back empty has established nothing.
  Neither has one narrowed to the branch this patch targets. A precedent sits
  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 holds it. That is `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 reporter filed the
  issue as. The argument a review makes is about the commit keyword instead. It
  says that an earlier bugfix of this kind owed an entry.

  The two disagree in both directions, so read the keyword where it stands.
  `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 live outside the core repository. So what the patch owes
  there is a follow-up rather than part of this patch. That is the finding
  rather than the reason to skip it.

  A review said the wording lived elsewhere and concluded that the patch owed no
  documentation change. The patch made the documented sentence about
  `stdWrap.override` false. Whether the patch owes a manual anything at all is
  `typo3_rule_lookup` asked for `documentation`. A system extension's own
  `Documentation/` is in the checkout, and 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. Whether
  an idiom has a foothold 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: 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 ties to no line in this diff belongs in the issue.

Where the patch is one of a set, read a finding against the end of the set
first. What a later patch in the same set removes is not a defect of the set. To
establish that is a reading of that patch rather than of what a message promises
about it. Still report it where each patch has to stand on its own, and name the
later one that settles it.

## Verification is the project's own, and the diff narrows it

`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. 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 it says what it ran and what
it printed. **It then writes out, by name, the suites on that list it did not
run.** Left out, they make four green suites read as a finished verification.
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. You do not edit the patch under
review, which is the boundary that matters. A probe writes files and restores
them, and you verify the restoration rather than assume it.

This is what turns "this would presumably throw" into a pasted error.
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.

**Put the tree back to what the probe found, which is not always the committed
state.** `git checkout -- <path>` restores the file from the index, and in a
review the index holds the patch set. On a file that carries nothing else, that
undoes the probe. On a file that also carries a change the user asked you for,
it undoes the change with it. Nothing reports the loss.

Where the file carries work of your own, copy it aside before the probe and back
afterwards. Or `git add <path>` first, so the restore lands on your work. Verify
with `git diff --stat <path>` rather than with `git status`. A clean status is
the confirmation on a file you did not edit, and the loss on one you did.

**A finding that turns on what the frontend rendered is one no reading can
settle.** TypoScript defaults, TypoScript in an `ext_localconf.php` and anything
below `lib.parseFunc` are the obvious half. A PHP change to the request
pipeline, an error handler or a page renderer caller is the same case. What
changed is what comes out, not what the file says.

Where no test covers the constellation, the suites stay green on either side of
it. A throwaway functional test that renders one page and prints what came out
settles it. `typo3_rule_lookup` with
`documentId="core/testing/proving-a-rendering"` says how you build one. It says
how you get the output out of a run that would otherwise print nothing. It says
one marker per region, so the response says which part of it changed. It says
what a service holds while the request still runs.

## 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. That is the subject that
describes the wrong action, or the missing issue reference. It is 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 stated target its diff does not fit is a finding of its
own.

## Report

Call `typo3_gerrit_lookup` again before you write, where time has passed since
you read the change. How much is enough depends on how active the patch is, and
on a busy one it is hours. Votes, messages and alternatives pushed as separate
changes arrive on the server with no signal in the checkout. One review read its
change on the first day. It found the CI vote and two alternatives on the
second, by a call made for another reason.

Order by what stops the patch, and say why each one stops it:

1. what blocks the submission of the patch at all;
2. what a reviewer would send it back for;
3. what is worth a change and would not block it;
4. what you checked and found correct, briefly, so a reader can tell a silent
   surface from a verified one;
5. what you raised while you read 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 reader cannot tell a review that reports only
findings 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 makes it long, and length makes the form
matter. Somebody carries a review into the change, the issue or a chat, and
rendered output does not survive the move. Write it to a file only where the
caller asks for one, at a path outside the checkout under review. A modified or
untracked file beside the patch is a surface this review reports on.

## Where the review ends and the rework begins

**When the user asks you to make the change, invoke
`typo3-core-patch-development` and work from it.** That includes the amend and
the push. An instruction to change the patch asks for it: "finish it", "fix it",
"amend it", "write the test". It looks like nothing at all from the inside. It
is a sentence in a conversation in the middle of a session that goes well.

A session that carries on under review rules holds "it does not change the
patch" while it changes the patch. One did. It edited `ColumnMap.php`, added a
fixture column, wrote a functional test, ran seven suites and amended the
commit. All of that was still inside this skill. Nothing broke and the tree
stayed clean, which is why nothing marked the crossing.

**Before the first edit to a file meant to survive, ask whether
`typo3-core-patch-development` should run.** You have to recognise a sentence,
and one has arrived in words this enumeration does not reach. An edit is an act
you already perform. A scratch probe is not that edit: you put it back and it
leaves no diff. The verification section above draws its boundary.

**Three of its calls take an argument this review has just established. That
makes them a restart rather than a repeat.** Those are `typo3_task_guide` with
the change type you are about to write, and `typo3_hint_lookup` with the paths
you will edit. The third is the deprecation sweep. That sweep is
`typo3_changelog_lookup` with `type: deprecation`, which a review is exempt from
and a change is not. You walked the base once at the start of this review,
against files nobody was going to write.

**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 it asks for 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. The reader had meant that the missing test was reason enough to
reject the patch. Where the sentence could be either, ask which the user meant.
A switch costs a turn under the wrong skill's rules, and a question costs one
sentence.

Until the user asks you 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 it reports its findings
in. A review of an extension, a sitepackage or a site project belongs to
`typo3-extension-health` and its checklist. That reads different surfaces
against different rules.
```

<a id="references"></a>

## References

<a id="where-every-task-starts"></a>

### Where every task starts

```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 there or not, and neither side notices. So the first
call below is also the check.

- A client may carry this server's name in each tool's name:
  `mcp__<server>__typo3_project_describe`. So a search for the bare name comes
  back empty where the server is there. A search for a tool's schema needs the
  same form. A `select:` on the bare names returns nothing where the tools are
  there. Look for the qualified form before you read an empty result as an
  answer about the server.
- No `typo3_` tool in this session, or a first call that errors: stop. Say that
  this workflow needs the server and it is not there, and name what came back.
- Do not fall back to general TYPO3 knowledge, and do not start to read the
  checkout. That answer carries this workflow's order and confidence and none of
  its evidence. Nothing in it says which of the two it is.
- Continue only when the user asks you to after you said so. Repeat it in the
  answer and in every finding a lookup would have carried.

## The order

This is an order rather than a list. Each step decides what the next one is
worth. Where a step below carries a condition to skip it, that condition is
narrow on purpose. A skipped prescription teaches the next reader to skip the
ones that matter too.

1. **`typo3_project_describe`** — the repository and whether it holds an
   installation yet. It reports the TYPO3 and PHP version, the project's own
   extensions, its sites, and the commands this repository declares. That
   version filters every later answer. A check the repository does not declare
   is a wrong answer however sensible it sounds.

   The answer ends with the whole procedures this server carries, as ids. That
   list is the only place a client that renders no resource list sees their
   names. Each one is a `typo3_rule_lookup` with that `documentId` rather than a
   search.
2. **`typo3_extension_describe`** for each extension in scope. It says what the
   extension registers, and what it ships beside that. That is its manual, its
   README, its test layers, and its XLF files with the source language each one
   declares. It also says what the extension does *not* ship, and that is the
   half no file listing gives you.

   Where step 1 reported no extension, that answer is this step, and there is
   nothing to call. Say so. A core checkout is that case, because step 1 names
   the project's own extensions and not TYPO3's.
3. **`typo3_task_guide`** with a short English task, the paths it touches, the
   target version and the change type. It answers 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 guide builds
   the brief from the paths as well as the task text. No skill knows which paths
   the caller holds.

   A skill that covers the task is not that brief. A skipped step costs the
   hints and the core checks those paths match. Where the guide's own answer
   named this skill, this is one call for an answer already in the session. 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 you still owe this
   step. A brief that carried everything the lookup matched says so: "these are
   everything typo3_hint_lookup matches for these paths". There the guide made
   the call, and the same query returns the same hints. A brief that stopped
   short says that instead and names the ids it left. You owe those: fetch them
   by id rather than repeat the query.

   Read the sentence rather than the populated `hints` key. That key is present
   either way and does not tell the two apart. `omittedHints` is that sentence
   as data. It is empty where the brief carried everything, and it holds the ids
   the brief left where it stopped short.
5. **`typo3_changelog_lookup` with `type: deprecation`**, at each major the
   package declares. Omit the query and raise `limit` to carry that major whole.
   Those two 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. The core titled those entries about its own code.

   That is one call per declared major, and what comes back is the major. Every
   entry carries its own index tags. `ext:core`, `ext:frontend`, `ext:form` and
   the rest name the system extension a change is **in**. `TCA`, `TypoScript`,
   `Fluid`, `YAML`, `Backend`, `Frontend` name the surface.

   Step 2 picks the package's entries out of that answer by those tags. The tags
   are the system extensions it requires, renders through or registers into, and
   the kinds of file it ships. That costs no further call. An extension key of
   your own is not among them and matches nothing. `tag` narrows one question
   inside a major rather than composes the sweep out of eleven.

   You check the answers against step 2, which is the other half the words did.
   Verify each identifier that comes back in the checkout. A deprecation nothing
   here calls is not a finding.

   Carry the `FullyScanned` / `PartiallyScanned` tag into the answer. It says
   whether the Extension Scanner can find the remaining call sites or whether
   that reading is yours. Bounded this way, you can write the sweep before you
   open a file. That is why it is a step of the order rather than something the
   reading stumbles into.

   **What its silence is worth.** 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. Ask
   it 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, section paths and what
   each manual declares by name, never the text of a page. Declared is a
   property, a class or method the manual documents, a console command. You
   reach one by its own name where the query writes that name the way code
   does. You also reach it where the query is nothing but the name. A PHP
   identifier the manual does not declare has no page named after it.

   An identifier goes to `typo3_changelog_lookup` under its own name. That
   reaches the entries that write it, however the core titled the change. Then
   it goes 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 second declared major.** A package that declares more than one asks a
   second question of every deprecation the sweep returns. Is the replacement on
   the lower one? The entry's `issue` is a query of its own, and it reaches
   every entry filed under that number. The Feature the core announced the
   replacement in is among them.

   The version the core released it in settles that question. Where the number
   reaches no sibling, nobody wrote an entry for the replacement.
   `typo3_rule_lookup` with
   `documentId="extension/compatibility/a-declared-major-that-is-not-installed"`
   is the reading that closes it.

   **Where you do not owe the sweep.** A task that produces no change does not
   reach this step at all. The property is what the task produces. A triage, a
   reproduction and a review illustrate it; they are not the list you read it
   off. The sweep asks what a package will have to stop calling. A task that
   writes nothing is not going to call anything.

   The exemption ends where the workflow produces a change. A review asked to
   make the change is that other workflow. It starts this order again with the
   files it is about to write. To carry somebody else's patch onto current code
   is on the same side. It writes commits. The sweep says whether the code that
   moved under the patch deprecated something the patch calls.

   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. The sweep is empty before it runs.

   That condition is worth a statement, because this step is the largest answer
   the order asks for. It is one call per declared major, with that major's
   deprecations whole. You read which side a change falls on off the files it
   touches, never off the task it started as. One PHP file edited along the way
   puts it back among the ordinary ones.

   A skip there costs the deprecation no finding would have walked into. How
   small the change is decides nothing either. Three statements can call a
   deprecated API as easily as three hundred.

   A test file is one of those wherever it sits. It calls the API it exercises
   and the framework around it, and both deprecate. A fixture is exempt where it
   is data the suite reads, and not where it is a class.

**Before the reading**, write down what the order established. That is the
version that filters every later answer, the packages in scope, and the commands
this repository declares. Write down which steps what discharged. Those are
answers already in the session rather than a second reading. What the files show
belongs to the report at the other end. A caller who cannot see what an answer
rests on cannot tell it from one that rests on nothing.

**Then** read the checkout. Not before. A file list first makes everything after
the list look optional. The conventions then arrive as a footnote to a verdict
that has already formed.

**Before the first edit**, name the files this change will create, change or
delete. A deletion is the caller's to ask for, and this is somebody else's
checkout. It is the one act nothing here can put back.

**Last**, the report names every step of this order it did not reach, and what
stood in for it. That is an answer already in the session, a condition that made
the step empty, or an exemption. A reader cannot tell a step passed over in
silence from one somebody dropped.

## When the lookups run out

A behaviour question that survives the lookups above is one you read out of the
installed source. Do not guess at it. The class that implements the behaviour
and the one it inherits from answer it. That reading is the step after the
lookups. It replaces a change to the code until it works.

A first change that did not work is evidence about the reading. So the second
attempt at one failure reads the source rather than changes the code again.

What it settles is what this installation does and never what TYPO3 supports. So
a finding says that you could not settle the question beyond the version
installed. An answer built on the reading names the version it holds for.

## What each runtime lookup adds after the extension answer

`typo3_extension_describe` in step 2 says what one package registers. The
lookups below say what the installation resolved. That is a different fact even
where the words are the same. So step 2 has made none of these calls:

- `typo3_backend_module_lookup` — the tree position, the labels, the access
  level, the routes and the navigation component the parent module supplies.
  Step 2 lists the modules the package declares. A declaration cannot show that
  inheritance.
- `typo3_icon_lookup` — whether any installed package registers an identifier.
  That validates the ones a template uses. Step 2 lists the identifiers this
  package contributes.
- `typo3_label_lookup` — the labels as the installation resolves them, with its
  overrides applied. Step 2 lists the package's XLF files and the source
  language each declares, never what a unit says here.
- `typo3_fluid_namespace_list` — the prefixes any template may use without a
  declaration, from every package at once. Step 2 lists the package's own
  declarations. So an empty list there is no evidence that no package registers
  a prefix globally.
- `typo3_configuration_lookup` — the resolved configuration value, after every
  extension has had its say. For a form data group it gives the order the
  providers really run in. Step 2 answers nothing about that surface at all.
  What a registration declares is not what the installation resolves.
- `typo3_service_lookup` — the class the container really injects for a service
  id, an interface or a tag. Decorations and overrides count. Step 2 lists what
  the package's own `Services.yaml` declares, never what won.
- `typo3_schema_lookup` — the columns TYPO3 derives for a table from its TCA. It
  gives the type, the nullability and the default each one gets. Step 2 lists
  the tables the package registers and nothing about their shape.
- `typo3_flexform_lookup` — the data structure the installation resolves a
  `type=flex` field to, sheet by sheet, with listeners and migrations applied.
  Step 2 lists the content elements a package registers, never the structure
  each one's form builds.
- `typo3_record_lookup` — the rows of any table the installation has TCA for. It
  says how many there are and where they sit. It says what one column holds
  across them and which rows depart from its default. Step 2 has no row in its
  answer at all.

None of these says whether what it reports is right. `typo3_hint_lookup` and
`typo3_documentation_lookup` do. A subsystem its own runtime lookup confirmed
can still break every rule that governs it. So it is not established until you
asked both.

## A rule reads in both directions

It says what new code should do, and it says what this checkout already does
wrong. A file that 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 because it costs. Before you
report one, find what it is there for and say so. That is the manual, the
README, the changelog, the setting that drives it, or the declared versions.

Where the documentation states a purpose, what you have is a trade-off, not a
defect. Name it with its cost and its alternative. Where you cannot find one,
the finding says that you could not establish one, not that none exists. If you
skip this, your review is 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 you read, at its path and its line. A command you
ran, with what it printed. A mechanism you traced into an installed package. Say
which of the three it is. If you leave it unsaid, a finding from a CI file
weighs as much as one with a verified line.

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. The linter the
repository already declares is the cheapest evidence in it.

Do not run a change under that instruction. Name an unknown in the answer as
evidence that is available, and do not run it unasked. An unknown is a test
suite, a shell pipeline, a console command.

What a check prints is not the finding. The configuration that makes it fail is
still what the finding is about. The run takes that finding from derived to
established.

## What this server does not know

It does not read your working tree. You establish which files changed, which
branch you are on, and whether a path or an identifier still exists there. Then
pass the concrete paths back, because that turns a general convention into an
answer about this code.

## Query it in English

The knowledge is English and the match is lexical. So a query in another
language reaches the loanwords the two happen to share and nothing else.
Translate the subject before the call and the answer back afterwards, whatever
language you speak with the user.
```

<a id="core-patch-review-checklist"></a>

### Core patch review checklist

```markdown
# Core patch review checklist

Write the surfaces below down whole before you read the diff a second time.
Answer each one 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 you name it.

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. That is the file you opened, the
call site you followed, the command you ran, the lookup that answered.

Assessed with nothing under it is the cheapest sentence in a review, and the one
a reader cannot check. So where the reading did not happen, write 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. A read of the new
  code does not show this surface. So enumerate it from the diff's deletions,
  and from every signature line its additions touch. A parameter added to a
  public or protected method on a non-final class fatals every subclass that
  overrides it. It reads in the diff as an addition.

  No suite in the checkout can fail on that. So answer the surface from the
  declaration rather than from a green run.
- **Behaviour.** What the patch changes for code that calls it, the paths it
  does not touch included. That is a guard that was unreachable before and is
  live after. Or it is a value the code used to write and now does not. It is 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.
  Answer the surface with both halves: what ran, and which of the suites the
  guide returned nobody started.

  Where the change sits in a chain, what the follow-up touches decides what a
  test is worth. One that pins behaviour the next change rewrites is churn. One
  that covers what the follow-up leaves alone lasts.
- **Documentation and changelog.** What the diff obliges: the entry, its
  directory, its file name, its cross-references. Equally, whether the patch
  owes an entry at all. A demand for 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.
  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 the patch owes none.
- **Commit shape.** Subject, body, issue reference, target branch line, and the
  markers the change type requires.
- **Review readiness.** Whether a reader can understand the patch from the issue
  and the message alone. Whether a reviewer can reproduce what it claims to fix.
  Read the issue for that; do not infer it 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 checkout
  holds the commit of that patch set. Also 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.
- **The chain this change sits in.** You rate the patch on its own, and the
  chain says whether a shape in it is preparation. `typo3_gerrit_lookup` answers
  the chain with the change itself. An entry stacked above is a follow-up. Its
  own file list says what a one-class namespace, a non-final class or a service
  without a caller is for.

  Read it before calling one of those an oversight. Report what it explains as
  the question it is.
- **Security.** Where the diff touches authorization, user input, output
  escaping, or file paths. Or a boundary between what a role may and may not do.
  A finding here is a value and a sink. Establish both, or it is not a finding.
- **The working tree around the patch.** A modified or untracked file beside the
  commit. 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 somebody would notice;
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.
Nothing in the report says you did not mean that.

Report what the patch did not introduce in those words. It stays in the review
where it blocks submission on its own, and goes to the issue tracker otherwise.
The reading the user asked for is of a change. A list of what was already wrong
around it is a second review nobody ordered.

Tell what you verified from what you reasoned. A behaviour you traced into the
installed code, a command's output, and a reading of the diff weigh differently.
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 nothing records the drop. Name each
candidate you raised while you read and then let go, with what let it go. That
is the guard that was there after all, its caller, or the rule that does not
apply. One sentence each. It tells the reader that a quiet surface went quiet
after the reading rather than before it.

The two directions do not meet the same bar. To raise a candidate costs a
reading. To drop one costs the author a finding, silently, and nothing
afterwards says it happened. So drop a candidate only where something concretely
disproves it. Report one you can neither establish nor disprove 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. 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. That is a guard nobody can pass
  or a caller that cannot exist, at a line.

Before you report a finding, make the author's case against it. That is the
caller they know holds the guard, the subsystem's invariant, and the choice the
commit message already states. It is the change stacked on this one that a shape
here may prepare.

You still rate the patch on its own. Report what the follow-up explains as a
question rather than as an oversight. Report what survives that together with
what it survived. What does not survive is a dropped candidate, with the same
evidence written down.

## Severity

- **Blocks submission** — the patch cannot go up as it is. The hook rejects the
  message, the change breaks what the rules forbid, or the diff does not match
  its message.
- **Sent back in review** — a reviewer would ask for it. That is missing
  coverage, a missing changelog entry, an unhandled case, a public API
  obligation not met.
- **Worth a change** — real and not blocking. Say so, and do not spend the
  reader's first paragraphs on it.
- **Correct and checked** — short, and kept in. It is the only thing that
  separates a surface you read from one you skipped. It names what you 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. A real finding ranked down because it
looked hard to reach is the mistake this rubric cannot recover from. Where you
could not establish the path, rank on consequence and say so.
```