diff --git a/AGENTS.md b/AGENTS.md index 6173916..ebab9ab 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,6 +93,8 @@ mureo/ │ ├── _handlers_search_console.py # Search Console handlers │ ├── tools_rollback.py # rollback_plan_get / rollback_apply │ ├── _handlers_rollback.py # Rollback handlers (lazy-resolve dispatcher) +│ ├── tools_batch.py # mureo_batch_begin / _end / _status (#549) +│ ├── _handlers_batch.py # Batch lifecycle handlers │ ├── tools_analysis.py # analysis_anomalies_check / analysis_exclusion_impact_preview │ ├── _handlers_analysis.py # Anomaly detector composition handler │ ├── _handlers_exclusion_impact.py # analysis_exclusion_impact_preview handler (#547) @@ -124,7 +126,8 @@ mureo/ ├── context/ # File-based strategy context (no DB) │ ├── strategy.py # STRATEGY.md parser/writer │ ├── state.py # STATE.json parser/writer -│ ├── models.py # StrategyEntry, StateDocument, CampaignSnapshot, ActionLogEntry (rollback_of) +│ ├── models.py # StrategyEntry, StateDocument, CampaignSnapshot, ActionLogEntry (rollback_of, batch_id), BatchRecord +│ ├── batch.py # Batch id minting + the action_log stamping rule (#549) │ └── errors.py # Context-specific errors ├── analysis/ # Analysis utilities │ ├── lp_analyzer.py # Landing page analyzer @@ -137,8 +140,9 @@ mureo/ │ │ └── surfaces.py # Which tools are exclusion surfaces (mureo's + plugin-registered) │ └── tracking/ # Tracking-parameter consistency: platform-neutral detector + per-platform URL accessors (#550) ├── rollback/ # Rollback feature (allow-list gated, append-only audit trail) -│ ├── models.py # RollbackStatus enum + RollbackPlan dataclass +│ ├── models.py # RollbackStatus / RollbackPlan + batch verdicts (BatchCoverage, BatchRollbackPlan) │ ├── planner.py # plan_rollback(ActionLogEntry) -> RollbackPlan | None +│ ├── batch.py # plan_batch_rollback(doc, batch_id) -> every member, gaps included (#549) │ └── executor.py # execute_rollback(...) -> appends ActionLogEntry(rollback_of=index) ├── adapters/ # Provider adapters wrapping each ad-platform client as a registry Protocol ├── analytics/ # Analytics-module registry for external MCP / plugin platforms (#120) @@ -239,6 +243,7 @@ These families are not tied to a single ad platform. Tool names are the exact MC |--------|-------| | Analytics Registry (#440) | `mureo_analytics_modules_list`, `mureo_analytics_run` | | Rollback | `rollback_plan_get`, `rollback_apply` | +| Batch (#549) | `mureo_batch_begin`, `mureo_batch_end`, `mureo_batch_status` | | Analysis | `analysis_anomalies_check`, `analysis_exclusion_impact_preview` | | Creative Studio | `creative_studio_providers_list`, `creative_studio_generate_visual`, `creative_studio_edit_visual`, `creative_studio_compose`, `creative_studio_brand_kit_get` | | Learning | `mureo_learning_insights_get`, `mureo_consult_advisor` | diff --git a/CHANGELOG.md b/CHANGELOG.md index 86537a7..13e651f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,63 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ### Added +- **A bulk change is one revertible unit** (#549). mureo had rollback, but it + reasoned about one allow-listed operation at a time, so "undo what I did on + Monday" was not expressible: after a bulk pass the operator had to work out + by hand which entries a change set contained. An unverifiable revert is + nearly as bad as no revert — it leaves the operator unable to rule their own + fix out as a variable. + + - `mureo_batch_begin` / `mureo_batch_end` / `mureo_batch_status` declare the + boundary of a change set. A bulk pass is many tool calls and nothing in a + single call says which others belong with it, so the boundary is declared + rather than guessed at from timing or target. + - Membership is stamped at the one place every recording path already + converges (`append_action_log`), not through tool arguments. That is what + makes it platform-agnostic with no per-platform code and no ABI change: a + native Google/Meta status toggle, a mutation an agent records for a hosted + connector, and a bridged/plugin tool call mureo promotes all join the same + batch — including tools whose input schemas mureo does not own. + - `rollback_plan_get` accepts `batch_id` and returns a plan covering **every** + member, with `coverage` (`full` / `partial` / `none` / `empty`), the same + verdict **per platform**, per-member reversibility, the reason each + irreversible member cannot be reversed, and an `apply_order`. Reversibility + is not uniform across platforms, and a plan that listed only the reversible + members would read as a complete revert; a batch where 60 of 80 members can + be restored says so before anything is applied. + - Each member is classified by the existing `plan_rollback` allow-list, so + grouping loosens no guarantee. Reversals appended by `rollback_apply` never + join an open batch — otherwise reverting a batch would grow it. + + Honest limits, stated in the docs rather than smoothed over: native + mutations other than status toggles (budget, keywords, exclusions) join a + batch only when the agent records them with `mureo_state_action_log_append`; + a bridged/plugin reversal is executed only when it names a registered plugin + tool, and is otherwise reported `irreversible`; hosted-connector members are + never reversed by mureo, so their plan is an accurate manual checklist; and + Search Console mutations are not recorded in `action_log` at all, so they + cannot join a batch today. + + Membership is validated, not trusted. An explicit `batch_id` on + `mureo_state_action_log_append` must name a batch that was actually declared + and is still open; an unknown id is refused, and so is a closed one. The + check lives at the `append_action_log` choke point, so no caller — handler, + library user or future recorder — can conjure a change set or grow one whose + membership `mureo_batch_end` already reported. + + A forgotten `mureo_batch_end` announces itself. After 24 hours open, + `mureo_batch_status` returns a warning and one is appended to every mutating + tool result, so the agent that forgot is told without having to ask. Nothing + is ever closed automatically: a timeout would trade a visible wrong answer + for an invisible one. + + STATE.json gains an optional `batches` array and an optional `batch_id` on + each `action_log` entry, both emitted only when present — an existing + STATE.json parses unchanged and gains no new key on the next write. + + - **A bulk exclusion now says how much of your delivery it removes, before it is applied — and a threshold in `STRATEGY.md` can refuse it** (#547). mureo would happily apply an exclusion / block / negative-keyword batch without @@ -145,6 +199,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `_resolve_path`, the workspace sandbox boundary shared by the STATE.json / + STRATEGY.md tools, moved from `mureo/mcp/_handlers_mureo_context.py` to + `mureo/mcp/_helpers.py` as `resolve_workspace_path`. It is a security check; + a sibling module reaching into another handler's privates to borrow it — + or copying it — is a place for the two to drift. + + - **The rollback planner's destructive-verb net now has one explicit, bounded exemption** (#544). That net refuses to plan any reversal whose operation name contains `_remove` / `_delete` / …, which is the right @@ -2702,7 +2763,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - READMEs now link the commercial editions (mureo.jp): the cloud-hosted service and the local Agency edition. (#380) - ## [0.10.19] - 2026-07-09 ### Added diff --git a/README.ja.md b/README.ja.md index cdfb2ca..ca7729e 100644 --- a/README.ja.md +++ b/README.ja.md @@ -270,7 +270,7 @@ STATE.jsonから接続媒体を検出: ### MCP サーバーとツール一覧 -mureo は **213 の MCP ツール** を stdio で公開します。Google広告（89）、Meta広告（90）、Search Console（10）に加え、rollback、異常検知、除外の配信インパクトプレビュー、トラッキングパラメータ整合性、戦略と状態のコンテキスト、分析モジュールレジストリ、学習、学習期間リセットのプリフライト、Creative Studio を含みます。Amazon 広告を設定している場合は、ローカルのマニフェストからブリッジされた Amazon のツールがこれに加わります（ツール名も本数も Amazon 側のもので、mureo が定義するものではありません。詳細は [docs/amazon-ads.ja.md](docs/amazon-ads.ja.md)）。MCP 対応クライアントなら何からでも接続できます。 +mureo は **216 の MCP ツール** を stdio で公開します。Google広告（89）、Meta広告（90）、Search Console（10）に加え、rollback、バッチ（一括変更を1つの取り消し単位にまとめる）、異常検知、除外の配信インパクトプレビュー、トラッキングパラメータ整合性、戦略と状態のコンテキスト、分析モジュールレジストリ、学習、学習期間リセットのプリフライト、Creative Studio を含みます。Amazon 広告を設定している場合は、ローカルのマニフェストからブリッジされた Amazon のツールがこれに加わります（ツール名も本数も Amazon 側のもので、mureo が定義するものではありません。詳細は [docs/amazon-ads.ja.md](docs/amazon-ads.ja.md)）。MCP 対応クライアントなら何からでも接続できます。 ```json { diff --git a/README.md b/README.md index ff62559..e2fc680 100644 --- a/README.md +++ b/README.md @@ -304,7 +304,7 @@ Why this matters: `link_click` vs `pixel_lead` optimization is a tracking distin ### MCP server & tool list -mureo exposes **213 MCP tools** over stdio: Google Ads (89), Meta Ads (90), Search Console (10), plus rollback, anomaly detection, exclusion delivery-impact preview, tracking-parameter consistency, strategy/state context, analytics registry, learning, learning-period reset pre-flight, and Creative Studio. When Amazon Ads is configured, the bridged Amazon tools are added on top from the local manifest (their names and count are Amazon's, not mureo's — see [docs/amazon-ads.md](docs/amazon-ads.md)). Any MCP-compatible client can connect: +mureo exposes **216 MCP tools** over stdio: Google Ads (89), Meta Ads (90), Search Console (10), plus rollback, batch (group a bulk change into one revertible unit), anomaly detection, exclusion delivery-impact preview, tracking-parameter consistency, strategy/state context, analytics registry, learning, learning-period reset pre-flight, and Creative Studio. When Amazon Ads is configured, the bridged Amazon tools are added on top from the local manifest (their names and count are Amazon's, not mureo's — see [docs/amazon-ads.md](docs/amazon-ads.md)). Any MCP-compatible client can connect: ```json { diff --git a/docs/architecture.md b/docs/architecture.md index bf15412..5cf86ca 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -133,12 +133,14 @@ mureo/ │ ├── convention.py # Opt-in '## Tracking Convention' section of STRATEGY.md │ └── sources.py # One thin accessor per platform ('this ad's destination URLs') ├── rollback/ # Rollback feature (allow-list gated, append-only) -│ ├── models.py # RollbackStatus enum + RollbackPlan dataclass +│ ├── models.py # RollbackStatus / RollbackPlan + batch verdicts (BatchCoverage, BatchRollbackPlan) │ ├── planner.py # plan_rollback(ActionLogEntry) -> RollbackPlan | None +│ ├── batch.py # plan_batch_rollback(doc, batch_id) -> every member, gaps included (#549) │ └── executor.py # execute_rollback(...) -> appends ActionLogEntry(rollback_of=index) ├── context/ # File-based context (STRATEGY.md, STATE.json) -│ ├── models.py # Immutable dataclasses (ActionLogEntry.rollback_of for audit trail) +│ ├── models.py # Immutable dataclasses (ActionLogEntry.rollback_of / .batch_id, BatchRecord) │ ├── strategy.py # STRATEGY.md parser / renderer +│ ├── batch.py # Batch id minting, the stamping rule, membership queries (#549) │ ├── state.py # STATE.json read / mutate / atomic write + state lock (re-exports the two below) │ ├── state_codec.py # STATE.json <-> StateDocument codec (parse_state / render_state) │ ├── conversion_overrides.py # Per-account conversion action_type override lookup (#342) @@ -182,6 +184,8 @@ mureo/ │ ├── _handlers_search_console.py # Search Console handlers │ ├── tools_rollback.py # rollback_plan_get / rollback_apply │ ├── _handlers_rollback.py # Rollback handlers (lazy-resolve dispatcher) +│ ├── tools_batch.py # mureo_batch_begin / _end / _status +│ ├── _handlers_batch.py # Batch lifecycle handlers │ ├── tools_analysis.py # analysis_anomalies_check / analysis_tracking_consistency_check │ ├── _handlers_analysis.py # Anomaly detector composition handler │ ├── _handlers_tracking.py # Tracking-parameter consistency handler (#550) @@ -275,6 +279,8 @@ mureo assumes the caller is an AI agent susceptible to prompt injection, not a t 4. **Tracking-parameter consistency** — `mureo/analysis/tracking/` detects ads whose final-URL tracking parameters disagree with the campaign they live in: a silent defect, because delivery and spend look healthy while the analytics everyone downstream trusts is quietly wrong. The detector is platform-neutral (it sees only `AdTrackingRecord`) with one thin accessor per platform, and it derives its verdict from evidence already in the account — never from a guessed naming convention. Operator intent is declared in STRATEGY.md's `## Tracking Convention` and parsed by mureo, not interpreted by the agent. Exposed as `analysis_tracking_consistency_check`, used by `/tracking-health` for the account audit and as a pre-flight before ads are created. See [tracking-consistency.md](tracking-consistency.md) for the exhaustive list of what it cannot detect. 5. **Rollback with allow-list gating** — `mureo/rollback/` turns agent-authored `reversible_params` hints into concrete `RollbackPlan` records. `reversible_params` is untrusted input for the rollback executor, so the planner enforces an explicit allow-list of operations (budget update + status toggles across Google/Meta Ads), refuses destructive verbs (`.delete` / `.remove` / `.destroy` / `.purge` / `.transfer`), and rejects unexpected parameter keys — a compromised agent cannot smuggle a privileged call through the rollback path. The `mureo rollback list` / `show` CLI commands are inspection-only; execution stays with the MCP dispatcher so it re-enters the same policy gate as forward actions, and control characters from STATE.json are stripped before terminal output to prevent ANSI-escape spoofing. + A bulk change is planned as **one unit** (#549): `mureo_batch_begin` / `mureo_batch_end` declare the boundary, every `action_log` entry written in between is stamped with the batch id at the single `append_action_log` choke point (so native, hosted-connector and bridged/plugin recordings all join without any per-platform code), and `rollback_plan_get` takes that id and classifies **every** member. Coverage is reported overall and per platform as `full` / `partial` / `none` — because reversibility is not uniform across platforms, and a plan that quietly omitted the members mureo cannot reverse would read as a complete revert. The same allow-list decides each member, so nothing about the guarantee is loosened by grouping. + See [SECURITY.md](../SECURITY.md) for the full threat model. ## Mixin Architecture @@ -456,7 +462,7 @@ The Amazon bridge is the reason mureo sits in the request path rather than letti ## Command-Based Workflow System -In addition to the 213 individual MCP tools, mureo provides **workflow commands** as Claude Code native slash skills (deployed to `~/.claude/skills/`). These commands are **platform-agnostic orchestration instructions** that guide the AI agent to discover platforms, select tools, and synthesize cross-platform insights — all driven by the strategy context in `STRATEGY.md`. +In addition to the 216 individual MCP tools, mureo provides **workflow commands** as Claude Code native slash skills (deployed to `~/.claude/skills/`). These commands are **platform-agnostic orchestration instructions** that guide the AI agent to discover platforms, select tools, and synthesize cross-platform insights — all driven by the strategy context in `STRATEGY.md`. ### How It Works diff --git a/docs/cli.md b/docs/cli.md index 7be2252..6599798 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -241,6 +241,10 @@ Agent: rollback_apply({index: 0, confirm: true}) → dispatches. `confirm` must be the literal boolean `true` (truthy non-booleans are refused). On success the executor appends a new log entry tagged `rollback_of=`; a second apply of the same index is refused. `state_file` resolves strictly inside the MCP server's current working directory — `..`-traversal and symlink escape are refused so an attacker-crafted `STATE.json` elsewhere on disk cannot be used as the reversal source. +### Reverting a whole bulk change + +A bulk pass wrapped in a batch (`mureo_batch_begin` / `mureo_batch_end`) is planned as one unit by `rollback_plan_get` with `batch_id` instead of `index` — it reports every member, overall and per-platform coverage (`full` / `partial` / `none`), and the reason each member it cannot reverse. That surface is **MCP-only**: `mureo rollback list` / `show` still work entry by entry, and neither the batch tools nor batch planning has a CLI command today. Ask the agent for the batch plan before applying anything; a batch where only some members can be restored will say so there. + ## BYOD Commands (Bring Your Own Data) Analyse your ad-account data locally without OAuth or a developer token. The importer accepts a single XLSX produced by either the mureo Google Ads Script (`scripts/sheet-template/google-ads-script.js`) or a Meta Ads Manager Saved Report. Activated automatically when `~/.mureo/byod/manifest.json` registers a platform — no `--byod` flag exists. Adapter dispatch is by workbook header signature, so no `--google-ads / --meta-ads` flags on `import` are needed. See [`docs/byod.md`](byod.md) for the full walkthrough. diff --git a/docs/mcp-server.md b/docs/mcp-server.md index bc41e67..b4ec5f0 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -1,6 +1,6 @@ # MCP Server Guide -mureo exposes 213 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 189 advertising and SEO operation tools across Google Ads (89), Meta Ads (90), and Search Console (10), 2 rollback tools, 3 cross-platform analysis tools (anomaly detection, the exclusion delivery-impact preview and tracking-parameter consistency), 9 mureo-context tools (strategy / state / reports / outcome evaluation), 2 analytics-registry tools, 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)), 1 learning-period pre-flight tool (`mureo_learning_reset_preflight` — is a pending change reset-triggering, and is the campaign already learning; see [Learning-period reset pre-flight](#learning-period-reset-pre-flight)), and 5 Creative Studio tools (text-free key-visual generation + banner composition). 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). The count covers mureo's own tool families only — tools bridged from the official **Amazon Ads** MCP (and from any installed provider plugin) are appended on top at server start and vary per operator; see [Amazon Ads (official-MCP bridge)](#amazon-ads-official-mcp-bridge) below. +mureo exposes 216 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 189 advertising and SEO operation tools across Google Ads (89), Meta Ads (90), and Search Console (10), 2 rollback tools, 3 batch tools (group a bulk change into one revertible unit), 3 cross-platform analysis tools (anomaly detection, the exclusion delivery-impact preview and tracking-parameter consistency), 9 mureo-context tools (strategy / state / reports / outcome evaluation), 2 analytics-registry tools, 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)), 1 learning-period pre-flight tool (`mureo_learning_reset_preflight` — is a pending change reset-triggering, and is the campaign already learning; see [Learning-period reset pre-flight](#learning-period-reset-pre-flight)), and 5 Creative Studio tools (text-free key-visual generation + banner composition). 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). The count covers mureo's own tool families only — tools bridged from the official **Amazon Ads** MCP (and from any installed provider plugin) are appended on top at server start and vary per operator; see [Amazon Ads (official-MCP bridge)](#amazon-ads-official-mcp-bridge) below. ## Starting the Server @@ -522,11 +522,52 @@ Cross-platform tools for inspecting and applying the reversal of a previously-re | Tool | Description | Required Parameters | |------|-------------|-------------------| -| `rollback_plan_get` | Inspect the reversal plan for an `action_log` entry (`supported` / `partial` / `not_supported`), its `operation` + `params`, and any caveats. Read-only. | `index` | +| `rollback_plan_get` | Inspect the reversal plan for one `action_log` entry (`supported` / `partial` / `not_supported`), its `operation` + `params`, and any caveats — or, with `batch_id` instead, for a whole batch (see below). Read-only. | exactly one of `index` / `batch_id` | | `rollback_apply` | Execute the reversal plan for `action_log[index]`. Requires `confirm=true` as a literal boolean. Appends a new log entry tagged `rollback_of=`. | `index`, `confirm` | Both tools accept an optional `state_file` argument (default `STATE.json`), which is resolved strictly inside the MCP server's current working directory. Path traversal, symlink escape, and `rollback.*` self-recursion are all refused. A second apply of the same index is refused (idempotency is enforced by scanning later log entries for a matching `rollback_of` marker). Downstream SDK exceptions are logged server-side only; the MCP response returns a generic message so tokens and account identifiers cannot leak into model context. +#### Planning a whole batch (#549) + +`rollback_plan_get` with `batch_id` returns one plan covering **every** member of that batch (see [Batch](#batch) below for how membership is declared), so a bulk pass is reviewed as one unit instead of entry by entry: + +| Field | Meaning | +|-------|---------| +| `coverage` | `full` / `partial` / `none` / `empty` — how much of the batch a reversal would actually restore | +| `platform_coverage` | The same verdict **per platform key**, because reversibility is not uniform across platforms | +| `counts` | Members per verdict (`reversible`, `reversible_with_caveats`, `irreversible`, `nothing_to_reverse`, `already_reversed`, `total`) | +| `apply_order` | The reversible members' `action_log` indices, newest first — the order to feed `rollback_apply` | +| `members[]` | Every member with its `index`, `platform`, `reversibility` verdict, the `reason` when it cannot be reversed, and its `operation` / `params` / `caveats` when it can | + +The point of the response is the part that is **not** reversible. A batch where 60 of 80 members can be restored reports `coverage: "partial"` with the other 20 listed and explained, before anything is applied — a revert whose completeness the operator cannot verify leaves them unable to rule their own fix out as a variable. + +`rollback_plan_get` is read-only and `rollback_apply` still takes one `index` at a time, so applying a batch reversal is a loop over `apply_order` — each call re-entering the same policy gate as a forward action. There is deliberately no "apply the whole batch" call: a single result code for 80 dispatches would have to summarize partial failure, which is the reporting problem this feature exists to remove. + +### Batch + +Declare the boundary of a bulk change so it becomes one reviewable, plannable unit in `action_log`. A bulk pass is many tool calls and nothing in a single call says which others belong with it, so the boundary is declared rather than guessed. + +| Tool | Description | Required Parameters | +|------|-------------|-------------------| +| `mureo_batch_begin` | Open a batch. Every `action_log` entry recorded until it is closed is tagged with the returned `batch_id`. Refused if one is already open. | `label` | +| `mureo_batch_end` | Close the open batch and return its exact membership (`member_indices`, `member_count`, `platforms`). Closing is **final**. Refused if none is open. | *(none)* | +| `mureo_batch_status` | Report which batch is collecting (or `null`), how many members it holds, which platforms they span, and a `warning` when it has been open too long. Read-only. | *(none)* | + +**Membership cannot be forged or grown after the fact.** `mureo_state_action_log_append` accepts an optional `batch_id`, but it is validated, not trusted: it must name a batch that was actually declared and is still open. An unknown id is refused (an id naming no batch is a typo or a fabrication, not a change set), and a **closed** batch is refused too — `mureo_batch_end` reports a `member_count` the operator keeps, and a membership that can still grow afterwards makes that number silently false. To group imported or backfilled history, open a batch for the import rather than reattaching to an old one. + +**A forgotten `mureo_batch_end` announces itself.** A missed `begin` yields no batch, which is obvious and harmless; a missed `end` yields a batch that keeps swallowing unrelated changes for days and then reports them, confidently, as one unit. After 24 hours open, `mureo_batch_status` returns a `warning`, and one is appended to the result of every mutating tool call so the agent that forgot is told without having to ask. mureo never closes a batch for you — an automatic timeout would trade a visible wrong answer for an invisible one. Suppress the appended reminder with `MUREO_DISABLE_BATCH_REMINDER=1`. + +Membership is stamped where every recording path already converges (`append_action_log`), not through tool arguments — which is what makes it work for platforms whose tool schemas mureo does not own. What that means per platform: + +| Platform kind | Joins a batch | Reversal of a member | +|---------------|---------------|----------------------| +| Native Google Ads / Meta Ads | Yes. Status toggles are recorded automatically; **every other mutation** (budget, keywords, placement exclusions, …) joins only if the agent records it with `mureo_state_action_log_append` | Executed, for the allow-listed operations | +| Bridged / plugin (`plugin::`, e.g. Amazon Ads) | Yes — successful mutations are promoted to `action_log` automatically | Recorded for visibility; executed only when the reversal names a *registered* plugin tool. Otherwise the member is reported `irreversible` with the reason | +| Hosted connectors (`tiktok_ads`) | Yes, for entries recorded with `mureo_state_action_log_append` — mureo is not in the data path, so nothing is automatic | Not executed by design. Members are reported `irreversible`, so the batch plan is an accurate manual checklist | +| Search Console | **No.** Its mutations (`sitemaps_submit`) are not recorded in `action_log` at all, so there is nothing to group | n/a | + +Batch state lives in STATE.json (`batches`), not in process memory, so a host that restarts the MCP server mid-pass does not silently stop collecting members. Records are kept after close (with `ended_at`) so a `batch_id` still resolves to the operator's own label weeks later. + ### Analysis Cross-platform analysis that operates on data the caller supplies — an `action_log` history, ad records, a delivery report — rather than on a platform API the tool picks for itself. `analysis_anomalies_check` and `analysis_tracking_consistency_check` reach no platform API at all, so they behave identically for native, plugin, bridged and hosted-connector platforms; `analysis_exclusion_impact_preview` issues one read of the account's own report unless the caller supplies `delivery_records`. diff --git a/docs/strategy-context.md b/docs/strategy-context.md index db9cf75..00cc597 100644 --- a/docs/strategy-context.md +++ b/docs/strategy-context.md @@ -409,6 +409,7 @@ detail view shows it, labelled as the document sync it is. | `last_synced_at` | `string \| null` | ISO 8601 timestamp of last sync | | `platforms` | `object \| null` | Per-platform state (v2) | | `action_log` | `array` | Log of actions with outcome tracking | +| `batches` | `array` | Declared bulk change sets (see below). Absent until the first `mureo_batch_begin` | | `customer_id` | `string \| null` | Legacy v1 field (kept for backward compatibility) | | `campaigns` | `array` | Legacy v1 field (kept for backward compatibility) | @@ -455,9 +456,23 @@ Each entry in `action_log` records an action taken by a workflow command, with o | `summary` | `string` | No | Human-readable summary | | `metrics_at_action` | `object` | No | Key metrics at the time of action (e.g., `{"cpa": 5200, "conversions": 45}`) | | `observation_due` | `string` | No | ISO 8601 date when the outcome should be evaluated (e.g., `"2026-04-15"`) | +| `batch_id` | `string` | No — server-stamped | The bulk change set this action belongs to. Stamped automatically while a batch is open (see below). You may supply it as an explicit assertion, but it is **validated**: it must name a declared batch that is still open, so membership can neither be invented nor added to a batch already closed. Absent means the action was standalone | The `metrics_at_action` and `observation_due` fields enable evidence-based outcome evaluation. When an action's observation window has passed, the agent compares current metrics against `metrics_at_action` to assess the action's impact. See `skills/_mureo-learning/SKILL.md` for the evidence-based decision framework. +#### Batch Record + +Each entry in `batches` is one **declared** bulk change set (#549). A bulk pass is many tool calls and nothing in a single call says which others belong with it, so the boundary is declared with `mureo_batch_begin` / `mureo_batch_end` rather than inferred; every `action_log` entry written in between carries the batch's `batch_id`. `rollback_plan_get` then takes that id and reports the reversibility of **every** member — including the ones it cannot reverse — before anything is applied. + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `batch_id` | `string` | Yes | The id stamped onto member `action_log` entries | +| `label` | `string` | Yes | What the change set is, in the operator's words | +| `started_at` | `string` | No — server-stamped | ISO 8601 timestamp with UTC offset | +| `ended_at` | `string` | No — server-stamped | When the batch was closed. **Absent means the batch is open** and still collecting; at most one may be open. Once set, membership is final — no later entry can join | + +The record is kept after the batch closes rather than deleted, so a `batch_id` found in `action_log` still resolves to its label later. What can join a batch differs by platform — native non-status mutations must be recorded by the agent, and Search Console mutations are not recorded at all — see [`docs/mcp-server.md`](mcp-server.md#batch). + ### Python API ```python diff --git a/mureo/_data/skills/_mureo-shared/SKILL.md b/mureo/_data/skills/_mureo-shared/SKILL.md index 598a41e..0e409bd 100644 --- a/mureo/_data/skills/_mureo-shared/SKILL.md +++ b/mureo/_data/skills/_mureo-shared/SKILL.md @@ -107,6 +107,7 @@ Skills and commands describe "Read STRATEGY.md", "Update STATE.json", and "Appen | Read STATE.json | `Read` tool | `mureo_state_get` MCP tool | | Establish the current date | `mureo_state_get` MCP tool (`server_now`) | `mureo_state_get` MCP tool (`server_now`) | | Append action_log entry | `mureo_state_action_log_append` MCP tool | `mureo_state_action_log_append` MCP tool | +| Group a bulk change as one unit | `mureo_batch_begin` / `mureo_batch_end` MCP tools | `mureo_batch_begin` / `mureo_batch_end` MCP tools | | Upsert campaign snapshot | `mureo_state_upsert_campaign` MCP tool | `mureo_state_upsert_campaign` MCP tool | When you don't have direct filesystem tools (Desktop / Cowork / web), always reach for the corresponding `mureo_*` MCP tool — they encode the same atomic-write semantics so you can't corrupt the file mid-edit. @@ -206,6 +207,29 @@ The MCP server exposes tools for Google Ads, Meta Ads, and Search Console over s Once configured, the AI agent can call `google_ads_campaigns_list` or `meta_ads_campaigns_list` to verify the connection is working. +## Bulk changes are one revertible unit + +Any pass that changes **more than one entity** — N placement/app exclusions, N keywords, N ad status changes, a pause across several campaigns — must be wrapped: + +1. `mureo_batch_begin` with a `label` in the operator's words (e.g. `"exclude low-quality display placements"`). It returns a `batch_id`. +2. Do the work. Every `action_log` entry recorded until you close the batch is tagged with that id automatically — **on every platform**, whether the entry came from a native status toggle, from a bridged/plugin tool mureo promoted, or from your own `mureo_state_action_log_append` call. +3. `mureo_batch_end`. It returns the exact member list (`member_indices`, `platforms`). **Report the `batch_id` and the member count to the operator** — that is the record which removes any later need to reconstruct the change set from memory. Closing is final: nothing can join afterwards, so that count stays true. + +**Close it.** A missed `begin` yields no batch and is harmless; a missed `end` yields a batch that keeps swallowing every later change — including work from another session entirely — and then reports the lot as one unit. If a batch has been open more than a day, mureo appends a warning to mutating tool results and to `mureo_batch_status`; when you see it, either close the batch or tell the operator it is still open. mureo will not close it for you. Never pass a `batch_id` you did not get from `mureo_batch_begin` in this session: an unknown id, or one whose batch is closed, is refused. + +Then `rollback_plan_get` with `batch_id` (instead of `index`) plans the whole thing: `coverage` (`full` / `partial` / `none`), `platform_coverage`, per-member verdicts, and `apply_order`. + +**Report coverage honestly and BEFORE applying anything.** If a batch of 80 reports 60 reversible and 20 `irreversible`, say exactly that, name the irreversible members and their `reason`, and say which platform they are on — do not describe the revert as complete. `rollback_apply` still takes one `index` at a time; walk `apply_order` in the order given (newest first). + +What can join a batch, and how far a reversal can actually go, differs by platform — state the limit rather than implying uniform coverage: + +- **Native `google_ads_*` / `meta_ads_*`** — status toggles are recorded for you. **Every other mutation** (budget, keywords, exclusions, creative) is recorded only if YOU call `mureo_state_action_log_append`; without that it is not in the batch and not in the plan. +- **Plugin / bridged platforms** (`plugin::`, e.g. Amazon Ads) — successful mutations join automatically. A reversal is executed only when the hint names a registered plugin tool; otherwise the member is reported `irreversible` with the reason, and reversing it is manual. +- **Hosted connectors** (`tiktok_ads`) — join only through your own `mureo_state_action_log_append` calls, and their reversal is never executed by mureo. The batch plan is still worth having: it is an accurate manual checklist instead of a memory exercise. +- **Search Console** — its mutations are not recorded in `action_log` at all, so they cannot join a batch today. + +If you cannot open a batch (older mureo without the tools), say so and record each entry individually — do not silently do a bulk pass with no grouping. + ## Security Rules > CRITICAL: AI agents MUST follow these rules when using mureo tools. @@ -239,6 +263,7 @@ When pausing or removing multiple entities: - List all affected entities with their current performance - Show total impact (e.g., "This will pause 5 campaigns with 1,200 clicks/day") - Require explicit confirmation +- **Wrap the whole pass in a batch** so it can be reviewed and reverted as ONE unit — see *Bulk changes are one revertible unit* above ### 3b. Bulk Exclusions: Size Them Before Applying (#547) @@ -460,6 +485,12 @@ shows fewer campaigns than you wrote — get these exact names right: not-recently-synced. `mureo_state_upsert_campaign` / `_platform_metrics_set` / `_report_set` set it for you (`_action_log_append` does not); on the Code `Write` path you must set it yourself. +- **Top-level `batches`** (declared bulk change sets, #549) — **carry it over + verbatim** on the Code `Write` path, together with each `action_log` entry's + `batch_id`. Dropping either detaches a change set from its members, which is + precisely the "reconstruct what I did from memory" state batches exist to + remove. A record with no `ended_at` is an OPEN batch; do not invent, close or + renumber one by hand — use `mureo_batch_begin` / `mureo_batch_end`. Canonical STATE.json shape (note `campaign_name`, `account_id`, `last_synced_at`): diff --git a/mureo/_data/skills/search-term-cleanup/SKILL.md b/mureo/_data/skills/search-term-cleanup/SKILL.md index 97b2273..3184c48 100644 --- a/mureo/_data/skills/search-term-cleanup/SKILL.md +++ b/mureo/_data/skills/search-term-cleanup/SKILL.md @@ -56,10 +56,14 @@ Review and clean up search terms and keywords across all platforms. 9. **Check pending observations**: Before executing, check `action_log` for this campaign. If a previous action is still within its observation window, warn that stacking changes will make outcome evaluation difficult. Recommend waiting if possible. -10. **Execute**: Use each platform's keyword management tools to apply approved changes (add negative keywords, add positive keywords, adjust bids). +10. **Open a batch**: call `mureo_batch_begin` with a `label` naming this pass (e.g. `"search-term cleanup 2026-08-07"`). A cleanup changes many entities across possibly several platforms, and the batch is what makes it one reviewable, plannable unit instead of a set of entries nobody can re-identify later. See `../_mureo-shared/SKILL.md` → *Bulk changes are one revertible unit*. -11. **Record outcome context**: For each campaign modified, log to `action_log` with `metrics_at_action` (current CPA, conversions, clicks, CTR, impressions, cost) and `observation_due` (14 days from `server_now`'s date). This enables evidence-based evaluation later. +11. **Execute**: Use each platform's keyword management tools to apply approved changes (add negative keywords, add positive keywords, adjust bids). -12. **Update STATE.json** with notes about the cleanup. +12. **Record outcome context**: For each campaign modified, log to `action_log` with `metrics_at_action` (current CPA, conversions, clicks, CTR, impressions, cost) and `observation_due` (14 days from `server_now`'s date). This enables evidence-based evaluation later. Keyword and negative-keyword changes are **not** auto-recorded, so this step is what puts them in the batch at all — an entry you do not append is invisible to any later revert. + +13. **Close the batch**: call `mureo_batch_end` and report the returned `batch_id` and member count to the operator, so undoing this pass later is `rollback_plan_get` with that id rather than a reconstruction from memory. + +14. **Update STATE.json** with notes about the cleanup. IMPORTANT: Always explain WHY a term should be excluded/added, referencing the Persona or USP from STRATEGY.md. Consult past action_log entries — if a similar cleanup was previously evaluated, reference whether it was effective. diff --git a/mureo/context/__init__.py b/mureo/context/__init__.py index da7183a..61d1e94 100644 --- a/mureo/context/__init__.py +++ b/mureo/context/__init__.py @@ -1,9 +1,17 @@ """mureo context -- File-based strategy context (STRATEGY.md / STATE.json).""" +from mureo.context.batch import ( + BatchError, + active_batch, + batch_members, + batch_platforms, + find_batch, +) from mureo.context.errors import ContextFileError from mureo.context.models import ( ActionLogEntry, AdState, + BatchRecord, CampaignSnapshot, PlatformState, StateDocument, @@ -11,6 +19,8 @@ ) from mureo.context.state import ( append_action_log, + begin_batch, + end_batch, get_campaign, parse_state, read_state_file, @@ -30,9 +40,16 @@ __all__ = [ # errors "ContextFileError", + # batch (#549) + "BatchError", + "active_batch", + "batch_members", + "batch_platforms", + "find_batch", # models "ActionLogEntry", "AdState", + "BatchRecord", "CampaignSnapshot", "PlatformState", "StateDocument", @@ -46,6 +63,8 @@ "write_strategy_file", # state "append_action_log", + "begin_batch", + "end_batch", "get_campaign", "parse_state", "read_state_file", diff --git a/mureo/context/batch.py b/mureo/context/batch.py new file mode 100644 index 0000000..1207695 --- /dev/null +++ b/mureo/context/batch.py @@ -0,0 +1,283 @@ +"""A bulk change as one named unit in ``action_log`` (#549). + +The pure half of the batch feature: id minting, the stamping rule, and the +membership queries. The mutators that open and close a batch live in +:mod:`mureo.context.state` with the rest of STATE.json's targeted mutators — +they need its file lock, and putting them here would close an import cycle. + +**Where the boundary comes from.** A bulk pass is many tool calls, and no +single call carries a signal about which other calls belong with it. Inferring +one (same minute, same campaign, same tool) would be a heuristic, and a +heuristic that silently omits a member re-creates the exact failure this module +exists to prevent — an operator reconstructing a change set from memory. So the +boundary is **declared**: ``mureo_batch_begin`` opens a batch, every +``action_log`` write until ``mureo_batch_end`` belongs to it. + +**Why the stamp is applied at the append choke point.** Every platform mureo +drives — native Google/Meta, hosted connectors recorded through +``mureo_state_action_log_append``, and bridged / plugin tools promoted by +:func:`mureo.mcp.plugin_semantics.record_mutation_action_log` — reaches +``action_log`` through :func:`mureo.context.state.append_action_log`. Stamping +there is what makes batch membership platform-agnostic with no per-platform +code and no ABI change: a bridged tool's arguments belong to the bridged +platform, not to mureo, so threading a ``batch_id`` through tool schemas would +work for native tools only. + +**Known limit.** The batch lifecycle tools resolve STATE.json through the +active :class:`StateStore` (via +:func:`mureo.mcp._helpers.resolve_workspace_path`), while the native and plugin +recorders write to ``Path.cwd() / "STATE.json"`` directly — a pre-existing +asymmetry, not one introduced here. They coincide in the default file-backed +configuration, which is every OSS install; under an alternate +``mureo.runtime_context_factory`` backend that points elsewhere, the two would +address different files and automatic membership would not apply. Anything +recorded through ``mureo_state_action_log_append`` uses the store path and is +unaffected. + +Not to be confused with :class:`mureo.amazon_ads.batch.SessionBatch`, which is +a transport concern (one Amazon session for a sequence of calls) and has +nothing to do with ``action_log`` membership. +""" + +from __future__ import annotations + +import secrets +from dataclasses import replace +from datetime import datetime, timezone +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from mureo.context.models import ActionLogEntry, BatchRecord, StateDocument + + +class BatchError(Exception): + """A batch lifecycle call that cannot be honoured. + + Raised for opening a batch while one is already open, for closing one when + none is, and for naming a batch that does not exist or has already closed. + All are refused rather than resolved silently: flattening a nested begin + would merge two change sets an operator meant to keep apart, a no-op end + would report a batch that never collected anything, and an unchecked id + would let membership be invented or grown after the fact. + """ + + +#: Number of random bytes in the id suffix. Ids are workspace-local names, not +#: secrets — this is collision avoidance across same-second batches, nothing +#: more. +_ID_ENTROPY_BYTES = 4 + +#: How long a batch may stay open before it is reported as stale. A bulk pass +#: is one working session; a batch still open a day later has almost certainly +#: been forgotten rather than deliberately kept. The threshold only controls +#: when mureo *says something* — nothing closes a batch automatically, because +#: an auto-close would trade a visible wrong answer for an invisible one: the +#: entries after it would silently stop joining and no one would be told. +STALE_AFTER_HOURS = 24 + + +def new_batch_id(started_at: str = "") -> str: + """Mint a batch id. + + Prefixed with the start timestamp's date-time when one is supplied, because + an operator reads these ids in tool output and types them back into + ``rollback_plan_get`` — ``batch-20260807T101500-1f4c9a02`` says which pass + it was; a bare UUID does not. Uniqueness comes from the random suffix, not + from the timestamp, so two batches opened in the same second are still + distinct. + """ + suffix = secrets.token_hex(_ID_ENTROPY_BYTES) + stamp = "".join(ch for ch in started_at[:19] if ch.isdigit() or ch == "T") + return f"batch-{stamp}-{suffix}" if stamp else f"batch-{suffix}" + + +def active_batch(doc: StateDocument) -> BatchRecord | None: + """Return the workspace's open batch, or ``None``. + + "Open" is ``ended_at is None``. :func:`mureo.context.state.begin_batch` + refuses to open a second one, so there is at most one; if a hand-edited + file somehow holds several, the most recently declared wins — the same + one a fresh ``begin_batch`` would have refused to displace. + """ + for record in reversed(doc.batches): + if record.ended_at is None: + return record + return None + + +def find_batch(doc: StateDocument, batch_id: str) -> BatchRecord | None: + """Return the record for ``batch_id``, open or closed, or ``None``.""" + wanted = batch_id.strip() + for record in doc.batches: + if record.batch_id == wanted: + return record + return None + + +def ensure_joinable(doc: StateDocument, batch_id: str) -> BatchRecord: + """Return the record ``batch_id`` names, or raise if it cannot be joined. + + A batch id supplied by a caller is untrusted, and membership is the one + thing this whole feature asks the operator to trust. Two refusals: + + - **Unknown id.** An id that names no declared batch is a typo or a + fabrication, not a batch. Accepting it would let an entry manufacture a + change set out of nothing, which ``rollback_plan_get`` would then report + as a legitimate — and, having no record, unlabelled — unit. + - **Closed batch.** ``mureo_batch_end`` reports a ``member_count`` the + operator keeps. If a later append could still join, that number silently + stops being true, and a change set whose membership drifts after it was + reported is exactly the "confidently wrong" state this feature exists to + remove. Closing is final. + + Backfill and import (#545) therefore do not reattach to a closed batch: + they declare their own with ``begin_batch``, which gives the imported set + an honest label and start time instead of retrofitting someone else's. + """ + record = find_batch(doc, batch_id) + if record is None: + raise BatchError( + f"Unknown batch_id {batch_id.strip()!r}. Open a batch with " + "mureo_batch_begin; an id that names no declared batch cannot be " + "joined." + ) + if record.ended_at is not None: + raise BatchError( + f"Batch {record.batch_id!r} closed at {record.ended_at}; its " + "membership is final and was already reported. Open a new batch " + "for further changes." + ) + return record + + +def _parse_iso(value: str) -> datetime | None: + """Parse an ISO 8601 timestamp, or ``None`` when it is unusable. + + Naive values are read as UTC so a hand-edited or legacy record still + compares against ``now`` instead of raising. + """ + try: + parsed = datetime.fromisoformat(value) + except (TypeError, ValueError): + return None + return parsed if parsed.tzinfo else parsed.replace(tzinfo=timezone.utc) + + +def batch_open_hours(record: BatchRecord, now: datetime | None = None) -> float | None: + """How long ``record`` has been open, or ``None`` if that is unknowable. + + ``None`` for a closed batch and for one whose ``started_at`` cannot be + parsed — an unknown age must not be reported as a small one. + + ``now`` defaults to :func:`mureo.core.clock.server_now`, the one clock seam + (#460): reaching for ``datetime.now`` directly would leave the production + path outside the seam every test freezes, so a drift there would go + unnoticed. Resolved through the MODULE (``clock.server_now()``) rather than + a bound name, which is what keeps + ``monkeypatch.setattr(mureo.core.clock, "server_now", …)`` effective. + + The import is deliberately lazy: ``mureo.core.__init__`` → ``runtime_context`` + → ``state_store`` → ``mureo.context.state`` → this module is a real import + chain, so reaching ``mureo.core`` at module load would close the cycle. + """ + if record.ended_at is not None: + return None + started = _parse_iso(record.started_at) + if started is None: + return None + if now is None: + from mureo.core import clock + + now = clock.server_now() + current = now if now.tzinfo else now.replace(tzinfo=timezone.utc) + return (current - started).total_seconds() / 3600.0 + + +def stale_batch_warning( + record: BatchRecord | None, now: datetime | None = None +) -> str | None: + """Warn that a batch has been open too long, or ``None``. + + The asymmetry this exists for: a missed ``begin`` yields no batch, which is + obvious and harmless. A missed ``end`` yields a batch that keeps swallowing + unrelated changes for days and then reports them, confidently, as one + reviewable unit — a wrong answer that looks like a right one. So the open + batch has to announce itself; it is never closed on the operator's behalf. + """ + if record is None: + return None + hours = batch_open_hours(record, now) + if hours is None or hours < STALE_AFTER_HOURS: + return None + return ( + f"Batch {record.batch_id!r} ({record.label!r}) has been open for " + f"{hours:.0f}h. Every action_log entry recorded since it opened has " + "joined it, including any unrelated to that change set. If the bulk " + "pass is finished, close it with mureo_batch_end; mureo will not close " + "it for you." + ) + + +def stamp_batch(entry: ActionLogEntry, batch: BatchRecord | None) -> ActionLogEntry: + """Return ``entry`` bound to ``batch``, or unchanged. + + An explicit ``batch_id`` already on the entry always wins: it is how an + imported or backfilled record keeps the batch it actually belonged to, + which must not be overwritten by whatever happens to be open now. + + **Never rebuild the entry field-by-field here.** ``dataclasses.replace`` + changes ``batch_id`` and carries everything else across by construction; an + enumerated constructor silently drops any field added to + :class:`ActionLogEntry` after this function was written, and joining a + batch is the DEFAULT path (``join_active_batch=True``), so the loss would + hit ordinary appends. The failure is silent — a dropped field reads as + "the caller did not set it" — so nothing downstream would flag it. + + That is not hypothetical: an enumerated version of this function dropped + the provenance fields (``origin`` / ``external_id``), and because + ``is_external`` is derived from ``origin``, an externally-imported entry + lost the very marker that stops a forged ``reversible_params`` from being + planned as a reversal. + """ + if batch is None or entry.batch_id is not None: + return entry + return replace(entry, batch_id=batch.batch_id) + + +def batch_members( + doc: StateDocument, batch_id: str +) -> tuple[tuple[int, ActionLogEntry], ...]: + """Return ``(index, entry)`` for every member of ``batch_id``, in log order. + + The index is the position in the FULL append-only log — the same index + ``rollback_apply`` and ``rollback_of`` use — so a caller holding only the + batch can still address each member individually. + """ + wanted = batch_id.strip() + if not wanted: + return () + return tuple( + (index, entry) + for index, entry in enumerate(doc.action_log) + if entry.batch_id == wanted + ) + + +def batch_platforms(doc: StateDocument, batch_id: str) -> tuple[str, ...]: + """Distinct platform keys represented in ``batch_id``, sorted.""" + return tuple(sorted({entry.platform for _, entry in batch_members(doc, batch_id)})) + + +__all__ = [ + "STALE_AFTER_HOURS", + "BatchError", + "active_batch", + "batch_members", + "batch_open_hours", + "batch_platforms", + "ensure_joinable", + "find_batch", + "new_batch_id", + "stale_batch_warning", + "stamp_batch", +] diff --git a/mureo/context/conversion_overrides.py b/mureo/context/conversion_overrides.py index 6d59a74..f48e409 100644 --- a/mureo/context/conversion_overrides.py +++ b/mureo/context/conversion_overrides.py @@ -41,7 +41,8 @@ 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 + Mirrors the default resolution of + :func:`mureo.mcp._helpers.resolve_workspace_path` (``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 diff --git a/mureo/context/models.py b/mureo/context/models.py index 2dd350d..20a6b8f 100644 --- a/mureo/context/models.py +++ b/mureo/context/models.py @@ -89,6 +89,35 @@ def __post_init__(self) -> None: object.__setattr__(self, "metrics", copy.deepcopy(self.metrics)) +@dataclass(frozen=True) +class BatchRecord: + """One declared bulk change set (#549). + + A bulk change is normally many tool calls, and nothing in a single call + tells mureo which other calls belong with it. So the boundary is + **declared**: ``mureo_batch_begin`` opens a record, every ``action_log`` + write until ``mureo_batch_end`` is stamped with its ``batch_id``, and the + rollback plan for that id covers exactly those entries. + + Stored in STATE.json rather than in process memory because the MCP server + can be restarted by its host between two calls of the same operator + session, and a batch that silently stopped collecting members is the + failure mode this whole feature exists to prevent. + + The record outlives the batch: ``ended_at`` is set on close rather than the + record being deleted, so ``label`` is still there weeks later when the + operator asks what a batch id actually was. ``ended_at is None`` means the + batch is open, and at most one record may be open at a time. + + Both timestamps are stamped SERVER-side (the #460 rule). + """ + + batch_id: str + label: str + started_at: str + ended_at: str | None = None + + @dataclass(frozen=True) class ActionLogEntry: """Immutable record of a single action performed on a campaign. @@ -124,6 +153,16 @@ class ActionLogEntry: entry with ``evaluation_of=`` records that the outcome was reviewed and takes the source out of the pending set. ``None`` means this entry is not an evaluation record. + batch_id: The logical batch this action was dispatched as part of (#549), or + ``None`` for a standalone action — which every entry written before this + field existed is, so old STATE.json files parse unchanged and gain no new + key on the next write. Membership is what makes "undo what I did on + Monday" expressible: ``rollback_plan_get`` takes the id and reports the + reversibility of EVERY member before anything is applied, so the operator + never has to reconstruct a change set from memory. Stamped automatically + by :func:`mureo.context.state.append_action_log` from the workspace's open + batch, so a native, hosted-connector and bridged/plugin mutation all join + the same unit without any per-platform code. """ timestamp: str @@ -142,9 +181,15 @@ class ActionLogEntry: # compatibility for third-party callers of this public dataclass. entity_type: str | None = None entity_id: str | None = None + # Appended after every pre-#549 field, same positional-compatibility rule. + batch_id: str | None = None def __post_init__(self) -> None: """Take defensive copies of mutable dict fields.""" + if self.batch_id is not None: + if not isinstance(self.batch_id, str) or not self.batch_id.strip(): + raise ValueError("batch_id must be a non-empty string") + object.__setattr__(self, "batch_id", self.batch_id.strip()) if (self.entity_type is None) != (self.entity_id is None): raise ValueError("entity_type and entity_id must be provided together") if self.entity_type is not None and self.entity_id is not None: @@ -237,6 +282,10 @@ class StateDocument: # skill writes it yet. Optional with a None default so old STATE.json # files parse unchanged and emit no extra key. reports: dict[str, Any] | None = None + # Declared bulk change sets (#549), open and closed. Empty by default and + # emitted only when non-empty, so a STATE.json written before this field + # existed parses unchanged and gains no new key. + batches: tuple[BatchRecord, ...] = field(default_factory=tuple) def __post_init__(self) -> None: """Defensive copies for mutable fields.""" @@ -244,5 +293,7 @@ def __post_init__(self) -> None: object.__setattr__(self, "platforms", dict(self.platforms)) if not isinstance(self.action_log, tuple): object.__setattr__(self, "action_log", tuple(self.action_log)) + if not isinstance(self.batches, tuple): + object.__setattr__(self, "batches", tuple(self.batches)) if self.reports is not None: object.__setattr__(self, "reports", copy.deepcopy(self.reports)) diff --git a/mureo/context/state.py b/mureo/context/state.py index 9ec1c91..df23c67 100644 --- a/mureo/context/state.py +++ b/mureo/context/state.py @@ -35,9 +35,17 @@ from mureo.context.models import ActionLogEntry, CampaignSnapshot +from mureo.context.batch import ( + BatchError, + active_batch, + batch_members, + ensure_joinable, + new_batch_id, + stamp_batch, +) from mureo.context.conversion_overrides import load_conversion_action_types from mureo.context.errors import ContextFileError -from mureo.context.models import PlatformState, StateDocument +from mureo.context.models import BatchRecord, PlatformState, StateDocument from mureo.context.platform_guards import ( guard_platform_entry_write, warn_on_duplicate_accounts, @@ -298,25 +306,137 @@ def _build(doc: StateDocument) -> StateDocument: return _locked_state_mutation(path, _build) -def append_action_log(path: Path, entry: ActionLogEntry) -> StateDocument: +def append_action_log( + path: Path, entry: ActionLogEntry, *, join_active_batch: bool = True +) -> StateDocument: """Append an action log entry to STATE.json. Reads the current state, appends the entry, and writes back atomically. + This is the single choke point every recording path funnels through — + native status toggles, ``mureo_state_action_log_append``, and the + bridged / plugin promotion — so it is also where the workspace's open batch + is stamped onto the entry (#549). Doing it here, inside the lock, is what + makes batch membership platform-agnostic: no tool schema, no per-platform + recorder and no plugin ABI has to know the batch exists. + + An explicit ``batch_id`` on the entry is **validated, not trusted**: it must + name a batch that exists and is still open + (:func:`mureo.context.batch.ensure_joinable`). Checking here rather than in + the MCP handler means no caller — handler, library user or future recorder + — can invent a change set or grow one after it was closed and reported. + + Args: + path: STATE.json location. + entry: The entry to append. An explicit ``batch_id`` on it wins over the + open batch (see :func:`mureo.context.batch.stamp_batch`) once it has + passed :func:`~mureo.context.batch.ensure_joinable`. + join_active_batch: Pass ``False`` for an entry that must NOT become a + member of whatever batch is open — the rollback executor's + ``rollback_of`` record does, because a reversal joining the batch + it reverses would grow that batch and make the next plan offer the + reversals as things still to reverse. + Returns: Updated StateDocument + + Raises: + BatchError: ``entry.batch_id`` names no declared batch, or names one + that has already been closed. """ def _build(doc: StateDocument) -> StateDocument: + if entry.batch_id is not None: + ensure_joinable(doc, entry.batch_id) + stamped = stamp_batch(entry, active_batch(doc)) if join_active_batch else entry # ``last_synced_at`` is deliberately NOT re-stamped: appending an action # is not a sync, and the dashboard's "Synced N ago" freshness must keep # reflecting the last real sync. Every other section is carried over by - # ``replace``. - return replace(doc, action_log=(*doc.action_log, entry)) + # ``replace`` — including ``batches`` (#549), which is why this no longer + # enumerates the untouched fields by hand. + return replace(doc, action_log=(*doc.action_log, stamped)) return _locked_state_mutation(path, _build) +def begin_batch(path: Path, *, label: str) -> BatchRecord: + """Open a batch: every later ``action_log`` entry joins it until it is ended. + + Args: + path: STATE.json location. + label: What this change set is, in the operator's words. Required and + non-blank — an unlabelled batch id is a string the operator has to + decode later, which is the reconstruction work #549 removes. + + Returns: + The opened :class:`~mureo.context.models.BatchRecord`. + + Raises: + BatchError: A batch is already open. Refused rather than nested: two + change sets merged into one cannot be told apart afterwards. + ValueError: ``label`` is blank. + """ + cleaned = label.strip() if isinstance(label, str) else "" + if not cleaned: + raise ValueError("label must be a non-empty string") + + opened: list[BatchRecord] = [] + + def _build(doc: StateDocument) -> StateDocument: + open_batch = active_batch(doc) + if open_batch is not None: + raise BatchError( + f"Batch {open_batch.batch_id!r} ({open_batch.label!r}) is already " + "open; end it before beginning another." + ) + started_at = _now_iso() + batch = BatchRecord( + batch_id=new_batch_id(started_at), + label=cleaned, + started_at=started_at, + ) + opened.append(batch) + return replace(doc, batches=(*doc.batches, batch)) + + _locked_state_mutation(path, _build) + return opened[0] + + +def end_batch(path: Path) -> tuple[BatchRecord, tuple[int, ...]]: + """Close the open batch and report exactly what it collected. + + The record is kept (with ``ended_at`` set) rather than deleted, so the + batch's label still answers "what was batch-2026… ?" long after the pass. + + Returns: + The closed :class:`~mureo.context.models.BatchRecord` and the + ``action_log`` indices of its members — the checklist that replaces + reconstructing the change set by hand. + + Raises: + BatchError: No batch is open. + """ + closed: list[tuple[BatchRecord, tuple[int, ...]]] = [] + + def _build(doc: StateDocument) -> StateDocument: + open_batch = active_batch(doc) + if open_batch is None: + raise BatchError("No batch is open.") + ended = replace(open_batch, ended_at=_now_iso()) + closed.append( + (ended, tuple(index for index, _ in batch_members(doc, ended.batch_id))) + ) + return replace( + doc, + batches=tuple( + ended if b.batch_id == ended.batch_id else b for b in doc.batches + ), + ) + + _locked_state_mutation(path, _build) + return closed[0] + + def set_report(path: Path, report: str, summary: dict[str, Any]) -> StateDocument: """Persist a structured analysis ``summary`` into STATE.json ``reports``. @@ -511,6 +631,8 @@ def get_campaign(doc: StateDocument, campaign_id: str) -> CampaignSnapshot | Non "render_state", # Defined here. "append_action_log", + "begin_batch", + "end_batch", "get_campaign", "read_state_file", "set_conversion_action_types", diff --git a/mureo/context/state_codec.py b/mureo/context/state_codec.py index 297ff60..4f7de5e 100644 --- a/mureo/context/state_codec.py +++ b/mureo/context/state_codec.py @@ -42,6 +42,7 @@ from mureo.context.models import ( ActionLogEntry, AdState, + BatchRecord, CampaignSnapshot, PlatformState, StateDocument, @@ -84,6 +85,7 @@ "platforms", "action_log", "reports", + "batches", } ), "parse_state / render_state", @@ -120,6 +122,7 @@ "reversible_params", "rollback_of", "evaluation_of", + "batch_id", } ), "_parse_action_log_entry / _action_log_entry_to_dict", @@ -148,6 +151,11 @@ frozenset({"ad_id", "name", "status", "effective_status", "as_of"}), "_parse_ad / _ad_state_to_dict", ), + ( + BatchRecord, + frozenset({"batch_id", "label", "started_at", "ended_at"}), + "_parse_batches / _batch_record_to_dict", + ), ) @@ -319,6 +327,40 @@ def _parse_conversion_action_types(raw: Any) -> tuple[str, ...] | None: return cleaned or None +def _parse_batches(raw: Any) -> tuple[BatchRecord, ...]: + """Parse the declared bulk change sets (#549). + + Tolerant in both modes, unlike the campaign / action_log lists: a batch + record is bookkeeping ABOUT history, not history itself — the members are + in ``action_log`` and their ``batch_id`` stands on its own. A malformed + record therefore costs a label, and dropping it is a far better trade than + letting a hand-edited entry blank a whole document. + """ + if not isinstance(raw, list): + return () + records: list[BatchRecord] = [] + for item in raw: + if not isinstance(item, dict): + logger.debug("skipping non-object batch record: %r", item) + continue + batch_id = item.get("batch_id") + if not isinstance(batch_id, str) or not batch_id.strip(): + logger.debug("skipping batch record without a usable batch_id: %r", item) + continue + label = item.get("label") + started_at = item.get("started_at") + ended_at = item.get("ended_at") + records.append( + BatchRecord( + batch_id=batch_id.strip(), + label=label if isinstance(label, str) else "", + started_at=started_at if isinstance(started_at, str) else "", + ended_at=ended_at if isinstance(ended_at, str) else None, + ) + ) + return tuple(records) + + def parse_state(text: str, *, strict: bool = True) -> StateDocument: """Parse a JSON string and return a StateDocument. @@ -373,6 +415,7 @@ def parse_state(text: str, *, strict: bool = True) -> StateDocument: platforms=platforms, action_log=action_log, reports=data.get("reports"), + batches=_parse_batches(data.get("batches")), ) @@ -386,6 +429,7 @@ def _parse_action_log_entry(e: dict[str, Any]) -> ActionLogEntry: ad_id=e.get("ad_id"), entity_type=e.get("entity_type"), entity_id=e.get("entity_id"), + batch_id=e.get("batch_id"), summary=e.get("summary"), command=e.get("command"), metrics_at_action=e.get("metrics_at_action"), @@ -445,6 +489,12 @@ def render_state(doc: StateDocument) -> str: if doc.reports is not None: data["reports"] = copy.deepcopy(doc.reports) + # Declared batches (#549): emit only when the workspace has any, so a + # document written before this field existed stays byte-stable on + # round-trip. ``ended_at`` is omitted while a batch is open. + if doc.batches: + data["batches"] = [_batch_record_to_dict(b) for b in doc.batches] + return json.dumps(data, ensure_ascii=False, indent=2) @@ -485,6 +535,8 @@ def _action_log_entry_to_dict(e: ActionLogEntry) -> dict[str, Any]: result["entity_type"] = e.entity_type if e.entity_id is not None: result["entity_id"] = e.entity_id + if e.batch_id is not None: + result["batch_id"] = e.batch_id if e.summary is not None: result["summary"] = e.summary if e.command is not None: @@ -502,6 +554,22 @@ def _action_log_entry_to_dict(e: ActionLogEntry) -> dict[str, Any]: return result +def _batch_record_to_dict(b: BatchRecord) -> dict[str, Any]: + """Convert a :class:`BatchRecord` to a dictionary. + + ``ended_at`` is emitted only once the batch is closed, so "open" is the + absence of the key rather than a null a reader could misparse as a time. + """ + result: dict[str, Any] = { + "batch_id": b.batch_id, + "label": b.label, + "started_at": b.started_at, + } + if b.ended_at is not None: + result["ended_at"] = b.ended_at + return result + + def _snapshot_to_dict(c: CampaignSnapshot) -> dict[str, Any]: """Convert a CampaignSnapshot to a dictionary.""" device_targeting: list[dict[str, Any]] | None = None diff --git a/mureo/mcp/_handlers_batch.py b/mureo/mcp/_handlers_batch.py new file mode 100644 index 0000000..884aaa2 --- /dev/null +++ b/mureo/mcp/_handlers_batch.py @@ -0,0 +1,137 @@ +"""MCP handlers for the ``mureo_batch_*`` tool family (#549). + +Three calls that declare the boundary of a bulk change: open one, close one, +ask which is open. Everything in between joins the batch automatically — +see :mod:`mureo.context.batch` for why the stamp is applied at the +``append_action_log`` choke point rather than through tool arguments. + +Path resolution uses the shared :func:`mureo.mcp._helpers.resolve_workspace_path`. +It is a security boundary — an MCP caller must not be able to point these at a +STATE.json outside the active workspace — and a second copy of that check is a +place for the two to drift apart silently. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from mureo.context.batch import ( + BatchError, + active_batch, + batch_members, + batch_platforms, + stale_batch_warning, +) +from mureo.context.state import begin_batch, end_batch, read_state_file +from mureo.mcp._helpers import _json_result, _require, resolve_workspace_path + +if TYPE_CHECKING: + from mcp.types import TextContent + + from mureo.context.models import BatchRecord + + +def _record_to_dict(record: BatchRecord) -> dict[str, Any]: + """Serialize a batch record; ``ended_at`` is absent while it is open.""" + payload: dict[str, Any] = { + "batch_id": record.batch_id, + "label": record.label, + "started_at": record.started_at, + } + if record.ended_at is not None: + payload["ended_at"] = record.ended_at + return payload + + +async def handle_batch_begin(arguments: dict[str, Any]) -> list[TextContent]: + """Open a batch. Refuses when one is already open.""" + label = _require(arguments, "label") + if not isinstance(label, str) or not label.strip(): + raise ValueError("label must be a non-empty string") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") + try: + record = begin_batch(path, label=label) + except BatchError as exc: + return _json_result({"status": "refused", "error": str(exc)}) + return _json_result({"status": "open", **_record_to_dict(record)}) + + +async def handle_batch_end(arguments: dict[str, Any]) -> list[TextContent]: + """Close the open batch and return exactly what it collected. + + The member indices are the point of the response: they are the checklist + that replaces reconstructing a change set from memory, and they are what + ``rollback_plan_get`` will report on next. Closing is final — no later + append can join a closed batch (see + :func:`mureo.context.batch.ensure_joinable`), so the ``member_count`` + returned here stays true. + """ + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") + try: + record, indices = end_batch(path) + except BatchError as exc: + return _json_result({"status": "refused", "error": str(exc)}) + doc = read_state_file(path) + return _json_result( + { + "status": "closed", + **_record_to_dict(record), + "member_count": len(indices), + "member_indices": list(indices), + "platforms": list(batch_platforms(doc, record.batch_id)), + } + ) + + +async def handle_batch_status(arguments: dict[str, Any]) -> list[TextContent]: + """Report the open batch (or ``null``) and how much it has collected. + + Carries the staleness ``warning`` when one has been open too long — the + pull half of the signal, for a caller who does think to ask. The push half + (a reminder appended to mutating tool results) is what reaches the caller + who does not; see :func:`maybe_build_batch_reminder`. + """ + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") + doc = read_state_file(path) + record = active_batch(doc) + if record is None: + return _json_result({"active_batch": None, "member_count": 0, "warning": None}) + members = batch_members(doc, record.batch_id) + return _json_result( + { + "active_batch": _record_to_dict(record), + "member_count": len(members), + "member_indices": [index for index, _ in members], + "platforms": list(batch_platforms(doc, record.batch_id)), + "warning": stale_batch_warning(record), + } + ) + + +def maybe_build_batch_reminder() -> str | None: + """Text to append to a mutating tool result when a batch is stale, else None. + + Best-effort and read-only: any failure (no STATE.json, unreadable, corrupt) + returns ``None`` and the tool result is untouched. Opt out with + ``MUREO_DISABLE_BATCH_REMINDER=1`` (exact string, matching the established + ``MUREO_DISABLE_*`` pattern). + """ + import os + + if os.environ.get("MUREO_DISABLE_BATCH_REMINDER") == "1": + return None + try: + path = resolve_workspace_path({}, "STATE.json", store_attr="state_path") + if not path.is_file(): + return None + return stale_batch_warning(active_batch(read_state_file(path))) + except Exception: # noqa: BLE001 — a reminder must never break a tool call + return None + + +__all__ = [ + "handle_batch_begin", + "handle_batch_end", + "handle_batch_status", + "maybe_build_batch_reminder", +] diff --git a/mureo/mcp/_handlers_mureo_context.py b/mureo/mcp/_handlers_mureo_context.py index 601a838..f8956f4 100644 --- a/mureo/mcp/_handlers_mureo_context.py +++ b/mureo/mcp/_handlers_mureo_context.py @@ -42,10 +42,10 @@ from __future__ import annotations -from pathlib import Path from typing import TYPE_CHECKING, Any from mureo.analysis.report_flags import normalize_flags +from mureo.context.batch import BatchError from mureo.context.errors import ContextFileError from mureo.context.models import ( ActionLogEntry, @@ -64,72 +64,20 @@ ) from mureo.context.strategy import RAW_HEADING_TYPE, parse_strategy, write_strategy_file from mureo.core.clock import server_now_iso -from mureo.core.runtime_context import get_runtime_context from mureo.fsutil import backup_file -from mureo.mcp._helpers import _json_result, _require +from mureo.mcp._helpers import _json_result, _require, resolve_workspace_path if TYPE_CHECKING: from mcp.types import TextContent -def _resolve_path( - arguments: dict[str, Any], default_name: str, *, store_attr: str | None = None -) -> Path: - """Resolve a user-supplied path, refusing anything outside the workspace. - - Resolution rules: - - - ``path`` argument missing or empty (``None`` or ``""``) → the - workspace-derived default (``getattr(store, store_attr)`` when - available, otherwise ``workspace / default_name``). Picks up - any alternate :class:`StateStore` wired via the - ``mureo.runtime_context_factory`` entry-point group without the - caller having to know about it. Note: the empty-string case - used to dispatch to ``Path(".")`` under the old ``_opt``-based - implementation; the new behaviour is intentional and safer. - - ``path`` argument present → resolved relative to the workspace - (not the process CWD — they coincide in the default file-backed - configuration but may diverge under an alternate runtime), - then security-checked: ``Path.resolve()`` follows symlinks, so a - file inside the workspace that symlinks to ``/etc/passwd`` - resolves to the target and is correctly refused. - """ - store = get_runtime_context().state_store - workspace = getattr(store, "workspace", Path.cwd()).resolve() - raw = arguments.get("path") - if not raw: - if store_attr is not None: - attr = getattr(store, store_attr, None) - if attr is not None: - # Backend-owned path: trusted output of an installed - # ``StateStore`` (the entry-point factory is host code, - # not an untrusted MCP caller). Skip the workspace - # boundary check so a backend can legitimately point - # outside ``workspace`` if its design requires it. - return Path(attr) - return workspace / default_name - - candidate = Path(raw) - resolved = ( - workspace / candidate if not candidate.is_absolute() else candidate - ).resolve() - try: - resolved.relative_to(workspace) - except ValueError as exc: - raise ValueError( - f"Refusing to read/write outside workspace: " - f"{resolved} is not inside {workspace}" - ) from exc - return resolved - - # --------------------------------------------------------------------------- # STRATEGY.md # --------------------------------------------------------------------------- async def handle_strategy_get(arguments: dict[str, Any]) -> list[TextContent]: - path = _resolve_path(arguments, "STRATEGY.md", store_attr="strategy_path") + path = resolve_workspace_path(arguments, "STRATEGY.md", store_attr="strategy_path") # ``server_now`` on both branches: a skill that starts from STRATEGY.md # (or runs before onboarding, when neither file exists) must still be # able to establish the current date without a second call. @@ -161,7 +109,7 @@ async def handle_strategy_set(arguments: dict[str, Any]) -> list[TextContent]: # rejects "" / None; this also catches whitespace-only payloads. if not markdown.strip(): raise ValueError("markdown must not be empty or whitespace-only") - path = _resolve_path(arguments, "STRATEGY.md", store_attr="strategy_path") + path = resolve_workspace_path(arguments, "STRATEGY.md", store_attr="strategy_path") # Round-trip through parse so callers can't write a STRATEGY.md # whose subsequent parse_strategy() call breaks downstream skills. # Unrecognized headings are preserved (raw passthrough), not dropped. @@ -275,7 +223,7 @@ def _apply_action_log_scope(payload: dict[str, Any], scope: Any) -> None: async def handle_state_get(arguments: dict[str, Any]) -> list[TextContent]: - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") # read_state_file already returns an empty default StateDocument when # the file is absent; round-trip through render_state to keep the # missing-file and present-file branches in lockstep. @@ -324,7 +272,7 @@ async def handle_state_action_log_append( # Required per ActionLogEntry contract. action = _require(raw, "action") platform = _require(raw, "platform") - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") # Validate the closure indices against the CURRENT log length before the # append. The log is append-only so its length only grows; an index that # is valid now stays valid, and reading once here avoids a stray value @@ -353,8 +301,18 @@ async def handle_state_action_log_append( reversible_params=raw.get("reversible_params"), rollback_of=raw.get("rollback_of"), evaluation_of=raw.get("evaluation_of"), + # #549: normally omitted — the open batch is stamped on by + # ``append_action_log``, which also VALIDATES an explicit value + # against the declared batches. A caller cannot invent a batch id or + # reattach to a closed one. + batch_id=raw.get("batch_id"), ) - doc = append_action_log(path, entry) + try: + doc = append_action_log(path, entry) + except BatchError as exc: + # A refused batch_id is caller error, not a server fault: report it as + # the tool's own refusal rather than as an unhandled exception. + raise ValueError(str(exc)) from exc return _json_result(_state_to_dict(doc)) @@ -423,7 +381,7 @@ async def handle_state_upsert_campaign( metrics=raw.get("metrics"), ads=_parse_ads_argument(raw.get("ads")), ) - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") try: doc = upsert_campaign(path, campaign, platform=platform, account_id=account_id) except ContextFileError as exc: @@ -449,7 +407,7 @@ async def handle_state_report_set( # pass through untouched; a ``summary`` without ``flags`` is left as-is. if "flags" in summary: summary = {**summary, "flags": normalize_flags(summary.get("flags"))} - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") doc = set_report(path, report, summary) return _json_result(_state_to_dict(doc)) @@ -477,7 +435,7 @@ async def handle_state_platform_metrics_set( for window, bucket in periods.items(): if not isinstance(bucket, dict): raise ValueError(f"periods[{window!r}] must be an object") - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") try: doc = set_platform_metrics( path, @@ -505,7 +463,7 @@ async def handle_state_set_conversion_events( 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") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") try: doc = set_conversion_action_types(path, platform, account_id, raw) except ContextFileError as exc: diff --git a/mureo/mcp/_handlers_rollback.py b/mureo/mcp/_handlers_rollback.py index 98a577e..871e151 100644 --- a/mureo/mcp/_handlers_rollback.py +++ b/mureo/mcp/_handlers_rollback.py @@ -1,8 +1,14 @@ """MCP handlers for the ``rollback.*`` tool family. -``rollback_plan_get`` — inspect the reversal plan for one action_log entry. +``rollback_plan_get`` — inspect the reversal plan for one action_log entry +(``index``), or for a whole declared batch (``batch_id``, #549). The batch +form reports every member's verdict and the overall / per-platform coverage, +so partial reversibility is known before anything is applied. ``rollback_apply`` — execute that plan, re-entering the same MCP -dispatch path used for forward actions. +dispatch path used for forward actions. Deliberately still one ``index`` per +call: applying a batch is a loop over the plan's ``apply_order``, so each +reversal keeps its own result instead of being folded into one summary status +that would have to gloss over partial failure. The dispatcher used by ``rollback_apply`` is resolved lazily via :func:`_get_dispatcher` so that ``mureo.mcp.server`` and this module @@ -24,6 +30,7 @@ from mureo.rollback import ( RollbackExecutionError, execute_rollback, + plan_batch_rollback, plan_rollback, ) @@ -32,6 +39,8 @@ from mcp.types import TextContent + from mureo.rollback import BatchMemberPlan, BatchRollbackPlan + logger = logging.getLogger(__name__) @@ -105,13 +114,77 @@ def _is_truthy_confirm(value: Any) -> bool: return value is True +def _batch_plan_payload(plan: BatchRollbackPlan) -> dict[str, Any]: + """Serialize a whole-batch plan, gaps and all. + + Every member appears — including the ones mureo cannot reverse, each with + the reason. A response that listed only the reversible members would read + as a complete revert and is exactly the failure #549 exists to prevent. + """ + return { + "batch_id": plan.batch_id, + "label": plan.label, + "coverage": plan.coverage.value, + "counts": plan.counts, + "platform_coverage": { + platform: coverage.value for platform, coverage in plan.platform_coverage + }, + # Reverse-chronological: feed these to rollback_apply in this order. + "apply_order": list(plan.apply_order), + "members": [_batch_member_payload(member) for member in plan.members], + } + + +def _batch_member_payload(member: BatchMemberPlan) -> dict[str, Any]: + """Serialize one batch member: its verdict first, its plan second.""" + payload: dict[str, Any] = { + "index": member.index, + "timestamp": member.timestamp, + "action": member.action, + "platform": member.platform, + "reversibility": member.status.value, + "reason": member.reason, + "plan_status": None, + "operation": None, + "params": None, + "caveats": [], + } + if member.plan is not None: + payload["plan_status"] = member.plan.status.value + payload["operation"] = member.plan.operation + payload["params"] = member.plan.params + payload["caveats"] = list(member.plan.caveats) + return payload + + +def _selector(arguments: dict[str, Any]) -> tuple[str, Any]: + """Return ``("index", int)`` or ``("batch_id", str)``. + + The MCP schema declares the exclusivity, but the schema is not the only + caller: rejecting both-or-neither here keeps a direct handler invocation + from silently planning something the operator did not ask for. + """ + raw_index = arguments.get("index") + raw_batch = arguments.get("batch_id") + has_index = raw_index is not None + has_batch = isinstance(raw_batch, str) and bool(raw_batch.strip()) + if has_index == has_batch: + raise ValueError( + "Provide exactly one of 'index' (a single action_log entry) or " + "'batch_id' (a whole batch)." + ) + if raw_index is not None: + return ("index", int(raw_index)) + return ("batch_id", str(raw_batch).strip()) + + async def handle_plan_get(arguments: dict[str, Any]) -> list[TextContent]: - """Return the :class:`RollbackPlan` for ``action_log[index]`` as JSON.""" + """Return the reversal plan for one entry, or for a whole batch, as JSON.""" try: state_file = _resolve_state_file(arguments) + kind, selector = _selector(arguments) except ValueError as exc: return _json_result({"plan": None, "reason": str(exc)}) - index = int(_require(arguments, "index")) if not state_file.exists(): return _json_result( @@ -122,6 +195,10 @@ async def handle_plan_get(arguments: dict[str, Any]) -> list[TextContent]: except ContextFileError as exc: return _json_result({"plan": None, "reason": str(exc)}) + if kind == "batch_id": + return _json_result(_batch_plan_payload(plan_batch_rollback(doc, selector))) + + index = selector if index < 0 or index >= len(doc.action_log): return _json_result( { diff --git a/mureo/mcp/_helpers.py b/mureo/mcp/_helpers.py index a5a27f4..db1ec95 100644 --- a/mureo/mcp/_helpers.py +++ b/mureo/mcp/_helpers.py @@ -2,6 +2,12 @@ Provides utility functions and API error handling decorators shared by Google Ads / Meta Ads handlers. + +:func:`resolve_workspace_path` lives here rather than in whichever handler +happened to need it first: it is the workspace **sandbox boundary** every +STATE.json / STRATEGY.md tool relies on, so a second copy — or a sibling +module reaching into another's privates to borrow it — is a place for the +check to drift. One definition, imported by name. """ from __future__ import annotations @@ -11,6 +17,7 @@ import inspect import json import logging +from pathlib import Path from typing import TYPE_CHECKING, Any if TYPE_CHECKING: @@ -96,6 +103,61 @@ def _validate_positive_money(arguments: dict[str, Any], *keys: str) -> None: raise ValueError(f"{key} must be greater than 0 (got {value!r})") +def resolve_workspace_path( + arguments: dict[str, Any], default_name: str, *, store_attr: str | None = None +) -> Path: + """Resolve a user-supplied path, refusing anything outside the workspace. + + Resolution rules: + + - ``path`` argument missing or empty (``None`` or ``""``) → the + workspace-derived default (``getattr(store, store_attr)`` when + available, otherwise ``workspace / default_name``). Picks up + any alternate :class:`StateStore` wired via the + ``mureo.runtime_context_factory`` entry-point group without the + caller having to know about it. Note: the empty-string case + used to dispatch to ``Path(".")`` under the old ``_opt``-based + implementation; the new behaviour is intentional and safer. + - ``path`` argument present → resolved relative to the workspace + (not the process CWD — they coincide in the default file-backed + configuration but may diverge under an alternate runtime), + then security-checked: ``Path.resolve()`` follows symlinks, so a + file inside the workspace that symlinks to ``/etc/passwd`` + resolves to the target and is correctly refused. + """ + # Lazy import: ``mureo.core.runtime_context`` pulls in the state layer, + # and ``_helpers`` is imported by every handler module at load time. + from mureo.core.runtime_context import get_runtime_context + + store = get_runtime_context().state_store + workspace = getattr(store, "workspace", Path.cwd()).resolve() + raw = arguments.get("path") + if not raw: + if store_attr is not None: + attr = getattr(store, store_attr, None) + if attr is not None: + # Backend-owned path: trusted output of an installed + # ``StateStore`` (the entry-point factory is host code, + # not an untrusted MCP caller). Skip the workspace + # boundary check so a backend can legitimately point + # outside ``workspace`` if its design requires it. + return Path(attr) + return workspace / default_name + + candidate = Path(raw) + resolved = ( + workspace / candidate if not candidate.is_absolute() else candidate + ).resolve() + try: + resolved.relative_to(workspace) + except ValueError as exc: + raise ValueError( + f"Refusing to read/write outside workspace: " + f"{resolved} is not inside {workspace}" + ) from exc + return resolved + + def _json_result(data: Any) -> list[TextContent]: """Convert a result to a list of TextContent containing a JSON string.""" return [TextContent(type="text", text=json.dumps(data, ensure_ascii=False))] diff --git a/mureo/mcp/server.py b/mureo/mcp/server.py index 265df06..5cc118f 100644 --- a/mureo/mcp/server.py +++ b/mureo/mcp/server.py @@ -51,6 +51,7 @@ from mureo.mcp.tool_provider import MCPToolProvider from mureo.core.control_flow import STOP_EXCEPTIONS +from mureo.core.strategy_reminder import is_mutating_builtin_tool from mureo.mcp._helpers import is_error_result from mureo.mcp.exclusion_preflight import ( append_notice as append_exclusion_impact_notice, @@ -82,6 +83,8 @@ from mureo.mcp.tools_analytics_registry import ( handle_tool as handle_analytics_registry_tool, ) +from mureo.mcp.tools_batch import TOOLS as BATCH_TOOLS +from mureo.mcp.tools_batch import handle_tool as handle_batch_tool from mureo.mcp.tools_creative_studio import TOOLS as CREATIVE_STUDIO_TOOLS from mureo.mcp.tools_creative_studio import ( handle_tool as handle_creative_studio_tool, @@ -144,6 +147,7 @@ def _is_disabled(env_var: str) -> bool: *(META_ADS_TOOLS if _META_ADS_ENABLED else []), *SEARCH_CONSOLE_TOOLS, *ROLLBACK_TOOLS, + *BATCH_TOOLS, *ANALYSIS_TOOLS, *MUREO_CONTEXT_TOOLS, *ANALYTICS_REGISTRY_TOOLS, @@ -159,6 +163,7 @@ def _is_disabled(env_var: str) -> bool: ) _SEARCH_CONSOLE_NAMES: frozenset[str] = frozenset(t.name for t in SEARCH_CONSOLE_TOOLS) _ROLLBACK_NAMES: frozenset[str] = frozenset(t.name for t in ROLLBACK_TOOLS) +_BATCH_NAMES: frozenset[str] = frozenset(t.name for t in BATCH_TOOLS) _ANALYSIS_NAMES: frozenset[str] = frozenset(t.name for t in ANALYSIS_TOOLS) _MUREO_CONTEXT_NAMES: frozenset[str] = frozenset(t.name for t in MUREO_CONTEXT_TOOLS) _ANALYTICS_REGISTRY_NAMES: frozenset[str] = frozenset( @@ -242,6 +247,7 @@ def _discover_with_amazon() -> tuple[Any, ...]: | _META_ADS_NAMES | _SEARCH_CONSOLE_NAMES | _ROLLBACK_NAMES + | _BATCH_NAMES | _ANALYSIS_NAMES | _MUREO_CONTEXT_NAMES | _ANALYTICS_REGISTRY_NAMES @@ -810,6 +816,35 @@ def _refuse_text_content(name: str, decision: PolicyDecision) -> list[Any]: return [TextContent(type="text", text=body)] +def _maybe_append_batch_reminder(result: list[Any], *, is_mutation: bool) -> list[Any]: + """Warn, on a mutation, that a batch has been open too long (#549). + + Push, not pull. ``mureo_batch_status`` reports the same staleness, but a + caller who FORGOT the batch is open is by definition not asking — and every + mutation dispatched meanwhile is another entry silently joining a change + set it does not belong to. So the warning rides out on the mutation itself, + the same soft-enforcement shape as the STRATEGY.md reminder. + + Re-emitted per mutation rather than latched once per process: each one adds + a member, so each one is a new instance of the problem, not a repeat of the + old one. Never refuses, never replaces the tool's content, never raises; + suppress with ``MUREO_DISABLE_BATCH_REMINDER=1``. + """ + if not is_mutation: + # Reads add no members, so a read is not another instance of the + # problem — warning on one would only cost context. + return result + + from mcp.types import TextContent + + from mureo.mcp._handlers_batch import maybe_build_batch_reminder + + warning = maybe_build_batch_reminder() + if warning is None: + return result + return [*result, TextContent(type="text", text=warning)] + + def _maybe_append_strategy_reminder(name: str, result: list[Any]) -> list[Any]: """Best-effort soft-enforcement of the "strategy-driven" claim. @@ -1054,13 +1089,19 @@ async def _dispatch_tool(name: str, arguments: dict[str, Any]) -> list[Any]: result = await handle_google_ads_tool(name, arguments) if record_mutations: record_native_mutation(name, arguments, before, result) - return _maybe_append_strategy_reminder(name, result) + return _maybe_append_batch_reminder( + _maybe_append_strategy_reminder(name, result), + is_mutation=is_mutating_builtin_tool(name), + ) if name in _META_ADS_NAMES: before = await capture_before_state(name, arguments) result = await handle_meta_ads_tool(name, arguments) if record_mutations: record_native_mutation(name, arguments, before, result) - return _maybe_append_strategy_reminder(name, result) + return _maybe_append_batch_reminder( + _maybe_append_strategy_reminder(name, result), + is_mutation=is_mutating_builtin_tool(name), + ) if name in _SEARCH_CONSOLE_NAMES: return _maybe_append_strategy_reminder( name, await handle_search_console_tool(name, arguments) @@ -1069,6 +1110,8 @@ async def _dispatch_tool(name: str, arguments: dict[str, Any]) -> list[Any]: return _maybe_append_strategy_reminder( name, await handle_rollback_tool(name, arguments) ) + if name in _BATCH_NAMES: + return await handle_batch_tool(name, arguments) if name in _ANALYSIS_NAMES: return await handle_analysis_tool(name, arguments) if name in _MUREO_CONTEXT_NAMES: @@ -1177,6 +1220,10 @@ async def _dispatch_tool(name: str, arguments: dict[str, Any]) -> list[Any]: # mutation — appended regardless of the result envelope, matching # the built-in dispatch. Read-only plugin tools skip it. result = _maybe_append_plugin_strategy_reminder(name, result) + # This branch runs only for a mutating plugin tool, so the call + # just added a member to any open batch — same reason the built-in + # mutating branches warn (#549). + result = _maybe_append_batch_reminder(result, is_mutation=True) return result raise ValueError(f"Unknown tool: {name}") diff --git a/mureo/mcp/tools_batch.py b/mureo/mcp/tools_batch.py new file mode 100644 index 0000000..50cb83d --- /dev/null +++ b/mureo/mcp/tools_batch.py @@ -0,0 +1,120 @@ +"""Batch MCP tool definitions and handler mapping (#549). + +Three tools that make a bulk change one named, reviewable unit: + +- ``mureo_batch_begin`` — declare the start of a change set. +- ``mureo_batch_end`` — close it and get the exact member list back. +- ``mureo_batch_status`` — ask which batch, if any, is currently collecting. + +Platform-agnostic on purpose. Membership is stamped where every recording path +already converges (``append_action_log``), so a native Google/Meta mutation, a +hosted-connector mutation an agent records by hand, and a bridged / plugin tool +call all join the same batch without any of them knowing it exists. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from mcp.types import Tool + +from mureo.mcp._handlers_batch import ( + handle_batch_begin, + handle_batch_end, + handle_batch_status, +) + +if TYPE_CHECKING: + from mcp.types import TextContent + + +_PATH_PROPERTY = { + "type": "string", + "description": ( + "Optional path to STATE.json. Defaults to STATE.json in the MCP " + "server's current working directory. Paths outside it are refused." + ), +} + + +TOOLS: list[Tool] = [ + Tool( + name="mureo_batch_begin", + description=( + "Declare the start of a bulk change so it can be reviewed and " + "reversed as ONE unit. Every action_log entry recorded until " + "mureo_batch_end — on any platform, native, hosted connector or " + "bridged/plugin — is tagged with the returned batch_id. Call this " + "BEFORE a multi-entity pass (N placement exclusions, N keywords, " + "N ad status changes); afterwards, rollback_plan_get with that " + "batch_id reports what can and cannot be reversed. Refused if a " + "batch is already open." + ), + inputSchema={ + "type": "object", + "properties": { + "label": { + "type": "string", + "minLength": 1, + "description": ( + "What this change set is, in the operator's words " + "(e.g. 'exclude low-quality display placements'). " + "Stored with the batch so the id still means " + "something weeks later." + ), + }, + "path": _PATH_PROPERTY, + }, + "required": ["label"], + "additionalProperties": False, + }, + ), + Tool( + name="mureo_batch_end", + description=( + "Close the open batch and return its exact membership: the " + "action_log indices it collected and the platforms they span. " + "Keep that list — it is the record that removes the need to " + "reconstruct a change set from memory later. Closing is FINAL: no " + "later entry can join, so the member count stays true. Refused if " + "no batch is open." + ), + inputSchema={ + "type": "object", + "properties": {"path": _PATH_PROPERTY}, + "additionalProperties": False, + }, + ), + Tool( + name="mureo_batch_status", + description=( + "Report which batch is currently collecting action_log entries " + "(null when none is), how many members it holds so far, and " + "which platforms they span. Also returns a ``warning`` when a " + "batch has been open unusually long — a forgotten batch keeps " + "swallowing unrelated changes. Read-only; mureo never closes a " + "batch on your behalf." + ), + inputSchema={ + "type": "object", + "properties": {"path": _PATH_PROPERTY}, + "additionalProperties": False, + }, + ), +] + +_HANDLERS: dict[str, Any] = { + "mureo_batch_begin": handle_batch_begin, + "mureo_batch_end": handle_batch_end, + "mureo_batch_status": handle_batch_status, +} + +_TOOL_NAMES: frozenset[str] = frozenset(t.name for t in TOOLS) + + +async def handle_tool(name: str, arguments: dict[str, Any]) -> list[TextContent]: + """Dispatch a ``mureo_batch_*`` tool call to its handler.""" + if name not in _TOOL_NAMES: + raise ValueError(f"Unknown tool: {name}") + handler = _HANDLERS[name] + return await handler(arguments) # type: ignore[no-any-return] diff --git a/mureo/mcp/tools_mureo_context.py b/mureo/mcp/tools_mureo_context.py index eeb5774..d7b9c7d 100644 --- a/mureo/mcp/tools_mureo_context.py +++ b/mureo/mcp/tools_mureo_context.py @@ -49,7 +49,8 @@ "platform (google_ads / meta_ads / etc.). The ``timestamp`` is " "stamped by the server — do not compute it. Optional: campaign_id, " "ad_id, entity_type, entity_id, summary, command, metrics_at_action, " - "observation_due, reversible_params, rollback_of, evaluation_of." + "observation_due, reversible_params, rollback_of, evaluation_of, " + "batch_id (normally stamped by the server — see the field)." ), "properties": { "timestamp": { @@ -115,6 +116,22 @@ "pending scope reads the returned ``index`` field to fill it." ), }, + "batch_id": { + "type": "string", + "minLength": 1, + "description": ( + "Normally OMIT this. While a batch is open (mureo_batch_begin) " + "the server stamps the entry with it automatically, so a bulk " + "pass groups itself. Supplying it is an explicit ASSERTION " + "that this entry belongs to that batch, and it is validated: " + "the id must name a declared batch that is still open. An " + "unknown id, or one whose batch has been closed, is REFUSED — " + "membership cannot be invented, and a closed batch's reported " + "member count cannot be made false after the fact. To group " + "imported or backfilled history, open a batch for the import " + "rather than reattaching to an old one." + ), + }, }, "required": ["action", "platform"], "dependentRequired": { diff --git a/mureo/mcp/tools_rollback.py b/mureo/mcp/tools_rollback.py index 0b5751e..1cb23ad 100644 --- a/mureo/mcp/tools_rollback.py +++ b/mureo/mcp/tools_rollback.py @@ -25,10 +25,16 @@ Tool( name="rollback_plan_get", description=( - "Inspect the reversal plan for a recorded action_log entry in " - "STATE.json. Returns the planner's status (supported / partial / " - "not_supported), the operation that would be dispatched, its " - "parameters, and any caveats. Does not execute anything." + "Inspect the reversal plan for recorded action_log entries in " + "STATE.json. Pass ``index`` for ONE entry: returns the planner's " + "status (supported / partial / not_supported), the operation that " + "would be dispatched, its parameters, and any caveats. Pass " + "``batch_id`` (from mureo_batch_begin) for a WHOLE bulk change: " + "returns every member with its own verdict, plus overall and " + "per-platform coverage (full / partial / none) and the reason each " + "irreversible member cannot be reversed. Exactly one of the two is " + "required. Read-only — nothing is executed, so partial coverage is " + "known BEFORE anything is applied." ), inputSchema={ "type": "object", @@ -45,8 +51,19 @@ "minimum": 0, "description": "Index into action_log (0-based).", }, + "batch_id": { + "type": "string", + "minLength": 1, + "description": ( + "Batch id to plan as one unit. Covers every action_log " + "entry tagged with it, across every platform they " + "touched." + ), + }, }, - "required": ["index"], + # Exactly one selector. ``index`` alone is the pre-#549 contract + # and keeps working unchanged; the alternative is additive. + "oneOf": [{"required": ["index"]}, {"required": ["batch_id"]}], "additionalProperties": False, }, ), diff --git a/mureo/rollback/__init__.py b/mureo/rollback/__init__.py index 5584a66..da5272f 100644 --- a/mureo/rollback/__init__.py +++ b/mureo/rollback/__init__.py @@ -8,18 +8,36 @@ This package is the *data-model and planning* half of the rollback feature. Actual execution — turning a plan into a live API call — is a separate concern that lives with the MCP dispatcher. + +:func:`plan_batch_rollback` (#549) does the same for a whole declared batch +(see :mod:`mureo.context.batch`): it classifies EVERY member through the same +planner and reports overall and per-platform coverage, so a batch that can +only be partly restored says so before anything is applied. """ from __future__ import annotations +from mureo.rollback.batch import plan_batch_rollback from mureo.rollback.executor import RollbackExecutionError, execute_rollback -from mureo.rollback.models import RollbackPlan, RollbackStatus +from mureo.rollback.models import ( + BatchCoverage, + BatchMemberPlan, + BatchMemberStatus, + BatchRollbackPlan, + RollbackPlan, + RollbackStatus, +) from mureo.rollback.planner import plan_rollback __all__ = [ + "BatchCoverage", + "BatchMemberPlan", + "BatchMemberStatus", + "BatchRollbackPlan", "RollbackExecutionError", "RollbackPlan", "RollbackStatus", "execute_rollback", + "plan_batch_rollback", "plan_rollback", ] diff --git a/mureo/rollback/batch.py b/mureo/rollback/batch.py new file mode 100644 index 0000000..4976171 --- /dev/null +++ b/mureo/rollback/batch.py @@ -0,0 +1,174 @@ +"""Plan the reversal of a whole batch, gaps included (#549). + +:func:`plan_rollback` answers "can this one entry be undone?". This module +answers the question an operator actually has after a bulk pass: "how much of +what I did on Monday can be undone, and what exactly cannot?" + +The second half is the point. A revert that reports success while restoring +60 of 80 members is worse than useless — it leaves the operator unable to +eliminate their own fix as a variable, which is how an incident turns into a +rebuild. So this module classifies **every** member and reports the gaps +before anything is applied. + +**Reversibility is decided by the existing planner, not re-invented here.** +Each member goes through :func:`mureo.rollback.planner.plan_rollback`, so the +allow-list, the destructive-verb refusal, the param-key bounding and the +plugin escape hatch all apply exactly as they do for a single entry. That also +means the per-platform differences fall out honestly: a native status toggle +with an allow-listed inverse plans as reversible, while a bridged tool's +reversal hint naming an operation mureo cannot dispatch plans as +``not_supported`` and is reported as a gap rather than quietly counted as +covered. + +Pure and read-only: it takes a parsed :class:`StateDocument` and returns data. +Nothing here dispatches, and nothing here writes. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from mureo.context.batch import batch_members, find_batch +from mureo.rollback.models import ( + BatchCoverage, + BatchMemberPlan, + BatchMemberStatus, + BatchRollbackPlan, + RollbackStatus, +) +from mureo.rollback.planner import plan_rollback + +if TYPE_CHECKING: + from mureo.context.models import ActionLogEntry, StateDocument + + +def _reversed_indices(doc: StateDocument) -> frozenset[int]: + """Indices already closed by a later ``rollback_of`` marker. + + Same rule the executor enforces before dispatching, read here so the plan + does not offer a member whose apply would be refused. + """ + return frozenset( + entry.rollback_of for entry in doc.action_log if entry.rollback_of is not None + ) + + +def _classify( + index: int, entry: ActionLogEntry, *, already_reversed: bool +) -> BatchMemberPlan: + """Verdict for one member, with the reason when it is a gap.""" + plan = plan_rollback(entry) + if already_reversed: + status = BatchMemberStatus.ALREADY_REVERSED + reason = f"Entry #{index} was already rolled back by a later log entry." + elif plan is None: + status = BatchMemberStatus.NOTHING_TO_REVERSE + reason = f"{entry.action} is a read-only action; it changed no state." + elif plan.status is RollbackStatus.NOT_SUPPORTED: + status = BatchMemberStatus.IRREVERSIBLE + reason = plan.notes or ( + f"mureo cannot reverse {entry.action} on {entry.platform}." + ) + elif plan.status is RollbackStatus.PARTIAL: + status = BatchMemberStatus.REVERSIBLE_WITH_CAVEATS + reason = "; ".join(plan.caveats) + else: + status = BatchMemberStatus.REVERSIBLE + reason = "" + return BatchMemberPlan( + index=index, + timestamp=entry.timestamp, + action=entry.action, + platform=entry.platform, + status=status, + plan=plan, + reason=reason, + ) + + +def _coverage(members: tuple[BatchMemberPlan, ...]) -> BatchCoverage: + """Aggregate coverage over the members that still need reversing. + + Members with nothing to reverse (reads) and members already reversed are + excluded from the verdict: neither is an outstanding gap, and counting + them would make a fully-reverted batch report ``partial`` forever. + Everything else is either reversible or a gap, and a batch with both is + ``PARTIAL`` — never rounded up to ``FULL``. + """ + if not members: + return BatchCoverage.EMPTY + outstanding = [ + m + for m in members + if m.status + not in ( + BatchMemberStatus.NOTHING_TO_REVERSE, + BatchMemberStatus.ALREADY_REVERSED, + ) + ] + if not outstanding: + return BatchCoverage.FULL + reversible = sum(1 for m in outstanding if m.is_reversible) + if reversible == len(outstanding): + return BatchCoverage.FULL + if reversible == 0: + return BatchCoverage.NONE + return BatchCoverage.PARTIAL + + +def _platform_coverage( + members: tuple[BatchMemberPlan, ...], +) -> tuple[tuple[str, BatchCoverage], ...]: + """Coverage per platform key, sorted by key. + + A batch that spans a native and a bridged platform usually has different + answers for each, and the operator's next step depends on which platform + they have to finish by hand. + """ + by_platform: dict[str, list[BatchMemberPlan]] = {} + for member in members: + by_platform.setdefault(member.platform, []).append(member) + return tuple( + (platform, _coverage(tuple(group))) + for platform, group in sorted(by_platform.items()) + ) + + +def plan_batch_rollback(doc: StateDocument, batch_id: str) -> BatchRollbackPlan: + """Build the reversal plan for every member of ``batch_id``. + + An unknown or empty batch returns a plan with + :data:`~mureo.rollback.models.BatchCoverage.EMPTY` and no members, rather + than raising — "this id collected nothing" is a truthful answer the + operator needs, and it is distinguishable from "nothing can be reversed". + + Args: + doc: The parsed STATE.json to read. Not mutated. + batch_id: The batch id recorded on the member entries. + + Returns: + A :class:`~mureo.rollback.models.BatchRollbackPlan` covering every + member, each with its own verdict and — for the gaps — the reason. + """ + already = _reversed_indices(doc) + members = tuple( + _classify(index, entry, already_reversed=index in already) + for index, entry in batch_members(doc, batch_id) + ) + # The record outlives the batch (BatchRecord.ended_at), so a closed batch + # still reports the operator's own words for what it was. + record = find_batch(doc, batch_id) + return BatchRollbackPlan( + batch_id=batch_id.strip(), + label=record.label if record is not None else None, + coverage=_coverage(members), + members=members, + platform_coverage=_platform_coverage(members), + # Newest first — see BatchRollbackPlan.apply_order. + apply_order=tuple( + m.index for m in sorted(members, key=lambda m: -m.index) if m.is_reversible + ), + ) + + +__all__ = ["plan_batch_rollback"] diff --git a/mureo/rollback/executor.py b/mureo/rollback/executor.py index f0181a6..41ff0ad 100644 --- a/mureo/rollback/executor.py +++ b/mureo/rollback/executor.py @@ -200,7 +200,10 @@ async def execute_rollback( rollback_of=index, reversible_params=None, ) - append_action_log(state_file, new_entry) + # A reversal must not join whatever batch happens to be open (#549): + # doing so would grow the batch it reverses, and the next batch plan + # would offer the reversals themselves as things still to reverse. + append_action_log(state_file, new_entry, join_active_batch=False) return { "status": "applied", diff --git a/mureo/rollback/models.py b/mureo/rollback/models.py index 22d358c..d88cc51 100644 --- a/mureo/rollback/models.py +++ b/mureo/rollback/models.py @@ -56,3 +56,120 @@ class RollbackPlan: def __post_init__(self) -> None: if self.params is not None: object.__setattr__(self, "params", copy.deepcopy(self.params)) + + +class BatchMemberStatus(str, Enum): + """What a single member of a batch can expect from a rollback (#549). + + Deliberately finer-grained than :class:`RollbackStatus`: a batch plan has + to distinguish "there is nothing here to undo" from "there is something + and mureo cannot undo it", because only the second is a gap in the revert + the operator must close by hand. + """ + + REVERSIBLE = "reversible" + """A plan exists and replaying it restores the prior state.""" + + REVERSIBLE_WITH_CAVEATS = "reversible_with_caveats" + """A plan exists, but side effects (spend incurred, impressions served) + remain. The member's plan carries the caveats.""" + + IRREVERSIBLE = "irreversible" + """mureo cannot reverse this member — no hint, a hint outside the + allow-list, or an operation on a platform mureo cannot dispatch to. The + member's ``reason`` says which.""" + + NOTHING_TO_REVERSE = "nothing_to_reverse" + """A read-only action. Not a gap: there is no state change to undo.""" + + ALREADY_REVERSED = "already_reversed" + """A later ``action_log`` entry already carries ``rollback_of`` for this + index, so applying again would be refused.""" + + +class BatchCoverage(str, Enum): + """How much of a batch a rollback would actually restore. + + This is the answer the motivating incident never got. An unverifiable + revert leaves the operator unable to eliminate their own fix as a + variable, so the honest values matter more than the optimistic one: + ``PARTIAL`` and ``NONE`` must be reachable and must be reported BEFORE + anything is applied. + """ + + FULL = "full" + """Every member that changed state can be reversed.""" + + PARTIAL = "partial" + """Some members can be reversed and some cannot.""" + + NONE = "none" + """Nothing that changed state can be reversed.""" + + EMPTY = "empty" + """The batch has no members — an unknown id, or one that collected + nothing.""" + + +@dataclass(frozen=True) +class BatchMemberPlan: + """One member of a batch, with its verdict. + + ``plan`` is the underlying :class:`RollbackPlan` when the planner produced + one (including a ``not_supported`` plan, which carries the planner's + reasoning), and ``None`` for a read-only member. + """ + + index: int + timestamp: str + action: str + platform: str + status: BatchMemberStatus + plan: RollbackPlan | None + reason: str = "" + + @property + def is_reversible(self) -> bool: + """Would applying this member's plan restore state now? + + ``ALREADY_REVERSED`` is False: it needs no action and a second apply + is refused. ``NOTHING_TO_REVERSE`` is False for the same reason — and + neither is counted as a gap. + """ + return self.status in ( + BatchMemberStatus.REVERSIBLE, + BatchMemberStatus.REVERSIBLE_WITH_CAVEATS, + ) + + +@dataclass(frozen=True) +class BatchRollbackPlan: + """The reversal plan for a whole batch — every member, reversible or not. + + ``platform_coverage`` exists because reversibility is not uniform across + platforms: an operation with a clean inverse on a native platform may have + none on a bridged one whose tool set mureo does not own. Reporting one + aggregate number would let a core abstraction paper over exactly that, so + the per-platform breakdown is part of the plan, not a derived nicety. + + ``apply_order`` is reverse-chronological (newest member first), the order a + caller should feed the indices to ``rollback_apply``: later members may + depend on earlier ones, so undoing forwards can re-apply an effect the + previous step just removed. + """ + + batch_id: str + label: str | None + coverage: BatchCoverage + members: tuple[BatchMemberPlan, ...] + platform_coverage: tuple[tuple[str, BatchCoverage], ...] = () + apply_order: tuple[int, ...] = () + + @property + def counts(self) -> dict[str, int]: + """Member counts per :class:`BatchMemberStatus`, plus ``total``.""" + result = {status.value: 0 for status in BatchMemberStatus} + for member in self.members: + result[member.status.value] += 1 + result["total"] = len(self.members) + return result diff --git a/skills/_mureo-shared/SKILL.md b/skills/_mureo-shared/SKILL.md index 598a41e..0e409bd 100644 --- a/skills/_mureo-shared/SKILL.md +++ b/skills/_mureo-shared/SKILL.md @@ -107,6 +107,7 @@ Skills and commands describe "Read STRATEGY.md", "Update STATE.json", and "Appen | Read STATE.json | `Read` tool | `mureo_state_get` MCP tool | | Establish the current date | `mureo_state_get` MCP tool (`server_now`) | `mureo_state_get` MCP tool (`server_now`) | | Append action_log entry | `mureo_state_action_log_append` MCP tool | `mureo_state_action_log_append` MCP tool | +| Group a bulk change as one unit | `mureo_batch_begin` / `mureo_batch_end` MCP tools | `mureo_batch_begin` / `mureo_batch_end` MCP tools | | Upsert campaign snapshot | `mureo_state_upsert_campaign` MCP tool | `mureo_state_upsert_campaign` MCP tool | When you don't have direct filesystem tools (Desktop / Cowork / web), always reach for the corresponding `mureo_*` MCP tool — they encode the same atomic-write semantics so you can't corrupt the file mid-edit. @@ -206,6 +207,29 @@ The MCP server exposes tools for Google Ads, Meta Ads, and Search Console over s Once configured, the AI agent can call `google_ads_campaigns_list` or `meta_ads_campaigns_list` to verify the connection is working. +## Bulk changes are one revertible unit + +Any pass that changes **more than one entity** — N placement/app exclusions, N keywords, N ad status changes, a pause across several campaigns — must be wrapped: + +1. `mureo_batch_begin` with a `label` in the operator's words (e.g. `"exclude low-quality display placements"`). It returns a `batch_id`. +2. Do the work. Every `action_log` entry recorded until you close the batch is tagged with that id automatically — **on every platform**, whether the entry came from a native status toggle, from a bridged/plugin tool mureo promoted, or from your own `mureo_state_action_log_append` call. +3. `mureo_batch_end`. It returns the exact member list (`member_indices`, `platforms`). **Report the `batch_id` and the member count to the operator** — that is the record which removes any later need to reconstruct the change set from memory. Closing is final: nothing can join afterwards, so that count stays true. + +**Close it.** A missed `begin` yields no batch and is harmless; a missed `end` yields a batch that keeps swallowing every later change — including work from another session entirely — and then reports the lot as one unit. If a batch has been open more than a day, mureo appends a warning to mutating tool results and to `mureo_batch_status`; when you see it, either close the batch or tell the operator it is still open. mureo will not close it for you. Never pass a `batch_id` you did not get from `mureo_batch_begin` in this session: an unknown id, or one whose batch is closed, is refused. + +Then `rollback_plan_get` with `batch_id` (instead of `index`) plans the whole thing: `coverage` (`full` / `partial` / `none`), `platform_coverage`, per-member verdicts, and `apply_order`. + +**Report coverage honestly and BEFORE applying anything.** If a batch of 80 reports 60 reversible and 20 `irreversible`, say exactly that, name the irreversible members and their `reason`, and say which platform they are on — do not describe the revert as complete. `rollback_apply` still takes one `index` at a time; walk `apply_order` in the order given (newest first). + +What can join a batch, and how far a reversal can actually go, differs by platform — state the limit rather than implying uniform coverage: + +- **Native `google_ads_*` / `meta_ads_*`** — status toggles are recorded for you. **Every other mutation** (budget, keywords, exclusions, creative) is recorded only if YOU call `mureo_state_action_log_append`; without that it is not in the batch and not in the plan. +- **Plugin / bridged platforms** (`plugin::`, e.g. Amazon Ads) — successful mutations join automatically. A reversal is executed only when the hint names a registered plugin tool; otherwise the member is reported `irreversible` with the reason, and reversing it is manual. +- **Hosted connectors** (`tiktok_ads`) — join only through your own `mureo_state_action_log_append` calls, and their reversal is never executed by mureo. The batch plan is still worth having: it is an accurate manual checklist instead of a memory exercise. +- **Search Console** — its mutations are not recorded in `action_log` at all, so they cannot join a batch today. + +If you cannot open a batch (older mureo without the tools), say so and record each entry individually — do not silently do a bulk pass with no grouping. + ## Security Rules > CRITICAL: AI agents MUST follow these rules when using mureo tools. @@ -239,6 +263,7 @@ When pausing or removing multiple entities: - List all affected entities with their current performance - Show total impact (e.g., "This will pause 5 campaigns with 1,200 clicks/day") - Require explicit confirmation +- **Wrap the whole pass in a batch** so it can be reviewed and reverted as ONE unit — see *Bulk changes are one revertible unit* above ### 3b. Bulk Exclusions: Size Them Before Applying (#547) @@ -460,6 +485,12 @@ shows fewer campaigns than you wrote — get these exact names right: not-recently-synced. `mureo_state_upsert_campaign` / `_platform_metrics_set` / `_report_set` set it for you (`_action_log_append` does not); on the Code `Write` path you must set it yourself. +- **Top-level `batches`** (declared bulk change sets, #549) — **carry it over + verbatim** on the Code `Write` path, together with each `action_log` entry's + `batch_id`. Dropping either detaches a change set from its members, which is + precisely the "reconstruct what I did from memory" state batches exist to + remove. A record with no `ended_at` is an OPEN batch; do not invent, close or + renumber one by hand — use `mureo_batch_begin` / `mureo_batch_end`. Canonical STATE.json shape (note `campaign_name`, `account_id`, `last_synced_at`): diff --git a/skills/search-term-cleanup/SKILL.md b/skills/search-term-cleanup/SKILL.md index 97b2273..3184c48 100644 --- a/skills/search-term-cleanup/SKILL.md +++ b/skills/search-term-cleanup/SKILL.md @@ -56,10 +56,14 @@ Review and clean up search terms and keywords across all platforms. 9. **Check pending observations**: Before executing, check `action_log` for this campaign. If a previous action is still within its observation window, warn that stacking changes will make outcome evaluation difficult. Recommend waiting if possible. -10. **Execute**: Use each platform's keyword management tools to apply approved changes (add negative keywords, add positive keywords, adjust bids). +10. **Open a batch**: call `mureo_batch_begin` with a `label` naming this pass (e.g. `"search-term cleanup 2026-08-07"`). A cleanup changes many entities across possibly several platforms, and the batch is what makes it one reviewable, plannable unit instead of a set of entries nobody can re-identify later. See `../_mureo-shared/SKILL.md` → *Bulk changes are one revertible unit*. -11. **Record outcome context**: For each campaign modified, log to `action_log` with `metrics_at_action` (current CPA, conversions, clicks, CTR, impressions, cost) and `observation_due` (14 days from `server_now`'s date). This enables evidence-based evaluation later. +11. **Execute**: Use each platform's keyword management tools to apply approved changes (add negative keywords, add positive keywords, adjust bids). -12. **Update STATE.json** with notes about the cleanup. +12. **Record outcome context**: For each campaign modified, log to `action_log` with `metrics_at_action` (current CPA, conversions, clicks, CTR, impressions, cost) and `observation_due` (14 days from `server_now`'s date). This enables evidence-based evaluation later. Keyword and negative-keyword changes are **not** auto-recorded, so this step is what puts them in the batch at all — an entry you do not append is invisible to any later revert. + +13. **Close the batch**: call `mureo_batch_end` and report the returned `batch_id` and member count to the operator, so undoing this pass later is `rollback_plan_get` with that id rather than a reconstruction from memory. + +14. **Update STATE.json** with notes about the cleanup. IMPORTANT: Always explain WHY a term should be excluded/added, referencing the Persona or USP from STRATEGY.md. Consult past action_log entries — if a similar cleanup was previously evaluated, reference whether it was effective. diff --git a/tests/test_batch_revertible_unit.py b/tests/test_batch_revertible_unit.py new file mode 100644 index 0000000..16ead50 --- /dev/null +++ b/tests/test_batch_revertible_unit.py @@ -0,0 +1,931 @@ +"""A bulk operation is one revertible unit (#549). + +Three properties, one per section below: + +1. **Grouping.** Every ``action_log`` write path stamps the open batch's id, + so N operations dispatched as one logical batch become one reviewable set + — regardless of which platform each member ran on. +2. **Coverage.** ``rollback_plan_get`` accepts that batch id and returns a + plan covering EVERY member, not just the reversible ones. +3. **Honesty.** A batch whose members are not uniformly reversible says so + BEFORE anything is applied — per member, and per platform. + +The platform mix is deliberate. Reversibility is not uniform across +platforms, so a single-platform test would prove nothing about the case the +issue exists for. These tests cover ``google_ads`` and ``meta_ads`` (native), +``tiktok_ads`` (hosted connector, recorded through +``mureo_state_action_log_append``) and ``plugin::`` (the +bridged / plugin ABI path, recorded through +``plugin_semantics.record_mutation_action_log``). +""" + +from __future__ import annotations + +import json +from dataclasses import fields, replace +from datetime import datetime, timedelta, timezone +from pathlib import Path +from typing import Any + +import pytest + +from mureo.context.batch import ( + STALE_AFTER_HOURS, + BatchError, + active_batch, + batch_members, + batch_open_hours, + stale_batch_warning, + stamp_batch, +) +from mureo.context.models import ActionLogEntry, BatchRecord, StateDocument +from mureo.context.state import ( + append_action_log, + begin_batch, + end_batch, + read_state_file, + write_state_file, +) +from mureo.mcp.tools_batch import TOOLS as BATCH_TOOLS +from mureo.mcp.tools_batch import handle_tool as handle_batch_tool +from mureo.mcp.tools_rollback import handle_tool as handle_rollback_tool +from mureo.rollback.batch import plan_batch_rollback +from mureo.rollback.models import BatchCoverage, BatchMemberStatus + +_PLUGIN_PLATFORM = "plugin:mureo-amazon-ads-bridge:amazon_ads" + + +@pytest.fixture(autouse=True) +def _clear_runtime_context_cache(): + """Reset the workspace resolver cache around every test (see + tests/test_mcp_tools_rollback.py — same reason).""" + from mureo.core.runtime_context import reset_runtime_context + + reset_runtime_context() + yield + reset_runtime_context() + + +@pytest.fixture +def workspace(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A workspace with an existing STATE.json, as cwd.""" + monkeypatch.chdir(tmp_path) + write_state_file(tmp_path / "STATE.json", StateDocument(version="2")) + return tmp_path + + +def _payload(result: list[Any]) -> dict[str, Any]: + parsed: dict[str, Any] = json.loads(result[0].text) + return parsed + + +def _entry( + action: str, + platform: str, + *, + reversible_params: dict[str, Any] | None = None, +) -> ActionLogEntry: + return ActionLogEntry( + timestamp="2026-08-07T10:00:00+09:00", + action=action, + platform=platform, + reversible_params=reversible_params, + ) + + +# Reversal hints, one per platform family, all through the SAME core shape. +_GOOGLE_REVERSAL = { + "operation": "google_ads_campaigns_update_status", + "params": {"campaign_id": "G1", "status": "ENABLED"}, +} +_META_REVERSAL = { + "operation": "meta_ads_ad_sets_enable", + "params": {"ad_set_id": "M1"}, +} +_META_REVERSAL_WITH_CAVEAT = { + "operation": "meta_ads_campaigns_enable", + "params": {"campaign_id": "M9"}, + "caveats": ["Spend already incurred cannot be refunded."], +} +# A bridged/plugin tool mureo does not own: the hint names the provider's own +# operation, which is NOT in the built-in allow-list and (with no such plugin +# tool registered) cannot be dispatched. This is the member that must be +# reported as irreversible rather than quietly counted as covered. +_BRIDGED_REVERSAL = { + "operation": "amazon_ads_negative_keywords_restore", + "params": {"keyword_id": "A1"}, +} + + +# --------------------------------------------------------------------------- +# 1. Grouping — N operations, one batch id, four platforms +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestBatchGrouping: + @pytest.mark.asyncio + async def test_members_of_one_dispatched_batch_share_a_batch_id( + self, workspace: Path + ) -> None: + """Native, hosted-connector and bridged/plugin writes join one batch. + + Each of the four members enters ``action_log`` through a DIFFERENT + recording path, which is the point: batch membership is a core + concern, not a per-platform one. + """ + state_file = workspace / "STATE.json" + begin = _payload( + await handle_batch_tool( + "mureo_batch_begin", {"label": "bulk exclusion pass"} + ) + ) + batch_id = begin["batch_id"] + assert batch_id + + # (a) native google_ads + (b) hosted connector tiktok_ads, both via the + # generic append tool an agent uses for a non-auto-recorded mutation. + from mureo.mcp.tools_mureo_context import handle_tool as handle_context_tool + + for action, platform in ( + ("google_ads_placement_exclusions_add", "google_ads"), + ("tiktok_ads_ad_groups_update", "tiktok_ads"), + ): + await handle_context_tool( + "mureo_state_action_log_append", + {"entry": {"action": action, "platform": platform}}, + ) + + # (c) native meta_ads status toggle, via the native recording path. + from mureo.mcp.native_reversal import record_native_mutation + + record_native_mutation( + "meta_ads_ad_sets_pause", {"ad_set_id": "M1"}, "ACTIVE", None + ) + + # (d) bridged / plugin mutation, via the plugin ABI recording path. + from mureo.mcp.plugin_semantics import record_mutation_action_log + + record_mutation_action_log( + tool="amazon_ads_negative_keywords_create", + source="mureo-amazon-ads-bridge", + provider="amazon_ads", + reversal=None, + arguments={"campaign_id": "A9"}, + ) + + end = _payload(await handle_batch_tool("mureo_batch_end", {})) + assert end["batch_id"] == batch_id + assert end["member_count"] == 4 + + doc = read_state_file(state_file) + assert [e.batch_id for e in doc.action_log] == [batch_id] * 4 + assert {e.platform for e in doc.action_log} == { + "google_ads", + "tiktok_ads", + "meta_ads", + _PLUGIN_PLATFORM, + } + assert [i for i, _ in batch_members(doc, batch_id)] == [0, 1, 2, 3] + + def test_entries_written_outside_a_batch_carry_no_batch_id( + self, workspace: Path + ) -> None: + state_file = workspace / "STATE.json" + append_action_log(state_file, _entry("google_ads_budget_update", "google_ads")) + doc = read_state_file(state_file) + assert doc.action_log[0].batch_id is None + assert active_batch(doc) is None + + def test_begin_refuses_to_nest(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + begin_batch(state_file, label="first") + with pytest.raises(BatchError): + begin_batch(state_file, label="second") + + def test_end_without_an_open_batch_is_refused(self, workspace: Path) -> None: + with pytest.raises(BatchError): + end_batch(workspace / "STATE.json") + + def test_explicit_batch_id_must_name_the_open_batch(self, workspace: Path) -> None: + """An explicit ``batch_id`` is an assertion, and it is checked.""" + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="open") + append_action_log( + state_file, + ActionLogEntry( + timestamp="2026-08-07T10:00:00+09:00", + action="google_ads_budget_update", + platform="google_ads", + batch_id=batch.batch_id, + ), + ) + assert read_state_file(state_file).action_log[0].batch_id == batch.batch_id + + def test_rollback_entries_do_not_join_the_open_batch(self, workspace: Path) -> None: + """A reversal appended while a batch is open must not become a member. + + Otherwise reverting a batch would grow the batch it is reverting, and + the next plan would list the reversals as things still to reverse. + """ + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="open") + append_action_log( + state_file, + _entry("google_ads_campaigns_update_status", "google_ads"), + join_active_batch=False, + ) + doc = read_state_file(state_file) + assert doc.action_log[0].batch_id is None + assert active_batch(doc) is not None + assert batch_members(doc, batch.batch_id) == () + + +#: One distinctive value per :class:`ActionLogEntry` field. Driven off the +#: dataclass's own field list by the tests below, so ADDING A FIELD TO +#: ``ActionLogEntry`` WITHOUT ADDING IT HERE FAILS — which is the point. The +#: hazard being guarded is silent: a field dropped while joining a batch reads +#: downstream as "the caller never set it", so nothing else would notice. +_ENTRY_FIELD_VALUES: dict[str, Any] = { + "timestamp": "2026-08-07T10:00:00+09:00", + "action": "google_ads_placement_exclusions_add", + "platform": "google_ads", + "campaign_id": "C-1", + "ad_id": "A-1", + "summary": "excluded 12 placements", + "command": "/search-term-cleanup", + "metrics_at_action": {"cpa": 5200, "conversions": 45}, + "observation_due": "2026-08-21", + "reversible_params": { + "operation": "google_ads_campaigns_update_status", + "params": {"campaign_id": "C-1", "status": "ENABLED"}, + }, + "rollback_of": 3, + "evaluation_of": 4, + "entity_type": "ad_group", + "entity_id": "G-1", + # The one field the round-trip is ALLOWED to change: it arrives unset and + # comes back carrying the open batch. + "batch_id": None, +} + + +def _fully_populated_entry() -> ActionLogEntry: + """An entry with every field set, checked against the dataclass itself.""" + declared = {f.name for f in fields(ActionLogEntry)} + missing = declared - set(_ENTRY_FIELD_VALUES) + assert not missing, ( + f"ActionLogEntry gained field(s) {sorted(missing)} with no value in " + "_ENTRY_FIELD_VALUES. Add one, then confirm the field survives " + "stamp_batch and the STATE.json codec — a new field that is silently " + "dropped when an entry joins a batch is exactly the bug this guards." + ) + stale = set(_ENTRY_FIELD_VALUES) - declared + assert not stale, f"_ENTRY_FIELD_VALUES names removed field(s) {sorted(stale)}" + return ActionLogEntry(**_ENTRY_FIELD_VALUES) + + +@pytest.mark.unit +class TestJoiningABatchPreservesTheEntry: + """Joining a batch must change ``batch_id`` and nothing else. + + ``stamp_batch`` used to rebuild the entry field-by-field, so any field + added to :class:`ActionLogEntry` afterwards was dropped the moment an entry + joined an open batch — and ``join_active_batch`` defaults to ``True``, so + that is the ordinary path, not a corner. The loss was silent: a missing + field is indistinguishable from one the caller never set. It cost the + provenance fields (``origin`` / ``external_id``), and with them the + ``is_external`` marker that stops a forged ``reversible_params`` on an + imported entry from being planned as a real reversal. + + Both tests below are driven off ``dataclasses.fields(ActionLogEntry)`` + rather than a hand-written list, so they cannot rot the same way. + """ + + def test_stamp_batch_changes_only_batch_id(self) -> None: + """The pure function, so a failure localizes here and not in the codec.""" + entry = _fully_populated_entry() + record = BatchRecord( + batch_id="batch-x", label="pass", started_at="2026-08-07T09:00:00+09:00" + ) + stamped = stamp_batch(entry, record) + + assert stamped.batch_id == "batch-x" + for field in fields(ActionLogEntry): + if field.name == "batch_id": + continue + assert getattr(stamped, field.name) == getattr(entry, field.name), ( + f"stamp_batch dropped or altered {field.name!r} while joining a " + "batch. Use dataclasses.replace; never enumerate fields." + ) + + def test_every_field_survives_the_append_round_trip(self, workspace: Path) -> None: + """End to end: through ``stamp_batch``, the codec, and back off disk. + + Covers the second enumerating surface too — ``state_codec`` maps the + entry to JSON field-by-field in both directions, so a field missing + from either half is lost on the way to STATE.json rather than on the + way into the batch. + """ + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="preserve everything") + entry = _fully_populated_entry() + append_action_log(state_file, entry) + + stored = read_state_file(state_file).action_log[0] + assert stored.batch_id == batch.batch_id + for field in fields(ActionLogEntry): + if field.name == "batch_id": + continue + assert getattr(stored, field.name) == _ENTRY_FIELD_VALUES[field.name], ( + f"{field.name!r} did not survive append_action_log with an open " + "batch — check stamp_batch and both halves of state_codec." + ) + + +@pytest.mark.unit +class TestMembershipCannotBeForged: + """Membership is the one thing this feature asks the operator to trust. + + A batch id supplied by a caller is untrusted input: an unchecked one lets + an entry conjure a change set that never happened, or grow one whose + membership was already reported as final. + """ + + def test_an_unknown_batch_id_is_refused(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + with pytest.raises(BatchError, match="Unknown batch_id"): + append_action_log( + state_file, + ActionLogEntry( + timestamp="2026-08-07T10:00:00+09:00", + action="google_ads_budget_update", + platform="google_ads", + batch_id="batch-i-made-this-up", + ), + ) + assert read_state_file(state_file).action_log == () + + def test_a_closed_batch_cannot_be_rejoined(self, workspace: Path) -> None: + """The member_count mureo_batch_end reported must stay true.""" + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="monday pass") + append_action_log(state_file, _entry("google_ads_budget_update", "google_ads")) + _, indices = end_batch(state_file) + assert indices == (0,) + + with pytest.raises(BatchError, match="closed"): + append_action_log( + state_file, + ActionLogEntry( + timestamp="2026-08-07T12:00:00+09:00", + action="google_ads_keywords_add", + platform="google_ads", + batch_id=batch.batch_id, + ), + ) + doc = read_state_file(state_file) + assert [i for i, _ in batch_members(doc, batch.batch_id)] == list(indices) + + @pytest.mark.asyncio + async def test_forged_batch_id_is_refused_through_the_mcp_tool( + self, workspace: Path + ) -> None: + """The reproduction from review: no begin, arbitrary id, real batch.""" + from mureo.mcp.tools_mureo_context import handle_tool as handle_context_tool + + with pytest.raises(ValueError, match="Unknown batch_id"): + await handle_context_tool( + "mureo_state_action_log_append", + { + "entry": { + "action": "google_ads_placement_exclusions_add", + "platform": "google_ads", + "batch_id": "batch-fabricated", + } + }, + ) + payload = _payload( + await handle_rollback_tool( + "rollback_plan_get", {"batch_id": "batch-fabricated"} + ) + ) + assert payload["coverage"] == "empty" + assert payload["members"] == [] + + +@pytest.mark.unit +class TestForgottenBatchAnnouncesItself: + """A missed ``end`` is worse than a missed ``begin``. + + A missed begin yields no batch — obvious and harmless. A missed end yields + a batch that keeps swallowing unrelated changes and then reports them, + confidently, as one unit. Nothing auto-closes: that would trade a visible + wrong answer for an invisible one. + """ + + def test_a_fresh_batch_is_not_stale(self, workspace: Path) -> None: + batch = begin_batch(workspace / "STATE.json", label="just opened") + assert stale_batch_warning(batch) is None + + def test_a_long_open_batch_warns(self) -> None: + started = datetime(2026, 8, 1, 9, 0, tzinfo=timezone.utc) + record = BatchRecord( + batch_id="batch-old", + label="monday pass", + started_at=started.isoformat(), + ) + now = started + timedelta(hours=STALE_AFTER_HOURS + 1) + warning = stale_batch_warning(record, now) + assert warning is not None + assert "batch-old" in warning + assert "mureo_batch_end" in warning + # Never auto-closed — the record is untouched. + assert record.ended_at is None + + def test_a_closed_batch_never_warns(self) -> None: + record = BatchRecord( + batch_id="batch-done", + label="done", + started_at="2026-08-01T09:00:00+00:00", + ended_at="2026-08-01T10:00:00+00:00", + ) + assert stale_batch_warning(record, datetime.now(timezone.utc)) is None + + def test_staleness_follows_the_server_clock_seam( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The default ``now`` is ``clock.server_now``, not a raw wall clock. + + Without this, the production path would be the only caller outside the + one clock seam (#460) — every other test here passes ``now`` explicitly, + so a drift back to ``datetime.now`` would pass unnoticed. Freezing the + seam must move the verdict. + """ + from mureo.core import clock + + started = datetime(2026, 8, 1, 9, 0, tzinfo=timezone.utc) + record = BatchRecord( + batch_id="batch-seam", label="pass", started_at=started.isoformat() + ) + + monkeypatch.setattr(clock, "server_now", lambda: started + timedelta(hours=1)) + assert batch_open_hours(record) == pytest.approx(1.0) + assert stale_batch_warning(record) is None + + monkeypatch.setattr( + clock, "server_now", lambda: started + timedelta(hours=STALE_AFTER_HOURS) + ) + assert batch_open_hours(record) == pytest.approx(float(STALE_AFTER_HOURS)) + assert stale_batch_warning(record) is not None + + def test_an_unparseable_start_is_not_reported_as_fresh(self) -> None: + """An unknown age must not pass for a small one.""" + record = BatchRecord(batch_id="b", label="l", started_at="not-a-date") + assert batch_open_hours(record) is None + assert stale_batch_warning(record) is None + + @pytest.mark.asyncio + async def test_batch_status_carries_the_warning(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + begin_batch(state_file, label="forgotten") + doc = read_state_file(state_file) + stale = replace( + doc.batches[0], + started_at=( + datetime.now(timezone.utc) - timedelta(hours=STALE_AFTER_HOURS + 2) + ).isoformat(), + ) + write_state_file(state_file, replace(doc, batches=(stale,))) + + payload = _payload(await handle_batch_tool("mureo_batch_status", {})) + assert payload["warning"] is not None + assert "mureo_batch_end" in payload["warning"] + + @pytest.mark.asyncio + async def test_reminder_fires_only_while_a_stale_batch_is_open( + self, workspace: Path + ) -> None: + from mureo.mcp._handlers_batch import maybe_build_batch_reminder + + assert maybe_build_batch_reminder() is None # nothing open + + state_file = workspace / "STATE.json" + begin_batch(state_file, label="forgotten") + assert maybe_build_batch_reminder() is None # open, but fresh + + doc = read_state_file(state_file) + stale = replace( + doc.batches[0], + started_at=( + datetime.now(timezone.utc) - timedelta(hours=STALE_AFTER_HOURS + 2) + ).isoformat(), + ) + write_state_file(state_file, replace(doc, batches=(stale,))) + assert maybe_build_batch_reminder() is not None + + @pytest.mark.asyncio + async def test_reminder_can_be_disabled( + self, workspace: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from mureo.mcp._handlers_batch import maybe_build_batch_reminder + + state_file = workspace / "STATE.json" + begin_batch(state_file, label="forgotten") + doc = read_state_file(state_file) + stale = replace( + doc.batches[0], + started_at=( + datetime.now(timezone.utc) - timedelta(hours=STALE_AFTER_HOURS + 2) + ).isoformat(), + ) + write_state_file(state_file, replace(doc, batches=(stale,))) + assert maybe_build_batch_reminder() is not None + + monkeypatch.setenv("MUREO_DISABLE_BATCH_REMINDER", "1") + assert maybe_build_batch_reminder() is None + + +# --------------------------------------------------------------------------- +# 2/3. Coverage + honesty — the plan reports every member and every gap +# --------------------------------------------------------------------------- + + +def _mixed_batch(state_file: Path) -> str: + """Write a four-member, three-platform batch with mixed reversibility.""" + batch = begin_batch(state_file, label="monday bulk pass") + append_action_log( + state_file, + _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ), + ) + append_action_log( + state_file, + _entry( + "meta_ads_campaigns_pause", + "meta_ads", + reversible_params=_META_REVERSAL_WITH_CAVEAT, + ), + ) + append_action_log( + state_file, + _entry( + "amazon_ads_negative_keywords_create", + _PLUGIN_PLATFORM, + reversible_params=_BRIDGED_REVERSAL, + ), + ) + append_action_log( + state_file, + _entry("tiktok_ads_ad_groups_update", "tiktok_ads"), + ) + end_batch(state_file) + return batch.batch_id + + +@pytest.mark.unit +class TestBatchPlanCoverage: + def test_plan_covers_every_member_of_the_batch(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + batch_id = _mixed_batch(state_file) + plan = plan_batch_rollback(read_state_file(state_file), batch_id) + + assert plan.batch_id == batch_id + assert plan.label == "monday bulk pass" + assert [m.index for m in plan.members] == [0, 1, 2, 3] + + def test_partial_reversibility_is_reported_before_anything_is_applied( + self, workspace: Path + ) -> None: + state_file = workspace / "STATE.json" + batch_id = _mixed_batch(state_file) + before = read_state_file(state_file) + plan = plan_batch_rollback(before, batch_id) + + statuses = {m.index: m.status for m in plan.members} + assert statuses[0] is BatchMemberStatus.REVERSIBLE + assert statuses[1] is BatchMemberStatus.REVERSIBLE_WITH_CAVEATS + # The bridged member names an operation mureo cannot dispatch. + assert statuses[2] is BatchMemberStatus.IRREVERSIBLE + # The hosted-connector member was recorded with no reversal hint. + assert statuses[3] is BatchMemberStatus.IRREVERSIBLE + + assert plan.coverage is BatchCoverage.PARTIAL + # Every irreversible member states WHY, in the plan, up front. + assert all(m.reason for m in plan.members if not m.is_reversible) + # Planning is pure: nothing was applied, nothing was written. + assert read_state_file(state_file).action_log == before.action_log + + def test_coverage_is_reported_per_platform(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + batch_id = _mixed_batch(state_file) + plan = plan_batch_rollback(read_state_file(state_file), batch_id) + + coverage = dict(plan.platform_coverage) + assert coverage["google_ads"] is BatchCoverage.FULL + assert coverage["meta_ads"] is BatchCoverage.FULL + assert coverage[_PLUGIN_PLATFORM] is BatchCoverage.NONE + assert coverage["tiktok_ads"] is BatchCoverage.NONE + + def test_fully_reversible_batch_reports_full(self, workspace: Path) -> None: + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="two platforms, both reversible") + append_action_log( + state_file, + _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ), + ) + append_action_log( + state_file, + _entry( + "meta_ads_ad_sets_pause", "meta_ads", reversible_params=_META_REVERSAL + ), + ) + end_batch(state_file) + plan = plan_batch_rollback(read_state_file(state_file), batch.batch_id) + assert plan.coverage is BatchCoverage.FULL + assert plan.apply_order == (1, 0) + + def test_already_reversed_member_is_not_offered_again( + self, workspace: Path + ) -> None: + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="one member") + append_action_log( + state_file, + _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ), + ) + end_batch(state_file) + append_action_log( + state_file, + ActionLogEntry( + timestamp="2026-08-07T11:00:00+09:00", + action="google_ads_campaigns_update_status", + platform="google_ads", + rollback_of=0, + ), + ) + plan = plan_batch_rollback(read_state_file(state_file), batch.batch_id) + assert plan.members[0].status is BatchMemberStatus.ALREADY_REVERSED + assert plan.apply_order == () + + def test_native_read_only_member_is_not_counted_as_a_gap( + self, workspace: Path + ) -> None: + """A read in the batch is not something the operator must undo by hand. + + KNOWN DEFECT, pinned deliberately — read this before "fixing" it. + + **What is wrong.** ``mureo.core.tool_names.is_read_only_tool_name`` + anchors its verbs at the START of a hyphen-delimited name segment + (``list_campaigns``), but mureo's own tools put the verb at the END + (``google_ads_campaigns_list``). So:: + + is_read_only_tool_name("google_ads_campaigns_list") # False, wrong + + A NATIVE read therefore reaches ``plan_rollback`` as a write with no + ``reversible_params`` hint and is classified IRREVERSIBLE instead of + NOTHING_TO_REVERSE. The error direction is safe — nothing is offered + for reversal that should not be — but the batch report shows the + operator a read among the "cannot be reverted" items, which is untrue + and corrodes trust in exactly the surface #549 adds. The bridged + spelling (``campaign_management-list_campaigns``) is matched correctly + today, which is why both are asserted here. + + **Why it is not fixed in the #549 PR.** The obvious fix — also match a + verb at the END of a segment — is wrong, not merely broad. Three + modules share this vocabulary, and one of them gates a DENIAL: + ``mureo.mcp.server._register_pattern_fallbacks`` skips + ``register_pattern_fallback_tool(name)`` when the name reads as a read, + so a name wrongly classified as a read loses its guardrail money + pattern-scan. Measured on a 294-tool installed plugin surface, a naive + suffix rule flips 23 names, and **13 of them are** + ``ToolSemantics(mutating=True)`` — i.e. 13 real mutations would be + newly exempted from the money scan:: + + amc-execute_query + logly_ads_context_merge_adgroup_list + reporting-create_campaign_report + reporting-create_inventory_report + reporting-create_product_report + reporting-create_report + reporting-delete_report <- a DELETE reading as a read + yahoo_ads_create_placement_url_list + yahoo_ads_display_create_placement_url_list + yahoo_ads_display_remove_placement_url_list + yahoo_ads_display_update_placement_url_list + yahoo_ads_remove_placement_url_list + yahoo_ads_update_placement_url_list + + (On the native side the same rule flips 70 of 208 names, none carrying + a write verb — the native direction alone is safe.) + + **What a correct fix must do.** Match a trailing verb only when no + write verb (``create`` / ``update`` / ``delete`` / ``remove`` / ``set`` + / ``add`` / ``merge`` / ``execute`` …) appears elsewhere in the same + segment, so ``google_ads_campaigns_list`` becomes a read while + ``reporting-delete_report`` and ``yahoo_ads_update_placement_url_list`` + stay writes. It changes plugin guardrail registration, plugin + ``derive_semantics`` classification and ``mureo rollback list`` output, + so it needs its own tests in ``test_strategy_gate_pattern_fallback.py``, + ``test_mcp_plugin_semantics.py``, ``test_rollback.py`` and + ``test_cli_rollback.py``. + + **What flips here when it lands.** The ``by_index[0]`` assertion below + becomes ``BatchMemberStatus.NOTHING_TO_REVERSE``. ``by_index[1]``, + ``by_index[2]`` and ``apply_order`` are unchanged. + """ + state_file = workspace / "STATE.json" + batch = begin_batch(state_file, label="a pass that also read things") + append_action_log(state_file, _entry("google_ads_campaigns_list", "google_ads")) + append_action_log( + state_file, + _entry("campaign_management-list_campaigns", _PLUGIN_PLATFORM), + ) + append_action_log( + state_file, + _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ), + ) + end_batch(state_file) + + plan = plan_batch_rollback(read_state_file(state_file), batch.batch_id) + by_index = {m.index: m for m in plan.members} + # Bridged spelling: correctly recognised as a read today. + assert by_index[1].status is BatchMemberStatus.NOTHING_TO_REVERSE + # Native spelling: misclassified today. Flip this to + # NOTHING_TO_REVERSE with the tool_names fix. + assert by_index[0].status is BatchMemberStatus.IRREVERSIBLE + assert by_index[2].status is BatchMemberStatus.REVERSIBLE + # Either way a read is never offered for reversal. + assert plan.apply_order == (2,) + + def test_unknown_batch_id_is_empty_not_a_lie(self, workspace: Path) -> None: + plan = plan_batch_rollback(read_state_file(workspace / "STATE.json"), "nope") + assert plan.coverage is BatchCoverage.EMPTY + assert plan.members == () + + +# --------------------------------------------------------------------------- +# rollback_plan_get — the MCP surface named in the issue +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestRollbackPlanGetBatchMode: + @pytest.mark.asyncio + async def test_plan_get_by_batch_id_returns_every_member( + self, workspace: Path + ) -> None: + batch_id = _mixed_batch(workspace / "STATE.json") + payload = _payload( + await handle_rollback_tool("rollback_plan_get", {"batch_id": batch_id}) + ) + assert payload["batch_id"] == batch_id + assert [m["index"] for m in payload["members"]] == [0, 1, 2, 3] + assert payload["counts"]["total"] == 4 + + @pytest.mark.asyncio + async def test_plan_get_by_batch_id_names_the_irreversible_members( + self, workspace: Path + ) -> None: + batch_id = _mixed_batch(workspace / "STATE.json") + payload = _payload( + await handle_rollback_tool("rollback_plan_get", {"batch_id": batch_id}) + ) + assert payload["coverage"] == "partial" + assert payload["counts"]["irreversible"] == 2 + irreversible = [ + m for m in payload["members"] if m["reversibility"] == "irreversible" + ] + assert {m["platform"] for m in irreversible} == { + _PLUGIN_PLATFORM, + "tiktok_ads", + } + assert all(m["reason"] for m in irreversible) + assert payload["platform_coverage"]["google_ads"] == "full" + assert payload["platform_coverage"][_PLUGIN_PLATFORM] == "none" + + @pytest.mark.asyncio + async def test_plan_get_by_index_is_unchanged(self, workspace: Path) -> None: + """The single-entry contract keeps working byte-for-byte.""" + state_file = workspace / "STATE.json" + append_action_log( + state_file, + _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ), + ) + payload = _payload( + await handle_rollback_tool("rollback_plan_get", {"index": 0}) + ) + assert payload["status"] == "supported" + assert payload["operation"] == "google_ads_campaigns_update_status" + assert "members" not in payload + + @pytest.mark.asyncio + async def test_plan_get_requires_exactly_one_selector( + self, workspace: Path + ) -> None: + for arguments in ({}, {"index": 0, "batch_id": "b"}): + payload = _payload( + await handle_rollback_tool("rollback_plan_get", arguments) + ) + assert payload["plan"] is None + assert "exactly one" in payload["reason"] + + +# --------------------------------------------------------------------------- +# Backward compatibility with pre-#549 STATE.json files +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestBackwardCompatibility: + def test_legacy_action_log_round_trips_without_gaining_keys( + self, tmp_path: Path + ) -> None: + raw = { + "version": "2", + "last_synced_at": None, + "customer_id": None, + "campaigns": [], + "platforms": None, + "action_log": [ + { + "timestamp": "2026-04-15T10:00:00", + "action": "google_ads_budget_update", + "platform": "google_ads", + } + ], + } + path = tmp_path / "STATE.json" + path.write_text(json.dumps(raw, indent=2), encoding="utf-8") + doc = read_state_file(path) + assert doc.action_log[0].batch_id is None + assert doc.batches == () + + write_state_file(path, doc) + written = json.loads(path.read_text(encoding="utf-8")) + assert "batch_id" not in written["action_log"][0] + assert "batches" not in written + + def test_legacy_entries_are_planned_exactly_as_before(self, tmp_path: Path) -> None: + entry = _entry( + "google_ads_campaigns_update_status", + "google_ads", + reversible_params=_GOOGLE_REVERSAL, + ) + from mureo.rollback import plan_rollback + + plan = plan_rollback(entry) + assert plan is not None + assert plan.status.value == "supported" + + +# --------------------------------------------------------------------------- +# Tool registration +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestBatchToolRegistration: + def test_batch_tools_are_named_and_registered(self) -> None: + assert {t.name for t in BATCH_TOOLS} == { + "mureo_batch_begin", + "mureo_batch_end", + "mureo_batch_status", + } + + @pytest.mark.asyncio + async def test_batch_tools_are_in_the_server_tool_list(self) -> None: + from mureo.mcp.server import handle_list_tools + + names = {t.name for t in await handle_list_tools()} + assert {"mureo_batch_begin", "mureo_batch_end", "mureo_batch_status"} <= names + + @pytest.mark.asyncio + async def test_batch_status_reports_the_open_batch(self, workspace: Path) -> None: + idle = _payload(await handle_batch_tool("mureo_batch_status", {})) + assert idle["active_batch"] is None + begin = _payload( + await handle_batch_tool("mureo_batch_begin", {"label": "pass"}) + ) + status = _payload(await handle_batch_tool("mureo_batch_status", {})) + assert status["active_batch"]["batch_id"] == begin["batch_id"] + assert status["member_count"] == 0 diff --git a/tests/test_dataclass_field_preservation.py b/tests/test_dataclass_field_preservation.py index 9fdfbf4..338ee30 100644 --- a/tests/test_dataclass_field_preservation.py +++ b/tests/test_dataclass_field_preservation.py @@ -76,6 +76,7 @@ from mureo.context.models import ( ActionLogEntry, AdState, + BatchRecord, CampaignSnapshot, PlatformState, StateDocument, @@ -131,6 +132,15 @@ "evaluation_of": 4, "entity_type": "ad_group", "entity_id": "G-1", + "batch_id": "B-20260808-093000-a1b2", +} + +#: One distinctive value per :class:`BatchRecord` field (#549). +_BATCH_FIELD_VALUES: dict[str, Any] = { + "batch_id": "B-20260808-093000-a1b2", + "label": "pause the losing ad groups", + "started_at": "2026-08-08T09:30:00+09:00", + "ended_at": "2026-08-08T09:41:00+09:00", } #: One distinctive value per :class:`CampaignSnapshot` field, minus the id the @@ -173,6 +183,7 @@ def _campaign(campaign_id: str = "C-1") -> CampaignSnapshot: "platforms": {_PLATFORM: PlatformState(**_PLATFORM_FIELD_VALUES)}, "action_log": (ActionLogEntry(**_ACTION_LOG_FIELD_VALUES),), "reports": {"daily": {"narrative": "healthy"}}, + "batches": (BatchRecord(**_BATCH_FIELD_VALUES),), } #: Every model the STATE.json codec maps, with the map that must cover it. @@ -184,6 +195,7 @@ def _campaign(campaign_id: str = "C-1") -> CampaignSnapshot: (ActionLogEntry, _ACTION_LOG_FIELD_VALUES), (CampaignSnapshot, _CAMPAIGN_FIELD_VALUES), (AdState, _AD_STATE_FIELD_VALUES), + (BatchRecord, _BATCH_FIELD_VALUES), ) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 8d14fab..59d7de8 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -64,17 +64,18 @@ class TestListTools: async def test_list_tools_returns_all_tools(self) -> None: """list_tools returns all tools (Google Ads 89 + Meta Ads 90 + Search Console 10 - + Rollback 2 + Analysis 3 + Mureo Context 9 + Analytics Registry 2 - + Learning 2 + Learning pre-flight 1 + Creative Studio 5 = 213). + + Rollback 2 + Batch 3 + Analysis 3 + Mureo Context 9 + Analytics Registry 2 + + Learning 2 + Learning pre-flight 1 + Creative Studio 5 = 216). Analytics Registry is 2: mureo_analytics_modules_list + - mureo_analytics_run (#440). Analysis is 3: analysis_anomalies_check + + mureo_analytics_run (#440). Batch is 3: mureo_batch_begin / _end / + _status (#549). Analysis is 3: analysis_anomalies_check + analysis_exclusion_impact_preview (#547) + analysis_tracking_consistency_check (#550). Learning pre-flight is 1: mureo_learning_reset_preflight (#548).""" mod = _import_server_module() tools = await mod.handle_list_tools() - assert len(tools) == 213 + assert len(tools) == 216 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 46b4fc3..f8ab24b 100644 --- a/tests/test_mcp_tools_mureo_context.py +++ b/tests/test_mcp_tools_mureo_context.py @@ -28,7 +28,7 @@ @pytest.fixture(autouse=True) def _clear_runtime_context_cache(): """Reset the resolver cache before and after every test in this file - so the workspace-aware ``_resolve_path`` rebuilds a + so the workspace-aware ``resolve_workspace_path`` rebuilds a :class:`FilesystemStateStore` with the (per-test) CWD instead of reusing a stale one cached during an earlier test or test module.""" from mureo.core.runtime_context import reset_runtime_context