From c2c77f5995665729bbaa35790dd2e8ca2e58fba1 Mon Sep 17 00:00:00 2001 From: Matthew Runo <74583+inktomi@users.noreply.github.com> Date: Mon, 18 May 2026 11:05:26 -0700 Subject: [PATCH] fix(auto-save-guard): refuse only on content divergence, not mtime (#172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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/. (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 --- python/kicad_interface.py | 43 +++++++++++---- tests/test_auto_save_guard.py | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 11 deletions(-) diff --git a/python/kicad_interface.py b/python/kicad_interface.py index 3e9a5d1..bbe421c 100644 --- a/python/kicad_interface.py +++ b/python/kicad_interface.py @@ -624,7 +624,15 @@ class KiCADInterface: @staticmethod def _disk_signature(path: str) -> Optional[Tuple[int, str]]: - """Return (mtime_ns, sha256_hex) for the file, or None if missing/unreadable.""" + """Return (mtime_ns, sha256_hex) for the file, or None if missing/unreadable. + + The sha256 is always recomputed from disk: the conflict guard in + ``_auto_save_board`` compares hashes (content), not mtime, so we + cannot use mtime as a cache key without re-introducing the bug + where two writes inside one mtime tick on a coarse-resolution + filesystem (FAT32, network mounts, etc.) would mask a real + content change. + """ try: st = os.stat(path) h = hashlib.sha256() @@ -702,19 +710,27 @@ class KiCADInterface: expected = self._board_disk_signature current = self._disk_signature(board_path) - # If we have a recorded signature and disk has diverged, refuse to save. - # (If expected is None we treat this as "first save" and proceed — - # otherwise users with pre-existing setups would never be able to save.) - if expected is not None and current is not None and expected != current: + # Only refuse if the file's CONTENT (sha256) has actually diverged + # from what we recorded. mtime alone is not a conflict signal — + # `touch`, atime-driven backups, or even some MCP read paths can + # advance mtime without changing content, and refusing on that + # basis traps users in a state where every write needs an explicit + # save_project workaround. + # + # If expected is None, treat this as "first save" and proceed — + # otherwise pre-existing setups (open_project ran before this guard + # was introduced) would never be able to save. + if expected is not None and current is not None and expected[1] != current[1]: warning = ( - "Auto-save refused: the on-disk PCB file changed externally " - "since this MCP session loaded it. To avoid clobbering those " - "changes, the in-memory mutation has NOT been written to disk. " - "Reload via open_project to refresh, then re-apply the change." + "Auto-save refused: the on-disk PCB file's contents changed " + "externally since this MCP session loaded it. To avoid " + "clobbering those changes, the in-memory mutation has NOT " + "been written to disk. Reload via open_project to refresh, " + "then re-apply the change." ) logger.warning(f"{warning} ({board_path})") - logger.warning(f" expected mtime_ns={expected[0]} sha256={expected[1][:12]}…") - logger.warning(f" current mtime_ns={current[0]} sha256={current[1][:12]}…") + logger.warning(f" expected sha256={expected[1][:12]}… mtime_ns={expected[0]}") + logger.warning(f" current sha256={current[1][:12]}… mtime_ns={current[0]}") return { "saved": False, "warning": warning, @@ -725,6 +741,11 @@ class KiCADInterface: "memChangesUnsaved": True, } + # Content matches but mtime advanced (e.g. external `touch`): refresh + # the recorded mtime so we don't re-hash on every subsequent call. + if expected is not None and current is not None and expected != current: + self._board_disk_signature = current + # Make a rotating backup of the existing file (best-effort). backup_path: Optional[str] = None if current is not None: diff --git a/tests/test_auto_save_guard.py b/tests/test_auto_save_guard.py index fd2d3b5..32623ec 100644 --- a/tests/test_auto_save_guard.py +++ b/tests/test_auto_save_guard.py @@ -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 # ---------------------------------------------------------------------------