From b08fbfa0d7ac9dbfd1ccabec41544b79edb4f46e Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 18 Sep 2026 00:44:04 -0700 Subject: [PATCH] fix(profiles): clone re-creates NTFS skill junctions instead of deep-copying them `hermes profile create --clone/--clone-all` copies the source home with `shutil.copytree(symlinks=True)`. POSIX symlinks survive that (b7192b1cb02), but an NTFS junction is a reparse point, not a symlink: `os.path.islink()` is False and copytree descends into it, so a `skills/foo` junction into a `skills.external_dirs` root became a physical copy in the clone. The clone's copy and the external original are then two same-named candidates and `_locate_skill()` refuses to guess ("Ambiguous skill name"), which also aborts Kanban worker spawns carrying the name (`Unknown skill(s)`). `_copytree_keep_junctions` wraps both clone copies: an `ignore` callback excludes junction entries (`st_reparse_tag == IO_REPARSE_TAG_MOUNT_POINT`) and the recorded targets are re-created with `_winapi.CreateJunction`. A junction whose target is gone is skipped with a warning (CreateJunction requires an existing target), so a dangling link never fails the clone. Off Windows the predicate short-circuits and behaviour is unchanged. The export staging copy deliberately keeps deep-copying: `_scrub_export_secrets` rewrites staged files in place, and a preserved junction would let that redaction write through into the user's external originals. Co-authored-by: KoNit-K --- hermes_cli/profiles.py | 50 +++++++++++++++++++++++++--- tests/hermes_cli/test_profiles.py | 54 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index f99b0db359..5ca7885ccd 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -874,10 +874,53 @@ def _materialize_symlinked_files(profile_dir: Path) -> List[str]: return done +def _junction_target(path: str) -> Optional[str]: + """Target of an NTFS directory junction, else ``None``. A junction is a reparse point, not a + symlink: ``os.path.islink()`` is False and ``shutil.copytree(symlinks=True)`` descends into it.""" + if os.name != "nt": + return None + try: + if os.lstat(path).st_reparse_tag != stat.IO_REPARSE_TAG_MOUNT_POINT: + return None + target = os.readlink(path) + except OSError: + return None + # readlink hands back the substitute name; CreateJunction rejects the ``\\?\`` spelling. + if target.startswith("\\\\?\\UNC\\"): + return "\\" + target[7:] + return target[4:] if target.startswith("\\\\?\\") else target + + +def _copytree_keep_junctions(src: Path, dst: Path, ignore, dirs_exist_ok: bool = False) -> None: + """``shutil.copytree(symlinks=True)`` that re-creates NTFS junctions as junctions instead of + traversing them. A ``skills/foo`` junction into a ``skills.external_dirs`` root copied as a + physical tree is a second same-named candidate and ``_locate_skill`` refuses to guess (#113471). + A junction whose target is gone is skipped with a warning, never a crash.""" + junctions: Dict[str, str] = {} + + def _ignore(directory: str, names: List[str]) -> set: + ignored = set(ignore(directory, names)) + for name in names: + target = _junction_target(os.path.join(directory, name)) + if target is not None: + junctions[os.path.join(directory, name)] = target + ignored.add(name) + return ignored + + shutil.copytree(src, dst, symlinks=True, dirs_exist_ok=dirs_exist_ok, ignore=_ignore) + if junctions: + import _winapi # Windows-only stdlib module; only reachable once a junction was seen + for link, target in junctions.items(): + try: + _winapi.CreateJunction(target, os.path.join(dst, os.path.relpath(link, src))) + except OSError as exc: + logger.warning("clone: skipped junction %s -> %s (%s)", link, target, exc) + + def _clone_all_into(source_dir: Path, profile_dir: Path, canon: str) -> None: """--clone-all: full copytree minus infrastructure/history, then strip runtime files and cloned single-use OAuth grants.""" - shutil.copytree(source_dir, profile_dir, symlinks=True, ignore=_clone_all_copytree_ignore(source_dir)) + _copytree_keep_junctions(source_dir, profile_dir, _clone_all_copytree_ignore(source_dir)) materialized = _materialize_symlinked_files(profile_dir) if materialized: logger.info("profile %s: materialized symlinked %s so the clone never writes through to %s", @@ -919,10 +962,7 @@ def _bootstrap_profile_dir(profile_dir: Path, source_dir: Optional[Path], _clone_file(source_dir, profile_dir, relpath) source_skills = source_dir / "skills" if source_skills.is_dir(): - shutil.copytree( - source_skills, profile_dir / "skills", symlinks=True, dirs_exist_ok=True, - ignore=_non_exportable_entries, - ) + _copytree_keep_junctions(source_skills, profile_dir / "skills", _non_exportable_entries, dirs_exist_ok=True) for relpath in _CLONE_SUBDIR_FILES: _clone_file(source_dir, profile_dir, relpath) if sync_imports: diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index 48642b7853..298c316de7 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -242,6 +242,60 @@ class TestCreateProfile: (default_home / "config.yaml").write_text("model: changed") assert yaml.safe_load((synced / "config.yaml").read_text())["model"] == "test" + @staticmethod + def _home_with_linked_skill(profile_env): + """Source home: ``skills/foo`` links into an ``external_dirs`` root, ``skills/local`` is physical.""" + default_home = profile_env / ".hermes" + external = profile_env / "agents-skills" + (external / "foo").mkdir(parents=True) + (external / "foo" / "SKILL.md").write_text("# external foo\n", encoding="utf-8") + (default_home / "skills" / "local").mkdir(parents=True) + (default_home / "skills" / "local" / "SKILL.md").write_text("# local\n", encoding="utf-8") + (default_home / "config.yaml").write_text(f"model: test\nskills:\n external_dirs:\n - {external}\n") + return default_home, external + + @pytest.mark.parametrize("clone_kwargs", [{"clone_config": True}, {"clone_all": True}]) + def test_clone_recreates_skill_junctions_and_skips_dangling_ones(self, profile_env, monkeypatch, clone_kwargs): + """A junctioned skill stays a link (one candidate with its external original), a dangling + junction is skipped without failing the clone. The reparse-point predicate and CreateJunction + are Windows-only; simulate both so the copy/re-create contract runs on every host.""" + default_home, external = self._home_with_linked_skill(profile_env) + # copytree sees plain directories (what a junction looks like to os.stat on Windows). + (default_home / "skills" / "foo").mkdir() + (default_home / "skills" / "foo" / "SKILL.md").write_text("# a physical copy would come from here\n") + (default_home / "skills" / "gone").mkdir() + targets = {str(default_home / "skills" / "foo"): str(external / "foo"), + str(default_home / "skills" / "gone"): str(profile_env / "nowhere")} + monkeypatch.setattr(profiles, "_junction_target", lambda path: targets.get(path), raising=False) + + def _create_junction(target, dst): + if not os.path.isdir(target): + raise OSError("target missing") # what _winapi.CreateJunction does for a dangling junction + os.symlink(target, dst, target_is_directory=True) + monkeypatch.setitem(sys.modules, "_winapi", types.SimpleNamespace(CreateJunction=_create_junction)) + + clone = create_profile("clone", no_alias=True, **clone_kwargs) + foo = clone / "skills" / "foo" + assert foo.is_symlink() and foo.resolve() == (external / "foo").resolve() + assert (foo / "SKILL.md").read_text(encoding="utf-8") == "# external foo\n" + assert (clone / "skills" / "local" / "SKILL.md").is_file() + assert not (clone / "skills" / "gone").exists() + from tools.skills_tool import _collect_skill_candidates + assert len(_collect_skill_candidates("foo", None, [clone / "skills", external])) == 1 + + @pytest.mark.windows_only + def test_clone_keeps_real_ntfs_junction(self, profile_env): + import _winapi + default_home, external = self._home_with_linked_skill(profile_env) + _winapi.CreateJunction(str(external / "foo"), str(default_home / "skills" / "foo")) + + clone = create_profile("clone", clone_config=True, no_alias=True) + foo = clone / "skills" / "foo" + assert os.lstat(foo).st_reparse_tag == profiles.stat.IO_REPARSE_TAG_MOUNT_POINT + assert foo.resolve() == (external / "foo").resolve() + from tools.skills_tool import _collect_skill_candidates + assert len(_collect_skill_candidates("foo", None, [clone / "skills", external])) == 1 + def test_sync_imports_requires_a_clone_source(self, profile_env): with pytest.raises(ValueError, match="--sync-imports requires"): create_profile("lonely", sync_imports=True, no_alias=True)