Florian WildeandClaude Opus 5
80d97467da
Acht Meilensteine lagen ohne fremden Blick auf main. Ein Review mit eigener
Datenbank, Gegenproben und vollem Suitenlauf hat sie gegen die
Akzeptanzkriterien und gegen CLAUDE.md geprueft.
Die Kernregeln halten: fuenf von fuenf Gegenproben gegen Versionierung,
Mandantengrenze, Abschlussregel und Selbstauskunft wurden rot. Alle Befunde
lagen am Rand — und die drei wertvollsten waren Gegenproben, die gruen blieben.
Schwer:
- Der Link in der Mail "Verzeichnis-Abgleich fehlgeschlagen" war ein 404
(/verwaltung/verzeichnis statt /admin/verzeichnis). Die eine Meldung, die an
alle Administratoren geht, im Moment eines Ausfalls. Ein Test prueft jetzt
jeden Meldungspfad gegen die vorhandenen Routen.
- Der Export eines Entwurfs mit Baustein lieferte einen leeren Abschnitt unter
dem Platzhalter "wird ersetzt" — der Zweck des Formats ist, eine Vorlage in
eine zweite Installation zu bringen. Bausteine werden jetzt aufgeloest.
Mittel:
- Der Anhang-Handler hatte keinen Test; die ganze Rechtepruefung liess sich
durch einen hartcodierten ADMIN ersetzen, Suite gruen. Es ist die einzige
Stelle, die rohe Dateibytes ausliefert. tests/anhang-route.test.ts faehrt ihn
jetzt echt, acht Pruefungen.
- /api/scim in der Proxy-Allowlist stand in keinem Test. OEFFENTLICH ist
exportiert, der Test vergleicht beide Richtungen: eine Liste, die nur in einer
Richtung geprueft wird, laeuft dem Code hinterher.
- Ein offener Ereignisstrom ueberlebte Abmelden, Deaktivieren und
Sitzungswiderruf. Der Herzschlag prueft die Sitzung jetzt nach. Dass es kein
Test zeigte, lag an einer Attrappe `sitzung: {}` — eine Attrappe ohne die Form
der Wirklichkeit prueft etwas anderes als die Wirklichkeit.
- README kannte MITARBEITER nicht und behauptete, unbekannte Benutzer wuerden
angelegt. Wer auf v0.2.0 hebt und nur das README liest, haette genau diese
Leute ausgesperrt.
- sicherung.md nannte einen Anhaenge-Pfad, den es nirgends gibt — als
Rueckfallwert in einem Skript mit rm -rf. Jetzt bricht es ab statt zu raten.
- Attributnamen wurden nur in der Server Action geprueft, waehrend der
Graph-Leser sich im Kommentar darauf berief.
Dazu #027: die Ende-zu-Ende-Ablaeufe raeumen jetzt zentral auf, vor und nach
dem Lauf, und ein struktureller Test bewacht es. Beim ersten echten Lauf seit
dem 26. September waren sieben Ablaeufe rot, vier Ursachen, keine ein
Produktfehler — unbemerkt, weil pnpm test:e2e in keinem Tor steht. Das ist jetzt
#038 und keine Fussnote.
Ein Befund des Reviews war falsch (der Zustands-Handler habe keinen Test; er hat
acht direkte Aufrufe). Nachgeprueft, bevor etwas geaendert wurde.
1454 Vitest-Tests (vorher 1432), 113 E2E-Ablaeufe gruen.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
137 lines
9.5 KiB
Markdown
137 lines
9.5 KiB
Markdown
# #036 Reviewbefunde aus der Durchsicht von Phase 2
|
||
|
||
**Status:** erledigt
|
||
**Im Tracker:** –
|
||
**Meilenstein:** v0.2.0
|
||
**Labels:** phase-2, backend, test, docs
|
||
**Geschätzt:** 4 h **Tatsächlich:** 3 h 40 min
|
||
|
||
## Ziel
|
||
|
||
Acht Meilensteine von Phase 2 (M15–M22) lagen ohne fremden Blick auf `main`. Ein Review-Agent, der
|
||
den Code nicht geschrieben hat, hat sie gegen die Akzeptanzkriterien und gegen `CLAUDE.md` geprüft —
|
||
mit eigener Datenbank, Gegenproben und einem vollen Suitenlauf.
|
||
|
||
**Ergebnis:** Die Kernregeln halten. Fünf von fünf Gegenproben gegen Versionierung,
|
||
Mandantengrenze, Abschlussregel und Selbstauskunft wurden rot. Alle Befunde lagen am Rand — in zwei
|
||
Route Handlern, einem Export-Pfad, in Betriebsdokumentation und in Kommentaren.
|
||
|
||
**Die drei wertvollsten Befunde waren Gegenproben, die grün blieben.** Das ist das wiederkehrende
|
||
Muster dieses Projekts: nicht der falsche Code fällt auf, sondern der Code, der *aussieht*, als
|
||
prüfte er etwas.
|
||
|
||
## Behoben
|
||
|
||
### Schwer
|
||
|
||
- [x] **Der Link in der Mail „Verzeichnis-Abgleich fehlgeschlagen“ war ein 404.** Gebaut wurde
|
||
`/verwaltung/verzeichnis` — `verwaltung` ist der Name des **Rechts**, die Seite liegt unter
|
||
`/admin/verzeichnis`. Das ist die eine Meldung, die an **alle** Administratoren geht, und
|
||
zwar im Moment eines Ausfalls: der Empfänger klickt und landet auf der
|
||
Nicht-gefunden-Seite. Die Pfade stehen jetzt als `MELDUNGS_PFADE` an einer Stelle, und ein
|
||
Test prüft **jeden gegen die Routen unter `src/app/`** — ein Pfad, den es nicht gibt, fällt
|
||
beim Testlauf auf und nicht im Postfach.
|
||
- [x] **Der Export eines Entwurfs mit Baustein lieferte einen leeren Abschnitt**, unter dem
|
||
Editor-Platzhalter „wird ersetzt“, und der Import nahm das stillschweigend an. Ein Redakteur
|
||
bringt eine noch unveröffentlichte Vorlage in eine zweite Installation — genau der Zweck des
|
||
Formats —, und der geteilte Abschnitt kommt dort ohne einen einzigen Schritt an. Baustein-
|
||
Abschnitte werden beim Export jetzt **aufgelöst**, wie beim Veröffentlichen; der Verweis
|
||
wandert weiterhin nicht mit.
|
||
|
||
### Mittel
|
||
|
||
- [x] **Der Route Handler, der Anhänge ausliefert, hatte keinen einzigen Test.** Die ganze
|
||
Rechteprüfung ließ sich durch einen hartcodierten ADMIN ersetzen, ohne dass ein Test rot
|
||
wurde. Das ist die **einzige** Stelle der Anwendung, die rohe Dateibytes ausliefert. Neu:
|
||
`tests/anhang-route.test.ts` fährt den Handler echt, acht Prüfungen — darunter das
|
||
Akzeptanzkriterium von #032 („nicht abrufbar **über seine Adresse**“), das bis hierher nur
|
||
auf Dienstebene geprüft war. Die Gegenprobe ist jetzt dreifach rot.
|
||
- [x] **`/api/scim` in der Proxy-Allowlist stand in keinem Test.** Die Zeile ließ sich entfernen,
|
||
ohne dass etwas rot wurde — und ohne sie leitet der Proxy jede Zustellung auf `/anmelden` um:
|
||
der Anbieter meldet „Provisioning failed“, in Laufzettel steht nichts, das Provisioning hört
|
||
still auf zu arbeiten. `OEFFENTLICH` ist jetzt exportiert, und ein Test prüft **in beide
|
||
Richtungen**: wer die Liste erweitert, muss es im Test aufschreiben. Eine Aufzählung, die nur
|
||
in einer Richtung geprüft wird, läuft dem Code hinterher.
|
||
- [x] **Ein offener Ereignisstrom überlebte Abmelden, Deaktivieren und Sitzungswiderruf.** Das
|
||
Recht wurde genau einmal geprüft, beim Aufbau, und Kennung samt Rolle für die Lebensdauer des
|
||
Stroms festgehalten. Ein Administrator deaktiviert ein Konto — und eine offen gelassene
|
||
Lasche bekommt weiter live Status, Schrittzustände und Zählwerte, bis jemand sie schließt.
|
||
`docs/architektur.md` verspricht „ein deaktivierter Benutzer wird auch mit gültigem Cookie
|
||
abgewiesen“; für einen Strom galt das nicht. Der Herzschlag prüft jetzt nach
|
||
(`sitzungNochGueltig`), eine Herabstufung engt den Strom ein statt ihn zu beenden.
|
||
- [x] **`README.md` kannte die Rolle `MITARBEITER` nicht**, und ein Satz daneben war seit #035
|
||
falsch: „Unbekannte Benutzer werden beim ersten Login angelegt.“ Wer auf v0.2.0 hebt, liest
|
||
das README, nimmt an, für Leute ohne Rollengruppe bleibe alles wie bisher — und sperrt danach
|
||
genau diese Leute aus. #035 hatte den Satz in `docs/betrieb/authentik.md` richtiggestellt und
|
||
das README stehengelassen. Dazu: der Stand stand weiter auf „in Entwicklung auf v0.1.0“.
|
||
- [x] **`docs/betrieb/sicherung.md` nannte einen Anhänge-Pfad, den es nirgends gibt**
|
||
(`/opt/laufzettel/daten/anhaenge`) — als Rückfallwert in einem Skript, in dem ein `rm -rf`
|
||
steht. `deploy/install.sh` setzt `/var/lib/laufzettel/anhaenge`, und ohne die Variable nimmt
|
||
die Anwendung `./daten/anhaenge` relativ zu ihrem Arbeitsverzeichnis. Der Rückfall passte zu
|
||
**keinem** der drei. Jetzt `${ANHAENGE_PFAD:?…}`: fehlt die Variable, **bricht** der Befehl
|
||
ab, statt zu raten — vor dem `rm -rf`. Dasselbe Dokument hat denselben Fehler in #032 schon
|
||
einmal gehabt.
|
||
- [x] **Attributnamen wurden nur in der Server Action geprüft**, während sich
|
||
`src/server/graph/leser.ts` im Kommentar darauf berief („feste Feldnamen aus
|
||
`ATTRIBUT_MUSTER`, also ist hier nichts zu maskieren“). Nachgewiesen: ein Name mit Komma und
|
||
`$filter` kam durch den Dienst und schrieb einen **zweiten `$filter`** in die Graph-Adresse.
|
||
Hereinkommen konnte er nur mit `verwaltung` oder per Skript, Seed und `psql`, die
|
||
Sicherheitswirkung ist also klein — der Befund ist der **Ort** der Regel. Jetzt
|
||
`zuordnungPruefen` im Dienst (wirft) und Verwerfen in `zuordnungLesen` (damit eine kaputte
|
||
Zeile nicht die Quellenliste sprengt).
|
||
|
||
### Leicht
|
||
|
||
- [x] **Drei Kommentare versprachen mehr als der Code hält.** Der Kommentar am Konstanten-Block der
|
||
Wartezeiten sagte „nach dem vierten Fehlschlag wird aufgegeben“, während es der fünfte ist —
|
||
und das ist der Kommentar, den man zuerst liest. Die Migration der Benachrichtigungen
|
||
behauptete „der Dienst prüft dasselbe noch einmal, nur mit besserer Meldung“; tut er nicht,
|
||
und er soll es auch nicht (zwei Instanzen hinter einem Lastverteiler fragen beide und
|
||
bekommen beide „nein“ — nur der partielle Index hält). **Die Migration selbst bleibt
|
||
unverändert:** eine angewandte Migration wird nicht umgeschrieben, so wie eine
|
||
veröffentlichte `VorlagenVersion` nicht. Die Richtigstellung steht bei `einreihen`, wo sie
|
||
gelesen wird.
|
||
- [x] **Der Protokolleintrag „UPN mehrdeutig“ nannte immer „2“**, auch bei sieben Treffern:
|
||
`personZuUpn` hatte `take: 2`, und dieselbe Zahl ging in den Eintrag. Wer das Protokoll
|
||
liest, liest es, **um den Fall aufzulösen** — er muss wissen, wie viele Zeilen zu bereinigen
|
||
sind. `take` entfernt, Test mit vier Treffern.
|
||
- [x] **`anzahlSchritte` im Protokoll eines neuen Laufs zählte die Version, nicht den Lauf.** Bei
|
||
einer Vorlage mit abteilungsgebundenen Abschnitten (#031) behauptete der Eintrag mehr
|
||
Schritte, als der Lauf hat. Das Protokoll ist seit #023 ansehbar.
|
||
- [x] **Die Phase-2-Seiten fehlten in der 390-px-Prüfung.** Ergänzt: Bausteinliste, Schnellstart,
|
||
Protokoll, Zustellungen — gerade die Tabellen, also die Stellen, an denen horizontales
|
||
Scrollen entsteht. Jedes Issue berichtet eine Klickprobe von Hand; eine Probe von Hand ist
|
||
kein Regressionstest.
|
||
- [x] **Der Graph-Seitenabruf folgte `@odata.nextLink` ohne Host-Prüfung**, mit einem mandantenweit
|
||
gültigen Token im Kopf. Graph über TLS zu trauen ist vertretbar — aber `GRAPH_API_URL` ist
|
||
absichtlich einstellbar (nationale Clouds, Test-Doppelgänger), und ein Tippfehler darin
|
||
schickte das Token woandershin. `naechsteSeite` vergleicht jetzt den Ursprung.
|
||
|
||
## Nicht behoben, bewusst
|
||
|
||
- **`anhaengeDesLaufs` liegt in `services/` und wendet keine Mandantensicht an.** Beide Aufrufer
|
||
prüfen den Lauf vorher, und die Kommentare sagen das auch — aber es steht gegen die Regel aus
|
||
`CLAUDE.md` („`queries/*` wenden ihre Sicht selbst an, damit kein Aufrufer sie vergessen kann“).
|
||
Zwei Aufrufer in zwei Schichten gibt es schon; der dritte ist der, der es vergisst. Eigenes
|
||
Issue: [#037](037-anhaenge-lesemodell.md).
|
||
- **`/meine-geraete` fehlt weiter in der 390-px-Prüfung.** Die Seite verlangt `selbstauskunft`, und
|
||
das hat nur `MITARBEITER`; der E2E-Ablauf meldet sich als Admin an und würde die
|
||
Verweigerungsseite messen. Dafür braucht es ein eigenes Konto samt Personenbindung — siehe
|
||
[#037](037-anhaenge-lesemodell.md).
|
||
- **Das Graph-Token wird einmal je Abgleich geholt**, vor der Seitenschleife. Bei einem Mandanten,
|
||
dessen Abgleich länger als die Token-Lebensdauer läuft, hilft der Sicherheitsabstand nicht. Das
|
||
gehört auf die offene Prüfliste in `docs/betrieb/entra-id.md` und ist ohne echten Mandanten nicht
|
||
zu beurteilen.
|
||
|
||
## Was der Review *nicht* gefunden hat
|
||
|
||
Benannt, weil es den Befunden ihr Gewicht gibt: keine `kundeId` oder `quelleId` aus der Eingabe
|
||
statt aus Sitzung bzw. Token; kein Lesemodell, das seine Sicht ganz vergisst; keine
|
||
Client-Komponente mit einem Wert-Import aus `src/server/*`; kein Geheimnis in einer Sicht, Antwort
|
||
oder einem Protokolleintrag; kein gespeichertes `{{generiertes_passwort}}`; kein Codepfad, der
|
||
annimmt, dass nur ein Mensch einen Schritt erledigt.
|
||
|
||
Ein Befund des Reviews war **falsch**: der Zustands-Handler habe keinen Vitest-Test. Er hat acht
|
||
direkte Aufrufe in `tests/lauf-ereignisse.test.ts`, darunter „weist ohne Anmeldung ab“ und „liefert
|
||
für einen fremden Kunden keinen Strom“. Nachgeprüft, bevor etwas geändert wurde.
|