Skip to content

Commit 0662371

Browse files
authored
docs: write down the rule that every gate here presupposes (#799)
## Description Every gate in this repository runs on a diff that already exists. `make gate`, the api-surface snapshot, PHPStan, the CHANGELOG check and the ADR checkbox all need code before they can say anything. Measured: a grep over all eight `AGENTS.md` files finds no specification obligation — only false positives like "Specify PHP version" — and the documented Development Workflow begins with "Branch off `main`", which is after the design decisions have been taken. The repository has unusually strict post-implementation control and no pre-implementation control at all. That gap is the actual defect. This PR writes the missing rule down, moves the one existing design obligation to where it belongs, and adds the part of it that can be mechanically enforced. ### 1. The rule (`AGENTS.md`, Development Workflow step 1) Write down what the change must do, what it explicitly does **not** do, and which suite proves each requirement — before choosing where the code goes. With a table naming the classes of change it applies to, so it does not become "write a document for every typo": | Change | Specify first | |--------|---------------| | Bugfix, dependency update, small internal refactor, documentation only | no | | New provider | usually | | New feature, new public API contract, breaking or deprecating change, security or credential topic, a change spanning several layers, large compatibility rework | yes | **The format is deliberately not prescribed.** Which document that is remains open — [#798](#798) trials one answer — and this rule should not depend on settling it. If that trial is dropped, the rule stands. **And it says plainly that it is not machine-checked.** `ci:test:repo` sees the working tree, not the pull request; the job that could see one is `pr-quality`, which belongs to the shared `netresearch/.github` workflow and is not this repository's to extend. A rule that implies a gate it does not have is worse than one that admits it has none. ### 2. The ADR obligation moves before the implementation PR It was a PR checkbox — "I have added an ADR … if this changes the public surface" — evaluated against a diff that already existed. The decision is what the implementation follows from, and `api-surface.txt` already tells you when the surface is being touched, so there is nothing to wait for. The PR template is updated to match, because a template contradicting `AGENTS.md` is exactly the defect [#797](#797) just fixed. ### 3. `AdrReferenceIntegrityTest` — the enforceable part `AdrLifecycleTest` already binds the records to each other: status words, the `:Amended:` / `:Superseded:` pairing, and that the index resolves every record it names. All of that stays inside `Documentation/Adr`. Nothing checked a reference from **outside** it — which is where four dead pointers sat until [#795](#795), one of them published on the landing page as a security control's `evidence` URL, where it rendered as a public 404. The test asserts existence only. It deliberately does not judge whether the record says what the citing sentence claims, because nothing mechanical can: both references #795 fixed pointed at the *wrong record*, not at a misspelling of the right one. It also does not address [#793](#793) — a drifted line number still points at a line that exists. **Probed in both directions.** Green on the current tree. With the pre-#795 `AGENTS.md` restored it fails, naming both dead references and nothing else. Re-proved after the Rector refactor below, because the earlier proof described code that no longer existed. Two mistakes worth recording rather than quietly fixing. The first version failed on its own docblock: it scans `.php` and I had spelled the dead filenames out in full — the docblock now omits the extension and says why, so nobody "completes" them. And `ci / Rector` was red on the first push, because I carried over a conclusion from the sibling PR where the file sat in `Build/Scripts`, which Rector does not analyse; this one sits in `Tests/`, which `Build/rector/rector.php` lists explicitly. `LocallyCalledStaticMethodToNonStaticRector` wanted both private static helpers as instance methods, the second only after fixing the first cascaded into it. ## Related Issue None. Found while reviewing this repository's governance. ## Type of Change - [x] Documentation update ## Checklist - [x] My code follows the project's coding standards - [x] I have run `make gate` — in full, and it passes: `cgl`, `phpstan`, `unit`, `fuzzy`, `rector -n -p 8.2` and `functional -d sqlite`, plus the CHANGELOG check. It needed a dependency resolution at 8.2 in this worktree first: the `.Build` copied from `main` made the pinned Rector run die in `platform_check.php` before Rector started, and made the full unit suite fatal on `InMemoryVaultService` against a newer nr-vault than CI installs. Both disappeared after `runTests.sh -s composerUpdate -p 8.2`, which confirms by resolution what I had first only inferred — the fatal was the environment, not this branch. - [x] I have added tests that prove my fix/feature works — the new test is the deliverable; it was verified red against the defect it exists for. - [x] I have updated the documentation accordingly — `AGENTS.md` is the documentation here. - [ ] I have added a `CHANGELOG.md` entry — no user-facing change; docs-only commits in this repo do not carry one. - [ ] If this changes the public surface, the ADR landed before this PR — no public surface change. - [x] My changes generate no new warnings _Assisted by claude-code:claude-opus-5 — [Session](https://claude.ai/code/session_015QXXkquh2eQNBiTYA39Wss)_
2 parents 9f99245 + 2d74955 commit 0662371

3 files changed

Lines changed: 171 additions & 8 deletions

File tree

.github/PULL_REQUEST_TEMPLATE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,5 +21,5 @@
2121
- [ ] I have added tests that prove my fix/feature works
2222
- [ ] I have updated the documentation accordingly
2323
- [ ] I have added a `CHANGELOG.md` entry under `## [Unreleased]`
24-
- [ ] I have added an ADR under `Documentation/Adr/` if this changes the public surface
24+
- [ ] If this changes the public surface, the ADR under `Documentation/Adr/` landed before this PR (see step 1 of the Development Workflow in `AGENTS.md`)
2525
- [ ] My changes generate no new warnings

AGENTS.md

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,34 @@ The full `./Build/Scripts/runTests.sh -s functional` run includes ~34 provider-c
7676

7777
## Development Workflow
7878

79-
1. Branch off `main` (worktree convention — see project memory).
80-
2. Use `make` shortcuts (`make test-unit`, `make phpstan`, `make cgl`) — they delegate to `runTests.sh`.
81-
3. Pre-commit hooks via `Build/captainhook.json` (auto-installed by composer plugin) run cgl + phpstan + commit-msg checks.
82-
4. Sign commits with `git commit -S --signoff` (DCO required).
83-
5. **Pre-push gate: `make gate`.** It runs the six suites CI runs — `cgl`,
79+
1. **Specify before designing, for the classes of change below.** Write down what
80+
the change must do, what it explicitly does **not** do, and which suite proves
81+
each requirement — before choosing where the code goes. The format is not
82+
prescribed here; writing it at all is the rule.
83+
84+
| Change | Specify first |
85+
|--------|---------------|
86+
| Bugfix, dependency update, small internal refactor, documentation only | no |
87+
| New provider | usually |
88+
| New feature, new public API contract, breaking or deprecating change, security or credential topic, a change spanning several layers, large compatibility rework | yes |
89+
90+
Why this step exists as a rule: every other gate in this repository runs on a
91+
diff that already exists. `make gate`, the api-surface snapshot, PHPStan, the
92+
CHANGELOG check and the ADR checkbox all presuppose code. Nothing before this
93+
step checked anything, so a wrong premise about *what* was wanted survived
94+
until review, and a wrong premise about the public surface survived until
95+
`api-surface.txt` failed.
96+
97+
**This one is not machine-checked, and cannot be here.** `ci:test:repo` sees
98+
the working tree, not the pull request; the job that could see one is
99+
`pr-quality`, which belongs to the shared `netresearch/.github` workflow and
100+
is not this repository's to extend. Keep it because it is the rule, not
101+
because something will catch you.
102+
2. Branch off `main` (worktree convention — see project memory).
103+
3. Use `make` shortcuts (`make test-unit`, `make phpstan`, `make cgl`) — they delegate to `runTests.sh`.
104+
4. Pre-commit hooks via `Build/captainhook.json` (auto-installed by composer plugin) run cgl + phpstan + commit-msg checks.
105+
5. Sign commits with `git commit -S --signoff` (DCO required).
106+
6. **Pre-push gate: `make gate`.** It runs the six suites CI runs — `cgl`,
84107
`phpstan`, `unit`, `fuzzy`, `rector -n` (pinned to PHP 8.2, as in CI) and
85108
`functional -d sqlite` — plus the CHANGELOG check. Run it as one command:
86109
invoking the six by hand is how `rector` and `fuzzy` get skipped, which is
@@ -92,7 +115,7 @@ The full `./Build/Scripts/runTests.sh -s functional` run includes ~34 provider-c
92115

93116
Contract changes (setter clamps, validation ranges) have assertion twins in
94117
`Tests/Fuzzy/` — grep there before pushing.
95-
6. PRs target `main`. CI matrix: PHP 8.2–8.5 × TYPO3 `^13.4` / `^14.3`; merged via `--merge` strategy (preserves signatures).
118+
7. PRs target `main`. CI matrix: PHP 8.2–8.5 × TYPO3 `^13.4` / `^14.3`; merged via `--merge` strategy (preserves signatures).
96119
<!-- AGENTS-GENERATED:END development -->
97120

98121
<!-- AGENTS-GENERATED:START filemap -->
@@ -170,7 +193,7 @@ nr_llm/
170193
- **Where does TCA live?** Per-table file under `Configuration/TCA/` for new tables; `Configuration/TCA/Overrides/` to extend existing tables (incl. `pages`, `tt_content`).
171194
- **Stuck on a "this works locally but breaks in CI" issue?** Reproduce inside `Build/Scripts/runTests.sh -s <suite>` first — it uses the same Docker PHP image as CI.
172195
- **Adding a config option?** TCA + `LLL:` translation key in `Resources/Private/Language/locallang*.xlf` for both EN and DE.
173-
- **Touching the public surface?** Add an ADR under `Documentation/Adr/`. Format: `Adr<N>Description.rst`.
196+
- **Touching the public surface?** Add an ADR under `Documentation/Adr/`. Format: `Adr<N>Description.rst`. It lands **before** the implementation PR, not inside it — the decision is what the implementation follows from, and `api-surface.txt` already tells you when you are touching the surface, so there is nothing to wait for. `AdrReferenceIntegrityTest` refuses a reference to a record that does not exist; that the record arrived first is not machine-checked.
174197
- **Changing an `@api` signature?** `Tests/Unit/Api/api-surface.txt` freezes it, constructors included — the class's own, or the one it inherits from a base inside `Netresearch\NrLlm`; one inherited from TYPO3 core or the SPL is left out because it differs across the version matrix. The failure says whether the diff is additive (regenerate + `### Added`) or breaking (decide first). Removals follow `Documentation/Api/Deprecation.rst`, whose inventory is asserted against the `@deprecated` docblocks in both directions.
175198
- **Changing the supported TYPO3 / PHP range?** `VersionConsistencyTest` pins four surfaces against each other — `composer.json`, `ext_emconf.php`, the `ci.yml` matrix and `Documentation/Api/SupportMatrix.rst` — and fails on the first of those you forget. It does **not** see the prose: `README.md` (the two badges and the Requirements list), `Documentation/Installation/Index.rst`, `Documentation/Introduction/Index.rst`, `Documentation/Developer/FeatureServices/Index.rst`, `Documentation/Testing/CiConfiguration.rst` (a hand-copied matrix excerpt) and `Documentation/Developer/IntegrationGuide.rst` (the TER constraint in its `ext_emconf.php` example) repeat the same range with nothing checking them. Update those by hand in the same change — and grep for the old floor before you finish, because that list is what has been found, not a guarantee. `BASELINE.md`'s "Multi-version CI" row is the one prose surface that IS asserted, by `Tests/Unit/BaselineConsistencyTest`.
176199
- **Linking between backend controllers?** Use the full Extbase alias `Backend\<Name>` (e.g. `'controller' => 'Backend\\TaskWizard'`), not the short name — `resolveControllerAliasFromControllerClassName()` keeps the segment after `Controller\`, so a short alias yields an empty URL / `InvalidControllerNameException`. Namespaced backend arguments are OFF here, so use bare `controller`/`action` keys (NOT `tx_nrllm_task[...]`; the bot's suggested namespaced form is wrong for this instance). Introduced by ADR-027's TaskController split.
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
<?php
2+
3+
/*
4+
* Copyright (c) 2025-2026 Netresearch DTT GmbH
5+
* SPDX-License-Identifier: GPL-2.0-or-later
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace Netresearch\NrLlm\Tests\Unit;
11+
12+
use FilesystemIterator;
13+
use PHPUnit\Framework\Attributes\CoversNothing;
14+
use PHPUnit\Framework\Attributes\Test;
15+
use RecursiveCallbackFilterIterator;
16+
use RecursiveDirectoryIterator;
17+
use RecursiveIteratorIterator;
18+
use SplFileInfo;
19+
20+
/**
21+
* Every ADR named by filename anywhere in the repository must resolve.
22+
*
23+
* {@see AdrLifecycleTest} already binds the records to each other — status
24+
* words, the `:Amended:` / `:Superseded:` pairing, and that the index resolves
25+
* every record it names. All of that stays inside `Documentation/Adr`. What
26+
* nothing checked is a reference from OUTSIDE it, and that is where the rot
27+
* was: `AGENTS.md` sent readers to `Adr001ThreeTierProviderArchitecture` and
28+
* `Adr012ApiKeyStorageVault`, neither of which exists, and the landing page
29+
* published the second one as the `evidence` URL of a security control, where
30+
* it rendered as a public 404 (#795).
31+
*
32+
* Those two names are written WITHOUT the `.rst` extension on purpose. This
33+
* test scans `.php` files too, so spelling them in full here makes it fail on
34+
* its own docblock — which is exactly what happened on the first run. Do not
35+
* "complete" them.
36+
*
37+
* Both were wrong in a way a spell-check would not have caught. ADR-001 is
38+
* *Provider Abstraction Layer*; the record the sentence wanted was ADR-013. So
39+
* this test asserts existence only — that the file is there — and deliberately
40+
* does not try to judge whether the record says what the citing sentence
41+
* claims. Nothing mechanical can do that.
42+
*
43+
* It is the enforceable part of a larger problem. Issue #793 is about ADRs
44+
* citing code by line number and those citations rotting silently; this test
45+
* does not address that at all, because a line number that has drifted still
46+
* points at a line that exists.
47+
*/
48+
#[CoversNothing]
49+
final class AdrReferenceIntegrityTest extends AbstractUnitTestCase
50+
{
51+
/**
52+
* A concrete record filename: three digits and a name.
53+
*
54+
* Anchored on the digits on purpose, so the format template `AGENTS.md`
55+
* documents for new records — `Adr<N>Description.rst` — is not read as a
56+
* reference to a file called that.
57+
*/
58+
private const REFERENCE_PATTERN = '/\bAdr\d{3}[A-Za-z0-9]*\.rst\b/';
59+
60+
/** Directories that hold no repository content of ours. */
61+
private const SKIPPED_DIRECTORIES = ['.Build', '.git', 'node_modules', 'vendor', '__pycache__'];
62+
63+
/** Where a reference can be written. Binary and generated formats are not scanned. */
64+
private const SCANNED_EXTENSIONS = ['md', 'rst', 'json', 'php', 'yml', 'yaml', 'neon', 'txt', 'xml'];
65+
66+
private function repoRoot(): string
67+
{
68+
return dirname(__DIR__, 2);
69+
}
70+
71+
#[Test]
72+
public function everyAdrNamedByFilenameExists(): void
73+
{
74+
$adrDir = $this->repoRoot() . '/Documentation/Adr';
75+
$missing = [];
76+
77+
foreach ($this->scannableFiles() as $relative) {
78+
$contents = file_get_contents($this->repoRoot() . '/' . $relative);
79+
self::assertIsString($contents, $relative . ' could not be read.');
80+
81+
if (preg_match_all(self::REFERENCE_PATTERN, $contents, $matches) === 0) {
82+
continue;
83+
}
84+
85+
foreach (array_unique($matches[0]) as $basename) {
86+
if (!is_file($adrDir . '/' . $basename)) {
87+
$missing[] = sprintf('%s references %s, which does not exist', $relative, $basename);
88+
}
89+
}
90+
}
91+
92+
sort($missing);
93+
94+
self::assertSame([], $missing, sprintf(
95+
"These ADR references do not resolve:\n\n %s\n\n"
96+
. "Open Documentation/Adr and use the real filename. Check that the record you land on is\n"
97+
. "the one the sentence means — the two references this test was written for were both the\n"
98+
. 'wrong record, not a misspelling of the right one.',
99+
implode("\n ", $missing),
100+
));
101+
}
102+
103+
/**
104+
* @return list<string> repo-relative paths
105+
*/
106+
private function scannableFiles(): array
107+
{
108+
$found = [];
109+
$iterator = new RecursiveIteratorIterator(
110+
new RecursiveCallbackFilterIterator(
111+
new RecursiveDirectoryIterator($this->repoRoot(), FilesystemIterator::SKIP_DOTS),
112+
static fn(SplFileInfo $file): bool => !$file->isDir()
113+
|| !in_array($file->getFilename(), self::SKIPPED_DIRECTORIES, true),
114+
),
115+
);
116+
117+
foreach ($iterator as $file) {
118+
if (!$file instanceof SplFileInfo || !$file->isFile()) {
119+
continue;
120+
}
121+
122+
// The CLAUDE.md / GEMINI.md aliases are symlinks to the AGENTS.md
123+
// beside them. Scanning them too reports one bad reference three
124+
// times, which costs the reader two lookups to find one edit.
125+
if ($file->isLink()) {
126+
continue;
127+
}
128+
129+
if (!in_array(strtolower($file->getExtension()), self::SCANNED_EXTENSIONS, true)) {
130+
continue;
131+
}
132+
133+
$found[] = substr($file->getPathname(), strlen($this->repoRoot()) + 1);
134+
}
135+
136+
sort($found);
137+
138+
return $found;
139+
}
140+
}

0 commit comments

Comments
 (0)