Skip to content

Commit 89bcf4e

Browse files
committed
fix #134: CAS edit-conflict detection for MCP writes
Client-side head-revision check against the base recorded at read time - MediaWiki suppresses basetimestamp editconflicts for the same user, and human + agent share one bot account, so the server-side check alone can never catch the lost-update case that matters. - every read (get_page/markup/html/sections/preview) records the CAS base - update_page/update_section/commit_edit refuse stale-base writes with a re-read-and-reconcile error; writes without a prior session read refused - create_page refuses existing pages (no overwrite-via-create) - basetimestamp still passed as defense in depth - 26 unit tests (7 new CAS), e2e-verified against the test wiki
1 parent 4c70f12 commit 89bcf4e

3 files changed

Lines changed: 342 additions & 14 deletions

File tree

tests/test_mcp_server.py

Lines changed: 164 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,19 @@ def test_preview_edit_token_generation(self):
129129
self.assertEqual(result["new_content"], "New content")
130130

131131
def test_commit_edit_success(self):
132-
"""Test committing a previewed edit."""
133-
from wikibot3rd.mcp_server import (PREVIEW_STORE, commit_edit_impl,
134-
preview_edit_impl)
132+
"""Test committing a previewed edit (CAS: preview records the base)."""
133+
import time
135134

135+
from wikibot3rd.mcp_server import (PREVIEW_STORE, _page_base,
136+
commit_edit_impl, preview_edit_impl)
137+
138+
_page_base.clear()
139+
base_time = time.strptime("20260705120000", "%Y%m%d%H%M%S")
136140
mock_client = MagicMock()
137141
mock_page = MagicMock()
138142
mock_page.text.return_value = "Old content"
143+
mock_page.name = "Test Page"
144+
mock_page.last_rev_time = base_time
139145
mock_client.get_page.return_value = mock_page
140146

141147
with patch("wikibot3rd.mcp_server.get_wiki_client", return_value=mock_client):
@@ -156,8 +162,13 @@ def test_commit_edit_success(self):
156162

157163
self.assertTrue(result["success"])
158164
self.assertEqual(result["title"], "Test Page")
165+
# CAS (#134): the commit must pass the basetimestamp recorded at preview
159166
mock_client.save_page.assert_called_once_with(
160-
"Test Page", "New content", "Test edit", section=None
167+
"Test Page",
168+
"New content",
169+
"Test edit",
170+
section=None,
171+
basetimestamp="20260705120000",
161172
)
162173
self.assertNotIn(token, PREVIEW_STORE)
163174

@@ -394,5 +405,154 @@ def test_section_numbering_matches_mediawiki(self):
394405
self.assertIsNone(lead["title"])
395406

396407

408+
class TestMCPServerCAS(BaseWikiTest):
409+
"""
410+
Compare-and-swap (CAS) tests for issue #134: update_page must never
411+
silently overwrite a concurrent edit (lost-update), a write requires a
412+
prior read in the session, and create_page never overwrites an
413+
existing page.
414+
"""
415+
416+
def setUp(self, debug=False, profile=True):
417+
super().setUp(debug=debug, profile=profile)
418+
import time
419+
420+
from wikibot3rd.mcp_server import _page_base
421+
422+
_page_base.clear()
423+
self.base_time = time.strptime("20260705120000", "%Y%m%d%H%M%S")
424+
self.mock_client = MagicMock()
425+
self.mock_page = MagicMock()
426+
self.mock_page.name = "Test Page"
427+
self.mock_page.text.return_value = "Old content"
428+
self.mock_page.last_rev_time = self.base_time
429+
self.mock_page.exists = True
430+
self.mock_client.get_page.return_value = self.mock_page
431+
432+
def test_update_requires_prior_read(self):
433+
"""CAS: an update without a prior read in the session is refused."""
434+
from wikibot3rd.mcp_server import update_page_impl
435+
436+
with patch(
437+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
438+
):
439+
with self.assertRaises(ValueError) as context:
440+
update_page_impl(
441+
"test.wiki.org", "Test Page", "New content", "summary"
442+
)
443+
self.assertIn("was not read in this session", str(context.exception))
444+
self.mock_client.save_page.assert_not_called()
445+
446+
def test_update_passes_read_time_basetimestamp(self):
447+
"""CAS: update passes the basetimestamp recorded at READ time."""
448+
from wikibot3rd.mcp_server import get_page_impl, update_page_impl
449+
450+
with patch(
451+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
452+
):
453+
get_page_impl("test.wiki.org", "Test Page")
454+
update_page_impl("test.wiki.org", "Test Page", "New content", "summary")
455+
self.mock_client.save_page.assert_called_once_with(
456+
"Test Page",
457+
"New content",
458+
"summary",
459+
section=None,
460+
basetimestamp="20260705120000",
461+
)
462+
463+
def test_update_detects_same_user_conflict_client_side(self):
464+
"""
465+
CAS: a concurrent edit is detected CLIENT-SIDE by comparing the head
466+
revision to the read-time base. Essential because MediaWiki
467+
suppresses basetimestamp editconflicts for the SAME user, and human +
468+
agent typically share one bot account.
469+
"""
470+
import time
471+
472+
from wikibot3rd.mcp_server import get_page_impl, update_page_impl
473+
474+
with patch(
475+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
476+
):
477+
get_page_impl("test.wiki.org", "Test Page")
478+
# someone (same user!) edited after our read
479+
newer = time.strptime("20260705120500", "%Y%m%d%H%M%S")
480+
self.mock_page.revisions.return_value = iter([{"timestamp": newer}])
481+
with self.assertRaises(ValueError) as context:
482+
update_page_impl(
483+
"test.wiki.org", "Test Page", "New content", "summary"
484+
)
485+
self.assertIn("edit conflict", str(context.exception))
486+
self.mock_client.save_page.assert_not_called()
487+
488+
def test_update_surfaces_edit_conflict(self):
489+
"""CAS: an editconflict from the API becomes a clear ValueError."""
490+
import mwclient.errors
491+
492+
from wikibot3rd.mcp_server import get_page_impl, update_page_impl
493+
494+
self.mock_client.save_page.side_effect = mwclient.errors.APIError(
495+
"editconflict", "Edit conflict detected", {}
496+
)
497+
with patch(
498+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
499+
):
500+
get_page_impl("test.wiki.org", "Test Page")
501+
with self.assertRaises(ValueError) as context:
502+
update_page_impl(
503+
"test.wiki.org", "Test Page", "New content", "summary"
504+
)
505+
self.assertIn("edit conflict", str(context.exception))
506+
self.assertIn("re-read", str(context.exception))
507+
508+
def test_update_section_is_cas_guarded(self):
509+
"""CAS: update_section is guarded the same way as update_page."""
510+
from wikibot3rd.mcp_server import update_section_impl
511+
512+
with patch(
513+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
514+
):
515+
with self.assertRaises(ValueError) as context:
516+
update_section_impl(
517+
"test.wiki.org", "Test Page", "1", "New content", "summary"
518+
)
519+
self.assertIn("was not read in this session", str(context.exception))
520+
521+
def test_create_page_refuses_existing(self):
522+
"""CAS: create_page never overwrites an existing page."""
523+
from wikibot3rd.mcp_server import create_page_impl
524+
525+
with patch(
526+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
527+
):
528+
with self.assertRaises(ValueError) as context:
529+
create_page_impl(
530+
"test.wiki.org", "Test Page", "New content", "summary"
531+
)
532+
self.assertIn("already exists", str(context.exception))
533+
self.mock_client.save_page.assert_not_called()
534+
535+
def test_create_page_new_page_ok(self):
536+
"""CAS: create_page on a missing page works and records the base."""
537+
from wikibot3rd.mcp_server import create_page_impl
538+
539+
self.mock_page.exists = False
540+
saved_page = MagicMock()
541+
saved_page.name = "Test Page"
542+
saved_page.last_rev_time = self.base_time
543+
self.mock_client.save_page.return_value = saved_page
544+
545+
with patch(
546+
"wikibot3rd.mcp_server.get_wiki_client", return_value=self.mock_client
547+
):
548+
result = create_page_impl(
549+
"test.wiki.org", "Test Page", "New content", "summary"
550+
)
551+
self.assertTrue(result["success"])
552+
self.mock_client.save_page.assert_called_once_with(
553+
"Test Page", "New content", "summary"
554+
)
555+
556+
397557
if __name__ == "__main__":
398558
unittest.main()

0 commit comments

Comments
 (0)