Add AML observability proof of concept - #14
Conversation
ruthdelalucha
left a comment
There was a problem hiding this comment.
Solide Verknüpfung von Theorie (aixiv-Paper) und Praxis. Die fünf Layer werden durch den PoC greifbar – besonders die Compliance-Queries (why_flagged, why_not_flagged, what_changed) machen Observability debuggable. Tests + README-Updates komplett. LGTM 🐸
endvater
left a comment
There was a problem hiding this comment.
Code Review: AML Observability PoC
Gute Arbeit — der Layer ist von einem Platzhalter zu einem echten, testbaren Blueprint geworden. Die Schichtentrennung ist sauber, die frozen Dataclasses sind die richtige Wahl für unveränderliche Trace-Events, und die vier Tests decken die wichtigsten Pfade ab. Ein paar Punkte, die vor einer Paper-Referenzierung noch adressiert werden sollten:
🐛 Bug: next() ohne Fallback in explain_why_flagged — kann StopIteration werfen
observability/queries.py:28-29
rule_event = next(event for event in detection_events if event.event_type == TraceEventType.RULE_EVALUATED)
model_event = next(event for event in detection_events if event.event_type == TraceEventType.MODEL_SCORED)Wird ein Trace außerhalb von run_transaction gebaut (z.B. in Tests oder beim Einlesen aus JSONL), wirft das einen unkontrollierten StopIteration. Besser mit next(..., None) + Guard oder einem sprechenden ValueError wie bei der Alert-Prüfung darüber.
⚠️ combined_score wird berechnet, steuert aber alert_created nicht
observability/pipeline.py:87-88
combined_score = round((0.65 if rule_triggered else 0.0) + (0.35 * model_score), 2)
alert_created = rule_triggered or model_score >= 0.80combined_score landet in den decision_artifacts, treibt die Alert-Entscheidung aber nicht an — die basiert allein auf model_score >= 0.80. Entweder combined_score als Schwellwert nutzen oder aus den Artifacts entfernen, sonst ist es irreführend.
⚠️ explain_what_changed gibt immer denselben statischen Antworttext zurück
observability/queries.py:93-96
Der answer-String ist identisch, egal ob sich Features, Score oder Alert-Status tatsächlich geändert haben. Das gibt bei einer leeren feature_changes-Map keinen Mehrwert. Der Text sollte die tatsächlichen Änderungen reflektieren.
💡 UAE in HIGH_RISK_JURISDICTIONS ist faktisch veraltet
observability/pipeline.py:11
UAE wurde im Februar 2024 von der FATF Grey List gestrichen. Im Kontext eines AML-Papers sollte ein Kommentar klarstellen, dass diese Liste rein illustrativ ist und nicht als regulatorische Referenz dient.
💡 Span-Kette ist linear, nicht topologisch korrekt
observability/pipeline.py:44-45
last_span_id bildet eine lineare Kette, bei der jeder Event den vorherigen als Parent referenziert. Bei einem Fünf-Layer-Modell wäre realistischer, dass z.B. mehrere Detection-Events denselben Transformation-Span als Parent haben. Für den PoC ist das in Ordnung — ein kurzer Kommentar zur bewussten Vereinfachung würde helfen.
💡 sys.path-Manipulation in conftest.py ist fragil
tests/conftest.py:10-11
Funktioniert, ist aber abhängig vom Arbeitsverzeichnis. Eine pyproject.toml mit editable install (pip install -e .) wäre robuster und würde diesen Hack überflüssig machen.
Zusammenfassung:
| Kategorie | Bewertung |
|---|---|
| Design & Schichtentrennung | ✅ Sauber |
| Testabdeckung | ✅ Solide |
Bug (next() ohne Fallback) |
❌ Bitte fixen |
combined_score-Inkonsistenz |
|
Statischer what_changed-Text |
|
| UAE / Jurisdiktionsliste | 💡 Kommentar ergänzen |
Der PoC ist auf einem guten Weg. Der next()-Bug und die combined_score-Inkonsistenz sollten vor einer Weiterverwendung als Referenzimplementierung behoben werden.
Generated by Claude Code
|
Addressed the review points in 49e9f80:
I left the conftest.py import-path workaround in place for now since that feels more like a packaging/setup follow-up than a blocker for this PoC PR. |
|
Alle Punkte aus dem Review wurden adressiert — fixes in Branch Was geändert wurde:
✅ Alle 4 Tests bestehen — bereit zum Merge. Generated by Claude Code |
Summary
Testing