fix: recognise a read whose verb is at the end of the name - #585
Merged
Conversation
…n 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.
hyoshi
force-pushed
the
fix/read-only-verb-at-end-of-name
branch
from
August 12, 2026 03:08
aeefeb7 to
80fe81f
Compare
…ow-up) A re-review defeated the guard with campaign_del_list, budget_rm_check and a fullwidth del_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.
hyoshi
added a commit
that referenced
this pull request
Aug 12, 2026
#585 landed while this branch was open. The only conflict was the `_register_plugin_pattern_fallbacks` docstring, which both changes rewrite: #585 added the paragraph explaining why this surface uses the STRICT matcher and the rollback planner a looser sibling, and this branch rewrote the surrounding text to describe declaration-over-name precedence. Both are kept — the two-matcher split is deliberate and is not collapsed here. Two docstrings that #585 left describing the pre-split world are corrected while they are in front of us, since they now claim the exact invariant this branch reinforces: - `plugin_semantics._is_read` said the name vocabulary is shared by three surfaces "so the three surfaces cannot answer 'is this a read?' differently". Since #585 the rollback planner reads that list through `reads_as_a_report_only_action` instead, so the sentence now names two surfaces and points at the sibling for why the third is separate. - `test_the_exemption_uses_the_shared_read_vocabulary` said "one list, two safety surfaces"; it is one list read through two matchers, and what the test actually pins is that both still agree on a verb-FIRST name. No logic changed in this merge. Full suite: 12 failed, 8784 passed — the 12 are the known environment baseline. mypy strict / ruff / black clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From a post-merge review of #569. The defect is older than #569, but #569 is where it first produced a wrong answer an operator can see, and its author pinned it with a KNOWN DEFECT test that specifies the fix and predicts which assertion flips.
This PR was rewritten after review. The first attempt loosened the shared
is_read_only_tool_name; a review defeated its guard and blocked the merge. That critique was correct and this describes the design it forced.The defect
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). So a native read reached the rollback planner as a write with noreversible_paramshint and was classifiedIRREVERSIBLE. A batch containing one showed the operator a read among the items they "cannot revert", and reportedpartialcoverage for a change set that was in fact fully revertible.The direction was safe. What it cost was trust in the surface #549 exists to provide.
Why this is a second predicate rather than a wider one
is_read_only_tool_nameis unchanged. A newreads_as_a_report_only_actionalso accepts a trailing verb, and the rollback planner is its only caller.The shared predicate has three other callers, and every one decides something about plugin tools, whose names mureo does not choose:
server._register_plugin_pattern_fallbacksplugin_semantics.derive_semanticsaction_logat allpolicy.learning_resetWidening the shared rule widens all three at once. The first attempt guarded that with a hand-written verb list; the review defeated it by taking a real installed naming family and swapping the verb:
Each would have silently lost its
## Guardrailscap. 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 exactly those names.The guard that remains, and why it is enough here
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 hides 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 missingpatch,publish,attach,detach,cancelandduplicate; that vocabulary already knew all six.Also
The module docstring claimed two consumers of this vocabulary. There are four:
derive_semantics(#517) andlearning_resetwere never listed. Corrected.Tests
test_batch_revertible_unit.py— the KNOWN DEFECT test flips exactly as its docstring predicted, and the batch now reportsFULLcoverage.test_rollback.py— "native names are unchanged" splits into three, the last parametrized over all 13 real mutations plus the 8 adversarial names the review found, so a vocabulary gap fails here rather than in production.test_strategy_gate_pattern_fallback.py— new cases pinning that the denial gate does not take the looser rule, for verb-last native reads and for the adversarial plugin names alike.Verification
ruff,black,mypyclean.