Orchestrator/bahn/O2C-Harness/code-review.md

Code-Review (Self-Review mit GitLab Discussions)

WICHTIG: Code-Review erst durchführen wenn die MR-Pipeline GRÜN ist. Reihenfolge: Push → MR erstellen → Pipeline abwarten → Pipeline grün → Code-Review → Findings fixen → Merge. Nicht reviewen solange Pipeline noch läuft oder failed ist – erst Pipeline fixen!

Ablauf nach MR-Erstellung (ERST wenn MR-Pipeline grün!)

1. Code lokal reviewen (nicht nur Diff, sondern gesamten Code)
2. Findings als GitLab Discussions anlegen
3. Findings selbst fixen
4. Threads resolven
5. Erst mergen wenn alle Threads resolved

Review-Kriterien

Clean Code

• Single Responsibility Principle
• Sprechende Namen (Variablen, Methoden, Klassen)
• Kleine Methoden (max 20 Zeilen)
• Kein toter Code, keine auskommentierten Blöcke
• DRY (Don't Repeat Yourself)

Separation of Concerns

• Controller nur für HTTP-Handling (kein Business-Logic)
• Service-Layer für Business-Logik
• Repository nur für Datenzugriff
• DTOs für API-Kommunikation (nicht Entity direkt exponieren)

Best Practices

• Input-Validierung (@Valid, Bean Validation)
• Fehlerbehandlung (Exception Handler, sinnvolle HTTP Status Codes)
• Logging an kritischen Stellen
• Keine Secrets/Credentials im Code
• Immutable wo möglich

Tests

• Alle öffentlichen Methoden getestet
• Edge Cases abgedeckt (null, leer, ungültig)
• Given-When-Then Struktur
• Mocks nur wo nötig (keine Over-Mocking)

Security

• Keine SQL Injection (Parameterized Queries / JPA)
• Keine XSS (Output Encoding)
• Keine Secrets im Code
• Dependencies ohne bekannte CVEs

GitLab Discussion anlegen

# Discussion an einer bestimmten Datei/Zeile
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
  -f "body=**Clean Code:** Diese Methode hat zu viele Verantwortlichkeiten. Bitte in separate Methoden aufteilen." \
  -f "position[base_sha]=$(git merge-base main HEAD)" \
  -f "position[start_sha]=$(git merge-base main HEAD)" \
  -f "position[head_sha]=$(git rev-parse HEAD)" \
  -f "position[position_type]=text" \
  -f "position[new_path]=src/main/java/com/example/controller/AddressController.java" \
  -f "position[new_line]=42"

# Allgemeine Discussion (nicht an Datei gebunden)
glab api -X POST "projects/:id/merge_requests/{mr_iid}/discussions" \
  -f "body=**Architektur:** Service-Layer fehlt. Controller greift direkt auf Repository zu. Bitte AddressService einführen."

Findings fixen und Thread resolven

# Fix implementieren
# ... Code ändern ...
git add -A && git commit -m "fix(review): Service-Layer einführen (#issue)"
git push

# Thread resolven
glab api -X PUT "projects/:id/merge_requests/{mr_iid}/discussions/{discussion_id}" \
  -f "resolved=true"

Merge-Bedingung

# Prüfen ob alle Threads resolved sind
UNRESOLVED=$(glab api "projects/:id/merge_requests/{mr_iid}/discussions" | python3 -c "
import sys, json
discussions = json.load(sys.stdin)
unresolved = [d for d in discussions if any(n.get('resolvable') and not n.get('resolved') for n in d.get('notes',[]))]
print(len(unresolved))
")

if [ "$UNRESOLVED" = "0" ]; then
  echo "Alle Threads resolved - merge möglich"
  glab mr merge --when-pipeline-succeeds
else
  echo "Noch $UNRESOLVED offene Threads - erst fixen"
fi

Wichtig

• Review den GESAMTEN Code, nicht nur den Diff
• Mindestens 3 Review-Punkte prüfen (auch wenn alles gut aussieht → "LGTM" als Discussion)
• Findings mit Kategorie-Prefix: Clean Code:, Security:, Architektur:, Test:
• Jedes Finding muss actionable sein (konkreter Verbesserungsvorschlag)
• Erst mergen wenn 0 unresolved Threads