Repository navigation
Expand AML observability PoC structure - #15
Conversation
endvater
left a comment
There was a problem hiding this comment.
Code Review: AML Observability PoC — Erweiterung
Starker PR. Das Refactoring löst die strukturellen Schwächen aus PR #14 und alle Review-Punkte von damals wurden adressiert. Die Schichtentrennung ist jetzt deutlich klarer, die Detection-Logik ist in ein eigenes Objekt gewandert und die Privacy/Retention-Schicht ist eine sinnvolle Ergänzung.
✅ Positives
DetectionDecisionals eigenständiges Objekt —alert_createdist jetzt sauber als Property ancombined_score >= alert_thresholdgebunden, kein loser Entscheidungscode mehr in der Pipeline.combined_score = max(model_score, rule_override_score)— semantisch sauberer als der gewichtete Blend aus PR #14: die Regel setzt einen Mindest-Score (Floor), keine additive Verzerrung._require_eventHelper inqueries.py— dernext()-Bug aus PR #14 ist korrekt und konsistent gelöst; eine gemeinsame Hilfsfunktion für alle drei Query-Typen.privacy.py— datenschutzrechtlich sinnvolle Ergänzung; Non-Alert-Traces werden minimiert, Alert-Traces vollständig behalten. Das ist ein klares, nachvollziehbares Retention-Prinzip.auto_case_feedback-Parameter — nützlich für Demos ohne manuelle Disposition-Objekte.- UAE aus
ILLUSTRATIVE_HIGH_RISK_JURISDICTIONSentfernt — gut. test_explain_why_flagged_raises_clear_error_for_incomplete_trace— direkte Regression-Abdeckung des Bugs aus dem letzten Review.
⚠️ selective_trace_retention — Test prüft keine echte Filterung
tests/test_observability_trace.py:47-60
Der Eingabe-Trace hat genau 3 Events (FEATURES_DERIVED, MODEL_SCORED, NO_ALERT), die alle im Retained-Set liegen. Der Test beweist nicht, dass Events außerhalb des Sets (RECEIVED, INGESTED, RULE_EVALUATED) wirklich herausgefiltert werden. Ein Trace mit 5–6 Events und Assertion auf len == 3 wäre aussagekräftiger.
⚠️ Implementierungskonstanten im Public API
observability/__init__.py:7-9
DEFAULT_EVALUATED_RULES und ILLUSTRATIVE_HIGH_RISK_JURISDICTIONS sind in __all__ exportiert. Das sind interne PoC-Konstanten, keine stabilen API-Versprechen. Wenn externe Nutzer darauf aufbauen, erschwert das spätere Anpassungen. Empfehlung: aus __all__ entfernen oder mit einem _-Präfix als intern kennzeichnen.
⚠️ Demo-Skript hängt von nicht-eingecheckten Fixtures ab
observability/scripts/run_demo.py:43
Der Default-Pfad observability/data/synthetic_transactions.jsonl existiert nicht im Repository. run_demo.py ohne vorheriges Ausführen von generate_synthetic_data.py gibt einen unklaren FileNotFoundError. Eine klare Fehlermeldung oder ein Hinweis im README wäre hilfreich.
💡 Minimierter Trace gibt keinen Hinweis auf Retention-Modus
observability/privacy.py:54-60
Das von selective_trace_retention zurückgegebene AMLTrace-Objekt ist äußerlich identisch mit einem vollständigen Trace. Wer das Ergebnis weiterverarbeitet, kann nicht erkennen, dass Events fehlen. Zumindest die RetentionDecision sollte gemeinsam zurückgegeben oder geloggt werden.
💡 walkthrough.ipynb ist ein leerer Platzhalter
74 Zeilen ohne inhaltliche Zellen. Entweder mit einem minimalen Walkthrough füllen oder vorerst weglassen, um keine falschen Erwartungen zu wecken.
Zusammenfassung
| Kategorie | Bewertung |
|---|---|
| Architektur & Refactoring | ✅ Deutliche Verbesserung gegenüber PR #14 |
| PR #14-Review-Punkte adressiert | ✅ Alle |
| Testabdeckung (11 Tests, 3 Dateien) | ✅ Gut |
| Retention-Test-Tiefe | |
| Public-API-Scope der Konstanten | |
| Demo-Skript-Robustheit | |
| Minimierungs-Transparenz | 💡 Verbesserungswürdig |
| Notebook-Inhalt | 💡 Platzhalter |
Der PR ist in gutem Zustand. Die
Generated by Claude Code
|
Addressed the review points in 30aa20a:
One clarification: the synthetic JSONL fixtures are already checked into observability/data/, so the default demo path is available in the repository; the new error path mainly improves the experience when someone passes a missing custom path. Verification is still clean:
|
endvater
left a comment
There was a problem hiding this comment.
Follow-up Review: Commit 30aa20a — "Tighten observability demo and retention flow"
Alle vier
✅ Was gefixt wurde
| Review-Punkt | Umsetzung |
|---|---|
| Retention-Test zu schwach | Trace jetzt mit 6 Events (RECEIVED, INGESTED, RULE_EVALUATED + 3 retained); explizite Assertion, dass gefilterte Typen nicht im Ergebnis sind |
| Konstanten im Public API | ALERT_THRESHOLD, DEFAULT_EVALUATED_RULES, ILLUSTRATIVE_HIGH_RISK_JURISDICTIONS aus __init__.py und __all__ entfernt |
| Demo-Skript ohne Fehlermeldung | read_jsonl wirft jetzt FileNotFoundError mit Hinweis auf generate_synthetic_data |
| Minimierung unsichtbar | Neue apply_trace_retention-Funktion gibt (RetentionDecision, AMLTrace) zurück; Demo zeigt Event-Count-Delta wenn minimiert |
| Notebook leer | Notebook hat jetzt 5 inhaltliche Zellen mit vollständigem Walkthrough |
⚠️ Eine neue Inkonsistenz: README sagt "eingecheckte Fixtures", aber observability/data/ fehlt im Repo
observability/README.md und scripts/run_demo.py
Der Commit enthält diesen README-Text:
"Die Demo nutzt standardmäßig die eingecheckten Dateien in
observability/data/."
Und auch das Notebook geht davon aus, dass die Fixtures vorhanden sind. Aber observability/data/ ist nicht im Repository — git ls-tree findet keine JSONL-Dateien. Der verbesserte FileNotFoundError federt das ab, ist aber ein Widerspruch zur Dokumentation.
Entweder die generierten Fixtures eincheckken (generate_synthetic_data.py einmal ausführen und die JSONL-Dateien committen), oder die README auf die Zwei-Schritt-Anleitung zurücksetzen.
💡 Minor: test_read_jsonl_raises_helpful_error_for_missing_fixture nutzt try/except statt pytest.raises
# aktuell
try:
read_jsonl(missing_path)
except FileNotFoundError as exc:
message = str(exc)
else:
raise AssertionError(...)Idiomatischer wäre:
with pytest.raises(FileNotFoundError, match="generate_synthetic_data"):
read_jsonl(missing_path)Kein Bug, aber konsistenter mit dem Rest der Testsuite (die pytest.raises bereits verwendet).
Fazit: Der Kern des Review ist erledigt. Einziger echter Nachfolger: die fehlenden Fixture-Dateien im Repository, damit README und Notebook nicht ins Leere zeigen.
Generated by Claude Code
|
Added a paper-facing extension in 1472bcc:
Verification remains clean:
|
Temporary checkout of observability PoC expansion (PR #15) onto review branch to validate test results — 21/23 pass, 2 failures identified in test_aml_observability_poc.py due to UAE removal from high-risk list. https://claude.ai/code/session_01XFb5pbqZoteaMX1tDrYAR4
|
Addressed in 734f3f1. The README mismatch came from .gitignore: the JSONL fixtures existed locally but were not tracked. I updated .gitignore to allow observability/data/*.jsonl and added both files to the PR, so observability/data/synthetic_transactions.jsonl and observability/data/synthetic_cases.jsonl are now part of #15. |
Follow-up Review: Commit
|
Staged during test run review of PR #15 commit 734f3f1. https://claude.ai/code/session_01XFb5pbqZoteaMX1tDrYAR4
|
I checked the current PR head against both the branch and origin/main: this specific UAE-to-IRN issue is already resolved in the code under review. The old file tests/test_aml_observability_poc.py is no longer part of the PR head, and the current test coverage in tests/test_observability_pipeline.py plus tests/test_observability_queries.py already uses IRN. There are also no remaining UAE references in the tracked test or observability code, and the current CI checks on #15 are green. So this looks like a stale review observation rather than an open merge blocker on the latest commit. |
ruthdelalucha
left a comment
There was a problem hiding this comment.
Strukturelle Verbesserung des Observability-PoC. Die Aufteilung in Detection/CaseMgmt/Privacy-Module ist sauber, die Demo-Skripte und JSONL-Fixtures machen den PoC ausführbar statt nur theoretisch. Tests sind passend aufgeteilt, READMEs aktualisiert. Walkthrough-Notebook ist ein schönes Extra für Reviewer. Passt zum v1.1 Paper-Release. LGTM 🐸
Temporary checkout of observability PoC expansion (PR #15) onto review branch to validate test results — 21/23 pass, 2 failures identified in test_aml_observability_poc.py due to UAE removal from high-risk list. https://claude.ai/code/session_01XFb5pbqZoteaMX1tDrYAR4
Staged during test run review of PR #15 commit ab6e4cf. https://claude.ai/code/session_01XFb5pbqZoteaMX1tDrYAR4
* Add AML observability proof of concept * Harden AML observability PoC queries * Expand AML observability PoC structure * Add Mermaid diagrams to observability README * Improve Mermaid rendering in observability README * Tighten observability demo and retention flow * Add technical spec and missing-data walkthrough * Track observability fixture data --------- Co-authored-by: Jürgen <atlassian@schillergarcia.de>
Summary
Testing