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
This commit is contained in:
@@ -202,6 +202,104 @@ def test_auto_save_first_save_with_no_recorded_signature_proceeds(iface, board_f
|
||||
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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user