From 0199f6fcdc8fa43939b2d94202ef4a0b22eddf62 Mon Sep 17 00:00:00 2001 From: duang777 Date: Fri, 13 Nov 2026 17:46:03 +0800 Subject: [PATCH] fix(migration): refuse to overwrite diverged target active state `migrate-state --copy-active-state` published the rewritten legacy active-state file over the target path unconditionally, while the runtime copy next to it skips existing files. Re-running a migration (for example `--all-goals` after more legacy Goals appeared, or any retry after the target Goal had been worked) silently replaced the live target Todos with the stale legacy copy: completed Todos reopened and Todos added since the first copy were lost, with no backup and no warning in the preview. Check every target before the first copy and fail closed when an existing target differs from the bytes the migration would publish. An identical rerun and an in-place rewrite of the same file stay idempotent, and the conflict is raised in preview as well as execute so nothing is partially written. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: duang777 --- loopx/state_migration.py | 50 ++++++++++++++++++++++++-- tests/test_state_migration.py | 67 +++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/loopx/state_migration.py b/loopx/state_migration.py index cbd9fc2a18..d11c0a44f9 100644 --- a/loopx/state_migration.py +++ b/loopx/state_migration.py @@ -135,6 +135,37 @@ def copy_rewritten_text_file(source: Path, target: Path, *, goal_id_map: dict[st atomic_write_state_text(target, rewrite_text(text, goal_id_map=goal_id_map, path_map=path_map)) +def _planned_state_bytes(source: Path, *, goal_id_map: dict[str, str], path_map: dict[str, str]) -> bytes: + """The exact bytes ``copy_rewritten_text_file`` publishes for ``source``.""" + try: + text = source.read_text(encoding="utf-8") + except UnicodeDecodeError: + return source.read_bytes() + return rewrite_text(text, goal_id_map=goal_id_map, path_map=path_map).encode("utf-8") + + +def _require_no_active_state_clobber( + source_path: Path, + target_path: Path, + *, + goal_id_map: dict[str, str], + path_map: dict[str, str], +) -> None: + """Refuse to replace a different live target state with the legacy copy. + + A repeated migration (for example ``--all-goals`` after more legacy Goals + appear) must not roll back Todos completed or added at the target since the + first copy. Identical content and in-place rewrites remain idempotent. + """ + if not target_path.exists() or source_path.resolve() == target_path.resolve(): + return + if target_path.read_bytes() != _planned_state_bytes(source_path, goal_id_map=goal_id_map, path_map=path_map): + raise ValueError( + f"migrated active state already exists with different content: {target_path}; " + "refusing to overwrite it with the legacy copy" + ) + + def copy_active_state_files( pairs: list[tuple[dict[str, Any], dict[str, Any]]], *, @@ -142,10 +173,23 @@ def copy_active_state_files( path_map: dict[str, str], execute: bool, ) -> list[dict[str, Any]]: + resolved_pairs = [ + ( + source_goal, + target_goal, + resolve_goal_state(source_goal.get("repo"), source_goal.get("state_file")), + resolve_goal_state(target_goal.get("repo"), target_goal.get("state_file")), + ) + for source_goal, target_goal in pairs + ] + # Check every target before the first copy so a conflict leaves no partial write. + for _source_goal, _target_goal, source_path, target_path in resolved_pairs: + if source_path and target_path and source_path.exists(): + _require_no_active_state_clobber( + source_path, target_path, goal_id_map=goal_id_map, path_map=path_map + ) results: list[dict[str, Any]] = [] - for source_goal, target_goal in pairs: - source_path = resolve_goal_state(source_goal.get("repo"), source_goal.get("state_file")) - target_path = resolve_goal_state(target_goal.get("repo"), target_goal.get("state_file")) + for _source_goal, target_goal, source_path, target_path in resolved_pairs: row: dict[str, Any] = { "goal_id": target_goal.get("id"), "source": str(source_path) if source_path else None, diff --git a/tests/test_state_migration.py b/tests/test_state_migration.py index bf46235a7d..4661e08a23 100644 --- a/tests/test_state_migration.py +++ b/tests/test_state_migration.py @@ -76,3 +76,70 @@ def test_migration_rejects_target_goal_id_collisions_before_writing( for target_repo in target_repos.values() ) assert not (target_runtime / "goals" / "goal-b").exists() + + +def _active_state_migration_fixture(tmp_path: Path) -> dict[str, Path]: + repo = tmp_path / "repo" + source_state = repo / ".codex" / "goals" / "goal-a" / "ACTIVE_GOAL_STATE.md" + source_state.parent.mkdir(parents=True) + source_state.write_text("## Agent Todo\n\n- [ ] legacy todo\n", encoding="utf-8") + legacy_registry = tmp_path / "legacy-registry.json" + legacy_registry.write_text( + json.dumps( + { + "schema_version": "0.1", + "goals": [ + { + "id": "goal-a", + "status": "active", + "repo": str(repo), + "state_file": ".codex/goals/goal-a/ACTIVE_GOAL_STATE.md", + } + ], + } + ), + encoding="utf-8", + ) + return { + "repo": repo, + "legacy_registry": legacy_registry, + "target_registry": repo / ".loopx" / "registry.json", + "target_state": repo / ".loopx" / "goals" / "goal-a" / "ACTIVE_GOAL_STATE.md", + "target_runtime": tmp_path / "target-runtime", + } + + +def _migrate_active_state(paths: dict[str, Path], tmp_path: Path, *, execute: bool) -> dict: + return migrate_legacy_state( + legacy_registry_path=paths["legacy_registry"], + target_registry_path=paths["target_registry"], + legacy_runtime_root=tmp_path / "legacy-runtime", + target_runtime_root=paths["target_runtime"], + goal_ids=["goal-a"], + goal_id_map={}, + path_map={".codex/goals": ".loopx/goals"}, + copy_active_state=True, + copy_runtime=False, + execute=execute, + ) + + +def test_repeated_migration_does_not_overwrite_diverged_target_active_state( + tmp_path: Path, +) -> None: + paths = _active_state_migration_fixture(tmp_path) + first = _migrate_active_state(paths, tmp_path, execute=True) + assert first["active_state"][0]["copied"] is True + # An identical rerun is idempotent. + assert _migrate_active_state(paths, tmp_path, execute=True)["ok"] is True + + progressed = "## Agent Todo\n\n- [x] legacy todo\n- [ ] work added after migration\n" + paths["target_state"].write_text(progressed, encoding="utf-8") + registry_before = paths["target_registry"].read_bytes() + + for execute in (False, True): + with pytest.raises(ValueError, match="already exists with different content"): + _migrate_active_state(paths, tmp_path, execute=execute) + + assert paths["target_state"].read_text(encoding="utf-8") == progressed + assert paths["target_registry"].read_bytes() == registry_before