feat(conversions): offline conversion uploads + #43 fold (split 3/4 of #34) - #52
illia-sapryga wants to merge 4 commits into
Conversation
Adds two write tools to the conversion_actions module and wires them into
the safety flow:
* draft_upload_call_conversions — UploadCallConversions. caller_id (E.164)
is required raw by Google for call-to-click matching and cannot be
hashed; it is stored in the plan (apply needs it) but REDACTED in the
preview and audit log.
* draft_upload_enhanced_conversions_for_leads — UploadClickConversions
with user_identifiers. Email/phone/name are normalized then SHA-256
hashed AT PREVIEW TIME; only hashes land in the plan and the audit log
never sees raw PII.
Security + correctness properties:
- Apply builds upload protos from plan.changes["rows"] (frozen at preview
time) — the CSV is never re-read at apply. What you previewed is what
uploads, and no raw PII is re-read.
- success_count counts only result rows with a populated conversion_action
(Google echoes caller_id / user_identifiers back on FAILED rows, so the
old identifier-based heuristic always reported ~100%). Applied to BOTH
call and EC-for-leads paths.
- order_id dedup key: optional "Order ID" CSV column -> ClickConversion
.order_id; dedup_warnings[] surface missing/partial coverage.
- consent param -> ClickConversion/CallConversion.consent (ad_user_data +
ad_personalization mapped to ConsentStatus; default UNSPECIFIED). EEA
GDPR requirement.
- _normalize_phone_e164: strip exactly one domestic trunk 0, keep Italy's
+39 leading 0, map leading "00" -> "+".
- _gaql_escape: backslash escaping (GAQL, not SQL doubled-quotes) for
conversion-action name lookups; handles O'Brien-style names.
Audit path: confirm_and_apply now routes plan.changes through
_redact_changes_for_audit before every log_mutation call (dry-run, error,
success) and in the dry-run response payload.
Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
Covers the two upload tools end to end with fake PII only (user@example.com,
+15555550142, Test User, ORD-001), asserting on the pinned SHA-256 hex of
those known fakes:
- PII is normalized THEN hashed; parser leaks no raw-PII keys.
- _normalize_phone_e164 trunk-zero / Italy / "00" cases.
- _gaql_escape backslash escaping, used by both name resolvers.
- Call preview redacts caller_id in sample rows but keeps it raw in the
frozen plan rows; no csv_path persisted.
- EC plan.changes contains only hashes — no raw email/phone/name.
- Apply builds protos from plan rows (no CSV), sets order_id + consent.
- success_count keys off conversion_action (not echoed identifiers) for
both call and EC paths (0% / partial / full match).
- _redact_changes_for_audit redacts caller_id and drops the EC hash blob
without mutating the original changes dict.
- Both tools registered; upload ops wired into _execute_plan dispatch.
Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
bbd7f07 to
19b24c9
Compare
|
Reviewed at last — sorry for the delay, and this is the most carefully built of the four. Verified against the diff:
Holding this one briefly, for a reason on my side rather than yours. AdLoop Cloud deploys by cloning So merging this obliges a DPA update and a 30-day notice to customers before it can ship hosted. That is entirely our problem, caused by our deployment shape, not by anything in your code. It happens that we owe customers a terms notice anyway, so the fix is to fold the data-category change into that same notice rather than send a second one. Concretely: #51 is merged, #52 stays open for about a week while that goes out, then it merges unchanged. Nothing for you to do, and nothing here needs rewriting. Worth saying plainly: the fact that this is the PR with a legal consequence attached is a compliment to it. You built the hashing, the redaction and the consent handling carefully enough that the remaining question is about where our database sits, not about whether the data is handled properly. |
# Conflicts: # src/adloop/ads/write.py # src/adloop/server.py
Matches main's convention of a human-readable title on every tool.
Split 3/4 of #34 — conversion uploads (folds in #43)
Third of the split. Stacks on #51 (it extends the same
conversion_actions.pymodule), so please merge after #51. Registers only its own two tools.Tools
draft_upload_call_conversions→UploadCallConversionsdraft_upload_enhanced_conversions_for_leads→UploadClickConversionswithuser_identifiersYour six requirements — each implemented and verified
strip().lower(), phone → E.164) then SHA-256-hashes each field. The raw values are locals used only to compute the hash — the row that enters the plan holds only*_sha256keys.plan.changes/ audit log. EC rows are hash-only. Call rows must keep the raw E.164caller_id(Google requires it unhashed for call matching), so it lives in the plan for apply but is redacted at every audit boundary:_redact_changes_for_auditis applied at all threelog_mutationcall sites (dry-run/error/success) and in the dry-run response — callcaller_id→+155***0142, and the EC hash blob is dropped from the audit trail entirely (only non-PII counters remain)._apply_*start fromchanges["rows"]; the CSV path is never stored in the plan.consent={"ad_user_data","ad_personalization"}validated and mapped toConsentStatusEnumonClickConversion.consent/CallConversion.consent; absent → UNSPECIFIED._normalize_phone_e164fixed — strips exactly one domestic trunk zero (not all), keeps Italy's leading zero (via the+-prefixed verbatim path), and maps00…→+…._gaql_escapeapplied in both_resolve_conversion_action_idsand_resolve_upload_clicks_action(soO'Brien Leaddoesn't break the query). This is the fix you flagged in your feat(ads): asset extension tools + RSA update + conversion-action mgmt #34 notes; it lands here where the only by-name lookup lives.#43 folded in (per your instruction)
ClickConversion.order_id, backwards-compatible).conversion_actionpopulated — neutralizes thecaller_id/user_identifiersecho-back that made the base always report ~100%. Applied to both call and EC paths.rows_with_order_idandsample_rows[].order_id.Things I want your call on (flagging honestly)
caller_idas a per-row failure. For EC (hashed), a bare national number with no country code would hash to a value Google can't match — silently. The normalizer is conservative (only adds+when it can infer one) and documented; recommend the EC CSV require international/E.164 phones. Happy to add a per-row warning/skip if you'd prefer._normalize_phone_e164and_gaql_escapenow exist in bothwrite.py(from split (a)) andconversion_actions.py(here). They have different signatures — worth consolidating to one shared helper when the splits land.caller_id. Audit redaction covers the log, but the persistent plan store (hostedset_plan_store) would hold raw phone between draft and apply. If that store persists to disk/DB, it's a raw-PII-at-rest surface the audit redaction doesn't cover — flagging for the hosted deployment.Tests & hygiene
user@example.com,+15555550142,Test User.Next: (d) RSA update, folding #42 with the corrected learning-reset warning.
https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s