Skip to content

feat(conversion-action): CRUD tools (split 2/4 of #34) - #51

Merged
kLOsk merged 1 commit into
kLOsk:mainfrom
illia-sapryga:feat/conversion-action-crud
Sep 8, 2026
Merged

feat(conversion-action): CRUD tools (split 2/4 of #34)#51
kLOsk merged 1 commit into
kLOsk:mainfrom
illia-sapryga:feat/conversion-action-crud

Conversation

@illia-sapryga

Copy link
Copy Markdown
Contributor

Split 2/4 of #34 — conversion-action CRUD

Second of the four-way split, rebuilt fresh on main, registering only its own three tools (no uploads — those are split (c) — no assets/RSA).

Tools

  • draft_create_conversion_action
  • draft_update_conversion_action
  • draft_remove_conversion_action

Wrappers resolve config via current_config(); impls take config first; the three ops are wired into the _execute_plan dispatch with lazy imports (matching main's style). New src/adloop/ads/conversion_actions.py holds only the CRUD functions — split (c) will add the upload functions to it.

Your fix: default_value>0 no longer silently flips the flag

A positive default_value paired with always_use_default_value=False used to silently force the flag to True — turning a legal "tag value with fallback" config into "always override." It now emits a preview warning and leaves the flag exactly as the caller set it:

"default_value is set but always_use_default_value is False: Google will treat default_value as a FALLBACK, used only when the tag/import provides no value. If you want default_value to override every conversion's value, set always_use_default_value=True explicitly."

Applied to both create and update. Tests assert the warning fires and the flag is not force-set; default_value=0 / explicit True produce no warning.

Preserved (your "reviewed clean" items)

  • include_in_conversions_metric is immutable on create — the create op leaves the proto field unset (regression test asserts not HasField(...)); a test confirms it IS mutable on update.
  • view_through_lookback_window is settable only for AD_CALL.
  • Auto-managed types (SMART_CAMPAIGN_*, GOOGLE_HOSTED) are warned.

On your GAQL-escaping note

The apostrophe-doubling bug is in the by-name conversion-action lookup, which only exists in the upload resolvers — so the backslash-escaping fix lands in split (c) alongside that code, rather than as a dead helper here. Flagging so you know it isn't dropped.

Tests & hygiene

  • 450 passing, warning-free.
  • PII scrubbed — generic names (Example Co), no real client data.

Next: (c) conversion uploads (folds #43 + the GAQL fix), then (d) RSA update (folds #42).

https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s

Split (b) of kLOsk#34, rebuilt fresh on main — create/update/remove conversion
actions only (uploads are split c). Registers only its own three tools.

- current_config() resolution; the three ops wired into the _execute_plan
  dispatch, matching main's lazy-import style.
- Preserves the immutable/invalid-field handling the maintainer flagged as
  correct: include_in_conversions_metric is immutable on create; the
  view-through window is AD_CALL-only; auto-managed types are warned.
- Fix: a positive default_value with always_use_default_value=False no longer
  silently flips the flag to True. It now emits a preview warning and leaves
  the flag exactly as the caller set it (fallback vs override is the caller's
  decision, not a silent override).

The GAQL apostrophe-escaping fix lands in split (c), where the only by-name
conversion-action lookup (the upload resolvers) actually lives.

Claude-Session: https://claude.ai/code/session_01FKcPiYrtXWtg4b7dnW4o5s
@illia-sapryga
illia-sapryga force-pushed the feat/conversion-action-crud branch from 010441e to b636a4b Compare July 29, 2026 05:52
@kLOsk

kLOsk commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Reviewed properly at last — apologies for the seven weeks of silence. You built exactly what the split asked for and then heard nothing, which is not the signal I meant to send.

I checked the claims against the diff rather than taking the description's word for it:

  • Zero stale _config references; current_config() on every wrapper.
  • The three ops are wired into _execute_plan with lazy imports, matching main's style.
  • 36 new tests, CI green.
  • All three tools carry toolset tags=.

The default_value > 0 preview warning is the right call. Silently flipping always_use_default_value turned a legal "tag value with fallback" config into "always override", and a warning that says so beats both silence and a hard refusal.

Approving and merging. One thing I resolved on our side rather than bouncing back to you: main gained title= on every @mcp.tool on 6 September, after you opened this, so your three registrations conflicted. That's our change landing on top of your work, so it's ours to fix — I added the titles during the merge.

#52 next, and I've left a note there about a Cloud-side consideration that is mine to sort out, not a problem with your code.

@kLOsk
kLOsk merged commit 1c3a31a into kLOsk:main Sep 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants