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 # ---------------------------------------------------------------------------