diff --git a/CHANGELOG.md b/CHANGELOG.md index 10d425d6..a55e9739 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,3 @@ -# Changelog - -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - ## [Unreleased] ### Fixed @@ -28,6 +21,53 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 their app secret — and writes to other `meta_ads` fields leave the stamp alone, since it still describes the token on disk. +- **A platform whose credentials failed still shipped a `/daily-check` + report that looked complete.** When a token had expired or no + credential was configured, the run went through every step and + delivered a report with error prose — `"API error: Meta API request + failed (status=400, ...)"` or `"Credentials not found. Set environment + variable ..."` — sitting where the numbers belonged, next to real + figures from the platforms that did answer, followed by the usual + recommendations. Nothing marked the report partial, so a reader + skimming it concluded the platform had been quiet rather than + unreadable. + + The cause was that an auth failure arrived as an ordinary *successful* + tool result whose text happened to be a sentence about credentials, so + nothing downstream could tell "no spend" from "could not read". Auth + failure is now a first-class, machine-readable outcome on **every** + platform: the result carries `{"status": "auth_error", "auth_cause": + "no_credentials" | "token_invalid", "detail": ...}`, reusing the + `status`-field convention `blind_spots` and the delivery-collapse + report already use rather than adding a second one. `detail` keeps the + operator-facing sentence, so nothing readable is lost. The two causes + are separated because their recovery differs — configure the + credential vs. re-authorize a rejected one. + + It is produced in the two places every platform routes through, so + Google Ads, Meta Ads and Search Console behave identically: + `_no_creds_result` for a missing credential, and `@api_error_handler` + for a rejected one (an HTTP 401/403, a Google Ads + `authentication_error` / `authorization_error`, or Meta's + `OAuthException` — Meta answers an expired token with HTTP 400, so the + Meta client now names that case explicitly instead of letting it + flatten into a generic API error). Classification is deliberately + narrow: an unrecognized failure stays an ordinary `API error:`, since + mislabelling a quota or validation error would send an operator to + re-authorize a healthy account. + + `/daily-check` gained the branch it was missing. On an `auth_error` it + now marks the report **partial** in its opening line, names each + affected platform with its cause and recovery, withholds every + verdict, goal-progress line and recommendation that depends on the + missing platform's data, and still completes for every platform that + did answer. + + The shared `is_error_result` detector recognises the new envelope too, + so a mutation refused for a missing or rejected credential is still + kept out of `action_log` — it would otherwise have been recorded as a + change that never happened, complete with an `observation_due`. + ### Added - **A Meta token expiry notice and a re-authenticate control on the Meta diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 88f5bcd3..afd3fb6b 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -1026,26 +1026,36 @@ The `text` field contains a JSON string that your agent should parse. ### Authentication Errors -If credentials are missing, tools return a descriptive error message (not an exception): +An auth failure is returned as a result, not raised as an exception — but it is a **structured, machine-readable outcome**, not prose. Every platform uses the same envelope: ```json [ { "type": "text", - "text": "Authentication credentials not found. Set environment variables or ~/.mureo/credentials.json." + "text": "{\"status\": \"auth_error\", \"auth_cause\": \"no_credentials\", \"detail\": \"Credentials not found. Set environment variable (META_ADS_ACCESS_TOKEN) or configure ~/.mureo/credentials.json.\"}" } ] ``` +| Field | Meaning | +|-------|---------| +| `status` | Always `auth_error`. This is the marker to branch on: mureo could not read this platform at all, so it produced **no data** for this call. | +| `auth_cause` | `no_credentials` — nothing is configured for this platform. `token_invalid` — a credential exists and the platform rejected it (expired or revoked token, withdrawn permission). The two have different recovery actions. | +| `detail` | The operator-facing sentence: which environment variable to set, or what the platform said. | + +Both causes are produced centrally, so every platform behaves identically: `no_credentials` from the shared `_no_creds_result` helper, `token_invalid` from `@api_error_handler` when the underlying exception is an auth failure (`PlatformAuthError`, an HTTP 401/403, or a Google Ads `authentication_error` / `authorization_error`). The vocabulary lives in `mureo/core/auth_failure.py`. + +**An agent must never render an `auth_error` result as data.** A platform that could not be read is not a platform that was quiet, and a report containing one is partial — see the `/daily-check` skill's partial-report rule. mureo also treats the envelope as a failed call internally: a mutation that returns it is never written to `action_log`. + ### API Errors -API errors (rate limits, invalid parameters, etc.) are caught by the `@api_error_handler` decorator and returned as text: +API errors (rate limits, invalid parameters, etc.) are caught by the `@api_error_handler` decorator and returned as text, prefixed with `API error:`: ```json [ { "type": "text", - "text": "API Error: Meta API request failed (status=400, path=/act_123/campaigns)" + "text": "API error: Meta API request failed (status=400, path=/act_123/campaigns)" } ] ``` diff --git a/mureo/_data/skills/_mureo-shared/SKILL.md b/mureo/_data/skills/_mureo-shared/SKILL.md index 56d735f5..51fb363e 100644 --- a/mureo/_data/skills/_mureo-shared/SKILL.md +++ b/mureo/_data/skills/_mureo-shared/SKILL.md @@ -482,12 +482,20 @@ All tools return structured JSON via `TextContent`. The format depends on the to ### Authentication Error +Every platform returns the SAME envelope when mureo could not authenticate. It is a successful tool call, so you must branch on the payload: + ```json { - "error": "No credentials found. Set environment variables (GOOGLE_ADS_DEVELOPER_TOKEN, ...) or ~/.mureo/credentials.json" + "status": "auth_error", + "auth_cause": "no_credentials", + "detail": "Credentials not found. Set environment variables (GOOGLE_ADS_DEVELOPER_TOKEN, ...) or ~/.mureo/credentials.json." } ``` +`auth_cause` is `no_credentials` (nothing is configured — the operator runs `mureo configure` / `mureo auth setup`) or `token_invalid` (a credential exists and the platform rejected it, typically an expired token — the operator re-authorizes). `detail` is the operator-facing sentence. + +**This is a hole in your data, not a data point.** mureo read *nothing* from that platform for this call, so never print `detail` where a number belongs and never treat that platform's missing or empty figures as evidence that it was quiet. Say which platform failed and why, mark any report containing one as **partial**, and withhold conclusions that depend on the numbers you did not get. + ## STATE.json Schema (when writing on Code via `Write`) > **Tool output ≠ STATE.json.** The *Success Response* above is what a vendor diff --git a/mureo/_data/skills/daily-check/SKILL.md b/mureo/_data/skills/daily-check/SKILL.md index d34c8319..8df6a45d 100644 --- a/mureo/_data/skills/daily-check/SKILL.md +++ b/mureo/_data/skills/daily-check/SKILL.md @@ -55,6 +55,7 @@ Step 2b **imports** what mureo can fetch, so much of that work is now in `action - mureo BYOD mode applies only to mureo native tools — do **not** look for raw CSVs in the project directory; mureo BYOD data is centralized in the workspace `byod/` directory (or `~/.mureo/byod/` for legacy CLI users) and is only accessible through mureo MCP tools. 4. **Platform health checks**: Run health diagnostics on each configured ad platform. + - **Auth failure is not data — it is a hole in the report.** Any tool result carrying `"status": "auth_error"` means mureo could not read that platform in steps 3–4 at all: `auth_cause` is `no_credentials` (nothing is configured) or `token_invalid` (a credential exists and the platform rejected it — typically an expired token), and `detail` is the operator-facing sentence. **Never render `detail` where a metric belongs, and never read that platform's missing or empty numbers as "quiet"** — unreadable and quiet are different facts. Record the platform name with its `auth_cause` and carry both into step 10 exactly as step 2b's `blind_spots` are carried, then keep going for every other platform: one platform's credentials failing degrades the report, it never aborts the run. - **Analytics-first (both modes' first move).** Consult `mureo_analytics_modules_list` for **every** platform (not just external ones) — it maps each platform to its advertised capabilities (`detect_anomalies`, `diagnose_performance`, `audit_creative`, `analyze_budget_efficiency`, `detect_delivery_collapse`). Match its `platform` field against the platform keys you already hold: it is the **canonical platform key** (`plugin::` for a plugin, and the older `plugin:` still resolves), the same key STATE.json and `action_log` use, while `registry_name` / `source_distribution` are identifiers, not keys — never look a platform up by them (see `../_mureo-shared/SKILL.md` → *Canonical platform key*). Then per platform: - **Module registered AND advertises the capability you need** → run it via `mureo_analytics_run` (`DETECT_ANOMALIES` / `DIAGNOSE_PERFORMANCE`; pass `platform`, `capability`, `account_id`; add `window_days` for `detect_anomalies` or `scope` for `diagnose_performance`) and fold its **compact findings** (`result`) into the health summary. In **incremental** mode this REPLACES the raw pulls — do **not** ALSO fetch raw campaign / insights rows to re-derive what the module already summarized. A `status` other than `ok` (`no_analytics_module` / `capability_not_available` / `error`) means treat that capability as unavailable and fall through to the raw checks below — never fail the whole daily-check because one platform's module errored. - **No analytics module, or the module lacks the capability** → report `analytics_not_available_for_` for the **module-specific** deep diagnostics (RSA audit, `result_indicator`, budget-efficiency) — `` being the canonical platform key, so the notice names the same platform as the rest of the report — and do NOT fabricate heuristics from the integration's tool schemas (Issue #120). **BUT still run the generic, platform-agnostic `analysis_anomalies_check` per campaign** (metrics normalized to `campaign_id`/`cost`/`clicks`/`impressions`/`conversions`/`cpa`/`ctr`): it builds a median baseline from `action_log` and flags zero-spend / CPA-spike / CTR-drop for TikTok, plugins and official-MCP platforms too — a principled detector, not an auto-derived heuristic (see `_mureo-shared` → *analytics-module parity* → generic anomaly check). @@ -106,6 +107,8 @@ Step 2b **imports** what mureo can fetch, so much of that work is now in `action 10. **Report** — the mode decides the shape: + **Partial report (both modes) — this goes FIRST, above the verdicts.** If any platform came back `"status": "auth_error"` in steps 3–4, this report is **partial**: open it by saying so, naming each affected platform with its `auth_cause` and the recovery (`no_credentials` → configure the credential via `mureo configure` / `mureo auth setup`; `token_invalid` → re-authorize, the credential is present and dead). Then **withhold every verdict, goal-progress line and recommendation that depends on that platform's numbers** — an unreadable platform has no performance to judge, and cross-platform advice (budget shifts, "scale here, cut there") derived only from the platforms that did answer is not the advice you would give with all of them. Report the platforms that did answer as usual, and make clear which is which. + **Deep mode — full report.** Summarize findings as: **Healthy** (no action needed), **Watch** (minor issues to monitor), **Action needed** (requires immediate attention). For each issue, suggest specific actions aligned with the current Operation Mode. Do NOT recommend actions based on single-day fluctuations — at least 7 consecutive days of critical metrics (>30% off target) before suggesting rescue. **Incremental mode — diff-first.** Compare this run against the previous `reports.daily` summary and report ONLY the deltas: diff --git a/mureo/core/auth_failure.py b/mureo/core/auth_failure.py new file mode 100644 index 00000000..94c0e411 --- /dev/null +++ b/mureo/core/auth_failure.py @@ -0,0 +1,165 @@ +"""Auth failure as a first-class outcome, on every platform (#580). + +A platform whose credentials are missing, or whose token has expired, used +to answer a read with an ordinary *successful* MCP result whose text +happened to be a sentence about credentials — ``"Credentials not found. Set +environment variable ..."`` or ``"API error: Meta API request failed +(status=400, ...)"``. Nothing downstream could tell that apart from any +other error, let alone from real data, so a report skill folded the prose in +next to real figures and shipped a report that looked complete. "Could not +read" and "nothing to read" must never look alike. + +This module holds the one vocabulary that makes them different: + +- :data:`AUTH_ERROR_STATUS` — the ``status`` value stamped on the payload. + It reuses the ``status``-field convention the rest of mureo already uses + for exactly this "what did I fail to see" question (``blind_spots`` / + ``ChangeImportStatus`` in the change-import feed, ``no_credentials`` / + ``data_unavailable`` on :class:`~mureo.analytics.models.DeliveryCollapseReport`) + rather than inventing a second convention a skill would have to learn. +- :data:`AUTH_CAUSE_NO_CREDENTIALS` / :data:`AUTH_CAUSE_TOKEN_INVALID` — the + two causes, because they have different recovery actions: one was never + configured, the other was configured and rejected. + +It lives in ``mureo.core`` and not next to the MCP handlers because the +platform clients raise :class:`PlatformAuthError` and the MCP layer renders +it; a client importing from ``mureo.mcp`` would invert the layering. +""" + +from __future__ import annotations + +from typing import Any + +#: ``status`` value every auth-failure payload carries, whatever the +#: platform. This is the marker a skill keys on: one string, one meaning — +#: mureo could not read this platform at all, so it has no numbers for this +#: run and its silence is not evidence of anything. +AUTH_ERROR_STATUS = "auth_error" + +#: No credential is configured for the platform at all. +AUTH_CAUSE_NO_CREDENTIALS = "no_credentials" + +#: A credential exists and the platform rejected it — an expired or revoked +#: token, a withdrawn permission. Distinct from the above because the +#: recovery differs: configure vs. re-authorize. +AUTH_CAUSE_TOKEN_INVALID = "token_invalid" + +#: The closed vocabulary. A cause outside it would reach a skill that has no +#: branch for it, which is the failure this module exists to end. +AUTH_CAUSES = frozenset({AUTH_CAUSE_NO_CREDENTIALS, AUTH_CAUSE_TOKEN_INVALID}) + +#: HTTP statuses that mean "the credential was rejected" rather than "the +#: request was wrong". Deliberately narrow: labelling a 400/429/500 as an +#: auth failure would send an operator to re-authorize a healthy account and +#: would withhold a report section for no reason. +_AUTH_HTTP_STATUSES = frozenset({401, 403}) + +#: The Google Ads ``error_code`` oneof names that mean the credential, not +#: the request, was refused. +_GOOGLE_ADS_AUTH_ONEOFS = frozenset({"authentication_error", "authorization_error"}) + +#: How far up the ``__cause__`` / ``__context__`` chain to look. Clients +#: re-raise as ``RuntimeError(...) from exc``, so the auth signal is usually +#: one or two links down; the bound stops a self-referential chain. +_MAX_CHAIN_DEPTH = 5 + + +class PlatformAuthError(RuntimeError): + """A platform refused mureo's credential. + + Raised at the point where a client can actually tell an auth failure + from a bad request — which is platform-specific knowledge (Meta answers + an expired token with HTTP 400 and an ``OAuthException`` body, not a + 401), so it belongs in the client rather than in a downstream sniffer. + """ + + def __init__(self, message: str, *, cause: str = AUTH_CAUSE_TOKEN_INVALID) -> None: + super().__init__(message) + self.cause = cause if cause in AUTH_CAUSES else AUTH_CAUSE_TOKEN_INVALID + + +def auth_failure_payload(cause: str, detail: str) -> dict[str, Any]: + """Build the one payload shape every platform's auth failure carries. + + ``detail`` keeps the operator-facing sentence (which env var to set, + what the platform said) so nothing readable is lost — but it is a + *field*, never the whole answer, so no reader can mistake it for data. + """ + if cause not in AUTH_CAUSES: + raise ValueError( + f"Unknown auth cause {cause!r}; expected one of {sorted(AUTH_CAUSES)}" + ) + return {"status": AUTH_ERROR_STATUS, "auth_cause": cause, "detail": detail} + + +def is_auth_failure_payload(payload: object) -> bool: + """True if ``payload`` is an auth-failure payload.""" + return isinstance(payload, dict) and payload.get("status") == AUTH_ERROR_STATUS + + +def _http_status(exc: BaseException) -> int | None: + """The HTTP status an ``httpx.HTTPStatusError``-shaped exception carries. + + Duck-typed rather than imported so this module stays free of every + client's dependency; the attribute path is the same for httpx and for + the SDK wrappers that mimic it. + """ + status = getattr(getattr(exc, "response", None), "status_code", None) + return status if isinstance(status, int) else None + + +def _is_google_ads_auth_failure(exc: BaseException) -> bool: + """True for a ``GoogleAdsException`` whose failure is an auth failure. + + The ``error_code`` field is a protobuf oneof, so an unset member still + reads back as its zero enum value — ``WhichOneof`` is the only reliable + way to ask which error kind was actually set. + """ + errors = getattr(getattr(exc, "failure", None), "errors", None) or () + for error in errors: + which = getattr(getattr(error, "error_code", None), "WhichOneof", None) + if which is None: + continue + try: + name = which("error_code") + except Exception: # noqa: BLE001 - classification must never itself raise + continue + if name in _GOOGLE_ADS_AUTH_ONEOFS: + return True + return False + + +def classify_auth_exception(exc: BaseException | None) -> str | None: + """Return the auth cause behind ``exc``, or ``None`` if it is not one. + + ``None`` is the safe answer: an unclassified failure stays an ordinary + API error, which is what it was before this existed. Over-claiming is + the expensive direction — it would withhold a report section and send + the operator to fix credentials that are fine. + """ + seen: set[int] = set() + current: BaseException | None = exc + for _ in range(_MAX_CHAIN_DEPTH): + if current is None or id(current) in seen: + return None + seen.add(id(current)) + if isinstance(current, PlatformAuthError): + return current.cause + if _http_status(current) in _AUTH_HTTP_STATUSES: + return AUTH_CAUSE_TOKEN_INVALID + if _is_google_ads_auth_failure(current): + return AUTH_CAUSE_TOKEN_INVALID + current = current.__cause__ or current.__context__ + return None + + +__all__ = [ + "AUTH_CAUSES", + "AUTH_CAUSE_NO_CREDENTIALS", + "AUTH_CAUSE_TOKEN_INVALID", + "AUTH_ERROR_STATUS", + "PlatformAuthError", + "auth_failure_payload", + "classify_auth_exception", + "is_auth_failure_payload", +] diff --git a/mureo/mcp/_helpers.py b/mureo/mcp/_helpers.py index db1ec95b..cb1c7ef8 100644 --- a/mureo/mcp/_helpers.py +++ b/mureo/mcp/_helpers.py @@ -25,6 +25,14 @@ from mcp.types import TextContent +from mureo.core.auth_failure import ( + AUTH_CAUSE_NO_CREDENTIALS, + AUTH_ERROR_STATUS, + auth_failure_payload, + classify_auth_exception, + is_auth_failure_payload, +) + logger = logging.getLogger(__name__) @@ -163,9 +171,27 @@ def _json_result(data: Any) -> list[TextContent]: return [TextContent(type="text", text=json.dumps(data, ensure_ascii=False))] +def _auth_error_result(cause: str, detail: str) -> list[TextContent]: + """Return the machine-readable auth-failure envelope (#580). + + Every platform's auth failure leaves mureo through here or through + :func:`api_error_handler`, so one shape covers all of them. See + :mod:`mureo.core.auth_failure` for why the shape is a ``status`` field + rather than a sentence. + """ + return _json_result(auth_failure_payload(cause, detail)) + + def _no_creds_result(msg: str) -> list[TextContent]: - """Return a credentials-not-found error.""" - return [TextContent(type="text", text=msg)] + """Return a credentials-not-found error. + + ``msg`` survives verbatim as the payload's ``detail``, so the operator + still reads which env var to set — but it is a field of an + ``auth_error`` payload now, not the whole body, so a report skill can + tell "could not read this platform" from "this platform was quiet" + (#580). + """ + return _auth_error_result(AUTH_CAUSE_NO_CREDENTIALS, msg) # The single prefix ``api_error_handler`` stamps onto a caught-exception @@ -175,6 +201,19 @@ def _no_creds_result(msg: str) -> list[TextContent]: API_ERROR_PREFIX = "API error:" +def is_auth_error_result(result: list[Any] | None) -> bool: + """True if ``result`` is the :data:`AUTH_ERROR_STATUS` envelope (#580).""" + if not result: + return False + text = getattr(result[0], "text", "") + if not isinstance(text, str) or AUTH_ERROR_STATUS not in text: + return False + try: + return is_auth_failure_payload(json.loads(text)) + except ValueError: + return False + + def is_error_result(result: list[Any] | None) -> bool: """True if ``result`` is an :func:`api_error_handler` error envelope. @@ -184,11 +223,18 @@ def is_error_result(result: list[Any] | None) -> bool: that did not actually change platform state. Shared by the native (:mod:`mureo.mcp.native_reversal`) and plugin (:mod:`mureo.mcp.server`) promotion paths so both skip the identical envelope. + + The auth-failure envelope counts too: a mutation refused for a missing + or rejected credential never reached the platform either, and recording + it would put a change that did not happen into ``action_log`` — with an + ``observation_due`` and a reversal plan for it (#580). """ if not result: return False text = getattr(result[0], "text", "") - return isinstance(text, str) and text.startswith(API_ERROR_PREFIX) + if isinstance(text, str) and text.startswith(API_ERROR_PREFIX): + return True + return is_auth_error_result(result) def api_error_handler( @@ -209,6 +255,14 @@ async def wrapper(*args: Any, **kwargs: Any) -> list[TextContent]: raise except Exception as exc: logger.exception("%s failed", func.__name__) + # An auth failure is a different outcome, not a worse error: it + # means this platform produced NO data, so a report built on the + # platforms that did answer is partial. Flattening it into the + # same untyped string as a quota error is what let an expired + # token read as a quiet account (#580). + cause = classify_auth_exception(exc) + if cause is not None: + return _auth_error_result(cause, str(exc)) return [TextContent(type="text", text=f"{API_ERROR_PREFIX} {exc}")] finally: bucket = _active_clients.get() or [] diff --git a/mureo/meta_ads/client.py b/mureo/meta_ads/client.py index f733a83d..e0424fc7 100644 --- a/mureo/meta_ads/client.py +++ b/mureo/meta_ads/client.py @@ -3,10 +3,11 @@ import asyncio import json import logging -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, NoReturn import httpx +from mureo.core.auth_failure import PlatformAuthError from mureo.meta_ads._ad_rules import AdRulesMixin from mureo.meta_ads._ad_sets import AdSetsMixin from mureo.meta_ads._ads import AdsMixin @@ -40,6 +41,82 @@ _MAX_RETRIES = 3 _INITIAL_BACKOFF_SECONDS = 1.0 +# Meta answers an expired or revoked token with HTTP 400, not 401, so the +# status code alone cannot tell a dead credential from a bad request. +# ``OAuthException`` -- and error code 190 with its session sibling 102 -- is +# the discriminator Meta documents, and it is what lets an auth failure reach +# a report skill as a distinct outcome instead of as one more untyped +# ``API error: ...`` string that reads like a quiet account (#580). +_META_OAUTH_ERROR_TYPE = "OAuthException" +_META_AUTH_ERROR_CODES = frozenset({102, 190}) +_META_AUTH_HTTP_STATUS = 401 + +# Longest slice of a Meta error body kept for logs / fallback detail. +_MAX_ERROR_BODY_CHARS = 500 + + +def _meta_error_detail(error: dict[str, Any]) -> str: + """Join the human-readable parts of a Meta ``error`` object.""" + parts = [ + str(error[key]) + for key in ("message", "error_user_title", "error_user_msg") + if error.get(key) + ] + if error.get("error_subcode"): + parts.append(f"subcode={error['error_subcode']}") + if error.get("fbtrace_id"): + parts.append(f"fbtrace_id={error['fbtrace_id']}") + return " | ".join(parts) + + +def _is_meta_auth_failure(status_code: int, error: dict[str, Any]) -> bool: + """True when Meta refused the credential rather than the request. + + Deliberately narrow. Reporting a validation failure as an auth failure + would send the operator to re-authorize a healthy account and would + withhold a report section that had perfectly good data behind it. + """ + if status_code == _META_AUTH_HTTP_STATUS: + return True + if error.get("type") == _META_OAUTH_ERROR_TYPE: + return True + code = error.get("code") + return ( + isinstance(code, int) + and not isinstance(code, bool) + and code in _META_AUTH_ERROR_CODES + ) + + +def _raise_meta_api_error(resp: httpx.Response, method: str, path: str) -> NoReturn: + """Raise the exception class that matches a non-200 Meta response. + + :class:`~mureo.core.auth_failure.PlatformAuthError` for a refused + credential, a plain ``RuntimeError`` for everything else. + """ + error_body = resp.text[:_MAX_ERROR_BODY_CHARS] + logger.error( + "Meta API error: method=%s, path=%s, status=%d, body=%s", + method, + path, + resp.status_code, + error_body, + ) + error: dict[str, Any] = {} + try: + payload = resp.json() + raw = payload.get("error") if isinstance(payload, dict) else None + error = raw if isinstance(raw, dict) else {} + detail = _meta_error_detail(error) + except Exception: # noqa: BLE001 - an unparseable body still has to be reported + detail = error_body + message = ( + f"Meta API request failed (status={resp.status_code}, path={path}): {detail}" + ) + if _is_meta_auth_failure(resp.status_code, error): + raise PlatformAuthError(message) + raise RuntimeError(message) + def _rewind_file_parts(files: dict[str, Any] | None) -> None: """Seek every seekable part in an httpx ``files`` mapping back to byte 0. @@ -282,37 +359,7 @@ async def _request( continue if resp.status_code != 200: - error_body = resp.text[:500] - logger.error( - "Meta API error: method=%s, path=%s, status=%d, body=%s", - method, - path, - resp.status_code, - error_body, - ) - # Extract detailed error from Meta API response - detail = "" - try: - err = resp.json().get("error", {}) - parts = [] - if err.get("message"): - parts.append(err["message"]) - if err.get("error_user_title"): - parts.append(err["error_user_title"]) - if err.get("error_user_msg"): - parts.append(err["error_user_msg"]) - if err.get("error_subcode"): - parts.append(f"subcode={err['error_subcode']}") - if err.get("fbtrace_id"): - parts.append(f"fbtrace_id={err['fbtrace_id']}") - if parts: - detail = " | ".join(parts) - except Exception: - detail = error_body - raise RuntimeError( - f"Meta API request failed " - f"(status={resp.status_code}, path={path}): {detail}" - ) + _raise_meta_api_error(resp, method, path) return resp.json() # type: ignore[no-any-return] diff --git a/skills/_mureo-shared/SKILL.md b/skills/_mureo-shared/SKILL.md index 56d735f5..51fb363e 100644 --- a/skills/_mureo-shared/SKILL.md +++ b/skills/_mureo-shared/SKILL.md @@ -482,12 +482,20 @@ All tools return structured JSON via `TextContent`. The format depends on the to ### Authentication Error +Every platform returns the SAME envelope when mureo could not authenticate. It is a successful tool call, so you must branch on the payload: + ```json { - "error": "No credentials found. Set environment variables (GOOGLE_ADS_DEVELOPER_TOKEN, ...) or ~/.mureo/credentials.json" + "status": "auth_error", + "auth_cause": "no_credentials", + "detail": "Credentials not found. Set environment variables (GOOGLE_ADS_DEVELOPER_TOKEN, ...) or ~/.mureo/credentials.json." } ``` +`auth_cause` is `no_credentials` (nothing is configured — the operator runs `mureo configure` / `mureo auth setup`) or `token_invalid` (a credential exists and the platform rejected it, typically an expired token — the operator re-authorizes). `detail` is the operator-facing sentence. + +**This is a hole in your data, not a data point.** mureo read *nothing* from that platform for this call, so never print `detail` where a number belongs and never treat that platform's missing or empty figures as evidence that it was quiet. Say which platform failed and why, mark any report containing one as **partial**, and withhold conclusions that depend on the numbers you did not get. + ## STATE.json Schema (when writing on Code via `Write`) > **Tool output ≠ STATE.json.** The *Success Response* above is what a vendor diff --git a/skills/daily-check/SKILL.md b/skills/daily-check/SKILL.md index d34c8319..8df6a45d 100644 --- a/skills/daily-check/SKILL.md +++ b/skills/daily-check/SKILL.md @@ -55,6 +55,7 @@ Step 2b **imports** what mureo can fetch, so much of that work is now in `action - mureo BYOD mode applies only to mureo native tools — do **not** look for raw CSVs in the project directory; mureo BYOD data is centralized in the workspace `byod/` directory (or `~/.mureo/byod/` for legacy CLI users) and is only accessible through mureo MCP tools. 4. **Platform health checks**: Run health diagnostics on each configured ad platform. + - **Auth failure is not data — it is a hole in the report.** Any tool result carrying `"status": "auth_error"` means mureo could not read that platform in steps 3–4 at all: `auth_cause` is `no_credentials` (nothing is configured) or `token_invalid` (a credential exists and the platform rejected it — typically an expired token), and `detail` is the operator-facing sentence. **Never render `detail` where a metric belongs, and never read that platform's missing or empty numbers as "quiet"** — unreadable and quiet are different facts. Record the platform name with its `auth_cause` and carry both into step 10 exactly as step 2b's `blind_spots` are carried, then keep going for every other platform: one platform's credentials failing degrades the report, it never aborts the run. - **Analytics-first (both modes' first move).** Consult `mureo_analytics_modules_list` for **every** platform (not just external ones) — it maps each platform to its advertised capabilities (`detect_anomalies`, `diagnose_performance`, `audit_creative`, `analyze_budget_efficiency`, `detect_delivery_collapse`). Match its `platform` field against the platform keys you already hold: it is the **canonical platform key** (`plugin::` for a plugin, and the older `plugin:` still resolves), the same key STATE.json and `action_log` use, while `registry_name` / `source_distribution` are identifiers, not keys — never look a platform up by them (see `../_mureo-shared/SKILL.md` → *Canonical platform key*). Then per platform: - **Module registered AND advertises the capability you need** → run it via `mureo_analytics_run` (`DETECT_ANOMALIES` / `DIAGNOSE_PERFORMANCE`; pass `platform`, `capability`, `account_id`; add `window_days` for `detect_anomalies` or `scope` for `diagnose_performance`) and fold its **compact findings** (`result`) into the health summary. In **incremental** mode this REPLACES the raw pulls — do **not** ALSO fetch raw campaign / insights rows to re-derive what the module already summarized. A `status` other than `ok` (`no_analytics_module` / `capability_not_available` / `error`) means treat that capability as unavailable and fall through to the raw checks below — never fail the whole daily-check because one platform's module errored. - **No analytics module, or the module lacks the capability** → report `analytics_not_available_for_` for the **module-specific** deep diagnostics (RSA audit, `result_indicator`, budget-efficiency) — `` being the canonical platform key, so the notice names the same platform as the rest of the report — and do NOT fabricate heuristics from the integration's tool schemas (Issue #120). **BUT still run the generic, platform-agnostic `analysis_anomalies_check` per campaign** (metrics normalized to `campaign_id`/`cost`/`clicks`/`impressions`/`conversions`/`cpa`/`ctr`): it builds a median baseline from `action_log` and flags zero-spend / CPA-spike / CTR-drop for TikTok, plugins and official-MCP platforms too — a principled detector, not an auto-derived heuristic (see `_mureo-shared` → *analytics-module parity* → generic anomaly check). @@ -106,6 +107,8 @@ Step 2b **imports** what mureo can fetch, so much of that work is now in `action 10. **Report** — the mode decides the shape: + **Partial report (both modes) — this goes FIRST, above the verdicts.** If any platform came back `"status": "auth_error"` in steps 3–4, this report is **partial**: open it by saying so, naming each affected platform with its `auth_cause` and the recovery (`no_credentials` → configure the credential via `mureo configure` / `mureo auth setup`; `token_invalid` → re-authorize, the credential is present and dead). Then **withhold every verdict, goal-progress line and recommendation that depends on that platform's numbers** — an unreadable platform has no performance to judge, and cross-platform advice (budget shifts, "scale here, cut there") derived only from the platforms that did answer is not the advice you would give with all of them. Report the platforms that did answer as usual, and make clear which is which. + **Deep mode — full report.** Summarize findings as: **Healthy** (no action needed), **Watch** (minor issues to monitor), **Action needed** (requires immediate attention). For each issue, suggest specific actions aligned with the current Operation Mode. Do NOT recommend actions based on single-day fluctuations — at least 7 consecutive days of critical metrics (>30% off target) before suggesting rescue. **Incremental mode — diff-first.** Compare this run against the previous `reports.daily` summary and report ONLY the deltas: diff --git a/tests/test_auth_failure_envelope.py b/tests/test_auth_failure_envelope.py new file mode 100644 index 00000000..92443819 --- /dev/null +++ b/tests/test_auth_failure_envelope.py @@ -0,0 +1,243 @@ +"""An auth failure must be machine-distinguishable from data (#580). + +Before this suite, a platform whose credentials were missing or whose token +had expired answered an MCP read with a *successful* tool result whose text +happened to be a sentence about credentials. Nothing downstream could tell +"this platform spent nothing" from "this platform could not be read", so +``/daily-check`` shipped a report that looked complete with error prose +sitting where the numbers belonged. + +The fix gives every platform ONE payload shape for that outcome — the same +``status`` vocabulary ``blind_spots`` and ``DeliveryCollapseReport`` already +use — produced in the two central places every platform routes through: +``_no_creds_result`` and ``api_error_handler``. + +Marks: unit — pure in-process, no network. +""" + +from __future__ import annotations + +import json +from typing import Any + +import pytest +from mcp.types import TextContent + +from mureo.core.auth_failure import ( + AUTH_CAUSE_NO_CREDENTIALS, + AUTH_CAUSE_TOKEN_INVALID, + AUTH_ERROR_STATUS, + PlatformAuthError, + auth_failure_payload, + classify_auth_exception, +) +from mureo.mcp._helpers import ( + API_ERROR_PREFIX, + _json_result, + _no_creds_result, + api_error_handler, + is_auth_error_result, + is_error_result, +) + +pytestmark = pytest.mark.unit + + +def _payload(result: list[Any]) -> dict[str, Any]: + parsed = json.loads(result[0].text) + assert isinstance(parsed, dict) + return parsed + + +# --------------------------------------------------------------------------- +# The vocabulary itself +# --------------------------------------------------------------------------- + + +class TestAuthFailurePayload: + def test_carries_status_cause_and_human_detail(self) -> None: + payload = auth_failure_payload(AUTH_CAUSE_TOKEN_INVALID, "token expired") + assert payload == { + "status": AUTH_ERROR_STATUS, + "auth_cause": AUTH_CAUSE_TOKEN_INVALID, + "detail": "token expired", + } + + def test_rejects_a_cause_outside_the_vocabulary(self) -> None: + """An unknown cause would reach a skill that has no branch for it.""" + with pytest.raises(ValueError, match="auth cause"): + auth_failure_payload("probably_fine", "detail") + + +# --------------------------------------------------------------------------- +# Classifying the exception behind an error +# --------------------------------------------------------------------------- + + +class _FakeResponse: + def __init__(self, status_code: int) -> None: + self.status_code = status_code + + +class _FakeHttpStatusError(Exception): + """Shaped like ``httpx.HTTPStatusError`` (Search Console's raise path).""" + + def __init__(self, status_code: int) -> None: + super().__init__(f"status {status_code}") + self.response = _FakeResponse(status_code) + + +class _FakeErrorCode: + def __init__(self, oneof: str) -> None: + self._oneof = oneof + + def WhichOneof(self, name: str) -> str: # noqa: N802 - protobuf API name + assert name == "error_code" + return self._oneof + + +class _FakeGoogleAdsFailure: + def __init__(self, oneof: str) -> None: + self.errors = [type("E", (), {"error_code": _FakeErrorCode(oneof)})()] + + +class _FakeGoogleAdsError(Exception): + """Shaped like ``GoogleAdsException`` (Google Ads' read path).""" + + def __init__(self, oneof: str) -> None: + super().__init__("google ads failed") + self.failure = _FakeGoogleAdsFailure(oneof) + + +class TestClassifyAuthException: + def test_platform_auth_error_reports_its_own_cause(self) -> None: + exc = PlatformAuthError("nope", cause=AUTH_CAUSE_NO_CREDENTIALS) + assert classify_auth_exception(exc) == AUTH_CAUSE_NO_CREDENTIALS + + def test_platform_auth_error_defaults_to_token_invalid(self) -> None: + assert classify_auth_exception(PlatformAuthError("nope")) == ( + AUTH_CAUSE_TOKEN_INVALID + ) + + @pytest.mark.parametrize("status", [401, 403]) + def test_rejected_credential_http_statuses(self, status: int) -> None: + assert classify_auth_exception(_FakeHttpStatusError(status)) == ( + AUTH_CAUSE_TOKEN_INVALID + ) + + @pytest.mark.parametrize("status", [400, 404, 429, 500]) + def test_other_http_statuses_are_not_auth_failures(self, status: int) -> None: + """Mislabelling a 500 as an auth failure would send the operator to + re-authorize an account whose credentials are fine.""" + assert classify_auth_exception(_FakeHttpStatusError(status)) is None + + @pytest.mark.parametrize("oneof", ["authentication_error", "authorization_error"]) + def test_google_ads_auth_error_codes(self, oneof: str) -> None: + assert classify_auth_exception(_FakeGoogleAdsError(oneof)) == ( + AUTH_CAUSE_TOKEN_INVALID + ) + + def test_google_ads_non_auth_error_codes(self) -> None: + assert classify_auth_exception(_FakeGoogleAdsError("mutate_error")) is None + + def test_walks_the_exception_chain(self) -> None: + """Clients re-raise ``RuntimeError(...) from exc``; the cause must not + be lost behind the wrapper.""" + try: + try: + raise PlatformAuthError("token expired") + except PlatformAuthError as inner: + raise RuntimeError("An error occurred") from inner + except RuntimeError as outer: + assert classify_auth_exception(outer) == AUTH_CAUSE_TOKEN_INVALID + + def test_ordinary_failures_are_not_auth_failures(self) -> None: + assert classify_auth_exception(RuntimeError("quota exceeded")) is None + assert classify_auth_exception(None) is None + + +# --------------------------------------------------------------------------- +# The MCP result envelope +# --------------------------------------------------------------------------- + + +class TestNoCredsResult: + def test_is_a_structured_auth_error_not_prose(self) -> None: + result = _no_creds_result("Credentials not found. Set META_ADS_ACCESS_TOKEN.") + payload = _payload(result) + assert payload["status"] == AUTH_ERROR_STATUS + assert payload["auth_cause"] == AUTH_CAUSE_NO_CREDENTIALS + + def test_keeps_the_operator_facing_sentence(self) -> None: + msg = "Credentials not found. Set META_ADS_ACCESS_TOKEN." + assert _payload(_no_creds_result(msg))["detail"] == msg + + +class TestIsAuthErrorResult: + def test_true_for_the_auth_envelope(self) -> None: + assert is_auth_error_result(_no_creds_result("nope")) is True + + def test_false_for_an_ordinary_api_error(self) -> None: + api_error = [TextContent(type="text", text="API error: x")] + assert is_auth_error_result(api_error) is False + + def test_false_for_ordinary_json_data(self) -> None: + assert is_auth_error_result(_json_result({"campaigns": []})) is False + + def test_false_for_a_json_string_that_is_not_an_object(self) -> None: + assert is_auth_error_result([TextContent(type="text", text="[1, 2]")]) is False + + def test_false_for_empty_and_none(self) -> None: + assert is_auth_error_result(None) is False + assert is_auth_error_result([]) is False + + +class TestIsErrorResult: + """The mutation gate must keep recognising BOTH envelopes. + + ``is_error_result`` is what stops a mutation that never reached the + platform from being written to ``action_log``. An auth failure is exactly + such a mutation, so the new envelope must not fall out of that gate. + """ + + def test_still_true_for_the_api_error_envelope(self) -> None: + assert is_error_result([TextContent(type="text", text="API error: x")]) is True + + def test_true_for_the_auth_error_envelope(self) -> None: + assert is_error_result(_no_creds_result("nope")) is True + + def test_false_for_a_successful_result(self) -> None: + assert is_error_result(_json_result({"id": "1"})) is False + + +# --------------------------------------------------------------------------- +# api_error_handler routing +# --------------------------------------------------------------------------- + + +class TestApiErrorHandlerRouting: + async def test_auth_failure_becomes_the_auth_envelope(self) -> None: + @api_error_handler + async def handler() -> list[TextContent]: + raise PlatformAuthError("Meta API request failed (status=190)") + + payload = _payload(await handler()) + assert payload["status"] == AUTH_ERROR_STATUS + assert payload["auth_cause"] == AUTH_CAUSE_TOKEN_INVALID + assert "status=190" in payload["detail"] + + async def test_other_failures_stay_on_the_api_error_envelope(self) -> None: + @api_error_handler + async def handler() -> list[TextContent]: + raise RuntimeError("quota exceeded") + + result = await handler() + assert result[0].text == f"{API_ERROR_PREFIX} quota exceeded" + + async def test_value_error_still_propagates(self) -> None: + @api_error_handler + async def handler() -> list[TextContent]: + raise ValueError("Required parameter customer_id is not specified") + + with pytest.raises(ValueError, match="customer_id"): + await handler() diff --git a/tests/test_daily_check_auth_failure.py b/tests/test_daily_check_auth_failure.py new file mode 100644 index 00000000..7aaf1a74 --- /dev/null +++ b/tests/test_daily_check_auth_failure.py @@ -0,0 +1,78 @@ +"""/daily-check must treat a platform's auth failure as a first-class outcome. + +Field report (#580): a platform's token had expired, the run went through +every step, and the report shipped with ``"API error: Meta API request failed +(status=400, ...)"`` sitting where the numbers belonged, next to real figures +from the platforms that did answer, followed by the usual recommendations. +Nothing marked the report partial, so it read as "that platform was quiet" +rather than "that platform was unreadable". + +Every failure-handling line the skill already had addresses a different +condition and says to keep going — the analytics-module fall-through, the +official-hosted-MCP tool-surface fallbacks, and step 2b's ``blind_spots``. +This suite pins the missing branch, in BOTH the packaged copy and the +repo-root mirror, kept byte-identical. + +Marks: unit — pure on-disk file inspection, no network. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.unit + +_ROOT = Path(__file__).resolve().parent.parent +_PACKAGED = _ROOT / "mureo" / "_data" / "skills" / "daily-check" / "SKILL.md" +_MIRROR = _ROOT / "skills" / "daily-check" / "SKILL.md" + + +def _body() -> str: + return _PACKAGED.read_text(encoding="utf-8") + + +def test_copies_are_byte_identical() -> None: + assert _PACKAGED.read_bytes() == _MIRROR.read_bytes() + + +def test_names_the_machine_readable_marker() -> None: + """The skill must key on the payload, not on error prose — prose is + exactly what it failed to recognise.""" + body = _body() + assert '"status": "auth_error"' in body + assert "auth_cause" in body + + +def test_documents_both_causes() -> None: + body = _body() + assert "no_credentials" in body + assert "token_invalid" in body + + +def test_forbids_rendering_the_failure_as_data() -> None: + """An unreadable platform is never a quiet platform.""" + body = _body().lower() + assert "quiet" in body + assert "detail" in body + + +def test_marks_the_report_partial_and_names_the_platform() -> None: + body = _body().lower() + assert "partial" in body + auth_lines = [ln for ln in _body().splitlines() if "auth_error" in ln] + assert auth_lines, "no auth-failure branch in the skill" + assert any("platform" in ln.lower() for ln in auth_lines) + + +def test_withholds_recommendations_that_depend_on_the_missing_platform() -> None: + lowered = _body().lower() + assert "withhold" in lowered + assert "recommendation" in lowered + + +def test_does_not_abort_the_whole_run() -> None: + """The #440 rule — never fail the whole daily-check because one platform + broke — still holds; the report degrades, it does not stop.""" + assert "never fail the whole daily-check" in _body() diff --git a/tests/test_meta_ads_client.py b/tests/test_meta_ads_client.py index 8ad4e6cf..8a9ed237 100644 --- a/tests/test_meta_ads_client.py +++ b/tests/test_meta_ads_client.py @@ -12,6 +12,7 @@ import httpx import pytest +from mureo.core.auth_failure import PlatformAuthError from mureo.meta_ads.client import ( MetaAdsApiClient, ) @@ -214,6 +215,86 @@ async def test_unsupported_method_raises(self, client: MetaAdsApiClient) -> None await client._request("PATCH", "/test") +# --------------------------------------------------------------------------- +# Auth-failure classification (#580) +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestAuthFailureClassification: + """Meta answers an expired token with HTTP 400, not 401. + + So the status code alone cannot tell a dead credential from a bad + request, and both used to arrive downstream as the same untyped + ``API error: ...`` string. ``OAuthException`` / code 190 is the + discriminator, and raising :class:`PlatformAuthError` is what lets + ``api_error_handler`` stamp the machine-readable envelope a report + skill can branch on (#580). + """ + + @pytest.fixture() + def client(self) -> MetaAdsApiClient: + return MetaAdsApiClient("token", "act_123") + + @staticmethod + def _respond(client: MetaAdsApiClient, status: int, body: dict) -> None: + mock_resp = MagicMock() + mock_resp.status_code = status + mock_resp.text = json.dumps(body) + mock_resp.headers = {} + mock_resp.json.return_value = body + client._http = MagicMock() + client._http.get = AsyncMock(return_value=mock_resp) + + @pytest.mark.asyncio + async def test_expired_token_400_is_an_auth_error( + self, client: MetaAdsApiClient + ) -> None: + self._respond( + client, + 400, + { + "error": { + "message": "Error validating access token: Session has expired", + "type": "OAuthException", + "code": 190, + "error_subcode": 463, + } + }, + ) + with pytest.raises(PlatformAuthError, match="status=400"): + await client._get("/test") + + @pytest.mark.asyncio + async def test_401_is_an_auth_error_whatever_the_body( + self, client: MetaAdsApiClient + ) -> None: + self._respond(client, 401, {"error": {"message": "Unauthorized"}}) + with pytest.raises(PlatformAuthError): + await client._get("/test") + + @pytest.mark.asyncio + async def test_ordinary_400_stays_a_plain_runtime_error( + self, client: MetaAdsApiClient + ) -> None: + """A validation failure must NOT be reported as a credential problem — + it would send the operator to re-authorize a healthy account.""" + self._respond( + client, + 400, + { + "error": { + "message": "Invalid parameter", + "type": "GraphMethodException", + "code": 100, + } + }, + ) + with pytest.raises(RuntimeError) as excinfo: + await client._get("/test") + assert not isinstance(excinfo.value, PlatformAuthError) + + # --------------------------------------------------------------------------- # Context manager tests # ---------------------------------------------------------------------------