From 1d2f95125225908a0c7609857bb93814b25ad744 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:47:51 +0900 Subject: [PATCH 1/4] feat: treat a bulk operation as one revertible unit (#549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 the 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. Batch boundary. A bulk pass is many tool calls and nothing in a single call says which others belong with it, so the boundary is declared, not guessed: mureo_batch_begin / mureo_batch_end / mureo_batch_status. Inferring it from timing or target would be a heuristic, and a heuristic that silently omits a member re-creates the failure this exists to prevent. Membership is stamped where every recording path already converges (append_action_log), inside the state lock — not through tool arguments. That is what makes it platform-agnostic with no per-platform code and no ABI change: a native status toggle, a hosted-connector mutation an agent records, and a bridged/plugin call mureo promotes all join the same batch, including tools whose input schemas mureo does not own. Reversals appended by rollback_apply are excluded, or reverting a batch would grow it. rollback_plan_get accepts batch_id and returns a plan covering EVERY member: 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 listing only the reversible members would read as a complete revert; a batch where 60 of 80 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. Honest limits, documented rather than smoothed over: native mutations other than status toggles join only when the agent records them; a bridged/plugin reversal executes only when it names a registered plugin tool; hosted connectors are never reversed by mureo; Search Console mutations are not in action_log at all and cannot join a batch today. STATE.json gains an optional batches array and an optional batch_id per action_log entry, both emitted only when present — an existing file parses unchanged and gains no new key on the next write. --- AGENTS.md | 9 +- CHANGELOG.md | 43 ++ docs/architecture.md | 10 +- docs/cli.md | 4 + docs/mcp-server.md | 41 +- docs/strategy-context.md | 15 + mureo/_data/skills/_mureo-shared/SKILL.md | 29 + .../_data/skills/search-term-cleanup/SKILL.md | 10 +- mureo/context/__init__.py | 19 + mureo/context/batch.py | 166 ++++++ mureo/context/models.py | 51 ++ mureo/context/state.py | 128 ++++- mureo/context/state_codec.py | 61 ++ mureo/mcp/_handlers_batch.py | 102 ++++ mureo/mcp/_handlers_mureo_context.py | 5 + mureo/mcp/_handlers_rollback.py | 85 ++- mureo/mcp/server.py | 7 + mureo/mcp/tools_batch.py | 116 ++++ mureo/mcp/tools_mureo_context.py | 15 +- mureo/mcp/tools_rollback.py | 27 +- mureo/rollback/__init__.py | 20 +- mureo/rollback/batch.py | 174 ++++++ mureo/rollback/executor.py | 5 +- mureo/rollback/models.py | 117 ++++ skills/_mureo-shared/SKILL.md | 29 + skills/search-term-cleanup/SKILL.md | 10 +- tests/test_batch_revertible_unit.py | 523 ++++++++++++++++++ tests/test_mcp_server.py | 9 +- 28 files changed, 1799 insertions(+), 31 deletions(-) create mode 100644 mureo/context/batch.py create mode 100644 mureo/mcp/_handlers_batch.py create mode 100644 mureo/mcp/tools_batch.py create mode 100644 mureo/rollback/batch.py create mode 100644 tests/test_batch_revertible_unit.py diff --git a/AGENTS.md b/AGENTS.md index 10520d4b..1fbb1901 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,6 +90,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 │ ├── _handlers_analysis.py # Anomaly detector composition handler │ ├── tools_analytics_registry.py # mureo_analytics_modules_list / mureo_analytics_run (#440) @@ -117,14 +119,16 @@ 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 │ └── anomaly_detector.py # Zero-spend / CPA-spike / CTR-drop detection (pure, sample-size-gated) ├── 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) @@ -220,6 +224,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` | | 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 dfe9190a..10bd5c97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,49 @@ 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. + + 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. + ## [0.10.43] - 2026-08-07 ### Changed diff --git a/docs/architecture.md b/docs/architecture.md index 4ff8bd9b..68c0a83e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -128,12 +128,14 @@ mureo/ │ ├── lp_analyzer.py # Landing page analysis │ └── anomaly_detector.py # CPA spike / CTR drop / zero-spend detection with sample-size gates ├── 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) @@ -177,6 +179,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 │ ├── _handlers_analysis.py # Anomaly detector composition handler │ ├── tools_mureo_context.py # STRATEGY.md / STATE.json read-write + outcome eval @@ -268,6 +272,8 @@ mureo assumes the caller is an AI agent susceptible to prompt injection, not a t 3. **Anomaly detection** — `mureo/analysis/anomaly_detector.py` compares current campaign metrics against a median-based baseline built from historical `action_log` entries and emits prioritized alerts for zero spend (CRITICAL), CPA spikes (≥1.5×, critical at 2×), and CTR drops (≤0.5×, critical at 0.3×). Sample-size gates (30+ conversions, 1000+ impressions) follow the `_mureo-learning` skill's statistical-thinking rules to suppress single-day noise. Baselines tolerate malformed `metrics_at_action` rows; CPA/CTR are medianed per-entry so baseline values reflect a real historical day. 4. **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 diff --git a/docs/cli.md b/docs/cli.md index 7be2252b..65997980 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 0613a4ac..55c7440b 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -1,6 +1,6 @@ # MCP Server Guide -mureo exposes 205 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 184 advertising and SEO operation tools across Google Ads (86), Meta Ads (88), and Search Console (10), 2 rollback tools, 1 cross-platform anomaly-detection tool, 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)), 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 208 tools via the [Model Context Protocol](https://modelcontextprotocol.io/) (MCP): 184 advertising and SEO operation tools across Google Ads (86), Meta Ads (88), and Search Console (10), 2 rollback tools, 3 batch tools (group a bulk change into one revertible unit), 1 cross-platform anomaly-detection tool, 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)), 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 @@ -503,11 +503,48 @@ 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`). Refused if none is open. | *(none)* | +| `mureo_batch_status` | Report which batch is collecting (or `null`), how many members it holds, and which platforms they span. Read-only. | *(none)* | + +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 anomaly detection that operates on STATE.json's `action_log` history rather than a platform API directly. diff --git a/docs/strategy-context.md b/docs/strategy-context.md index d20c4d66..82f58810 100644 --- a/docs/strategy-context.md +++ b/docs/strategy-context.md @@ -392,6 +392,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) | @@ -438,9 +439,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); supply it yourself only when importing or backfilling an entry that belongs to a *different* change set, in which case your value wins. 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 | + +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 414ad770..f89727ae 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,27 @@ 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. + +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 +261,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 ### 4. Never Expose Raw Credentials @@ -360,6 +383,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 97b2273a..3184c484 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 da7183a2..61d1e949 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 00000000..81dfaa67 --- /dev/null +++ b/mureo/context/batch.py @@ -0,0 +1,166 @@ +"""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` (``_resolve_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 typing import TYPE_CHECKING + +from mureo.context.models import ActionLogEntry + +if TYPE_CHECKING: + from mureo.context.models import BatchRecord, StateDocument + + +class BatchError(Exception): + """A batch lifecycle call that cannot be honoured. + + Raised for opening a batch while one is already open, and for closing one + when none is. Both are refused rather than resolved silently: flattening a + nested begin would merge two change sets an operator meant to keep apart, + and a no-op end would report a batch that never collected anything. + """ + + +#: 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 + + +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 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. + """ + if batch is None or entry.batch_id is not None: + return entry + return ActionLogEntry( + timestamp=entry.timestamp, + action=entry.action, + platform=entry.platform, + campaign_id=entry.campaign_id, + ad_id=entry.ad_id, + summary=entry.summary, + command=entry.command, + metrics_at_action=entry.metrics_at_action, + observation_due=entry.observation_due, + reversible_params=entry.reversible_params, + rollback_of=entry.rollback_of, + evaluation_of=entry.evaluation_of, + entity_type=entry.entity_type, + entity_id=entry.entity_id, + 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__ = [ + "BatchError", + "active_batch", + "batch_members", + "batch_platforms", + "find_batch", + "new_batch_id", + "stamp_batch", +] diff --git a/mureo/context/models.py b/mureo/context/models.py index 2dd350d2..20a6b8f9 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 a1e9bf7b..ea5c028e 100644 --- a/mureo/context/state.py +++ b/mureo/context/state.py @@ -35,9 +35,16 @@ from mureo.context.models import ActionLogEntry, CampaignSnapshot +from mureo.context.batch import ( + BatchError, + active_batch, + batch_members, + 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, @@ -282,36 +289,138 @@ def _build(doc: StateDocument) -> StateDocument: # summaries the dashboard renders (every upsert after a report write # erased it). reports=doc.reports, + # Preserve the batch records (#549) — same rationale as reports. + batches=doc.batches, ) 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. + + Args: + path: STATE.json location. + entry: The entry to append. An explicit ``batch_id`` on it always wins + over the open batch (see + :func:`mureo.context.batch.stamp_batch`). + 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 """ def _build(doc: StateDocument) -> StateDocument: + stamped = stamp_batch(entry, active_batch(doc)) if join_active_batch else entry return StateDocument( version=doc.version, last_synced_at=doc.last_synced_at, customer_id=doc.customer_id, campaigns=doc.campaigns, platforms=doc.platforms, - action_log=(*doc.action_log, entry), + action_log=(*doc.action_log, stamped), # Preserve the analysis summaries — appending an action must not # wipe the daily/weekly/goal reports the dashboard renders. reports=doc.reports, + batches=doc.batches, ) 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``. @@ -346,6 +455,9 @@ def _build(doc: StateDocument) -> StateDocument: platforms=doc.platforms, action_log=doc.action_log, reports=reports, + # Preserve the batch records (#549): a report write has no batch + # input, so dropping them would silently close a bulk pass. + batches=doc.batches, ) return _locked_state_mutation(path, _build) @@ -445,6 +557,10 @@ def _build(doc: StateDocument) -> StateDocument: platforms=platforms, action_log=doc.action_log, reports=doc.reports, + # Preserve the batch records (#549): none of these mutators has a + # batch input, so dropping them here would silently close a bulk + # pass mid-flight and leave its later members unlabelled. + batches=doc.batches, ) return _locked_state_mutation(path, _build) @@ -518,6 +634,10 @@ def _build(doc: StateDocument) -> StateDocument: platforms=platforms, action_log=doc.action_log, reports=doc.reports, + # Preserve the batch records (#549): none of these mutators has a + # batch input, so dropping them here would silently close a bulk + # pass mid-flight and leave its later members unlabelled. + batches=doc.batches, ) return _locked_state_mutation(path, _build) @@ -541,6 +661,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 b356f791..dced582e 100644 --- a/mureo/context/state_codec.py +++ b/mureo/context/state_codec.py @@ -41,6 +41,7 @@ from mureo.context.models import ( ActionLogEntry, AdState, + BatchRecord, CampaignSnapshot, PlatformState, StateDocument, @@ -193,6 +194,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. @@ -247,6 +282,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")), ) @@ -260,6 +296,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"), @@ -319,6 +356,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) @@ -359,6 +402,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: @@ -376,6 +421,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 00000000..bb8377f0 --- /dev/null +++ b/mureo/mcp/_handlers_batch.py @@ -0,0 +1,102 @@ +"""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 reuses ``_handlers_mureo_context._resolve_path`` rather than +re-implementing it. 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, which is +the one failure mode a sandbox cannot afford. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from mureo.context.batch import ( + BatchError, + active_batch, + batch_members, + batch_platforms, +) +from mureo.context.state import begin_batch, end_batch, read_state_file +from mureo.mcp._handlers_mureo_context import _resolve_path +from mureo.mcp._helpers import _json_result, _require + +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_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. + """ + path = _resolve_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.""" + path = _resolve_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}) + 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)), + } + ) + + +__all__ = ["handle_batch_begin", "handle_batch_end", "handle_batch_status"] diff --git a/mureo/mcp/_handlers_mureo_context.py b/mureo/mcp/_handlers_mureo_context.py index 601a8380..28650c80 100644 --- a/mureo/mcp/_handlers_mureo_context.py +++ b/mureo/mcp/_handlers_mureo_context.py @@ -353,6 +353,11 @@ 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``. Supplying it explicitly is for the import / + # backfill case, where the entry belongs to a change set that is not + # the one open now. + batch_id=raw.get("batch_id"), ) doc = append_action_log(path, entry) return _json_result(_state_to_dict(doc)) diff --git a/mureo/mcp/_handlers_rollback.py b/mureo/mcp/_handlers_rollback.py index 98a577e6..871e151b 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/server.py b/mureo/mcp/server.py index fc350400..1449280f 100644 --- a/mureo/mcp/server.py +++ b/mureo/mcp/server.py @@ -73,6 +73,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, @@ -131,6 +133,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, @@ -145,6 +148,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( @@ -225,6 +229,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 @@ -1001,6 +1006,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: diff --git a/mureo/mcp/tools_batch.py b/mureo/mcp/tools_batch.py new file mode 100644 index 00000000..96b9f111 --- /dev/null +++ b/mureo/mcp/tools_batch.py @@ -0,0 +1,116 @@ +"""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. 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. Read-only." + ), + 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 5db53beb..e5bd22c4 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,18 @@ "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. Supply it only when recording an entry " + "that belongs to a DIFFERENT change set than the one open now " + "— importing or backfilling history — in which case the value " + "given here wins." + ), + }, }, "required": ["action", "platform"], "dependentRequired": { diff --git a/mureo/mcp/tools_rollback.py b/mureo/mcp/tools_rollback.py index 0b5751e1..1cb23adf 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 5584a660..da5272f9 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 00000000..4976171e --- /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 f0181a69..41ff0add 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 22d358c0..d88cc516 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 414ad770..f89727ae 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,27 @@ 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. + +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 +261,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 ### 4. Never Expose Raw Credentials @@ -360,6 +383,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 97b2273a..3184c484 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 00000000..3fada3fc --- /dev/null +++ b/tests/test_batch_revertible_unit.py @@ -0,0 +1,523 @@ +"""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 pathlib import Path +from typing import Any + +import pytest + +from mureo.context.batch import BatchError, active_batch, batch_members, new_batch_id +from mureo.context.models import ActionLogEntry, 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_on_the_entry_wins(self, workspace: Path) -> None: + """An imported / backfilled entry keeps the batch it declares.""" + state_file = workspace / "STATE.json" + begin_batch(state_file, label="open") + foreign = new_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=foreign, + ), + ) + assert read_state_file(state_file).action_log[0].batch_id == foreign + + 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) == () + + +# --------------------------------------------------------------------------- +# 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_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_mcp_server.py b/tests/test_mcp_server.py index 885d3728..94234c3b 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -64,14 +64,15 @@ class TestListTools: async def test_list_tools_returns_all_tools(self) -> None: """list_tools returns all tools (Google Ads 86 + Meta Ads 88 + Search Console 10 - + Rollback 2 + Analysis 1 + Mureo Context 9 + Analytics Registry 2 - + Learning 2 + Creative Studio 5 = 205). + + Rollback 2 + Batch 3 + Analysis 1 + Mureo Context 9 + Analytics Registry 2 + + Learning 2 + Creative Studio 5 = 208). Analytics Registry is 2: mureo_analytics_modules_list + - mureo_analytics_run (#440).""" + mureo_analytics_run (#440). Batch is 3: mureo_batch_begin / _end / + _status (#549).""" mod = _import_server_module() tools = await mod.handle_list_tools() - assert len(tools) == 205 + assert len(tools) == 208 async def test_list_tools_contains_google_and_meta(self) -> None: """Google Ads and Meta Ads tools are included.""" From 8f2681b3ead305c1bc4f3f65c182bbc593ad68da Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 05:40:08 +0900 Subject: [PATCH 2/4] fix: harden batch membership and signal a forgotten batch (#549 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings from PR #569. Membership was not tamper-proof. mureo_state_action_log_append accepted any batch_id string: with no batch ever opened, an append could conjure a change set that rollback_plan_get then reported as legitimate, and an append could rejoin a batch already closed — making the member_count mureo_batch_end had reported silently false. A change set whose membership drifts after it was reported is the "reconstruct it from memory" problem wearing a batch id. An explicit batch_id is now validated, not trusted: it must name a declared batch that is still open. The check sits in append_action_log, inside the lock, so no caller — handler, library user or future recorder — bypasses it. Closing is final; backfill and import declare their own batch rather than retrofitting someone else's, which also gives the imported set an honest label and start time. A forgotten end swallowed everything after it. The asymmetry matters: a missed begin yields no batch, which is obvious and harmless, while a missed end yields a batch that keeps collecting unrelated changes for days and then reports them, confidently, as one unit. Both halves of the signal are now there — mureo_batch_status carries a warning for the caller who asks, and one is appended to every mutating tool result for the caller who forgot and therefore is not asking. The push half is the one that reaches the person with the problem. Nothing is auto-closed: a timeout would trade a visible wrong answer for an invisible one, since entries after it would stop joining with no one told. _resolve_path moved from _handlers_mureo_context to _helpers as resolve_workspace_path. It is the workspace sandbox boundary; a sibling module reaching into another handler's privates to borrow a security check is a place for the two to drift. Tests: batch membership can be neither forged nor grown after close (including the reviewer's exact reproduction through the MCP tool), staleness warns and never auto-closes, an unparseable start is not reported as fresh, and the reminder respects MUREO_DISABLE_BATCH_REMINDER. Also adds a batch containing native and bridged read-only actions, which pins the known is_read_only_tool_name defect (native verbs are suffixes, not prefixes) with the assertion the follow-up PR will flip. --- CHANGELOG.md | 21 ++ docs/mcp-server.md | 8 +- docs/strategy-context.md | 4 +- mureo/_data/skills/_mureo-shared/SKILL.md | 4 +- mureo/context/batch.py | 114 +++++++++- mureo/context/state.py | 19 +- mureo/mcp/_handlers_batch.py | 63 ++++-- mureo/mcp/_handlers_mureo_context.py | 85 ++------ mureo/mcp/_helpers.py | 62 ++++++ mureo/mcp/server.py | 44 +++- mureo/mcp/tools_batch.py | 10 +- mureo/mcp/tools_mureo_context.py | 12 +- skills/_mureo-shared/SKILL.md | 4 +- tests/test_batch_revertible_unit.py | 248 +++++++++++++++++++++- 14 files changed, 588 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10bd5c97..0b5fdb12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,10 +46,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. +### 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. + ## [0.10.43] - 2026-08-07 ### Changed diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 55c7440b..fab5b7e6 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -531,8 +531,12 @@ Declare the boundary of a bulk change so it becomes one reviewable, plannable un | 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`). Refused if none is open. | *(none)* | -| `mureo_batch_status` | Report which batch is collecting (or `null`), how many members it holds, and which platforms they span. Read-only. | *(none)* | +| `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: diff --git a/docs/strategy-context.md b/docs/strategy-context.md index 82f58810..3146b1bb 100644 --- a/docs/strategy-context.md +++ b/docs/strategy-context.md @@ -439,7 +439,7 @@ 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); supply it yourself only when importing or backfilling an entry that belongs to a *different* change set, in which case your value wins. Absent means the action was standalone | +| `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. @@ -452,7 +452,7 @@ Each entry in `batches` is one **declared** bulk change set (#549). A bulk pass | `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 | +| `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). diff --git a/mureo/_data/skills/_mureo-shared/SKILL.md b/mureo/_data/skills/_mureo-shared/SKILL.md index f89727ae..94376e33 100644 --- a/mureo/_data/skills/_mureo-shared/SKILL.md +++ b/mureo/_data/skills/_mureo-shared/SKILL.md @@ -213,7 +213,9 @@ Any pass that changes **more than one entity** — N placement/app exclusions, N 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. +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`. diff --git a/mureo/context/batch.py b/mureo/context/batch.py index 81dfaa67..9879cbff 100644 --- a/mureo/context/batch.py +++ b/mureo/context/batch.py @@ -41,6 +41,7 @@ from __future__ import annotations import secrets +from datetime import datetime, timezone from typing import TYPE_CHECKING from mureo.context.models import ActionLogEntry @@ -52,10 +53,12 @@ class BatchError(Exception): """A batch lifecycle call that cannot be honoured. - Raised for opening a batch while one is already open, and for closing one - when none is. Both are refused rather than resolved silently: flattening a - nested begin would merge two change sets an operator meant to keep apart, - and a no-op end would report a batch that never collected anything. + 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. """ @@ -64,6 +67,14 @@ class BatchError(Exception): #: 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. @@ -103,6 +114,97 @@ def find_batch(doc: StateDocument, batch_id: str) -> BatchRecord | None: 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. + """ + if record.ended_at is not None: + return None + started = _parse_iso(record.started_at) + if started is None: + return None + current = now or datetime.now(timezone.utc) + if current.tzinfo is None: + current = current.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. @@ -156,11 +258,15 @@ def batch_platforms(doc: StateDocument, batch_id: str) -> tuple[str, ...]: __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/state.py b/mureo/context/state.py index ea5c028e..65dccd0e 100644 --- a/mureo/context/state.py +++ b/mureo/context/state.py @@ -39,6 +39,7 @@ BatchError, active_batch, batch_members, + ensure_joinable, new_batch_id, stamp_batch, ) @@ -310,11 +311,17 @@ def append_action_log( 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 always wins - over the open batch (see - :func:`mureo.context.batch.stamp_batch`). + 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 @@ -323,9 +330,15 @@ def append_action_log( 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 return StateDocument( version=doc.version, diff --git a/mureo/mcp/_handlers_batch.py b/mureo/mcp/_handlers_batch.py index bb8377f0..884aaa28 100644 --- a/mureo/mcp/_handlers_batch.py +++ b/mureo/mcp/_handlers_batch.py @@ -5,11 +5,10 @@ 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 reuses ``_handlers_mureo_context._resolve_path`` rather than -re-implementing it. 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, which is -the one failure mode a sandbox cannot afford. +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 @@ -21,10 +20,10 @@ active_batch, batch_members, batch_platforms, + stale_batch_warning, ) from mureo.context.state import begin_batch, end_batch, read_state_file -from mureo.mcp._handlers_mureo_context import _resolve_path -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 @@ -49,7 +48,7 @@ async def handle_batch_begin(arguments: dict[str, Any]) -> list[TextContent]: label = _require(arguments, "label") if not isinstance(label, str) or not label.strip(): raise ValueError("label must be a non-empty string") - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") try: record = begin_batch(path, label=label) except BatchError as exc: @@ -62,9 +61,12 @@ async def handle_batch_end(arguments: dict[str, Any]) -> list[TextContent]: 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. + ``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_path(arguments, "STATE.json", store_attr="state_path") + path = resolve_workspace_path(arguments, "STATE.json", store_attr="state_path") try: record, indices = end_batch(path) except BatchError as exc: @@ -82,12 +84,18 @@ async def handle_batch_end(arguments: dict[str, Any]) -> list[TextContent]: async def handle_batch_status(arguments: dict[str, Any]) -> list[TextContent]: - """Report the open batch (or ``null``) and how much it has collected.""" - path = _resolve_path(arguments, "STATE.json", store_attr="state_path") + """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}) + return _json_result({"active_batch": None, "member_count": 0, "warning": None}) members = batch_members(doc, record.batch_id) return _json_result( { @@ -95,8 +103,35 @@ async def handle_batch_status(arguments: dict[str, Any]) -> list[TextContent]: "member_count": len(members), "member_indices": [index for index, _ in members], "platforms": list(batch_platforms(doc, record.batch_id)), + "warning": stale_batch_warning(record), } ) -__all__ = ["handle_batch_begin", "handle_batch_end", "handle_batch_status"] +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 28650c80..f8956f43 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 @@ -354,12 +302,17 @@ async def handle_state_action_log_append( 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``. Supplying it explicitly is for the import / - # backfill case, where the entry belongs to a change set that is not - # the one open now. + # ``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)) @@ -428,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: @@ -454,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)) @@ -482,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, @@ -510,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/_helpers.py b/mureo/mcp/_helpers.py index a5a27f4c..db1ec95b 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 1449280f..51815877 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.native_reversal import capture_before_state, record_native_mutation from mureo.mcp.plugin_audit import record_plugin_call @@ -798,6 +799,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. @@ -991,13 +1021,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) @@ -1114,6 +1150,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 index 96b9f111..50cb83d3 100644 --- a/mureo/mcp/tools_batch.py +++ b/mureo/mcp/tools_batch.py @@ -75,8 +75,9 @@ "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. Refused if no batch " - "is open." + "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", @@ -89,7 +90,10 @@ 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. Read-only." + "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", diff --git a/mureo/mcp/tools_mureo_context.py b/mureo/mcp/tools_mureo_context.py index e5bd22c4..b6360070 100644 --- a/mureo/mcp/tools_mureo_context.py +++ b/mureo/mcp/tools_mureo_context.py @@ -122,10 +122,14 @@ "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. Supply it only when recording an entry " - "that belongs to a DIFFERENT change set than the one open now " - "— importing or backfilling history — in which case the value " - "given here wins." + "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." ), }, }, diff --git a/skills/_mureo-shared/SKILL.md b/skills/_mureo-shared/SKILL.md index f89727ae..94376e33 100644 --- a/skills/_mureo-shared/SKILL.md +++ b/skills/_mureo-shared/SKILL.md @@ -213,7 +213,9 @@ Any pass that changes **more than one entity** — N placement/app exclusions, N 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. +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`. diff --git a/tests/test_batch_revertible_unit.py b/tests/test_batch_revertible_unit.py index 3fada3fc..c3cef2a2 100644 --- a/tests/test_batch_revertible_unit.py +++ b/tests/test_batch_revertible_unit.py @@ -22,13 +22,22 @@ from __future__ import annotations import json +from dataclasses import replace +from datetime import datetime, timedelta, timezone from pathlib import Path from typing import Any import pytest -from mureo.context.batch import BatchError, active_batch, batch_members, new_batch_id -from mureo.context.models import ActionLogEntry, StateDocument +from mureo.context.batch import ( + STALE_AFTER_HOURS, + BatchError, + active_batch, + batch_members, + batch_open_hours, + stale_batch_warning, +) +from mureo.context.models import ActionLogEntry, BatchRecord, StateDocument from mureo.context.state import ( append_action_log, begin_batch, @@ -197,21 +206,20 @@ 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_on_the_entry_wins(self, workspace: Path) -> None: - """An imported / backfilled entry keeps the batch it declares.""" + 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" - begin_batch(state_file, label="open") - foreign = new_batch_id() + 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=foreign, + batch_id=batch.batch_id, ), ) - assert read_state_file(state_file).action_log[0].batch_id == foreign + 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. @@ -232,6 +240,182 @@ def test_rollback_entries_do_not_join_the_open_batch(self, workspace: Path) -> N assert batch_members(doc, batch.batch_id) == () +@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_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 # --------------------------------------------------------------------------- @@ -365,6 +549,54 @@ def test_already_reversed_member_is_not_offered_again( 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. ``is_read_only_tool_name`` anchors + its verbs at the START of a name segment (``list_campaigns``), but + mureo's own tools put the verb at the END + (``google_ads_campaigns_list``), so a NATIVE read is currently + classified ``irreversible`` rather than ``nothing_to_reverse``. The + error direction is safe — nothing is offered for reversal that should + not be — but it shows the operator a read among the "cannot be + reverted" items, which is not true and corrodes trust in this report. + + The bridged spelling (``campaign_management-list_campaigns``) IS + matched today, so both are asserted here: the fix is to the shared + vocabulary in ``mureo.core.tool_names`` and lands in its own PR, at + which point the native assertion flips to NOTHING_TO_REVERSE and the + bridged one is 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 From 4d880646d25aefbbdd52cbf0b95f845950487127 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 06:19:31 +0900 Subject: [PATCH 3/4] fix: route batch staleness through the server clock seam (#549 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three cleanups from the second review. batch_open_hours fell back to datetime.now(timezone.utc), and both production callers (stale_batch_warning, maybe_build_batch_reminder) omit ``now`` — so the production path was the only caller outside the one clock seam (#460), and every test here passes ``now`` explicitly, meaning a drift back to the wall clock would have gone unnoticed. It now defaults to clock.server_now(), resolved through the MODULE so monkeypatching the seam still works. The import is lazy, and has to be: mureo.core.__init__ -> runtime_context -> state_store -> mureo.context.state -> mureo.context.batch is a real chain, so a module-level ``from mureo.core import clock`` here raises ImportError on a partially initialised module. Verified, not assumed. A test now freezes clock.server_now and asserts the verdict follows it, so the seam is guarded rather than merely used: reverting to datetime.now fails that test and nothing else. Retargets three comments still naming the old private _resolve_path at mureo/context/batch.py, mureo/context/conversion_overrides.py and tests/test_mcp_tools_mureo_context.py. (mureo/policy/declarations._resolve_path is an unrelated function of the same name and is left alone.) Makes the deferred-fix test docstring self-sufficient, since no issue is being filed: it now states the defect with the failing call, names all 13 mutating plugin tools a naive suffix rule would strip of their guardrail money scan (reporting-delete_report among them), says what a correct fix must do and which test files it touches, and names the single assertion that flips when it lands. --- mureo/context/batch.py | 22 +++++- mureo/context/conversion_overrides.py | 3 +- tests/test_batch_revertible_unit.py | 101 ++++++++++++++++++++++---- tests/test_mcp_tools_mureo_context.py | 2 +- 4 files changed, 108 insertions(+), 20 deletions(-) diff --git a/mureo/context/batch.py b/mureo/context/batch.py index 9879cbff..e1acf378 100644 --- a/mureo/context/batch.py +++ b/mureo/context/batch.py @@ -24,7 +24,8 @@ work for native tools only. **Known limit.** The batch lifecycle tools resolve STATE.json through the -active :class:`StateStore` (``_resolve_path``), while the native and plugin +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 @@ -168,15 +169,28 @@ def batch_open_hours(record: BatchRecord, now: datetime | None = None) -> float ``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 - current = now or datetime.now(timezone.utc) - if current.tzinfo is None: - current = current.replace(tzinfo=timezone.utc) + 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 diff --git a/mureo/context/conversion_overrides.py b/mureo/context/conversion_overrides.py index 6d59a743..f48e4090 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/tests/test_batch_revertible_unit.py b/tests/test_batch_revertible_unit.py index c3cef2a2..62d0cb29 100644 --- a/tests/test_batch_revertible_unit.py +++ b/tests/test_batch_revertible_unit.py @@ -349,6 +349,33 @@ def test_a_closed_batch_never_warns(self) -> None: ) 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") @@ -554,20 +581,66 @@ def test_native_read_only_member_is_not_counted_as_a_gap( ) -> None: """A read in the batch is not something the operator must undo by hand. - KNOWN DEFECT, pinned deliberately. ``is_read_only_tool_name`` anchors - its verbs at the START of a name segment (``list_campaigns``), but - mureo's own tools put the verb at the END - (``google_ads_campaigns_list``), so a NATIVE read is currently - classified ``irreversible`` rather than ``nothing_to_reverse``. The - error direction is safe — nothing is offered for reversal that should - not be — but it shows the operator a read among the "cannot be - reverted" items, which is not true and corrodes trust in this report. - - The bridged spelling (``campaign_management-list_campaigns``) IS - matched today, so both are asserted here: the fix is to the shared - vocabulary in ``mureo.core.tool_names`` and lands in its own PR, at - which point the native assertion flips to NOTHING_TO_REVERSE and the - bridged one is unchanged. + 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") diff --git a/tests/test_mcp_tools_mureo_context.py b/tests/test_mcp_tools_mureo_context.py index 46b4fc3c..f8ab24b1 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 From 2d5d5c8928a1b8dba628c5ef18a6ae69f16db5f6 Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Sat, 8 Aug 2026 08:43:30 +0900 Subject: [PATCH 4/4] fix: stamp_batch must not rebuild ActionLogEntry field-by-field (#549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the CRITICAL found reviewing the stacked #545 PR. stamp_batch enumerated ActionLogEntry's fields to produce the batch-stamped copy, so any field added to the dataclass afterwards was dropped the moment an entry joined an open batch. join_active_batch defaults to True and both the import path and mureo_state_action_log_append use the default, so this was the ordinary path. The loss was silent, which is what made it dangerous: a dropped field is indistinguishable downstream from one the caller never set. #545's provenance fields (origin / external_id) went with it, and since is_external is derived from origin, an externally-imported entry lost the marker that stops a forged reversible_params from being planned as a real reversal — turning NOT_SUPPORTED into SUPPORTED. Fixed as a class of bug, not an instance: dataclasses.replace(entry, batch_id=...) carries every field across by construction, so no future field can opt out of batching by omission. Same defect shape as the agency #193 bug where update() rebuilt a registry entry field-by-field and dropped archived. Audited the other six ActionLogEntry construction sites. Five build genuinely new entries from scratch (native_reversal, plugin_semantics, creative_studio x2, rollback executor) and have nothing to carry over. The sixth, state_codec._parse_action_log_entry, enumerates because it must — it maps to an external JSON schema — and its omission is silent in the same way, so the round-trip test below covers it too. Tests are driven off dataclasses.fields(ActionLogEntry), not a hand-written list, so they cannot rot the way the code did: one asserts stamp_batch changes only batch_id, the other that every field survives append_action_log with an open batch and a trip through STATE.json. Adding a field to the dataclass without adding it to the test's value map fails loudly with an explanatory message. --- mureo/context/batch.py | 37 +++++----- tests/test_batch_revertible_unit.py | 105 +++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 21 deletions(-) diff --git a/mureo/context/batch.py b/mureo/context/batch.py index e1acf378..12076958 100644 --- a/mureo/context/batch.py +++ b/mureo/context/batch.py @@ -42,13 +42,12 @@ from __future__ import annotations import secrets +from dataclasses import replace from datetime import datetime, timezone from typing import TYPE_CHECKING -from mureo.context.models import ActionLogEntry - if TYPE_CHECKING: - from mureo.context.models import BatchRecord, StateDocument + from mureo.context.models import ActionLogEntry, BatchRecord, StateDocument class BatchError(Exception): @@ -225,26 +224,24 @@ def stamp_batch(entry: ActionLogEntry, batch: BatchRecord | None) -> ActionLogEn 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 ActionLogEntry( - timestamp=entry.timestamp, - action=entry.action, - platform=entry.platform, - campaign_id=entry.campaign_id, - ad_id=entry.ad_id, - summary=entry.summary, - command=entry.command, - metrics_at_action=entry.metrics_at_action, - observation_due=entry.observation_due, - reversible_params=entry.reversible_params, - rollback_of=entry.rollback_of, - evaluation_of=entry.evaluation_of, - entity_type=entry.entity_type, - entity_id=entry.entity_id, - batch_id=batch.batch_id, - ) + return replace(entry, batch_id=batch.batch_id) def batch_members( diff --git a/tests/test_batch_revertible_unit.py b/tests/test_batch_revertible_unit.py index 62d0cb29..16ead503 100644 --- a/tests/test_batch_revertible_unit.py +++ b/tests/test_batch_revertible_unit.py @@ -22,7 +22,7 @@ from __future__ import annotations import json -from dataclasses import replace +from dataclasses import fields, replace from datetime import datetime, timedelta, timezone from pathlib import Path from typing import Any @@ -36,6 +36,7 @@ batch_members, batch_open_hours, stale_batch_warning, + stamp_batch, ) from mureo.context.models import ActionLogEntry, BatchRecord, StateDocument from mureo.context.state import ( @@ -240,6 +241,108 @@ def test_rollback_entries_do_not_join_the_open_batch(self, workspace: Path) -> N 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.