From 80fe81fce91743b8e831f1c61d01063d170b2a8c Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:08:05 +0900 Subject: [PATCH 1/2] fix: read a verb-last name as a read, in the one place that can act on it mureo's tools put the verb LAST (google_ads_campaigns_list); the shared read vocabulary matched a verb FIRST only (list_campaigns, the bridged convention). Every native read therefore reached the rollback planner as a write with no reversal hint, so a batch containing one showed the operator a read among the items they "cannot revert" and reported partial coverage for a change set that was in fact fully revertible. The fix adds a SECOND predicate, reads_as_a_report_only_action, used by the rollback planner alone. is_read_only_tool_name is unchanged. That split is the change, not a detail of it. The first attempt at this loosened the shared predicate, and a review showed why that is unsafe: it has three other callers and every one decides something about PLUGIN tools, whose names mureo does not choose -- mcp.server._register_plugin_pattern_fallbacks skips the guardrail money pattern-scan for a name that reads as a read (a DENIAL); mcp.plugin_semantics.derive_semantics decides whether a call is promoted into action_log at all (#517); policy.learning_reset decides whether a change can restart a learning period. Widening the shared rule widens all three at once. The guard against that was a hand-written verb list, and the review defeated it by taking a real installed naming family and swapping the verb: yahoo_ads_patch_placement_url_list, ..._cancel_..., ..._duplicate_..., ..._attach_... each read as a read and would each have lost their money scan. No hardcoded vocabulary can be complete for names mureo does not control, so those three keep the strict rule and a test pins that they do -- including for those very names. The trailing reading still refuses a segment carrying a write verb, because this surface has its own honesty to keep: a plugin mutation misread here would hide a real gap inside a batch claiming full coverage. That is a smaller harm than losing a money guardrail, which is why the guarded reading is acceptable here and nowhere else. The vocabulary is single-sourced from mureo.byod._client_common._MUTATION_PREFIXES, which AGENTS.md calls authoritative, rather than hand-maintained a second time -- the first attempt's list was missing patch, publish, attach, detach, cancel and duplicate, all of which that vocabulary already knew. Also corrects the module docstring, which claimed two consumers of this vocabulary when there are four; derive_semantics (#517) and learning_reset were never listed. Two tests changed premise rather than being deleted, both having pinned the defect on purpose. The KNOWN DEFECT test in test_batch_revertible_unit.py flips as its own docstring predicted, and the batch now reports FULL coverage, which is the operator-visible point. test_rollback.py's "native names are unchanged" splits into a native write still being a write, a native read now being read-only, and a write verb still beating a trailing read verb -- the last parametrized over all thirteen real mutations plus the eight adversarial names the review found, so a vocabulary gap fails here rather than in production. --- CHANGELOG.md | 30 +++++ mureo/core/tool_names.py | 134 ++++++++++++++++++- mureo/rollback/planner.py | 16 ++- tests/test_batch_revertible_unit.py | 90 +++---------- tests/test_rollback.py | 77 ++++++++++- tests/test_strategy_gate_pattern_fallback.py | 38 ++++++ 6 files changed, 305 insertions(+), 80 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b7e837c..0e50f079 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **A read written mureo's own way is now recognised as a read, in the one + place that can safely act on it** (#549 follow-up). mureo's tools put the + verb LAST (`google_ads_campaigns_list`); the shared read vocabulary only + matched a verb FIRST (`list_campaigns`, the bridged convention). Every + native read therefore reached the rollback planner as a write with no + reversal hint, so a batch containing one showed the operator a read among + the items they "cannot revert" and reported `partial` coverage for a change + set that was in fact fully revertible. + + The fix is a SECOND predicate, `reads_as_a_report_only_action`, used by the + rollback planner alone. The shared `is_read_only_tool_name` is unchanged. + + That split is the substance of the change. The shared predicate has three + other callers and all three decide things about PLUGIN tools, whose names + mureo does not choose: the guardrail money pattern-scan registration + (a denial), promotion into `action_log`, and whether a change can restart a + learning period. Loosening the shared rule widens all three at once — and a + mutation admitted there silently loses a `## Guardrails` cap. Since no + hardcoded verb list can be complete for names mureo does not control, those + three keep the strict rule, and a test pins that they do. + + The trailing reading is still refused when a write verb appears in the same + segment, because this surface has its own honesty to keep: a plugin mutation + misread here would hide a real gap inside a batch claiming full coverage. + That vocabulary is single-sourced from + `mureo.byod._client_common._MUTATION_PREFIXES`, which AGENTS.md calls + authoritative, rather than hand-maintained a second time. + ### Added - **Delivery-collapse detection and diagnosis, across all platforms** (#546). diff --git a/mureo/core/tool_names.py b/mureo/core/tool_names.py index a4f2bb74..6dba2eb9 100644 --- a/mureo/core/tool_names.py +++ b/mureo/core/tool_names.py @@ -1,18 +1,31 @@ """Shared vocabulary for reading intent out of a tool name. -Two safety surfaces need the same answer to "does this name describe a read?" -and they must not drift apart: +FOUR surfaces need an answer to "does this name describe a read?", and they +must not drift apart: -- :mod:`mureo.rollback.planner` — a read has nothing to undo, so it plans no - rollback. - :mod:`mureo.mcp.server`'s guardrail pattern-fallback registration — a read moves no money, so subjecting it to a heuristic budget/bid scan could only produce false DENIALS. +- :func:`mureo.mcp.plugin_semantics.derive_semantics` — a read is not promoted + into ``action_log`` (#517). +- :mod:`mureo.policy.learning_reset` — a read cannot restart a learning + period on any platform. +- :mod:`mureo.rollback.planner` — a read has nothing to undo, so it plans no + rollback. -Two copies of the prefix list would eventually disagree, and the disagreement +Copies of the prefix list would eventually disagree, and the disagreement would be silent in both directions (an unrollback-able read here, a denied read there), so the list and the matcher live here once. +**The first three take the strict answer; the fourth takes a looser one.** +:func:`is_read_only_tool_name` matches a verb at the START of a segment and +nothing else. :func:`reads_as_a_report_only_action` also accepts a verb at the +END, which is how mureo names its own tools, and is used by the rollback +planner alone. The split is deliberate and is documented on that function: +the first three decide things about PLUGIN tools, whose names mureo does not +choose, and a mutation admitted there loses a ``## Guardrails`` cap in +silence. Do not collapse the two. + **Namespace-aware by construction.** A bridged MCP server commonly namespaces its tools with a hyphen (``campaign_management-list_campaigns``), and a plain ``startswith`` against the whole name matches none of those. Matching anchors @@ -30,7 +43,12 @@ from __future__ import annotations -__all__ = ["READ_ONLY_PREFIXES", "is_read_only_tool_name"] +__all__ = [ + "READ_ONLY_PREFIXES", + "WRITE_VERBS", + "is_read_only_tool_name", + "reads_as_a_report_only_action", +] #: Verb prefixes that mark a tool name as a read. Anchored at the start of a #: namespace segment (see :func:`is_read_only_tool_name`); the trailing @@ -62,3 +80,107 @@ def is_read_only_tool_name(name: str) -> bool: for segment in lowered.split("-") for prefix in READ_ONLY_PREFIXES ) + + +#: Verbs that make a name a write whatever else it says. +#: +#: Single-sourced from ``mureo.byod._client_common._MUTATION_PREFIXES``, which +#: AGENTS.md calls the authoritative mutation vocabulary, so the two cannot +#: drift and a verb learned in one place is known in the other. The extras +#: below are shapes that vocabulary has no reason to carry (it names Python +#: client methods, not MCP tools). +#: +#: Only ever used to REFUSE a reading, never to assert that a name is a write, +#: so an omission costs a missed read rather than a missed mutation. +def _write_verbs() -> frozenset[str]: + from mureo.byod._client_common import _MUTATION_PREFIXES + + return frozenset(p.rstrip("_") for p in _MUTATION_PREFIXES) | { + "put", + "post", + "write", + "insert", + "replace", + "upsert", + "mutate", + "purge", + "drop", + "clear", + "reset", + "restore", + "archive", + "stop", + "start", + "activate", + "deactivate", + "toggle", + "promote", + "install", + "uninstall", + "register", + "deregister", + "revoke", + "grant", + "link", + "unlink", + "assign", + "schedule", + "rename", + "move", + "copy", + "merge", + "execute", + "run", + "sync", + "import", + } + + +WRITE_VERBS: frozenset[str] = _write_verbs() + +#: The same verbs as :data:`READ_ONLY_PREFIXES`, as bare tokens. +_READ_VERBS: frozenset[str] = frozenset(p.rstrip("_") for p in READ_ONLY_PREFIXES) + + +def reads_as_a_report_only_action(name: str) -> bool: + """Like :func:`is_read_only_tool_name`, but also reads a TRAILING verb. + + **Do not use this to gate a denial or an exemption from one.** It exists + for exactly one caller — the rollback planner deciding whether an + ``action_log`` entry is something the operator could be asked to undo — + and the separation from :func:`is_read_only_tool_name` is the whole design, + not tidiness. + + The problem it solves. mureo's own tools put the verb LAST + (``google_ads_campaigns_list``) while the bridged convention puts it first + (``list_campaigns``), so the prefix-only rule read every native read as a + write. In a rollback batch that surfaced as a read listed among the items + the operator "cannot revert", and a change set that was in fact fully + revertible reporting ``partial`` coverage. + + Why it is not simply added to :func:`is_read_only_tool_name`. That + predicate has three other callers and every one of them is plugin-facing + and safety-relevant: ``mcp.server._register_plugin_pattern_fallbacks`` + skips the guardrail money pattern-scan for a name that reads as a read, + ``mcp.plugin_semantics.derive_semantics`` decides whether a call is + promoted into ``action_log`` at all, and ``policy.learning_reset`` + decides whether a change can restart a learning period. Loosening the + shared predicate widens all three at once, on names mureo does not + control — a plugin can ship any verb it likes. A mutation admitted there + silently loses a ``## Guardrails`` cap, which is the failure this + vocabulary exists to prevent, so those three keep the strict rule. + + The trailing reading is still guarded by :data:`WRITE_VERBS`, because this + surface has its own honesty to keep: a plugin mutation misread here would + be reported as nothing to revert, hiding a real gap in a batch's coverage. + That is a smaller harm than losing a money guardrail, which is why the + guarded rule is acceptable here and not there. + """ + lowered = name.lower() + for segment in lowered.split("-"): + if any(segment.startswith(prefix) for prefix in READ_ONLY_PREFIXES): + return True + tokens = segment.split("_") + if tokens[-1] in _READ_VERBS and not (set(tokens) & WRITE_VERBS): + return True + return False diff --git a/mureo/rollback/planner.py b/mureo/rollback/planner.py index 8c67ba98..55e74ff9 100644 --- a/mureo/rollback/planner.py +++ b/mureo/rollback/planner.py @@ -33,7 +33,10 @@ import copy from typing import TYPE_CHECKING, Any -from mureo.core.tool_names import READ_ONLY_PREFIXES, is_read_only_tool_name +from mureo.core.tool_names import ( + READ_ONLY_PREFIXES, + reads_as_a_report_only_action, +) from mureo.rollback.models import RollbackPlan, RollbackStatus if TYPE_CHECKING: @@ -300,8 +303,17 @@ def _is_read_only(action: str) -> bool: the planner emit a NOT_SUPPORTED plan for an action with nothing to undo. A name with no hyphen is one segment, so native behaviour is unchanged — including the deliberate non-match of mid-word hits like ``listing_update``. + + This surface — and ONLY this surface — also reads a verb at the END of a + segment, because mureo's own tools are named that way + (``google_ads_campaigns_list``) and the prefix-only rule reported every + native read as an item the operator cannot revert. The looser predicate + is deliberately not the shared one: its other three callers all gate + plugin-facing safety decisions, including the guardrail money scan, and + widening those on names mureo does not control is how a mutation loses a + cap. See :func:`mureo.core.tool_names.reads_as_a_report_only_action`. """ - return is_read_only_tool_name(action) + return reads_as_a_report_only_action(action) def _not_supported(entry: ActionLogEntry, *, notes: str) -> RollbackPlan: diff --git a/tests/test_batch_revertible_unit.py b/tests/test_batch_revertible_unit.py index b6ce3669..0b47dc16 100644 --- a/tests/test_batch_revertible_unit.py +++ b/tests/test_batch_revertible_unit.py @@ -693,71 +693,23 @@ 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: + def test_a_read_in_the_batch_is_not_counted_as_a_gap(self, workspace: Path) -> None: """A read in the batch is not something the operator must undo by hand. - KNOWN DEFECT, pinned deliberately — read this before "fixing" it. - - **What is wrong.** ``mureo.core.tool_names.is_read_only_tool_name`` - anchors its verbs at the START of a hyphen-delimited name segment - (``list_campaigns``), but mureo's own tools put the verb at the END - (``google_ads_campaigns_list``). So:: - - is_read_only_tool_name("google_ads_campaigns_list") # False, wrong - - A NATIVE read therefore reaches ``plan_rollback`` as a write with no - ``reversible_params`` hint and is classified IRREVERSIBLE instead of - NOTHING_TO_REVERSE. The error direction is safe — nothing is offered - for reversal that should not be — but the batch report shows the - operator a read among the "cannot be reverted" items, which is untrue - and corrodes trust in exactly the surface #549 adds. The bridged - spelling (``campaign_management-list_campaigns``) is matched correctly - today, which is why both are asserted here. - - **Why it is not fixed in the #549 PR.** The obvious fix — also match a - verb at the END of a segment — is wrong, not merely broad. Three - modules share this vocabulary, and one of them gates a DENIAL: - ``mureo.mcp.server._register_pattern_fallbacks`` skips - ``register_pattern_fallback_tool(name)`` when the name reads as a read, - so a name wrongly classified as a read loses its guardrail money - pattern-scan. Measured on a 294-tool installed plugin surface, a naive - suffix rule flips 23 names, and **13 of them are** - ``ToolSemantics(mutating=True)`` — i.e. 13 real mutations would be - newly exempted from the money scan:: - - amc-execute_query - logly_ads_context_merge_adgroup_list - reporting-create_campaign_report - reporting-create_inventory_report - reporting-create_product_report - reporting-create_report - reporting-delete_report <- a DELETE reading as a read - yahoo_ads_create_placement_url_list - yahoo_ads_display_create_placement_url_list - yahoo_ads_display_remove_placement_url_list - yahoo_ads_display_update_placement_url_list - yahoo_ads_remove_placement_url_list - yahoo_ads_update_placement_url_list - - (On the native side the same rule flips 70 of 208 names, none carrying - a write verb — the native direction alone is safe.) - - **What a correct fix must do.** Match a trailing verb only when no - write verb (``create`` / ``update`` / ``delete`` / ``remove`` / ``set`` - / ``add`` / ``merge`` / ``execute`` …) appears elsewhere in the same - segment, so ``google_ads_campaigns_list`` becomes a read while - ``reporting-delete_report`` and ``yahoo_ads_update_placement_url_list`` - stay writes. It changes plugin guardrail registration, plugin - ``derive_semantics`` classification and ``mureo rollback list`` output, - so it needs its own tests in ``test_strategy_gate_pattern_fallback.py``, - ``test_mcp_plugin_semantics.py``, ``test_rollback.py`` and - ``test_cli_rollback.py``. - - **What flips here when it lands.** The ``by_index[0]`` assertion below - becomes ``BatchMemberStatus.NOTHING_TO_REVERSE``. ``by_index[1]``, - ``by_index[2]`` and ``apply_order`` are unchanged. + Both spellings of a read are recognised now. The bridged convention + puts the verb first (``campaign_management-list_campaigns``); mureo's + own puts it last (``google_ads_campaigns_list``), and for a long time + only the first was matched — so every native read in a batch reached + ``plan_rollback`` as a write with no ``reversible_params`` hint and + was reported IRREVERSIBLE. The direction was safe (nothing was ever + offered for reversal that should not be) but the operator was shown a + read among the "cannot be reverted" items, which is untrue and + corrodes the exact surface #549 adds. + + The trailing reading is guarded by ``WRITE_VERBS`` — see + ``mureo.core.tool_names`` and the write-verb cases in + ``tests/test_rollback.py``, which is where the measurement behind that + guard is recorded. """ state_file = workspace / "STATE.json" batch = begin_batch(state_file, label="a pass that also read things") @@ -778,14 +730,16 @@ def test_native_read_only_member_is_not_counted_as_a_gap( 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. + # Native spelling, verb last. + assert by_index[0].status is BatchMemberStatus.NOTHING_TO_REVERSE + # Bridged spelling, verb first. 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. + # A read is never offered for reversal, and with both reads now + # recognised the batch reports as fully revertible rather than + # showing the operator a read they must somehow undo. assert plan.apply_order == (2,) + assert plan.coverage is BatchCoverage.FULL 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") diff --git a/tests/test_rollback.py b/tests/test_rollback.py index 86cd578a..ca6d792c 100644 --- a/tests/test_rollback.py +++ b/tests/test_rollback.py @@ -440,14 +440,83 @@ def test_a_hyphen_namespaced_write_is_still_plannable(self, monkeypatch) -> None @pytest.mark.parametrize( "action", [ - # Native names keep their exact behaviour: the read-only prefixes - # anchor at the start of the name, never mid-word. - "google_ads_campaigns_list", "update_budget", + # A verb that is only a prefix of a longer word is still not a + # verb: `listing` is not `list`, and the trailing-verb reading + # tokenises on `_` for the same reason. "listing_update", + "getter_config", + ], + ) + def test_a_native_write_is_still_a_write(self, action: str) -> None: + entry = _entry(action=action, reversible_params=None) + plan = plan_rollback(entry) + assert plan is not None + assert plan.status == RollbackStatus.NOT_SUPPORTED + + @pytest.mark.parametrize( + "action", + [ + # mureo's own convention puts the verb at the END, which the + # prefix-only rule never matched — so every native read reached + # here as a write with no hint and was reported as something the + # operator has to undo by hand. + "google_ads_campaigns_list", + "google_ads_budget_get", + "google_ads_search_terms_report", + "analysis_anomalies_check", + ], + ) + def test_a_native_read_is_read_only(self, action: str) -> None: + assert plan_rollback(_entry(action=action, reversible_params=None)) is None + + @pytest.mark.parametrize( + "action", + [ + # Real names from the installed plugin surface; each a mutation + # whose segment happens to END in a read verb. + "yahoo_ads_update_placement_url_list", + "yahoo_ads_remove_placement_url_list", + "yahoo_ads_create_placement_url_list", + "yahoo_ads_display_update_placement_url_list", + "yahoo_ads_display_remove_placement_url_list", + "yahoo_ads_display_create_placement_url_list", + "reporting-delete_report", + "reporting-create_report", + "reporting-create_campaign_report", + "reporting-create_inventory_report", + "reporting-create_product_report", + "amc-execute_query", + "logly_ads_context_merge_adgroup_list", + # Verbs no installed tool uses today. A hardcoded vocabulary is + # only as good as the next plugin's naming, so the ones a review + # found missing are pinned here rather than left to be + # rediscovered. + "yahoo_ads_patch_placement_url_list", + "yahoo_ads_replace_placement_url_list", + "yahoo_ads_cancel_placement_url_list", + "yahoo_ads_duplicate_placement_url_list", + "yahoo_ads_attach_placement_url_list", + "yahoo_ads_detach_placement_url_list", + "campaigns-publish_report", + "campaigns-restore_report", ], ) - def test_native_names_are_unchanged(self, action: str) -> None: + def test_a_write_verb_beats_a_trailing_read_verb(self, action: str) -> None: + """The guard that makes the trailing reading safe to have at all. + + This surface reports what an operator can be asked to undo, so a + mutation misread here is a real gap hidden inside a batch that claims + full coverage. The vocabulary is single-sourced from + ``mureo.byod._client_common._MUTATION_PREFIXES``, which AGENTS.md + calls authoritative, so a verb learned there is known here too. + + The guarded reading is confined to THIS surface. The plugin-facing + callers of ``is_read_only_tool_name`` — the guardrail money scan + above all — keep the strict verb-first rule, because mureo does not + name plugin tools and no hardcoded vocabulary can be complete for + names it does not control. + """ entry = _entry(action=action, reversible_params=None) plan = plan_rollback(entry) assert plan is not None diff --git a/tests/test_strategy_gate_pattern_fallback.py b/tests/test_strategy_gate_pattern_fallback.py index 59299432..64995666 100644 --- a/tests/test_strategy_gate_pattern_fallback.py +++ b/tests/test_strategy_gate_pattern_fallback.py @@ -1395,6 +1395,44 @@ def test_mutation_shaped_names_are_still_registered(self, name: str) -> None: _register_plugin_pattern_fallbacks(_semantics_for(name)) assert has_pattern_fallback(name) is True + @pytest.mark.parametrize( + "name", + [ + # mureo's own verb-last convention. The ROLLBACK planner reads + # these as reads (see reads_as_a_report_only_action); this gate + # deliberately does NOT, and that difference is the design. + "google_ads_campaigns_list", + "google_ads_budget_get", + "analysis_anomalies_check", + # A plugin could ship any verb it likes, including one no + # hardcoded vocabulary knows. Widening THIS gate on name shape + # is how such a mutation would silently lose its money scan. + "yahoo_ads_patch_placement_url_list", + "yahoo_ads_cancel_placement_url_list", + "yahoo_ads_duplicate_placement_url_list", + "reporting-delete_report", + "amc-execute_query", + ], + ) + def test_a_trailing_verb_never_exempts_a_plugin_tool(self, name: str) -> None: + """The denial gate keeps the STRICT rule, on purpose. + + A read whose verb is last is recognised for rollback reporting, where + the cost of being wrong is a misleading batch coverage line. It is NOT + recognised here, where the cost of being wrong is a ``## Guardrails`` + budget cap that never fires. mureo does not name plugin tools and + cannot enumerate the verbs a plugin might use, so this gate does not + try: the exemption stays keyed on the verb-first convention alone. + + The cost is stated rather than hidden: a genuine bridged read named + verb-last is scanned here for money-shaped arguments it does not + carry, which wastes a scan and denies nothing. + """ + from mureo.mcp.server import _register_plugin_pattern_fallbacks + + _register_plugin_pattern_fallbacks(_semantics_for(name)) + assert has_pattern_fallback(name) is True + def test_the_exemption_uses_the_shared_read_vocabulary(self) -> None: """One list, two safety surfaces — see mureo.core.tool_names.""" from mureo.core.tool_names import is_read_only_tool_name From 45246eafa71b6d6014abbeb67990586ecfb880ea Mon Sep 17 00:00:00 2001 From: hyoshi <4027404+hyoshi@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:44:49 +0900 Subject: [PATCH 2/2] fix: close the abbreviations the write-verb guard missed (review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A re-review defeated the guard with campaign_del_list, budget_rm_check and a fullwidth ｄｅｌ_list, and showed the consequence end to end: a batch holding yahoo_ads_placement_url_del_list -- a DELETE, with no reversible_params, which is the normal state for a delete -- reported BatchCoverage.FULL while classifying the delete as "nothing to revert". A hidden gap is exactly what the batch report exists to prevent. The vocabulary knew `delete` but not `del`, and compared strings without normalizing, so a fullwidth spelling was a different word. Adds the abbreviations and blunt synonyms, and NFKC-normalizes before matching. This does not make the vocabulary complete and the docstring says so: a verb written in another script is not reachable from a list of English words. That incompleteness is the reason the money scan is guarded by the STRICT matcher rather than this one, which is the design this PR argues for -- so the honest note belongs next to the guard, not in a commit message. Also from the same review: - plugin_semantics and learning_reset, the two strict-matcher consumers with no pin of their own, now each fail if pointed at the loose matcher. Mutation-tested: swapping the predicate previously left all 89 of their tests green. - server.py's docstring claimed the guardrail gate used "the same list and matcher the rollback planner uses". Since the split that is false and, worse, reads as an invitation to unify them again. Corrected there and in test_mcp_plugin_semantics.py, both pointing at the argument for keeping them apart. Left alone deliberately: server.py:452 lets a read-shaped NAME override a plugin's explicit readOnlyHint=False, so a plugin that correctly declares "I mutate" can still lose its guardrail. Pre-existing, outside this diff, and tracked separately -- a declaration should beat a guess, but fixing it means touching ToolSemantics' precedence and belongs in its own change. --- CHANGELOG.md | 9 ++++++- mureo/core/tool_names.py | 29 +++++++++++++++++++++- mureo/mcp/server.py | 12 +++++++-- tests/test_learning_reset_preflight.py | 25 +++++++++++++++++++ tests/test_mcp_plugin_semantics.py | 34 ++++++++++++++++++++++++-- 5 files changed, 103 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e50f079..802623f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,7 +35,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 misread here would hide a real gap inside a batch claiming full coverage. That vocabulary is single-sourced from `mureo.byod._client_common._MUTATION_PREFIXES`, which AGENTS.md calls - authoritative, rather than hand-maintained a second time. + authoritative, rather than hand-maintained a second time, and extended with + abbreviations and blunt synonyms (`del`, `rm`, `kill`, `terminate`, + `revert`, `undo`, …) after a review defeated the first list with + `campaign_del_list`. Names are NFKC-normalized before matching, so a + fullwidth `ｄｅｌ` is the same verb as `del`. None of that makes the + vocabulary complete — a verb in another script is not reachable from a list + of English words — which is precisely why the strict matcher, and not this + one, guards the money scan. ### Added diff --git a/mureo/core/tool_names.py b/mureo/core/tool_names.py index 6dba2eb9..cc2e08e5 100644 --- a/mureo/core/tool_names.py +++ b/mureo/core/tool_names.py @@ -43,6 +43,8 @@ from __future__ import annotations +import unicodedata + __all__ = [ "READ_ONLY_PREFIXES", "WRITE_VERBS", @@ -92,6 +94,10 @@ def is_read_only_tool_name(name: str) -> bool: #: #: Only ever used to REFUSE a reading, never to assert that a name is a write, #: so an omission costs a missed read rather than a missed mutation. +#: +#: Computed once at import; the ``_MUTATION_PREFIXES`` import is written inside +#: the function to keep the module-level import block free of a dependency on +#: a higher layer, not to defer it. def _write_verbs() -> frozenset[str]: from mureo.byod._client_common import _MUTATION_PREFIXES @@ -128,6 +134,21 @@ def _write_verbs() -> frozenset[str]: "rename", "move", "copy", + # Abbreviations and blunt synonyms. A review defeated an earlier + # version of this list with `campaign_del_list` and `budget_rm_check`: + # the trailing verb read as a report while the real verb was a + # shortening of one already listed. + "del", + "rm", + "kill", + "nuke", + "terminate", + "revert", + "undo", + "wipe", + "flush", + "prune", + "truncate", "merge", "execute", "run", @@ -176,7 +197,13 @@ def reads_as_a_report_only_action(name: str) -> bool: That is a smaller harm than losing a money guardrail, which is why the guarded rule is acceptable here and not there. """ - lowered = name.lower() + # NFKC first: a fullwidth `ｄｅｌ` is the same verb as `del` to a reader and + # a different string to `in`, and the whole guard is a string comparison. + # It does not make the vocabulary complete — a verb written in another + # script (`削除_list`) is not reachable from a list of English words, and + # this predicate does not pretend otherwise; see the honesty note above + # about what a misread costs on this surface. + lowered = unicodedata.normalize("NFKC", name).lower() for segment in lowered.split("-"): if any(segment.startswith(prefix) for prefix in READ_ONLY_PREFIXES): return True diff --git a/mureo/mcp/server.py b/mureo/mcp/server.py index 024c9e81..abc8e590 100644 --- a/mureo/mcp/server.py +++ b/mureo/mcp/server.py @@ -424,8 +424,16 @@ def _register_plugin_pattern_fallbacks( - ``annotations.readOnlyHint``, when the tool declares it; and - the tool NAME, via the shared read vocabulary in - :mod:`mureo.core.tool_names` (the same list and matcher the rollback - planner uses, single-sourced so the two cannot drift). + :mod:`mureo.core.tool_names`, single-sourced so the surfaces that use + it cannot drift. + + The matcher here is the STRICT one, :func:`~mureo.core.tool_names. + is_read_only_tool_name`, and that is deliberate. The rollback planner uses + a looser sibling that also reads a verb at the END of a name, because + mureo's own tools are named that way; this gate does not, because it + decides about PLUGIN tools whose names mureo does not choose, and a + mutation admitted here silently loses its ``## Guardrails`` cap. Do not + "unify" the two — see that sibling's docstring for the argument. The name check still matters after #517, which taught ``derive_semantics`` the same vocabulary for tools that declare NO ``readOnlyHint``: a plugin is diff --git a/tests/test_learning_reset_preflight.py b/tests/test_learning_reset_preflight.py index 5c75ba40..458f7f9e 100644 --- a/tests/test_learning_reset_preflight.py +++ b/tests/test_learning_reset_preflight.py @@ -569,3 +569,28 @@ def test_plugin_can_advertise_its_own_rules(self) -> None: ) assert classify_change("demo_update_target", {}).risk is ResetRisk.RESETS assert classify_change("demo_update_name", {}).risk is ResetRisk.NO_RESET + + +@pytest.mark.unit +@pytest.mark.parametrize( + "tool_name", + [ + # Verb-last names on a platform with no enumerated trigger list. The + # rollback planner reads these as reads; this consumer must not, or a + # real mutation is declared unable to restart a learning period and + # the pre-flight goes quiet on exactly the change that matters. + "acme_ads_campaigns_list", + "acme_ads_budget_get", + "yahoo_ads_patch_placement_url_list", + "yahoo_ads_cancel_placement_url_list", + "campaign_del_list", + ], +) +def test_a_trailing_verb_does_not_make_a_change_a_read(tool_name: str) -> None: + """Pinned so pointing this consumer at the loose matcher goes red. + + ``mureo.core.tool_names`` deliberately has two matchers: a strict one for + the plugin-facing decisions (this among them) and a looser one, used only + by the rollback planner, that also reads a verb at the end of a name. + """ + assert classify_change(tool_name, {}).risk is not ResetRisk.NO_RESET diff --git a/tests/test_mcp_plugin_semantics.py b/tests/test_mcp_plugin_semantics.py index e4a13df7..d675e21e 100644 --- a/tests/test_mcp_plugin_semantics.py +++ b/tests/test_mcp_plugin_semantics.py @@ -157,8 +157,13 @@ def test_an_explicit_hint_always_wins_over_the_name(self) -> None: assert read.mutating is False def test_the_fallback_uses_the_shared_read_vocabulary(self) -> None: - """Same answer as the rollback planner and the guardrail - pattern-fallback registration — three surfaces, one list.""" + """One list across the surfaces that share the STRICT matcher. + + The rollback planner reads the same list through a looser matcher of + its own (it also accepts a verb at the end of a name). This surface + does not — see ``mureo.core.tool_names.reads_as_a_report_only_action`` + for why the two are separate. + """ from mureo.core.tool_names import is_read_only_tool_name for name in ("billing-list_invoice_summaries", "acme-create_thing"): @@ -166,6 +171,31 @@ def test_the_fallback_uses_the_shared_read_vocabulary(self) -> None: is_read_only_tool_name(name) ) + @pytest.mark.parametrize( + "name", + [ + # Verb-last names. The rollback planner reads these as reads; this + # surface must not, or a plugin mutation stops being recorded in + # action_log at all. + "google_ads_campaigns_list", + "acme_ads_budget_get", + "yahoo_ads_patch_placement_url_list", + "yahoo_ads_cancel_placement_url_list", + "campaign_del_list", + ], + ) + def test_a_trailing_verb_does_not_make_a_plugin_tool_a_read( + self, name: str + ) -> None: + """Pinned so pointing this consumer at the loose matcher goes red. + + ``derive_semantics`` defaults an undeclared tool to mutating, which is + what gets it promoted into ``action_log``. Reading a verb-last name as + a read here would drop the record of a real mutation, and the loss is + silent. + """ + assert derive_semantics(_tool(name=name)).mutating is True + @pytest.mark.unit class TestRecordMutationActionLog: