Files
kicad-mcp-server/tests/test_auto_save_guard.py
Matthew Runo c2c77f5995 fix(auto-save-guard): refuse only on content divergence, not mtime (#172)
* feat: guard SWIG auto-save against external file changes

After every board-mutating SWIG command, kicad_interface._auto_save_board()
unconditionally calls pcbnew.SaveBoard() with the in-memory board. When the
on-disk .kicad_pcb has been modified externally between our LoadBoard and
SaveBoard (KiCad GUI's own save, git checkout, another process), the
in-memory state silently overwrites those external changes - losing data
the user can't see was at risk.

This change records the file's mtime_ns + sha256 at LoadBoard and verifies
the signature matches before each auto-save:

  * If the signature has diverged, refuse the save and attach a structured
    warning to the command result so callers know their mutation is
    in-memory only and they need to reload before retrying.
  * If it matches, copy the existing file to .mcp-backups/<name>.<ts>
    (rotating, keeps last 20) before overwriting.
  * Update the recorded signature after our own writes so subsequent
    saves are not falsely flagged.

Backwards compatible:
  * No tool schemas changed.
  * Successful saves return as before, with an extra `autoSave` field
    when the wrapper observed something noteworthy.
  * Refused saves return success: true (the in-memory mutation did
    succeed) plus warnings: [...] and autoSave.diskChangedExternally,
    so callers can detect the situation programmatically.

Adds tests/test_auto_save_guard.py (10 tests, all passing) covering:
signature math, refusal on external change, backup creation + content,
backup rotation, first-save semantics (no recorded signature proceeds
normally), and skip cases (no board / no path).

Motivation: the aircam-pdb fork-user lost ~480 traces and the full
footprint layout to a silent overwrite incident on 2026-05-03; recovery
was only possible because VS Code's local-history extension happened to
have a snapshot from a few minutes earlier. This guard makes that class
of incident loud and locally recoverable.

* fix(auto-save-guard): refuse only on content divergence, not mtime

The guard added in 9ba0010 records `(mtime_ns, sha256)` as the file's
disk signature and refuses auto-save when the recorded tuple no longer
matches the current one. Comparing the full tuple meant any mtime delta
fired the refusal — including a bare `touch` of the file, an atime-style
backup tool, or any MCP read path that opened the .kicad_pcb between
load and save. Users were trapped: every write needed an explicit
save_project call to bypass the false positive (documented as a
workaround in fork users' notes).

Compare on sha256 only; mtime is incidental. The actual data-loss
scenario the guard is meant to catch — an external write that genuinely
changed the file — produces a different content hash, which is what the
guard now keys off. After a touch-only mtime advance with content
unchanged, refresh the recorded signature so we don't re-hash on every
subsequent call.

Drops the mtime-equality fast path on _disk_signature: a filesystem with
coarse mtime resolution (FAT32, some network mounts) could accept two
writes inside one mtime tick; trusting mtime as a hash cache key would
re-introduce a class of silent overwrites the guard exists to prevent.
The hash itself is cheap (sha256 over a typical .kicad_pcb completes in
tens of milliseconds).

Adds 4 regression tests in test_auto_save_guard.py:
  - touch-only mtime advance proceeds and refreshes the signature
  - content change at the same mtime is still refused (hash divergence
    must drive the decision, not tuple equality)
  - the user-facing warning calls out "contents", not "mtime"
  - _disk_signature returns the same hash when content is unchanged
    even after the file's mtime advances
2026-05-18 14:05:26 -04:00

325 lines
12 KiB
Python

"""
Tests for the auto-save guard in kicad_interface._auto_save_board.
The guard is meant to prevent the MCP server from silently overwriting a
.kicad_pcb file that was modified externally between LoadBoard and
SaveBoard (e.g. by KiCad GUI's own save, a git checkout, or another
process). Behaviour exercised here:
- First-load semantics: with no recorded signature, auto-save proceeds.
- Detect external change: when the on-disk file has been altered since
the recorded signature, auto-save is refused and the in-memory
mutation is NOT written to disk.
- Backup creation: a successful save copies the prior file contents to
`.mcp-backups/<name>.<timestamp>` before overwriting.
- Backup pruning: only the most recent N backups are retained.
- Signature update: after a successful save, the recorded signature is
refreshed so subsequent saves are not falsely flagged.
"""
import os
import sys
import time
from pathlib import Path
from typing import Any
from unittest.mock import MagicMock, patch
import pytest
sys.path.insert(0, str(Path(__file__).parent.parent / "python"))
def _make_iface() -> Any:
"""Construct a KiCADInterface bypassing __init__ (avoids pcbnew / IPC)."""
with patch("kicad_interface.USE_IPC_BACKEND", False):
from kicad_interface import KiCADInterface
iface = KiCADInterface.__new__(KiCADInterface)
iface.board = None
iface._board_disk_signature = None
iface._auto_save_backup_keep = 5
return iface
@pytest.fixture()
def iface():
return _make_iface()
@pytest.fixture()
def board_file(tmp_path: Path) -> Path:
"""A temp .kicad_pcb file with placeholder contents."""
f = tmp_path / "test.kicad_pcb"
f.write_text("(kicad_pcb (version 1) (generator test))\n")
return f
def _fake_board(path: str) -> MagicMock:
"""A MagicMock that quacks like a pcbnew BOARD for our helpers."""
b = MagicMock()
b.GetFileName.return_value = path
return b
# ---------------------------------------------------------------------------
# _disk_signature: read-only, no side effects
# ---------------------------------------------------------------------------
def test_disk_signature_returns_mtime_and_hash(iface, board_file):
sig = iface._disk_signature(str(board_file))
assert sig is not None
mtime_ns, sha = sig
assert isinstance(mtime_ns, int) and mtime_ns > 0
assert isinstance(sha, str) and len(sha) == 64 # sha256 hex
def test_disk_signature_returns_none_for_missing_file(iface, tmp_path: Path):
assert iface._disk_signature(str(tmp_path / "does-not-exist.kicad_pcb")) is None
def test_disk_signature_changes_when_file_changes(iface, board_file):
s1 = iface._disk_signature(str(board_file))
# ensure mtime tick (filesystems vary; nanoseconds usually suffice but
# add a small sleep for resolutions that don't)
time.sleep(0.01)
board_file.write_text(board_file.read_text() + "; modified\n")
s2 = iface._disk_signature(str(board_file))
assert s1 != s2
assert s1[1] != s2[1] # hash differs
# ---------------------------------------------------------------------------
# _auto_save_board: skip cases (no board / no path)
# ---------------------------------------------------------------------------
def test_auto_save_skips_when_no_board(iface):
iface.board = None
result = iface._auto_save_board()
assert result == {"saved": False, "skipped": "no board loaded"}
def test_auto_save_skips_when_no_path(iface):
iface.board = MagicMock()
iface.board.GetFileName.return_value = ""
result = iface._auto_save_board()
assert result["saved"] is False
assert "skipped" in result
# ---------------------------------------------------------------------------
# _auto_save_board: happy-path save with signature tracking + backup
# ---------------------------------------------------------------------------
def test_auto_save_with_matching_signature_proceeds(iface, board_file):
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
pre_sig = iface._board_disk_signature
assert pre_sig is not None
save_calls = []
def fake_save(path, board):
save_calls.append((path, board))
# Simulate pcbnew rewriting the file
Path(path).write_text("(kicad_pcb (version 1) (generator test) ; saved)\n")
with patch("kicad_interface.pcbnew") as mock_pcb:
mock_pcb.SaveBoard.side_effect = fake_save
result = iface._auto_save_board()
assert result["saved"] is True
assert result["boardPath"] == str(board_file)
assert len(save_calls) == 1
# Signature should have been refreshed
assert iface._board_disk_signature is not None
assert iface._board_disk_signature != pre_sig
def test_auto_save_creates_backup_before_writing(iface, board_file):
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
original_contents = board_file.read_text()
def fake_save(path, board):
Path(path).write_text("(kicad_pcb ; overwritten)\n")
with patch("kicad_interface.pcbnew") as mock_pcb:
mock_pcb.SaveBoard.side_effect = fake_save
result = iface._auto_save_board()
assert result["saved"] is True
backup_dir = board_file.parent / ".mcp-backups"
assert backup_dir.is_dir()
backups = list(backup_dir.glob(f"{board_file.name}.*"))
assert len(backups) == 1
# Backup must contain the PRE-save contents (snapshot before overwrite)
assert backups[0].read_text() == original_contents
# Returned path matches the file we created
assert result["backup"] == str(backups[0])
# ---------------------------------------------------------------------------
# _auto_save_board: refuses when disk diverged from recorded signature
# ---------------------------------------------------------------------------
def test_auto_save_refuses_when_disk_changed_externally(iface, board_file):
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
# Simulate an external actor (KiCad GUI, git, another process)
# writing the file after we loaded it.
time.sleep(0.01)
board_file.write_text("(kicad_pcb ; changed by someone else)\n")
with patch("kicad_interface.pcbnew") as mock_pcb:
result = iface._auto_save_board()
assert mock_pcb.SaveBoard.call_count == 0 # MUST NOT save
assert result["saved"] is False
assert result["diskChangedExternally"] is True
assert result["memChangesUnsaved"] is True
assert "warning" in result
# File on disk must still hold the external content, untouched
assert "changed by someone else" in board_file.read_text()
def test_auto_save_first_save_with_no_recorded_signature_proceeds(iface, board_file):
"""If we never loaded the file (e.g. first save_project after create),
treat it as a normal first save rather than refusing."""
iface.board = _fake_board(str(board_file))
iface._board_disk_signature = None # explicit: nothing recorded yet
with patch("kicad_interface.pcbnew") as mock_pcb:
mock_pcb.SaveBoard.side_effect = lambda p, b: Path(p).write_text("first\n")
result = iface._auto_save_board()
assert result["saved"] is True
assert iface._board_disk_signature is not None # now recorded
def test_auto_save_proceeds_when_only_mtime_changed_via_touch(iface, board_file):
"""Touching the file (mtime advances, content unchanged) MUST NOT be
treated as an external write. Earlier behaviour compared the full
(mtime_ns, sha256) tuple, so any `touch` between MCP load and save
triggered a refusal — trapping users in a state where every write
needed an explicit save_project workaround.
"""
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
pre_sig = iface._board_disk_signature
assert pre_sig is not None
# Bump the mtime without changing content (the `touch` case). os.utime
# is more reliable than `Path.touch()` across filesystems.
new_mtime_ns = pre_sig[0] + 5_000_000_000 # +5 s
os.utime(board_file, ns=(new_mtime_ns, new_mtime_ns))
save_calls: list[tuple[Any, Any]] = []
def fake_save(path: str, board: Any) -> None:
save_calls.append((path, board))
Path(path).write_text("(kicad_pcb ; saved by mcp)\n")
with patch("kicad_interface.pcbnew") as mock_pcb:
mock_pcb.SaveBoard.side_effect = fake_save
result = iface._auto_save_board()
assert result["saved"] is True, result
assert "diskChangedExternally" not in result
assert len(save_calls) == 1
# Recorded signature was refreshed after the save.
assert iface._board_disk_signature is not None
assert iface._board_disk_signature != pre_sig
def test_auto_save_refuses_when_content_differs_even_at_same_mtime(iface, board_file):
"""If somehow the on-disk content differs (sha256 mismatch) the guard
must still refuse — this is the actual data-loss scenario the original
PR was guarding against. The signature-comparison path must not
short-circuit out of the content check just because the mtime tuple
happens to match what we recorded.
"""
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
expected = iface._board_disk_signature
assert expected is not None
# Replace contents but force the same mtime as recorded — simulates a
# filesystem with sub-second mtime resolution where two writes within
# the same tick produce different content under the same mtime stamp.
board_file.write_text("(kicad_pcb ; secretly different content)\n")
os.utime(board_file, ns=(expected[0], expected[0]))
with patch("kicad_interface.pcbnew") as mock_pcb:
result = iface._auto_save_board()
assert mock_pcb.SaveBoard.call_count == 0
assert result["saved"] is False
assert result["diskChangedExternally"] is True
assert result["memChangesUnsaved"] is True
def test_auto_save_refuses_message_mentions_contents_not_mtime(iface, board_file):
"""The user-facing warning should describe the failure as a content
conflict, not an mtime mismatch — the guard now blocks only on
sha256 divergence, so the message must reflect that.
"""
iface.board = _fake_board(str(board_file))
iface._record_board_signature()
time.sleep(0.01)
board_file.write_text("(kicad_pcb ; changed by someone else)\n")
with patch("kicad_interface.pcbnew"):
result = iface._auto_save_board()
assert result["saved"] is False
assert "warning" in result
assert "contents" in result["warning"].lower(), result["warning"]
def test_disk_signature_rehashes_when_mtime_advances_with_same_content(iface, board_file):
"""When the file's mtime advances but content is unchanged, _disk_signature
must still return the same sha256 — the touch-only case must not surface
as a content divergence to the auto-save guard.
"""
sig1 = iface._disk_signature(str(board_file))
assert sig1 is not None
new_mtime_ns = sig1[0] + 5_000_000_000
os.utime(board_file, ns=(new_mtime_ns, new_mtime_ns))
sig2 = iface._disk_signature(str(board_file))
assert sig2 is not None
assert sig2[0] == new_mtime_ns
assert sig2[1] == sig1[1] # content unchanged → hash unchanged
# ---------------------------------------------------------------------------
# Backup rotation: keep only N most-recent
# ---------------------------------------------------------------------------
def test_backup_pruning_keeps_only_n_most_recent(iface, board_file):
iface.board = _fake_board(str(board_file))
iface._auto_save_backup_keep = 3
def fake_save(path, board):
Path(path).write_text(f"(kicad_pcb ; save at {time.time_ns()})\n")
with patch("kicad_interface.pcbnew") as mock_pcb:
mock_pcb.SaveBoard.side_effect = fake_save
for _ in range(7):
iface._record_board_signature()
iface._auto_save_board()
time.sleep(0.005) # ensure unique timestamps
backup_dir = board_file.parent / ".mcp-backups"
backups = sorted(backup_dir.glob(f"{board_file.name}.*"))
assert len(backups) == 3