Laufzettel/docs/issues/036-reviewbefunde-phase-2.md

#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

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

• 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.
• /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.
• 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.
• 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“.
• 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.
• 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

• 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.
• 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.
• 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.
• 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.
• 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.
• /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.
• 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.