diff --git a/src/core-skills/bmad/references/setup.md b/src/core-skills/bmad/references/setup.md index c9b3be74f..4ec65a954 100644 --- a/src/core-skills/bmad/references/setup.md +++ b/src/core-skills/bmad/references/setup.md @@ -11,9 +11,10 @@ already set, do not ask again. A second run fills new keys in `config.toml` and module yaml while keeping existing answers, always replaces `_bmad/_config/bmad-help.csv` from this skill's `assets/bmad-help.csv`, and repairs `_bmad/scripts` when that path is -a broken or wrong symlink or a copy that is not byte-identical to -this help's `scripts/`. A right symlink and a byte-identical copy -are left as-is. Never touch `custom/` or existing `*.user.toml`. +a symlink or a copy that is not byte-identical to the packaged `bmad` +skill's `scripts/`. Every symlink is replaced with a plain copy; a +byte-identical copy is left as-is. Never touch `custom/` or existing +`*.user.toml`. Run: ``` @@ -22,6 +23,9 @@ uv run --no-cache "{skill-root}/scripts/setup.py" --project-root "{project-root} ### First run +Successful setup creates `_bmad/scripts` as a plain copied directory and +never attempts to create a symlink. + If that file is missing, ask these three questions first (defaults in parentheses). Do not invent answers. diff --git a/src/core-skills/bmad/scripts/setup.py b/src/core-skills/bmad/scripts/setup.py index af9f4f9fb..6e1af66ba 100644 --- a/src/core-skills/bmad/scripts/setup.py +++ b/src/core-skills/bmad/scripts/setup.py @@ -9,7 +9,6 @@ from __future__ import annotations import argparse import json -import os import shutil import sys import tempfile @@ -139,8 +138,21 @@ def materialize_bmad( # Seed staging so custom/, extra *.user.toml, and leftovers # survive replace_dir. if bmad.exists(): + scripts = bmad / "scripts" + + def ignore_scripts_link( + directory: str, _names: list[str] + ) -> set[str]: + if scripts.is_symlink() and Path(directory) == bmad: + return {"scripts"} + return set() + shutil.copytree( - bmad, staging, dirs_exist_ok=True, symlinks=True + bmad, + staging, + dirs_exist_ok=True, + symlinks=True, + ignore=ignore_scripts_link, ) stage_bmad( staging, @@ -208,35 +220,27 @@ def stringify(table: object) -> dict[str, str]: return out -def scripts_ok(dest: Path, src: Path) -> bool: - if dest.is_symlink(): - try: - return dest.resolve() == src.resolve() - except OSError: - return False - if not dest.is_dir(): - return False - dest_files = {p.name: p.read_bytes() for p in dest.iterdir() if p.is_file()} - src_files = {p.name: p.read_bytes() for p in src.iterdir() if p.is_file()} - return dest_files == src_files - - def ensure_scripts(dest: Path, src: Path) -> None: - # Right symlink or a byte-identical top-level copy stays. - if scripts_ok(dest, src): - return if dest.is_symlink() or dest.is_file(): dest.unlink() elif dest.is_dir(): + dest_items = list(dest.iterdir()) + dest_files = { + p.name: p.read_bytes() + for p in dest_items + if p.is_file() and not p.is_symlink() + } + src_files = { + p.name: p.read_bytes() for p in src.iterdir() if p.is_file() + } + if len(dest_items) == len(dest_files) and dest_files == src_files: + return shutil.rmtree(dest) dest.parent.mkdir(parents=True, exist_ok=True) - try: - os.symlink(src, dest, target_is_directory=True) - except OSError: - dest.mkdir() - for item in src.iterdir(): - if item.is_file(): - shutil.copy2(item, dest / item.name) + dest.mkdir() + for item in src.iterdir(): + if item.is_file(): + shutil.copy2(item, dest / item.name) def write_text(path: Path, content: str) -> None: diff --git a/tools/tests/test_bmad_setup.py b/tools/tests/test_bmad_setup.py index 554818ef6..82a2a558d 100644 --- a/tools/tests/test_bmad_setup.py +++ b/tools/tests/test_bmad_setup.py @@ -131,9 +131,14 @@ def user_toml_files(root: Path) -> list[Path]: def scripts_match(dest: Path, src: Path) -> bool: - dest_files = {p.name: p.read_bytes() for p in dest.iterdir() if p.is_file()} + dest_items = list(dest.iterdir()) + dest_files = { + p.name: p.read_bytes() + for p in dest_items + if p.is_file() and not p.is_symlink() + } src_files = {p.name: p.read_bytes() for p in src.iterdir() if p.is_file()} - return dest_files == src_files + return len(dest_items) == len(dest_files) and dest_files == src_files def symlink_to_temp_dir_succeeds() -> bool: @@ -250,20 +255,25 @@ class BmadSetupTests(unittest.TestCase): "John", ) - def test_symlink_refused_copies_scripts(self): + def test_first_setup_copies_scripts(self): setup = load_setup() with tempfile.TemporaryDirectory() as temp_dir: root = Path(temp_dir) project = root / "proj" skill = write_dest_bmad(root) project.mkdir() - with mock.patch("os.symlink", side_effect=OSError("operation not permitted")): - code = setup.main(["--project-root", str(project), "--skill", str(skill)]) + with mock.patch( + "os.symlink", + side_effect=AssertionError("setup must not create a symlink"), + ) as symlink: + code = setup.main( + ["--project-root", str(project), "--skill", str(skill)] + ) self.assertEqual(code, 0) - scripts = project / "_bmad" / "scripts" - self.assertFalse(scripts.is_symlink()) - self.assertTrue(scripts.is_dir()) - self.assertTrue(scripts_match(scripts, skill / "scripts")) + symlink.assert_not_called() + self._assert_scripts_identity( + project / "_bmad" / "scripts", skill / "scripts" + ) def test_already_present_paths_are_left_alone(self): with tempfile.TemporaryDirectory() as temp_dir: @@ -456,6 +466,39 @@ class BmadSetupTests(unittest.TestCase): self.assertEqual(result.returncode, 0, msg=result.stderr) self._assert_scripts_identity(bmad / "scripts", skill / "scripts") + def test_existing_scripts_link_does_not_require_symlink_permission(self): + if not symlink_to_temp_dir_succeeds(): + self.skipTest("symlinks not available") + setup = load_setup() + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + project = root / "proj" + skill = write_dest_bmad(root) + project.mkdir() + bmad = project / "_bmad" + bmad.mkdir() + write(bmad / "custom" / "keep.txt", "keep\n") + os.symlink( + skill / "scripts", + bmad / "scripts", + target_is_directory=True, + ) + + with mock.patch( + "os.symlink", side_effect=OSError("operation not permitted") + ) as symlink: + code = setup.main( + ["--project-root", str(project), "--skill", str(skill)] + ) + + self.assertEqual(code, 0) + symlink.assert_not_called() + self._assert_scripts_identity(bmad / "scripts", skill / "scripts") + self.assertEqual( + (bmad / "custom" / "keep.txt").read_text(encoding="utf-8"), + "keep\n", + ) + def test_stale_scripts_copy_is_replaced(self): with tempfile.TemporaryDirectory() as temp_dir: root = Path(temp_dir) @@ -471,6 +514,43 @@ class BmadSetupTests(unittest.TestCase): self._assert_scripts_identity(scripts, skill / "scripts") self.assertFalse((scripts / "leftover.py").exists()) + def test_identical_scripts_copy_with_extra_directory_is_replaced(self): + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + project = root / "proj" + skill = write_dest_bmad(root) + project.mkdir() + scripts = project / "_bmad" / "scripts" + scripts.mkdir(parents=True) + for item in (skill / "scripts").iterdir(): + if item.is_file(): + shutil.copy2(item, scripts / item.name) + (scripts / "leftover").mkdir() + + result = run_setup(project, skill) + self.assertEqual(result.returncode, 0, msg=result.stderr) + self._assert_scripts_identity(scripts, skill / "scripts") + self.assertFalse((scripts / "leftover").exists()) + + def test_expected_script_file_links_are_replaced_with_plain_files(self): + if not symlink_to_temp_dir_succeeds(): + self.skipTest("symlinks not available") + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + project = root / "proj" + skill = write_dest_bmad(root) + project.mkdir() + scripts = project / "_bmad" / "scripts" + scripts.mkdir(parents=True) + for item in (skill / "scripts").iterdir(): + if item.is_file(): + os.symlink(item, scripts / item.name) + self.assertTrue(all(item.is_symlink() for item in scripts.iterdir())) + + result = run_setup(project, skill) + self.assertEqual(result.returncode, 0, msg=result.stderr) + self._assert_scripts_identity(scripts, skill / "scripts") + def test_identical_scripts_copy_stays_a_copy(self): with tempfile.TemporaryDirectory() as temp_dir: root = Path(temp_dir) @@ -482,13 +562,23 @@ class BmadSetupTests(unittest.TestCase): for item in (skill / "scripts").iterdir(): if item.is_file(): shutil.copy2(item, scripts / item.name) + marker = scripts / "resolve_config.py" + source_marker = skill / "scripts" / marker.name + os.utime( + marker, + ns=(946_684_800_000_000_000, 946_684_800_000_000_000), + ) + preserved_mtime = marker.stat().st_mtime_ns + self.assertNotEqual( + preserved_mtime, source_marker.stat().st_mtime_ns + ) result = run_setup(project, skill) self.assertEqual(result.returncode, 0, msg=result.stderr) - self.assertFalse(scripts.is_symlink()) - self.assertTrue(scripts_match(scripts, skill / "scripts")) + self._assert_scripts_identity(scripts, skill / "scripts") + self.assertEqual(marker.stat().st_mtime_ns, preserved_mtime) - def test_right_scripts_symlink_is_left_alone(self): + def test_right_scripts_symlink_is_replaced_with_copy(self): if not symlink_to_temp_dir_succeeds(): self.skipTest("symlinks not available") with tempfile.TemporaryDirectory() as temp_dir: @@ -496,16 +586,16 @@ class BmadSetupTests(unittest.TestCase): project = root / "proj" skill = write_dest_bmad(root) project.mkdir() - first = run_setup(project, skill) - self.assertEqual(first.returncode, 0, msg=first.stderr) + bmad = project / "_bmad" + bmad.mkdir() scripts = project / "_bmad" / "scripts" - self.assertTrue(scripts.is_symlink()) - before = os.readlink(scripts) + os.symlink( + skill / "scripts", scripts, target_is_directory=True + ) - second = run_setup(project, skill) - self.assertEqual(second.returncode, 0, msg=second.stderr) - self.assertTrue(scripts.is_symlink()) - self.assertEqual(os.readlink(scripts), before) + result = run_setup(project, skill) + self.assertEqual(result.returncode, 0, msg=result.stderr) + self._assert_scripts_identity(scripts, skill / "scripts") def test_user_layers_and_leftovers_survive_second_setup(self): with tempfile.TemporaryDirectory() as temp_dir: @@ -668,10 +758,84 @@ class BmadSetupTests(unittest.TestCase): self.assertFalse((project / "_bmad").exists()) self.assertFalse((project / "_bmad-output").exists()) + def test_script_copy_failure_preserves_existing_bmad_and_cleans_staging( + self, + ): + setup = load_setup() + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + project = root / "proj" + skill = write_dest_bmad(root) + project.mkdir() + bmad = project / "_bmad" + write(bmad / "scripts" / "resolve_config.py", "# original\n") + write(bmad / "custom" / "keep.txt", "keep\n") + write(bmad / "config.user.toml", "# user\n") + (bmad / "empty-preserved").mkdir() + before_inode = bmad.stat().st_ino + before_files = { + path.relative_to(bmad): path.read_bytes() + for path in bmad.rglob("*") + if path.is_file() + } + before_dirs = { + path.relative_to(bmad) + for path in bmad.rglob("*") + if path.is_dir() + } + real_copy2 = shutil.copy2 + script_copy_attempts = 0 + skill_scripts = (skill / "scripts").resolve() + + def fail_second_script_copy(source, dest, *args, **kwargs): + nonlocal script_copy_attempts + source_path = Path(source) + dest_path = Path(dest) + if ( + source_path.parent.resolve() == skill_scripts + and dest_path.parent.name == "scripts" + and dest_path.parent.parent.name.startswith("_bmad.setup-") + ): + script_copy_attempts += 1 + if script_copy_attempts == 2: + raise OSError("script copy failed") + return real_copy2(source, dest, *args, **kwargs) + + with mock.patch.object( + setup.shutil, "copy2", side_effect=fail_second_script_copy + ): + with self.assertRaisesRegex(OSError, "script copy failed"): + setup.main( + [ + "--project-root", + str(project), + "--skill", + str(skill), + ] + ) + + self.assertEqual(script_copy_attempts, 2) + self.assertEqual(bmad.stat().st_ino, before_inode) + self.assertEqual( + { + path.relative_to(bmad): path.read_bytes() + for path in bmad.rglob("*") + if path.is_file() + }, + before_files, + ) + self.assertEqual( + { + path.relative_to(bmad) + for path in bmad.rglob("*") + if path.is_dir() + }, + before_dirs, + ) + self.assertEqual(list(project.glob("_bmad.setup-*")), []) + def _assert_scripts_identity(self, dest: Path, src: Path) -> None: - if dest.is_symlink(): - self.assertEqual(Path(os.readlink(dest)).resolve(), src.resolve()) - return + self.assertFalse(dest.is_symlink()) self.assertTrue(dest.is_dir()) self.assertTrue(scripts_match(dest, src)) @@ -711,12 +875,7 @@ class BmadSetupTests(unittest.TestCase): bmad = project / "_bmad" scripts = bmad / "scripts" skill_scripts = skill / "scripts" - if symlink_to_temp_dir_succeeds(): - self.assertTrue(scripts.is_symlink()) - self.assertEqual(Path(os.readlink(scripts)).resolve(), skill_scripts.resolve()) - else: - self.assertFalse(scripts.is_symlink()) - self.assertTrue(scripts_match(scripts, skill_scripts)) + self._assert_scripts_identity(scripts, skill_scripts) parsed = tomllib.loads((bmad / "config.toml").read_text(encoding="utf-8")) self.assertEqual(parsed["core"]["project_name"], project_name)