diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 194d14e..95b1626 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -1,6 +1,6 @@ # MCP Server Guide -mureo exposes 188 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 175 advertising and SEO operation tools across Google Ads (83), Meta Ads (82), and Search Console (10), 2 rollback tools, 1 cross-platform anomaly-detection tool, 7 mureo-context tools (strategy / state), 1 analytics-registry tool, and 2 learning tools (`mureo_learning_insights_get` for the operator's local `/learn` history and `mureo_consult_advisor` for federated retrieval against external advisor MCP servers — see [`docs/insight-federation.md`](insight-federation.md)). Any MCP-compatible client can connect and call these tools over stdio. Re-check this count when MCP tools are added or removed (`test_list_tools_returns_all_tools` pins the exact number). +mureo exposes 189 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 175 advertising and SEO operation tools across Google Ads (83), Meta Ads (82), and Search Console (10), 2 rollback tools, 1 cross-platform anomaly-detection tool, 8 mureo-context tools (strategy / state), 1 analytics-registry tool, and 2 learning tools (`mureo_learning_insights_get` for the operator's local `/learn` history and `mureo_consult_advisor` for federated retrieval against external advisor MCP servers — see [`docs/insight-federation.md`](insight-federation.md)). Any MCP-compatible client can connect and call these tools over stdio. Re-check this count when MCP tools are added or removed (`test_list_tools_returns_all_tools` pins the exact number). ## Starting the Server diff --git a/mureo/_data/skills/_mureo-meta-ads/SKILL.md b/mureo/_data/skills/_mureo-meta-ads/SKILL.md index e295941..3cc66bf 100644 --- a/mureo/_data/skills/_mureo-meta-ads/SKILL.md +++ b/mureo/_data/skills/_mureo-meta-ads/SKILL.md @@ -239,6 +239,31 @@ metadata: ``` Breakdown options: `age`, `gender`, `age,gender`, `country`, `region`, `publisher_platform`, `platform_position`, `device_platform`, `impression_device` +### conversion definition (custom / non-standard events) + +mureo counts conversions from the Insights `actions` array using a **default +deduped set of standard events** (`lead`, `purchase`, `complete_registration`). +If an advertiser's real conversion is a **custom pixel event** +(`offsite_conversion.custom.`, e.g. "Booking Completed"), or their account +only emits a component row (`offsite_conversion.fb_pixel_lead`) with no generic +aggregate, the default counts **0** for them — which silently wrong-foots CPA, +daily-check, and budget decisions. + +To fix, register the account's real conversion `action_type`(s): + +1. Look up the account's **actual** labels — call `meta_ads_insights_report` + (or `_breakdown`) and inspect the `actions` array's `action_type` values. + Don't guess the string; custom slugs like `offsite_conversion.custom.123` + are easy to mistype. +2. Confirm with the operator which label(s) are *their* conversion. +3. Call **`mureo_state_set_conversion_events`** with `platform="meta_ads"`, + the `account_id`, and `conversion_action_types=[…the exact string(s)…]`. + +This is a **replacement** (the listed types become the complete conversion set +for that account; never summed on top of the defaults). It is stored on +`platforms[meta_ads]` in STATE.json and applied by every Meta conversion +counter. Pass an empty list to clear it and restore the default. + ### analysis - `performance` -- Analyze overall performance trends. diff --git a/mureo/analytics/builtin/_budget_efficiency.py b/mureo/analytics/builtin/_budget_efficiency.py index 05687d2..be750c9 100644 --- a/mureo/analytics/builtin/_budget_efficiency.py +++ b/mureo/analytics/builtin/_budget_efficiency.py @@ -21,9 +21,13 @@ from __future__ import annotations -from typing import Any +from typing import TYPE_CHECKING, Any from mureo.analytics.models import BudgetEfficiency +from mureo.context.state import load_conversion_action_types + +if TYPE_CHECKING: + from collections.abc import Collection INEFFICIENT_THRESHOLD = 0.3 EFFICIENT_THRESHOLD = 0.7 @@ -34,6 +38,7 @@ def _extract_cost_and_conversions( *, spend_key: str, nested_metrics: bool, + conversion_action_types: Collection[str] | None = None, ) -> tuple[str, float, float] | None: """Return ``(campaign_id, cost, conversions)`` for one row, or ``None`` when the row is unusable (missing id / non-positive cost). @@ -69,7 +74,9 @@ def _extract_cost_and_conversions( count_conversions_from_actions, ) - conversions = count_conversions_from_actions(row.get("actions")) + conversions = count_conversions_from_actions( + row.get("actions"), conversion_action_types=conversion_action_types + ) if cost <= 0: return None @@ -99,9 +106,17 @@ def score_budget_efficiency( """ rates: dict[str, tuple[float, float]] = {} # campaign_id -> (rate, cost) total_unused = 0.0 + # #342 — the operator conversion override is Meta-specific; resolve once + # for the account (None for non-Meta platforms / unset accounts). + cv_types = ( + load_conversion_action_types(account_id) if platform == "meta_ads" else None + ) for row in rows: extracted = _extract_cost_and_conversions( - row, spend_key=spend_key, nested_metrics=nested_metrics + row, + spend_key=spend_key, + nested_metrics=nested_metrics, + conversion_action_types=cv_types, ) if extracted is None: continue diff --git a/mureo/analytics/builtin/_common.py b/mureo/analytics/builtin/_common.py index bf0258d..93082db 100644 --- a/mureo/analytics/builtin/_common.py +++ b/mureo/analytics/builtin/_common.py @@ -17,7 +17,7 @@ from __future__ import annotations -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Collection from typing import TYPE_CHECKING, Any, Protocol from mureo.analysis.anomaly_detector import ( @@ -70,7 +70,11 @@ def google_row_metrics(row: dict[str, Any]) -> dict[str, Any]: return row -def meta_row_conversions(row: dict[str, Any]) -> float: +def meta_row_conversions( + row: dict[str, Any], + *, + conversion_action_types: Collection[str] | None = None, +) -> float: """Return the conversion count for a Meta performance row. Live: conversions live inside an ``actions`` list keyed by @@ -79,6 +83,11 @@ def meta_row_conversions(row: dict[str, Any]) -> float: the #120 live-wiring validation — accepting only the live shape silently zeroes BYOD conversions. + ``conversion_action_types`` (#342) is the operator's per-account override + for which action_types count as conversions; ``None`` uses the built-in + deduped generic set. It applies only to the live ``actions`` shape (BYOD + rows carry a pre-aggregated total). + Expected runtime shape: :class:`MetaLivePerformanceRow` / :class:`MetaByodPerformanceRow`. Same caller-ergonomics rationale as :func:`google_row_metrics` for the looser parameter type. @@ -93,7 +102,9 @@ def meta_row_conversions(row: dict[str, Any]) -> float: # mureo.meta_ads client weight (this runs only on the live path). from mureo.meta_ads._conversion_count import count_conversions_from_actions - return count_conversions_from_actions(actions) + return count_conversions_from_actions( + actions, conversion_action_types=conversion_action_types + ) return float(row.get("conversions") or 0) diff --git a/mureo/analytics/builtin/_live_clients.py b/mureo/analytics/builtin/_live_clients.py index 2f6e54a..1390131 100644 --- a/mureo/analytics/builtin/_live_clients.py +++ b/mureo/analytics/builtin/_live_clients.py @@ -36,6 +36,7 @@ from mureo.analytics.builtin._common import ( meta_row_conversions as _meta_row_conversions, ) +from mureo.context.state import load_conversion_action_types if TYPE_CHECKING: from collections.abc import Callable @@ -294,19 +295,24 @@ async def fetch_google_ads_per_campaign_metrics( def _index_meta_rows_by_campaign( rows: list[dict[str, Any]], + account_id: str, ) -> dict[str, CampaignMetrics]: """Build ``{campaign_id: metrics}`` from Meta rows. Mirrors :func:`_index_google_rows_by_campaign` for Meta's flatter shape — Meta exposes ``spend`` and either an ``actions`` list - (Live) or a top-level ``conversions`` field (BYOD). + (Live) or a top-level ``conversions`` field (BYOD). ``account_id`` + resolves the operator's per-account conversion override (#342). """ + cv_types = load_conversion_action_types(account_id) indexed: dict[str, CampaignMetrics] = {} for row in rows: metric = _row_to_campaign_metrics( row, spend_key="spend", - conversion_getter=_meta_row_conversions, + conversion_getter=lambda r: _meta_row_conversions( + r, conversion_action_types=cv_types + ), ) if metric is None: continue @@ -363,8 +369,8 @@ async def fetch_meta_ads_per_campaign_metrics( period=baseline_period ) - current_index = _index_meta_rows_by_campaign(current_rows) - baseline_index = _index_meta_rows_by_campaign(baseline_rows) + current_index = _index_meta_rows_by_campaign(current_rows, account_id) + baseline_index = _index_meta_rows_by_campaign(baseline_rows, account_id) out: dict[str, tuple[CampaignMetrics, CampaignMetrics | None]] = {} for campaign_id, current in current_index.items(): @@ -402,11 +408,12 @@ def _aggregate_meta_metrics( impressions = 0 clicks = 0 conversions = 0.0 + cv_types = load_conversion_action_types(account_id) # #342 per-account override for row in rows: cost += float(row.get("spend") or 0) impressions += int(row.get("impressions") or 0) clicks += int(row.get("clicks") or 0) - conversions += _meta_row_conversions(row) + conversions += _meta_row_conversions(row, conversion_action_types=cv_types) return CampaignMetrics( campaign_id=account_id, cost=cost, diff --git a/mureo/analytics/builtin/meta_ads.py b/mureo/analytics/builtin/meta_ads.py index 5428d20..1d94205 100644 --- a/mureo/analytics/builtin/meta_ads.py +++ b/mureo/analytics/builtin/meta_ads.py @@ -32,6 +32,7 @@ meta_row_conversions, to_analytics_anomalies, ) +from mureo.context.state import load_conversion_action_types if TYPE_CHECKING: from mureo.analysis.anomaly_detector import CampaignMetrics @@ -295,13 +296,14 @@ def _summarise_meta_performance( impressions = 0 clicks = 0 conversions = 0.0 + cv_types = load_conversion_action_types(account_id) # #342 per-account override for row in rows: row_cost = float(row.get("spend") or 0) row_impressions = int(row.get("impressions") or 0) row_clicks = int(row.get("clicks") or 0) # Tolerates live (actions list) and BYOD (flat conversions); # both shapes are valid factory outputs. - row_conversions = meta_row_conversions(row) + row_conversions = meta_row_conversions(row, conversion_action_types=cv_types) cost += row_cost impressions += row_impressions clicks += row_clicks diff --git a/mureo/context/models.py b/mureo/context/models.py index 7274f34..945fe8b 100644 --- a/mureo/context/models.py +++ b/mureo/context/models.py @@ -122,6 +122,14 @@ class PlatformState: # legacy entries parse unchanged and emit no extra key. sync-state writes # LAST_30_DAYS; daily-check writes YESTERDAY. periods: dict[str, dict[str, Any]] | None = None + # Optional operator-declared conversion ``action_type`` allow-list (#342). + # When set (non-None), the Meta conversion counters treat EXACTLY these + # action_types as this account's conversions — overriding the default + # deduped generic set — so a custom-event advertiser + # (``offsite_conversion.custom.``) or a component-only account is + # counted correctly. None (the default) keeps the built-in generic set, so + # legacy entries parse unchanged and emit no extra key. + conversion_action_types: tuple[str, ...] | None = None def __post_init__(self) -> None: """Ensure campaigns is a tuple (defensive copy).""" @@ -131,6 +139,17 @@ def __post_init__(self) -> None: object.__setattr__(self, "totals", copy.deepcopy(self.totals)) if self.periods is not None: object.__setattr__(self, "periods", copy.deepcopy(self.periods)) + if self.conversion_action_types is not None and not isinstance( + self.conversion_action_types, tuple + ): + # A bare str must NOT be char-split into a tuple of letters; wrap + # it as a single action_type. Other iterables tuple-ify normally. + normalized = ( + (self.conversion_action_types,) + if isinstance(self.conversion_action_types, str) + else tuple(self.conversion_action_types) + ) + object.__setattr__(self, "conversion_action_types", normalized) @dataclass(frozen=True) diff --git a/mureo/context/state.py b/mureo/context/state.py index f761097..bd7a02a 100644 --- a/mureo/context/state.py +++ b/mureo/context/state.py @@ -108,6 +108,20 @@ def _platform_account_id( return "" +def _parse_conversion_action_types(raw: Any) -> tuple[str, ...] | None: + """Parse a platform's ``conversion_action_types`` override (#342). + + Returns a tuple of non-empty string action_types, or ``None`` when the + field is absent / not a list / has no usable entries (so the counters + fall back to the built-in generic set). Tolerant by design — a malformed + value degrades to "no override" rather than raising. + """ + if not isinstance(raw, list): + return None + cleaned = tuple(str(x).strip() for x in raw if isinstance(x, str) and x.strip()) + return cleaned or None + + def parse_state(text: str, *, strict: bool = True) -> StateDocument: """Parse a JSON string and return a StateDocument. @@ -139,6 +153,9 @@ def parse_state(text: str, *, strict: bool = True) -> StateDocument: totals=platform_data.get("totals"), metrics_period=platform_data.get("metrics_period"), periods=platform_data.get("periods"), + conversion_action_types=_parse_conversion_action_types( + platform_data.get("conversion_action_types") + ), ) # v2: action_log @@ -238,6 +255,10 @@ def _platform_state_to_dict(ps: PlatformState) -> dict[str, Any]: # entries with no per-period data) stay byte-stable on round-trip. if ps.periods: result["periods"] = copy.deepcopy(ps.periods) + # #342 — operator conversion override: emit only when set, as a JSON list, + # so legacy entries stay byte-stable. + if ps.conversion_action_types: + result["conversion_action_types"] = list(ps.conversion_action_types) return result @@ -429,6 +450,11 @@ def _build(doc: StateDocument) -> StateDocument: totals=existing.totals if existing is not None else None, metrics_period=existing.metrics_period if existing is not None else None, periods=existing.periods if existing is not None else None, + # #342 — the operator conversion override has no upsert input; + # inherit it so a campaign upsert never wipes the account setting. + conversion_action_types=( + existing.conversion_action_types if existing is not None else None + ), ) return StateDocument( @@ -584,8 +610,79 @@ def _build(doc: StateDocument) -> StateDocument: else (existing.metrics_period if existing is not None else None) ), periods=merged_periods, + # #342 — preserve the operator conversion override across a + # metrics write (it has no input here, same rationale as totals). + conversion_action_types=( + existing.conversion_action_types if existing is not None else None + ), + ) + + return StateDocument( + version=doc.version, + last_synced_at=_now_iso(), + customer_id=doc.customer_id, + campaigns=doc.campaigns, + platforms=platforms, + action_log=doc.action_log, + reports=doc.reports, + ) + + return _locked_state_mutation(path, _build) + + +def set_conversion_action_types( + path: Path, + platform: str, + account_id: str, + conversion_action_types: list[str] | None, +) -> StateDocument: + """Set a platform's operator conversion ``action_type`` override (#342). + + Declares EXACTLY which Meta ``action_type`` rows count as this account's + conversions — overriding the built-in deduped generic set + (``{lead, purchase, complete_registration}``) so a custom-event advertiser + (``offsite_conversion.custom.``) or a component-only account is counted + correctly. Pass ``None`` (or an empty list) to clear the override and + restore the default. + + Replacement semantics: the override is the *complete* conversion set for + the account — the counters use these and only these, never summed on top of + the generic set (so two overlapping alias rows can't double-count). + + The platform's campaigns / rollups and every OTHER platform are preserved; + the entry is created (carrying ``account_id``) when absent. Re-stamps + ``last_synced_at`` and writes back atomically under the state lock. + + Args: + path: STATE.json location. + platform: Platform key (e.g. ``"meta_ads"``). + account_id: The platform account id, always written onto the entry. + conversion_action_types: The exact action_types to count, or ``None`` / + ``[]`` to clear. + + Returns: + The updated :class:`StateDocument`. + """ + cleaned: tuple[str, ...] | None = None + if conversion_action_types: + cleaned = tuple( + str(x).strip() + for x in conversion_action_types + if isinstance(x, str) and x.strip() ) + cleaned = cleaned or None + def _build(doc: StateDocument) -> StateDocument: + platforms = dict(doc.platforms) if doc.platforms else {} + existing = platforms.get(platform) + platforms[platform] = PlatformState( + account_id=account_id, + campaigns=existing.campaigns if existing is not None else (), + totals=existing.totals if existing is not None else None, + metrics_period=existing.metrics_period if existing is not None else None, + periods=existing.periods if existing is not None else None, + conversion_action_types=cleaned, + ) return StateDocument( version=doc.version, last_synced_at=_now_iso(), @@ -599,6 +696,88 @@ def _build(doc: StateDocument) -> StateDocument: return _locked_state_mutation(path, _build) +def _workspace_state_path() -> Path: + """Resolve the ACTIVE workspace's STATE.json — the same file the MCP state + tools write to (#342). + + Mirrors ``_handlers_mureo_context._resolve_path``'s default resolution + (``store.state_path`` → ``store.workspace / STATE.json``) via the runtime + context, so the conversion override is read from the same file it is + written to — even under an agency / alternate ``StateStore`` where the + workspace diverges from the process cwd. ``get_runtime_context`` is + imported lazily to avoid an import cycle (``runtime_context`` builds on + ``context``). Any failure falls back to the cwd convention. + """ + from pathlib import Path as _Path + + try: + from mureo.core.runtime_context import get_runtime_context + + store = get_runtime_context().state_store + attr = getattr(store, "state_path", None) + if attr is not None: + return _Path(attr) + workspace = getattr(store, "workspace", None) + if workspace is not None: + return _Path(workspace) / "STATE.json" + except Exception: # noqa: BLE001 — best-effort; never break a live read. + pass + return _Path("STATE.json") + + +def _account_id_eq(a: str, b: str) -> bool: + """Compare Meta account ids tolerant of the optional ``act_`` prefix (#342). + + The MCP setter stores whatever id the operator/agent passed; the live + counters resolve with the ``act_*`` form the client enforces. Comparing + after stripping a leading ``act_`` keeps a bare-numeric override from + silently never matching. + """ + return a.removeprefix("act_") == b.removeprefix("act_") + + +def load_conversion_action_types( + account_id: str, + *, + path: Path | None = None, + platform: str = "meta_ads", +) -> tuple[str, ...] | None: + """Read an account's operator conversion override from STATE.json (#342). + + Returns the ``platforms[platform].conversion_action_types`` override when + it is set AND the entry's ``account_id`` matches ``account_id`` (tolerant + of the ``act_`` prefix); otherwise ``None`` so the conversion counters fall + back to the built-in generic set. + + Reads the ACTIVE workspace ``STATE.json`` (resolved via the runtime context + — the same file the MCP state tools write to). **Never raises**: a missing + / unreadable / malformed file, or an absent platform entry, all yield + ``None`` so a live analysis is never broken by a state-read failure. + """ + state_path = path if path is not None else _workspace_state_path() + try: + if not state_path.exists(): + return None + doc = parse_state(state_path.read_text(encoding="utf-8"), strict=False) + except Exception: # noqa: BLE001 — never break a live analysis on a bad file. + # parse_state can raise OSError / ContextFileError / ValueError + # (JSONDecodeError) AND AttributeError/TypeError on non-object JSON; + # a best-effort override read must swallow all of them. + return None + if doc.platforms is None: + return None + entry = doc.platforms.get(platform) + if entry is None or not entry.conversion_action_types: + return None + if ( + entry.account_id + and account_id + and not _account_id_eq(entry.account_id, account_id) + ): + return None + return entry.conversion_action_types + + def get_campaign(doc: StateDocument, campaign_id: str) -> CampaignSnapshot | None: """Search for a campaign by campaign_id.""" for c in doc.campaigns: diff --git a/mureo/mcp/_handlers_mureo_context.py b/mureo/mcp/_handlers_mureo_context.py index 950efd3..6dc9d77 100644 --- a/mureo/mcp/_handlers_mureo_context.py +++ b/mureo/mcp/_handlers_mureo_context.py @@ -36,6 +36,7 @@ append_action_log, read_state_file, render_state, + set_conversion_action_types, set_platform_metrics, set_report, upsert_campaign, @@ -282,3 +283,22 @@ async def handle_state_platform_metrics_set( # error rather than a 500-style server error (matches upsert_campaign). raise ValueError(str(exc)) from exc return _json_result(_state_to_dict(doc)) + + +async def handle_state_set_conversion_events( + arguments: dict[str, Any], +) -> list[TextContent]: + """Set/clear an account's operator conversion override (#342).""" + platform = _require(arguments, "platform") + account_id = _require(arguments, "account_id") + raw = arguments.get("conversion_action_types") + if raw is not None and not isinstance(raw, list): + raise ValueError("conversion_action_types must be a list of strings") + if isinstance(raw, list) and not all(isinstance(x, str) for x in raw): + raise ValueError("conversion_action_types entries must be strings") + path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + try: + doc = set_conversion_action_types(path, platform, account_id, raw) + except ContextFileError as exc: + raise ValueError(str(exc)) from exc + return _json_result(_state_to_dict(doc)) diff --git a/mureo/mcp/tools_mureo_context.py b/mureo/mcp/tools_mureo_context.py index 2407273..fef7da3 100644 --- a/mureo/mcp/tools_mureo_context.py +++ b/mureo/mcp/tools_mureo_context.py @@ -21,6 +21,7 @@ handle_state_get, handle_state_platform_metrics_set, handle_state_report_set, + handle_state_set_conversion_events, handle_state_upsert_campaign, handle_strategy_get, handle_strategy_set, @@ -310,6 +311,58 @@ "required": ["platform", "account_id"], }, ), + Tool( + name="mureo_state_set_conversion_events", + description=( + "Declare which Meta Insights ``action_type`` rows count as THIS " + "account's conversions, overriding mureo's built-in deduped generic " + "set (lead / purchase / complete_registration). Use this when an " + "advertiser's real conversion is a CUSTOM pixel event " + "(``offsite_conversion.custom.``) — otherwise it reports 0 " + "conversions — or when their account only emits a component row " + "(e.g. ``offsite_conversion.fb_pixel_lead``) with no generic " + "aggregate. Replacement semantics: the listed action_types are the " + "COMPLETE conversion set (never summed on top of the defaults), so " + "overlapping alias rows can't double-count. Tip: to avoid typos, " + "first call meta_ads_insights_report / _breakdown to see the " + "account's real action_type labels, confirm with the operator, then " + "set the exact string(s) here. Pass an empty list (or omit " + "``conversion_action_types``) to CLEAR the override and restore the " + "default. Stored on ``platforms[platform]`` and preserved across " + "syncs. Returns the updated state document." + ), + inputSchema={ + "type": "object", + "properties": { + "platform": { + "type": "string", + "description": ( + "Platform key — normally ``meta_ads`` (the override " + "only affects the Meta conversion counters)." + ), + }, + "account_id": { + "type": "string", + "description": ( + "The Meta ad account id (``act_*``). Always written " + "onto the platform entry." + ), + }, + "conversion_action_types": { + "type": "array", + "items": {"type": "string"}, + "description": ( + "Exact Meta ``action_type`` strings to count as " + "conversions (e.g. " + '["offsite_conversion.custom.123"]). Empty / omitted ' + "clears the override." + ), + }, + "path": _PATH_PROPERTY, + }, + "required": ["platform", "account_id"], + }, + ), ] @@ -321,6 +374,7 @@ "mureo_state_upsert_campaign": handle_state_upsert_campaign, "mureo_state_report_set": handle_state_report_set, "mureo_state_platform_metrics_set": handle_state_platform_metrics_set, + "mureo_state_set_conversion_events": handle_state_set_conversion_events, } diff --git a/mureo/meta_ads/_analysis.py b/mureo/meta_ads/_analysis.py index 650f1a6..97e5c47 100644 --- a/mureo/meta_ads/_analysis.py +++ b/mureo/meta_ads/_analysis.py @@ -6,11 +6,15 @@ from __future__ import annotations import logging -from typing import Any +from typing import TYPE_CHECKING, Any +from mureo.context.state import load_conversion_action_types from mureo.meta_ads._conversion_count import count_conversions_from_actions from mureo.meta_ads._period import previous_period as _previous_period +if TYPE_CHECKING: + from collections.abc import Collection + logger = logging.getLogger(__name__) @@ -21,15 +25,21 @@ def _safe_float(v: Any) -> float: return 0.0 -def _extract_cv(row: dict[str, Any]) -> float: +def _extract_cv( + row: dict[str, Any], + conversion_action_types: Collection[str] | None = None, +) -> float: """Conversion count for one insights row via the canonical counter (#340). Delegates to the shared :func:`count_conversions_from_actions` so this MCP-analysis path and the analytics/diagnose path (:func:`mureo.analytics.builtin._common.meta_row_conversions`) can never - drift on what counts as a conversion. + drift on what counts as a conversion. ``conversion_action_types`` (#342) + threads the operator's per-account override; ``None`` uses the default set. """ - return count_conversions_from_actions(row.get("actions")) + return count_conversions_from_actions( + row.get("actions"), conversion_action_types=conversion_action_types + ) class AnalysisMixin: @@ -39,6 +49,10 @@ class AnalysisMixin: get_performance_report / get_breakdown_report are provided by InsightsMixin. """ + # Provided by MetaAdsApiClient; used to resolve the per-account conversion + # override (#342). Declared here so mypy sees the attribute on the mixin. + _ad_account_id: str + async def get_performance_report( # type: ignore[empty-body] self, *, @@ -80,11 +94,12 @@ async def analyze_placements( } placements: list[dict[str, Any]] = [] + cv_types = load_conversion_action_types(self._ad_account_id) for row in data: spend = _safe_float(row.get("spend")) clicks = _safe_float(row.get("clicks")) impressions = _safe_float(row.get("impressions")) - cv = _extract_cv(row) + cv = _extract_cv(row, cv_types) cpa = round(spend / cv, 0) if cv > 0 else None placements.append( @@ -243,10 +258,11 @@ async def compare_ads( } ads: list[dict[str, Any]] = [] + cv_types = load_conversion_action_types(self._ad_account_id) for r in ads_data: ctr = _safe_float(r.get("ctr")) cpc = _safe_float(r.get("cpc")) - cv = _extract_cv(r) + cv = _extract_cv(r, cv_types) spend = _safe_float(r.get("spend")) # Score: CTR-weighted + CV bonus score = ctr * 10 + (cv * 5 if cv > 0 else 0) @@ -315,9 +331,10 @@ async def suggest_creative_improvements( } ads: list[dict[str, Any]] = [] + cv_types = load_conversion_action_types(self._ad_account_id) for r in data: ctr = _safe_float(r.get("ctr")) - cv = _extract_cv(r) + cv = _extract_cv(r, cv_types) spend = _safe_float(r.get("spend")) cpa = round(spend / cv, 0) if cv > 0 else None diff --git a/mureo/meta_ads/_conversion_count.py b/mureo/meta_ads/_conversion_count.py index b8a34ca..8760727 100644 --- a/mureo/meta_ads/_conversion_count.py +++ b/mureo/meta_ads/_conversion_count.py @@ -37,7 +37,7 @@ from __future__ import annotations -from collections.abc import Mapping +from collections.abc import Collection, Mapping from typing import Any # Deduped generic conversion ``action_type`` values. Each already rolls up @@ -58,19 +58,33 @@ def _safe_float(value: Any) -> float: return 0.0 -def count_conversions_from_actions(actions: Any) -> float: - """Sum the values of the canonical conversion ``action_type`` rows. +def count_conversions_from_actions( + actions: Any, + *, + conversion_action_types: Collection[str] | None = None, +) -> float: + """Sum the values of the conversion ``action_type`` rows. ``actions`` is the Insights ``actions`` array — a list of ``{"action_type": str, "value": str|number}`` mappings. A non-list (``None`` / BYOD shape / malformed) yields ``0.0`` so callers can pass ``row.get("actions")`` directly. Non-mapping entries are skipped. + + ``conversion_action_types`` (#342) is the operator's per-account override: + when provided (non-``None``), EXACTLY those action_types are counted — + replacing :data:`CONVERSION_ACTION_TYPES` — so a custom-event advertiser + (``offsite_conversion.custom.``) or a component-only account is counted + correctly. An empty collection is treated as "no override" (use the + default) so a cleared setting never zeroes every conversion. ``None`` (the + default) keeps the built-in deduped generic set. """ if not isinstance(actions, list): return 0.0 + allowed: Collection[str] = ( + conversion_action_types if conversion_action_types else CONVERSION_ACTION_TYPES + ) return sum( _safe_float(entry.get("value")) for entry in actions - if isinstance(entry, Mapping) - and entry.get("action_type") in CONVERSION_ACTION_TYPES + if isinstance(entry, Mapping) and entry.get("action_type") in allowed ) diff --git a/mureo/meta_ads/_insights.py b/mureo/meta_ads/_insights.py index 8439b70..0a50f88 100644 --- a/mureo/meta_ads/_insights.py +++ b/mureo/meta_ads/_insights.py @@ -4,6 +4,7 @@ import logging from typing import Any +from mureo.context.state import load_conversion_action_types from mureo.meta_ads._conversion_count import count_conversions_from_actions from mureo.meta_ads._period import previous_period, resolve_period @@ -218,15 +219,19 @@ async def analyze_audience( } segments: list[dict[str, Any]] = [] + cv_types = load_conversion_action_types(self._ad_account_id) for row in breakdown_data: spend = float(row.get("spend", 0) or 0) clicks = int(row.get("clicks", 0) or 0) impressions = int(row.get("impressions", 0) or 0) ctr = float(row.get("ctr", 0) or 0) - # Conversions via the canonical exact-match counter (#340) so - # this breakdown agrees with every other live path. - conversions = count_conversions_from_actions(row.get("actions")) + # Conversions via the canonical exact-match counter (#340) so this + # breakdown agrees with every other live path; #342 threads the + # operator's per-account conversion override. + conversions = count_conversions_from_actions( + row.get("actions"), conversion_action_types=cv_types + ) cpa = round(spend / conversions, 0) if conversions > 0 else None diff --git a/skills/_mureo-meta-ads/SKILL.md b/skills/_mureo-meta-ads/SKILL.md index e295941..3cc66bf 100644 --- a/skills/_mureo-meta-ads/SKILL.md +++ b/skills/_mureo-meta-ads/SKILL.md @@ -239,6 +239,31 @@ metadata: ``` Breakdown options: `age`, `gender`, `age,gender`, `country`, `region`, `publisher_platform`, `platform_position`, `device_platform`, `impression_device` +### conversion definition (custom / non-standard events) + +mureo counts conversions from the Insights `actions` array using a **default +deduped set of standard events** (`lead`, `purchase`, `complete_registration`). +If an advertiser's real conversion is a **custom pixel event** +(`offsite_conversion.custom.`, e.g. "Booking Completed"), or their account +only emits a component row (`offsite_conversion.fb_pixel_lead`) with no generic +aggregate, the default counts **0** for them — which silently wrong-foots CPA, +daily-check, and budget decisions. + +To fix, register the account's real conversion `action_type`(s): + +1. Look up the account's **actual** labels — call `meta_ads_insights_report` + (or `_breakdown`) and inspect the `actions` array's `action_type` values. + Don't guess the string; custom slugs like `offsite_conversion.custom.123` + are easy to mistype. +2. Confirm with the operator which label(s) are *their* conversion. +3. Call **`mureo_state_set_conversion_events`** with `platform="meta_ads"`, + the `account_id`, and `conversion_action_types=[…the exact string(s)…]`. + +This is a **replacement** (the listed types become the complete conversion set +for that account; never summed on top of the defaults). It is stored on +`platforms[meta_ads]` in STATE.json and applied by every Meta conversion +counter. Pass an empty list to clear it and restore the default. + ### analysis - `performance` -- Analyze overall performance trends. diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index bdd880e..57c463b 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -43,11 +43,11 @@ class TestListTools: async def test_list_tools_returns_all_tools(self) -> None: """list_tools returns all tools (Google Ads 83 + Meta Ads 82 + Search Console 10 - + Rollback 2 + Analysis 1 + Mureo Context 7 + Analytics Registry 1 - + Learning 2 = 188).""" + + Rollback 2 + Analysis 1 + Mureo Context 8 + Analytics Registry 1 + + Learning 2 = 189).""" mod = _import_server_module() tools = await mod.handle_list_tools() - assert len(tools) == 188 + assert len(tools) == 189 async def test_list_tools_contains_google_and_meta(self) -> None: """Google Ads and Meta Ads tools are included.""" diff --git a/tests/test_mcp_tools_mureo_context.py b/tests/test_mcp_tools_mureo_context.py index 4fe8ff4..d5c6f56 100644 --- a/tests/test_mcp_tools_mureo_context.py +++ b/tests/test_mcp_tools_mureo_context.py @@ -57,9 +57,9 @@ def _import_tools(): # --------------------------------------------------------------------------- -def test_tools_module_exports_seven_tools() -> None: +def test_tools_module_exports_eight_tools() -> None: mod = _import_tools() - assert len(mod.TOOLS) == 7 + assert len(mod.TOOLS) == 8 expected = { "mureo_strategy_get", "mureo_strategy_set", @@ -68,6 +68,7 @@ def test_tools_module_exports_seven_tools() -> None: "mureo_state_upsert_campaign", "mureo_state_report_set", "mureo_state_platform_metrics_set", + "mureo_state_set_conversion_events", } assert {t.name for t in mod.TOOLS} == expected @@ -673,3 +674,75 @@ async def test_platform_metrics_set_rejects_malformed_shapes(cwd_to_tmp) -> None await mod.handle_tool( "mureo_state_platform_metrics_set", {**base, "metrics_period": 30} ) + + +# --------------------------------------------------------------------------- +# mureo_state_set_conversion_events (#342) +# --------------------------------------------------------------------------- + + +async def test_set_conversion_events_sets_and_clears(cwd_to_tmp) -> None: + mod = _import_tools() + result = await mod.handle_tool( + "mureo_state_set_conversion_events", + { + "platform": "meta_ads", + "account_id": "act_9", + "conversion_action_types": ["offsite_conversion.custom.123"], + }, + ) + payload = json.loads(result[0].text) + assert payload["platforms"]["meta_ads"]["conversion_action_types"] == [ + "offsite_conversion.custom.123" + ] + # Clearing with an empty list removes the override. + result = await mod.handle_tool( + "mureo_state_set_conversion_events", + {"platform": "meta_ads", "account_id": "act_9", "conversion_action_types": []}, + ) + payload = json.loads(result[0].text) + assert "conversion_action_types" not in payload["platforms"]["meta_ads"] + + +async def test_set_conversion_events_requires_platform_and_account(cwd_to_tmp) -> None: + mod = _import_tools() + with pytest.raises(ValueError, match="platform"): + await mod.handle_tool( + "mureo_state_set_conversion_events", {"account_id": "act_9"} + ) + with pytest.raises(ValueError, match="account_id"): + await mod.handle_tool( + "mureo_state_set_conversion_events", {"platform": "meta_ads"} + ) + + +async def test_set_conversion_events_rejects_non_list(cwd_to_tmp) -> None: + mod = _import_tools() + with pytest.raises(ValueError, match="must be a list"): + await mod.handle_tool( + "mureo_state_set_conversion_events", + { + "platform": "meta_ads", + "account_id": "act_9", + "conversion_action_types": "lead", + }, + ) + + +async def test_set_conversion_events_read_write_path_agree(cwd_to_tmp) -> None: + """#342 HIGH — the override written via the MCP tool (workspace-resolved) + must be readable by the live counters' resolver with NO explicit path.""" + from mureo.context.state import load_conversion_action_types + + mod = _import_tools() + await mod.handle_tool( + "mureo_state_set_conversion_events", + { + "platform": "meta_ads", + "account_id": "act_42", + "conversion_action_types": ["offsite_conversion.custom.7"], + }, + ) + assert load_conversion_action_types("act_42") == ("offsite_conversion.custom.7",) + # act_ prefix tolerance: a bare-id live resolve still matches. + assert load_conversion_action_types("42") == ("offsite_conversion.custom.7",) diff --git a/tests/test_meta_ads_operations.py b/tests/test_meta_ads_operations.py index 0e5df79..9856000 100644 --- a/tests/test_meta_ads_operations.py +++ b/tests/test_meta_ads_operations.py @@ -931,6 +931,7 @@ class TestAnalysisMixin: def client(self): class MockAnalysisClient(AnalysisMixin): def __init__(self): + self._ad_account_id = "act_123" self.get_performance_report = AsyncMock(return_value=[]) self.get_breakdown_report = AsyncMock(return_value=[]) diff --git a/tests/test_meta_conversion_count.py b/tests/test_meta_conversion_count.py index 4165eda..b9dc077 100644 --- a/tests/test_meta_conversion_count.py +++ b/tests/test_meta_conversion_count.py @@ -48,7 +48,10 @@ def test_ignores_custom_conversion_slug_containing_lead() -> None: slug happens to contain 'lead'/'purchase' must not be counted.""" actions = [ {"action_type": "offsite_conversion.custom.my_lead_magnet", "value": "99"}, - {"action_type": "offsite_conversion.custom.black_friday_purchase", "value": "50"}, + { + "action_type": "offsite_conversion.custom.black_friday_purchase", + "value": "50", + }, {"action_type": "lead", "value": "2"}, ] assert count_conversions_from_actions(actions) == 2.0 @@ -106,3 +109,44 @@ def test_matches_extract_cv_byte_for_byte() -> None: ] for row in rows: assert _extract_cv(row) == count_conversions_from_actions(row.get("actions")) + + +@pytest.mark.unit +def test_override_replaces_default_set() -> None: + """#342 — when an override is given, EXACTLY those action_types count.""" + actions = [ + {"action_type": "lead", "value": "5"}, + {"action_type": "offsite_conversion.custom.123", "value": "9"}, + ] + # Default: only the generic 'lead'. + assert count_conversions_from_actions(actions) == 5.0 + # Override: only the custom event. + assert ( + count_conversions_from_actions( + actions, conversion_action_types={"offsite_conversion.custom.123"} + ) + == 9.0 + ) + + +@pytest.mark.unit +def test_empty_override_falls_back_to_default() -> None: + """An empty/cleared override must NOT zero every conversion — it means + 'use the default set'.""" + actions = [{"action_type": "purchase", "value": "4"}] + assert count_conversions_from_actions(actions, conversion_action_types=[]) == 4.0 + assert count_conversions_from_actions(actions, conversion_action_types=None) == 4.0 + + +@pytest.mark.unit +def test_override_counts_component_only_account() -> None: + """A component-only account (no generic aggregate) is counted once the + operator declares the component as their conversion (#342).""" + actions = [{"action_type": "offsite_conversion.fb_pixel_lead", "value": "7"}] + assert count_conversions_from_actions(actions) == 0.0 # default misses it + assert ( + count_conversions_from_actions( + actions, conversion_action_types=("offsite_conversion.fb_pixel_lead",) + ) + == 7.0 + ) diff --git a/tests/test_state.py b/tests/test_state.py index cfcbe5c..f8c51fb 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -1705,3 +1705,144 @@ def test_append_action_log_preserves_reports(self, tmp_path: Path) -> None: ) assert doc.reports == {"weekly": {"verdict": "Watch"}} assert read_state_file(fp).reports == {"weekly": {"verdict": "Watch"}} + + +@pytest.mark.unit +class TestConversionActionTypesOverride: + """#342 — operator-declared per-account conversion action_type override.""" + + def _state_with_meta(self, tmp_path: Path, account_id: str = "act_1") -> Path: + p = tmp_path / "STATE.json" + p.write_text( + json.dumps( + { + "version": "2", + "platforms": { + "meta_ads": {"account_id": account_id, "campaigns": []} + }, + } + ), + encoding="utf-8", + ) + return p + + def test_set_and_load_roundtrip(self, tmp_path: Path) -> None: + from mureo.context.state import ( + load_conversion_action_types, + set_conversion_action_types, + ) + + p = self._state_with_meta(tmp_path) + set_conversion_action_types( + p, "meta_ads", "act_1", ["offsite_conversion.custom.123"] + ) + assert load_conversion_action_types("act_1", path=p) == ( + "offsite_conversion.custom.123", + ) + # Serialized as a JSON list under the platform entry. + data = json.loads(p.read_text(encoding="utf-8")) + assert data["platforms"]["meta_ads"]["conversion_action_types"] == [ + "offsite_conversion.custom.123" + ] + + def test_clear_with_empty_list(self, tmp_path: Path) -> None: + from mureo.context.state import ( + load_conversion_action_types, + set_conversion_action_types, + ) + + p = self._state_with_meta(tmp_path) + set_conversion_action_types(p, "meta_ads", "act_1", ["lead"]) + set_conversion_action_types(p, "meta_ads", "act_1", []) + assert load_conversion_action_types("act_1", path=p) is None + data = json.loads(p.read_text(encoding="utf-8")) + assert "conversion_action_types" not in data["platforms"]["meta_ads"] + + def test_load_account_mismatch_returns_none(self, tmp_path: Path) -> None: + from mureo.context.state import ( + load_conversion_action_types, + set_conversion_action_types, + ) + + p = self._state_with_meta(tmp_path, account_id="act_1") + set_conversion_action_types(p, "meta_ads", "act_1", ["lead"]) + assert load_conversion_action_types("act_OTHER", path=p) is None + + def test_load_missing_file_or_unset_is_none(self, tmp_path: Path) -> None: + from mureo.context.state import load_conversion_action_types + + assert ( + load_conversion_action_types("act_1", path=tmp_path / "absent.json") is None + ) + p = self._state_with_meta(tmp_path) + assert load_conversion_action_types("act_1", path=p) is None # unset + + def test_preserved_across_upsert_and_metrics(self, tmp_path: Path) -> None: + from mureo.context.models import CampaignSnapshot + from mureo.context.state import ( + load_conversion_action_types, + set_conversion_action_types, + set_platform_metrics, + upsert_campaign, + ) + + p = self._state_with_meta(tmp_path) + set_conversion_action_types(p, "meta_ads", "act_1", ["lead"]) + upsert_campaign( + p, + CampaignSnapshot(campaign_id="c1", campaign_name="C1", status="ACTIVE"), + platform="meta_ads", + account_id="act_1", + ) + assert load_conversion_action_types("act_1", path=p) == ("lead",) + set_platform_metrics( + p, "meta_ads", "act_1", totals={"spend": 1.0}, metrics_period="LAST_30_DAYS" + ) + assert load_conversion_action_types("act_1", path=p) == ("lead",) + + def test_parse_render_roundtrip(self, tmp_path: Path) -> None: + from mureo.context.state import parse_state, render_state + + doc = parse_state( + json.dumps( + { + "version": "2", + "platforms": { + "meta_ads": { + "account_id": "act_1", + "campaigns": [], + "conversion_action_types": ["lead", "purchase"], + } + }, + } + ) + ) + ps = doc.platforms["meta_ads"] + assert ps.conversion_action_types == ("lead", "purchase") + # Round-trips through render. + re = parse_state(render_state(doc)) + assert re.platforms["meta_ads"].conversion_action_types == ("lead", "purchase") + + def test_load_never_raises_on_malformed_json(self, tmp_path: Path) -> None: + """#342 HIGH — the resolver runs inside live analysis; a non-object / + malformed STATE.json must yield None, never raise (which would take + down the whole report).""" + from mureo.context.state import load_conversion_action_types + + for bad in ("[1,2,3]", '"a string"', "123", "null", "{not json", ""): + p = tmp_path / "STATE.json" + p.write_text(bad, encoding="utf-8") + assert load_conversion_action_types("act_1", path=p) is None + + def test_load_tolerates_act_prefix_mismatch(self, tmp_path: Path) -> None: + """#342 — a bare-numeric stored id matches the act_* live id and vice + versa, so the override isn't silently dropped over the prefix.""" + from mureo.context.state import ( + load_conversion_action_types, + set_conversion_action_types, + ) + + p = self._state_with_meta(tmp_path, account_id="123456") # bare + set_conversion_action_types(p, "meta_ads", "123456", ["lead"]) + # Live counters resolve with the act_* form. + assert load_conversion_action_types("act_123456", path=p) == ("lead",)