Skip to content

fix(meta_ads): no silent fallback to last_7d for unknown period values (#134)#135

Merged
hyoshi merged 1 commit into
mainfrom
fix/meta-ads-period-silent-fallback
May 22, 2026
Merged

fix(meta_ads): no silent fallback to last_7d for unknown period values (#134)#135
hyoshi merged 1 commit into
mainfrom
fix/meta-ads-period-silent-fallback

Conversation

@hyoshi
Copy link
Copy Markdown
Collaborator

@hyoshi hyoshi commented May 22, 2026

Closes #134.

Problem

The Meta Ads MCP tools' period argument advertised last_14d, last_90d, and explicit YYYY-MM-DD..YYYY-MM-DD ranges, but the implementation accepted only six hard-coded preset names and silently returned last_7d data for anything else. Period-over-period analyses (meta_ads_analysis_performance, meta_ads_analysis_cost) doubled the bug: the "previous" window was likewise mapped via a tiny dict that, for last_7d, returned last_30d — a superset that overlaps the current window, making every delta meaningless.

Severity: silent data corruption. Operators acting on the returned numbers (budget reallocations, pause/enable decisions, A/B winner picks) were acting on the wrong window with no exception, log line, or warning string in the response.

Fix

Tool / function Before After
meta_ads_insights_report / meta_ads_insights_breakdown Unknown period → silent last_7d Unknown periodValueError surfaced at MCP boundary. YYYY-MM-DD..YYYY-MM-DD → forwarded as time_range. last_14d / last_90d → forwarded as date_preset.
meta_ads_analysis_cost (investigate_cost) prev_map.get(period, "last_30d") → previous overlapped current previous_period(period) → same-length window immediately before current
meta_ads_analysis_performance (analyze_performance) 4-key prev_period_map with same superset bug Same previous_period(period) helper
Tool description _PERIOD_PARAM Listed last_14d / last_90d / custom range without this_month / last_month Lists every preset the resolver accepts

Public behaviour

  • period accepts today, yesterday, last_7d, last_14d, last_30d, last_90d, this_month, last_month, or YYYY-MM-DD..YYYY-MM-DD (both endpoints inclusive).
  • Anything else raises ValueError (no silent fallback).
  • The previous-period for any current period is the same-length window immediately before it.

Implementation

  • New mureo/meta_ads/_period.pyResolvedPeriod dataclass + resolve_period() parser + previous_period() builder. The dataclass owns .to_api_params() so the date_preset | time_range narrowing lives next to the invariant.
  • _insights.pyget_performance_report, get_breakdown_report, and analyze_performance funnel through the resolver. _PERIOD_TO_DATE_PRESET deleted.
  • _analysis.pyinvestigate_cost uses previous_period(period).
  • _tools_meta_ads_insights.py_PERIOD_PARAM description aligned with the resolver's accepted surface.

Test plan

  • Targeted unit tests pass: 107 / 107 across tests/test_meta_ads_period.py (38 new) + tests/test_meta_ads_operations.py (+4 new in TestInsightsMixin / TestAnalysisMixin).
  • Coverage includes: every preset round-trip, custom-range round-trip, single-day range, twelve bad-input rejection cases (trailing/leading .., slash-style, since > until, whitespace, non-string, etc.), .days inclusive arithmetic, all previous_period branches, the no-overlap invariant.
  • Regression: meta_ads test suite as a whole shows 407 passed on this branch vs 364 on main (+43 expected — entirely from new tests). 67 pre-existing failures in test_meta_ads_leads.py / test_meta_ads_media.py are unchanged and unrelated (handler-mock setup issues; same count and same names on main). Zero new regressions.
  • Lint: ruff check and black --check clean on every file this PR touches.
  • mureo/mcp/_helpers.py api_error_handler re-raises ValueError unchanged, so the new error surfaces to the operator with the full message (preset list included).
  • CI green (3.10 / 3.11 / 3.12 / Windows, Analyze, CodeQL).

Backwards compatibility

  • Callers relying on the silent last_7d fallback now see a ValueError. This is the bug being fixed — they were always getting the wrong window.
  • BYOD (mureo/byod/clients.py) uses its own _period_to_range (LAST_30_DAYS shape) — unaffected.
  • Google Ads has its own comparison logic — unaffected.
  • Every previously-working preset (today / yesterday / last_7d / last_30d / this_month / last_month) keeps working, plus the previously-advertised-but-broken last_14d, last_90d, and custom range shapes.

…ues (#134)

The Meta Ads MCP tools' `period` argument advertised `last_14d`,
`last_90d`, and explicit `YYYY-MM-DD..YYYY-MM-DD` ranges, but the
implementation accepted only six hard-coded preset names. Any other
value silently became a `last_7d` Meta call. Period-over-period
analyses (`meta_ads_analysis_performance`,
`meta_ads_analysis_cost`) doubled the bug: the "previous" window
was likewise mapped via a tiny dict that returned `last_30d` for
`last_7d` — a superset that overlaps the current window, making
every delta meaningless.

This commit wires the full advertised surface through to the Meta
Graph API and routes the previous-window through correct arithmetic
so the same `period` shape on the way in is honoured on the way
back out.

Public behaviour:

* `period` accepts the documented preset surface (`today`,
  `yesterday`, `last_7d`, `last_14d`, `last_30d`, `last_90d`,
  `this_month`, `last_month`) plus `YYYY-MM-DD..YYYY-MM-DD` with
  both endpoints inclusive.
* A `YYYY-MM-DD..YYYY-MM-DD` value is now forwarded to Meta as
  `time_range`; a preset name is forwarded as `date_preset`.
* An unknown value raises `ValueError` (which the MCP error
  handler surfaces to the operator). No silent fallback. The
  operator hears about the typo instead of receiving last_7d data
  labelled with their period.
* The previous-period for any current `period` is the same-length
  window immediately before it. `this_month` round-trips to the
  matching preset `last_month`; `last_month` resolves to an
  explicit calendar range so callers don't redo the arithmetic.

Implementation:

* New `mureo/meta_ads/_period.py`: `ResolvedPeriod` dataclass +
  `resolve_period()` parser + `previous_period()` builder. The
  dataclass owns `.to_api_params()` so the type-narrowing for
  `date_preset | time_range` lives next to the invariant
  (`resolve_period` builds them, nothing else does).
* `_insights.py`: `get_performance_report`, `get_breakdown_report`,
  and `analyze_performance` all funnel through the resolver.
  `analyze_performance` previously used a 4-key dict
  (`last_7d` -> `last_30d`, etc.) that produced superset
  "previous" windows; the helper replaces it.
* `_analysis.py`: `investigate_cost` uses `previous_period(period)`
  so its current/previous block is correct for every accepted
  shape including custom date ranges.
* `_tools_meta_ads_insights.py`: `_PERIOD_PARAM` description now
  lists `this_month` / `last_month` (already accepted by the
  resolver) and clarifies that custom ranges are inclusive at
  both endpoints.

Tests (107 pass):

* `tests/test_meta_ads_period.py` (38 new): every preset name,
  custom-range round-trip, single-day range, twelve bad-input
  rejection cases (including trailing/leading `..`, slash-style
  dates, since > until, whitespace padding, non-string), inclusive
  day length, all eight branches of `previous_period` (custom
  range, today, yesterday, last_Nd, this_month, last_month, bad
  input), and a no-overlap invariant for `previous_period`.
* `tests/test_meta_ads_operations.py` (+4 new in `TestInsightsMixin`
  and `TestAnalysisMixin`): `get_performance_report` forwards
  custom ranges as `time_range` rather than coercing to
  `date_preset`; `last_14d` / `last_90d` round-trip into
  `date_preset`; unknown periods raise `ValueError`; the previous
  window for `investigate_cost("camp1", period="last_7d")` is no
  longer the `last_30d` superset.

Cross-cutting:

* The MCP error handler at `mureo/mcp/_helpers.py` re-raises
  `ValueError` unchanged, so unknown-period errors surface at the
  MCP boundary with their full message (preset list included).
* BYOD's `_period_to_range` and Google Ads' comparison logic both
  use their own period parsers and are not affected.
* `_period.py` is import-clean (stdlib only); ruff / black / 38
  unit tests pass in isolation.

Backwards compatibility:

* Callers relying on the silent `last_7d` fallback for an unknown
  string will now see a `ValueError` instead of incorrect numbers
  — this is the bug being fixed, not a regression. Operators were
  always getting the wrong window; now they hear about it.
* Every documented `period` value continues to work, plus the
  previously-advertised-but-broken `last_14d`, `last_90d`, and
  `YYYY-MM-DD..YYYY-MM-DD` shapes.
@hyoshi hyoshi force-pushed the fix/meta-ads-period-silent-fallback branch from 406d40c to 47bb257 Compare May 22, 2026 07:15
@hyoshi hyoshi merged commit c012802 into main May 22, 2026
9 checks passed
@hyoshi hyoshi deleted the fix/meta-ads-period-silent-fallback branch May 22, 2026 07:20
@hyoshi hyoshi mentioned this pull request May 22, 2026
3 tasks
hyoshi added a commit that referenced this pull request May 22, 2026
…#136)

See CHANGELOG.md for details.

The Meta Ads MCP tools' `period` argument silently degraded
unknown values to `last_7d` and produced overlapping
period-over-period windows (last_7d vs last_30d) — silent data
corruption for any operator who passed `last_14d`, `last_90d`, or
`YYYY-MM-DD..YYYY-MM-DD`. PR #135 wires the full advertised
surface through to the Meta Graph API and raises `ValueError` for
unknown values so the failure surfaces to the operator instead of
returning wrong numbers labelled with their period.

Version bumped to 0.9.8 across .claude-plugin/plugin.json,
mureo/__init__.py, and pyproject.toml.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant