fix: recognise a read whose verb is at the end of the name

CHANGELOG.md

@@ -9,6 +9,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### 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, 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.
 - **An observed change no longer joins the operator's open batch** (#545 x
   #549). "Imports never join a batch" is stated in `docs/change-import.md`
   without qualification, but it was enforced in one place: the polled

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,14 @@
 
 from __future__ import annotations
 
-__all__ = ["READ_ONLY_PREFIXES", "is_read_only_tool_name"]
+import unicodedata
+
+__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 +82,132 @@ 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.
+#:
+#: 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
+
+    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",
+        # 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",
+        "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.
+    """
+    # 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
+        tokens = segment.split("_")
+        if tokens[-1] in _READ_VERBS and not (set(tokens) & WRITE_VERBS):
+            return True
+    return False

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

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:

tests/test_batch_revertible_unit.py

@@ -757,71 +757,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")
@@ -842,14 +794,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")