diff --git a/modules/shallow_versioning.py b/modules/shallow_versioning.py index 48ee8489..fcacebd9 100644 --- a/modules/shallow_versioning.py +++ b/modules/shallow_versioning.py @@ -25,6 +25,29 @@ Concurrency: mutations (track_edit / make_snapshot) run inside a per-state-file process lock as reload -> mutate -> save critical sections, so short-lived manager instances (one per edit / per task end) can never lose each other's rows. Readers only see whole files because writes go through os.replace. + +Diff bases (version fallback) +----------------------------- +The diff shown for a message compares its final snapshot against the previous +message's snapshot. A file first tracked in the target message is absent from +every earlier snapshot: make_snapshot replaces the message's intermediate +track_edit row (which held the pre-edit backup) with the final row, so the +pre-edit state can no longer be found via snapshot pairing. + +It is recovered through two invariants of the backup store: + +- backup file names are deterministic: sha256 of the resolved absolute path, + suffixed with @v; +- backup versions are sequential per file: track_edit creates v1 (pre-edit), + each make_snapshot bumps at most one version, and backup files are never + garbage-collected. + +Hence for a first-appearance backup at version N >= 2, version N-1 is exactly +the pre-message state (for a file created by the message no v1 exists on disk, +so the base stays "missing" and the file correctly shows as added). A +first-appearance version-1 backup means the message left the file unchanged +since track_edit. This also heals state files written before the fix and +survives MAX_SNAPSHOTS eviction of older rows. """ from __future__ import annotations @@ -474,6 +497,40 @@ class ShallowVersioningManager: break return target_snapshot, previous_snapshot + def _resolve_diff_base_backup( + self, + tracking_path: str, + target_backup: FileBackup, + previous_snapshot: Optional[ShallowSnapshot], + ) -> Optional[FileBackup]: + """Resolve the backup representing the file's state BEFORE the target message. + + Normal case: the previous message snapshot references the file, so its + backup is the diff base. A file first tracked in the target message is + absent from every earlier snapshot (its intermediate track_edit row was + replaced by make_snapshot); the pre-message state is then recovered via + the version fallback documented in the module docstring: version N-1 of + the deterministic backup name, used only when that backup file still + exists on disk. A first-appearance version-1 backup means the file was + left unchanged since track_edit, so it is diffed against itself. + """ + if previous_snapshot is not None: + previous = previous_snapshot.tracked_file_backups.get(tracking_path) + if previous is not None: + return previous + if target_backup.version >= 2: + resolved = self._resolve_file_path(tracking_path) + candidate_name = self._backup_file_name(str(resolved), target_backup.version - 1) + if self._backup_path(candidate_name).exists(): + return FileBackup( + backup_file_name=candidate_name, + version=target_backup.version - 1, + backup_time=target_backup.backup_time, + ) + elif target_backup.version == 1 and target_backup.backup_file_name is not None: + return target_backup + return None + def get_diff_stats(self, message_id: str) -> Optional[Dict[str, Any]]: """Compute insertions/deletions/files_changed between the target snapshot and the previous snapshot.""" target_snapshot, previous_snapshot = self._find_snapshot_pair(message_id) @@ -489,7 +546,7 @@ class ShallowVersioningManager: if not target_backup: continue - previous_backup = previous_snapshot.tracked_file_backups.get(tracking_path) if previous_snapshot else None + previous_backup = self._resolve_diff_base_backup(tracking_path, target_backup, previous_snapshot) previous_lines = self._read_backup_lines(previous_backup) target_lines = self._read_backup_lines(target_backup) @@ -525,7 +582,7 @@ class ShallowVersioningManager: if not target_backup: continue - previous_backup = previous_snapshot.tracked_file_backups.get(tracking_path) if previous_snapshot else None + previous_backup = self._resolve_diff_base_backup(tracking_path, target_backup, previous_snapshot) previous_lines = self._read_backup_lines(previous_backup) target_lines = self._read_backup_lines(target_backup) @@ -558,7 +615,7 @@ class ShallowVersioningManager: if not target_backup: return {"lines": [], "truncated": False} - previous_backup = previous_snapshot.tracked_file_backups.get(tracking_path) if previous_snapshot else None + previous_backup = self._resolve_diff_base_backup(tracking_path, target_backup, previous_snapshot) previous_lines = self._read_backup_lines(previous_backup) target_lines = self._read_backup_lines(target_backup) diff --git a/test/test_shallow_versioning.py b/test/test_shallow_versioning.py index ad99def1..9c933dfd 100644 --- a/test/test_shallow_versioning.py +++ b/test/test_shallow_versioning.py @@ -49,18 +49,21 @@ class ShallowVersioningPairingTest(unittest.TestCase): # realistic multi-instance flow (the original bug scenario) # ------------------------------------------------------------------ def test_diff_pairs_with_previous_message_across_instances(self) -> None: + # Production order: track_edit runs BEFORE the tool applies the edit + # (tools_execution backs up first), so for newly created files + # track_edit sees a missing file (v1=None) and make_snapshot stores v2. # message 1: AI creates two new files (each track_edit runs in its own instance) - self._write("a.txt", "a1\na2\n") _new_manager(self.project, self.data).track_edit("a.txt", "msg_1") - self._write("b.txt", "b1\nb2\n") + self._write("a.txt", "a1\na2\n") _new_manager(self.project, self.data).track_edit("b.txt", "msg_1") + self._write("b.txt", "b1\nb2\n") _new_manager(self.project, self.data).make_snapshot("msg_1") # message 2: modify a.txt, create c.txt + _new_manager(self.project, self.data).track_edit("a.txt", "msg_2") # early-return: already tracked self._write("a.txt", "a1\na2 changed\na3\n") - _new_manager(self.project, self.data).track_edit("a.txt", "msg_2") - self._write("c.txt", "c1\n") _new_manager(self.project, self.data).track_edit("c.txt", "msg_2") + self._write("c.txt", "c1\n") _new_manager(self.project, self.data).make_snapshot("msg_2") # state file must hold exactly one row per message @@ -130,15 +133,16 @@ class ShallowVersioningPairingTest(unittest.TestCase): # accidentally correct pre-fix; lazy view was not) # ------------------------------------------------------------------ def test_single_new_file_message(self) -> None: - self._write("a.txt", "a1\n") + # Production order: track_edit before the edit is applied. _new_manager(self.project, self.data).track_edit("a.txt", "msg_1") + self._write("a.txt", "a1\n") _new_manager(self.project, self.data).make_snapshot("msg_1") # msg_2 edits a.txt and tracks one new file b.txt + _new_manager(self.project, self.data).track_edit("a.txt", "msg_2") # early-return: already tracked self._write("a.txt", "a1\na2\n") - _new_manager(self.project, self.data).track_edit("a.txt", "msg_2") - self._write("b.txt", "b1\nb2\nb3\n") _new_manager(self.project, self.data).track_edit("b.txt", "msg_2") + self._write("b.txt", "b1\nb2\nb3\n") _new_manager(self.project, self.data).make_snapshot("msg_2") reader = _new_manager(self.project, self.data) @@ -201,5 +205,128 @@ class ShallowVersioningPairingTest(unittest.TestCase): self.assertIsNone(reader.get_snapshot_by_seq(4)) +class ShallowVersioningFirstEditDiffBaseTest(unittest.TestCase): + """Regression tests for the diff base of files first tracked in a message. + + Production order is: file exists on disk -> track_edit backs up the + pre-edit content (v1) -> the edit is applied -> make_snapshot stores the + post-edit backup (v2), replacing the intermediate track_edit row. The + pre-edit backup then survives only on disk; diffs must recover it via the + version fallback instead of showing the whole file as added (+all -0). + """ + + def setUp(self) -> None: + self.tmp = Path(tempfile.mkdtemp(prefix="shallow_ver_base_test_")) + self.project = self.tmp / "project" + self.data = self.tmp / "data" + self.project.mkdir(parents=True) + self.data.mkdir(parents=True) + + def tearDown(self) -> None: + shutil.rmtree(self.tmp, ignore_errors=True) + + def _write(self, rel: str, content: str) -> None: + path = self.project / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + + # ------------------------------------------------------------------ + # the reported bug: first message edits a pre-existing file + # ------------------------------------------------------------------ + def test_first_message_edit_of_preexisting_file_shows_real_diff(self) -> None: + self._write("main.py", "line1\nline2\nline3\n") + _new_manager(self.project, self.data).track_edit("main.py", "msg_1") + self._write("main.py", "line1\nline2 changed\nline3\nline4\n") + _new_manager(self.project, self.data).make_snapshot("msg_1") + + # the intermediate track_edit row was replaced: only the final row + # remains, referencing v2; the pre-edit v1 survives only on disk. + rows = _read_state_rows(self.data) + self.assertEqual(len(rows), 1) + self.assertEqual(rows[0]["tracked_file_backups"]["main.py"]["version"], 2) + + reader = _new_manager(self.project, self.data) + files = {f["path"]: f for f in reader.get_file_diff_stats("msg_1")} + self.assertEqual(set(files), {"main.py"}) + self.assertEqual(files["main.py"]["status"], "modified") + self.assertEqual((files["main.py"]["insertions"], files["main.py"]["deletions"]), (2, 1)) + stats = reader.get_diff_stats("msg_1") or {} + self.assertEqual((stats.get("insertions"), stats.get("deletions")), (2, 1)) + + patch = reader.get_file_patch_lines("main.py", "msg_1") + adds = [l["content"] for l in patch["lines"] if l["type"] == "add"] + removes = [l["content"] for l in patch["lines"] if l["type"] == "remove"] + self.assertEqual(removes, ["line2"]) + self.assertEqual(adds, ["line2 changed", "line4"]) + + # ------------------------------------------------------------------ + # a file created by the first message must still show as added + # ------------------------------------------------------------------ + def test_first_message_new_file_still_added(self) -> None: + _new_manager(self.project, self.data).track_edit("new.txt", "msg_1") # absent at track time + self._write("new.txt", "n1\nn2\n") + _new_manager(self.project, self.data).make_snapshot("msg_1") + + reader = _new_manager(self.project, self.data) + files = {f["path"]: f for f in reader.get_file_diff_stats("msg_1")} + self.assertEqual(set(files), {"new.txt"}) + self.assertEqual(files["new.txt"]["status"], "added") + self.assertEqual((files["new.txt"]["insertions"], files["new.txt"]["deletions"]), (2, 0)) + + # ------------------------------------------------------------------ + # an edit that leaves content identical yields no changes (v1 kept) + # ------------------------------------------------------------------ + def test_first_message_unchanged_edit_shows_no_changes(self) -> None: + self._write("same.txt", "s1\ns2\n") + _new_manager(self.project, self.data).track_edit("same.txt", "msg_1") + _new_manager(self.project, self.data).make_snapshot("msg_1") + + rows = _read_state_rows(self.data) + self.assertEqual(rows[0]["tracked_file_backups"]["same.txt"]["version"], 1) + reader = _new_manager(self.project, self.data) + self.assertEqual(reader.get_file_diff_stats("msg_1"), []) + self.assertFalse(reader.has_any_changes("msg_1")) + + # ------------------------------------------------------------------ + # first message deletes a pre-existing file -> full deletions + # ------------------------------------------------------------------ + def test_first_message_delete_shows_deletions(self) -> None: + self._write("gone.txt", "g1\ng2\ng3\n") + _new_manager(self.project, self.data).track_edit("gone.txt", "msg_1") + (self.project / "gone.txt").unlink() + _new_manager(self.project, self.data).make_snapshot("msg_1") + + reader = _new_manager(self.project, self.data) + files = {f["path"]: f for f in reader.get_file_diff_stats("msg_1")} + self.assertEqual(set(files), {"gone.txt"}) + self.assertEqual(files["gone.txt"]["status"], "deleted") + self.assertEqual((files["gone.txt"]["insertions"], files["gone.txt"]["deletions"]), (0, 3)) + + # ------------------------------------------------------------------ + # the normal previous-snapshot path keeps winning for later messages + # ------------------------------------------------------------------ + def test_second_message_diff_uses_previous_snapshot_row(self) -> None: + self._write("a.txt", "a1\n") + _new_manager(self.project, self.data).track_edit("a.txt", "msg_1") + self._write("a.txt", "a1\na2\n") + _new_manager(self.project, self.data).make_snapshot("msg_1") + + # already tracked: track_edit early-returns in production, so a plain + # content change followed by make_snapshot is the realistic flow. + self._write("a.txt", "a1\na2\na3\n") + _new_manager(self.project, self.data).make_snapshot("msg_2") + + reader = _new_manager(self.project, self.data) + files = {f["path"]: f for f in reader.get_file_diff_stats("msg_2")} + self.assertEqual(set(files), {"a.txt"}) + self.assertEqual(files["a.txt"]["status"], "modified") + self.assertEqual((files["a.txt"]["insertions"], files["a.txt"]["deletions"]), (1, 0)) + + # msg_1 diff still resolves through the fallback (pre-existing file) + files1 = {f["path"]: f for f in reader.get_file_diff_stats("msg_1")} + self.assertEqual(files1["a.txt"]["status"], "modified") + self.assertEqual((files1["a.txt"]["insertions"], files1["a.txt"]["deletions"]), (1, 0)) + + if __name__ == "__main__": unittest.main()