diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index c4f858890..09fd0c469 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -61,14 +61,18 @@ def _invalidate_config_cache(): _config_cache = None -def update_config(updates): +def update_config(updates, *, remove_keys=()): """Update the configuration with new values. Args: updates (dict): Dictionary of configuration values to update. + remove_keys: Optional iterable of keys to remove before applying + updates. """ _invalidate_config_cache() config = get_config() + for key in remove_keys: + config.pop(key, None) config.update(updates) with open(CONFIG_FILE, "w", encoding="utf-8") as f: @@ -147,19 +151,13 @@ def set_temp_dir(path: str) -> None: def _unset_config_key(key: str) -> None: """Remove *key* from the config file atomically. - No-op when *key* is not present. Invalidates the in-process cache - before and after the write so subsequent reads see the updated state. + No-op when *key* is not present. Routes through ``update_config()`` + so all config writes share the same read-modify-write path. Args: key: The JSON key to remove from ``~/.apm/config.json``. """ - _invalidate_config_cache() - config = get_config() - if key in config: - del config[key] - with open(CONFIG_FILE, "w", encoding="utf-8") as f: - json.dump(config, f, indent=2) - _invalidate_config_cache() + update_config({}, remove_keys=(key,)) def unset_temp_dir() -> None: diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index 3264bdd37..f82734f1d 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -779,13 +779,7 @@ def _promote_sub_skills_standalone( continue is_primary = idx == 0 # first active target owns diagnostics - skills_mapping = target.primitives["skills"] - # Dynamic-root targets (cowork): use resolved_deploy_root. - if target.resolved_deploy_root is not None: - target_skills_root = target.resolved_deploy_root - else: - effective_root = skills_mapping.deploy_root or target.root_dir - target_skills_root = project_root / effective_root / "skills" + target_skills_root = target.deploy_path(project_root, primitive="skills") # Dedup: skip if same resolved skills root already processed. resolved_root = target_skills_root.resolve() @@ -917,13 +911,8 @@ def _integrate_native_skill( continue is_primary = idx == 0 # first active target owns diagnostics - skills_mapping = target.primitives["skills"] - # Dynamic-root targets (cowork): use resolved_deploy_root. - if target.resolved_deploy_root is not None: - target_skill_dir = target.resolved_deploy_root / skill_name - else: - effective_root = skills_mapping.deploy_root or target.root_dir - target_skill_dir = project_root / effective_root / "skills" / skill_name + target_skills_root = target.deploy_path(project_root, primitive="skills") + target_skill_dir = target.deploy_path(project_root, skill_name, primitive="skills") # Security: validate name + containment + symlink rejection. from apm_cli.utils.path_security import ( @@ -938,7 +927,7 @@ def _integrate_native_skill( f"Skill destination {target_skill_dir} is a symlink -- refusing to deploy" ) if target.resolved_deploy_root is None: - ensure_path_within(target_skill_dir, project_root / effective_root / "skills") + ensure_path_within(target_skill_dir, target_skills_root) # Dedup: skip if same resolved path already deployed. resolved = target_skill_dir.resolve() @@ -1017,10 +1006,6 @@ def _ignore_non_content_and_apm(directory, contents): files_copied = sum(1 for _ in target_skill_dir.rglob("*") if _.is_file()) # Promote sub-skills for this target - if target.resolved_deploy_root is not None: - target_skills_root = target.resolved_deploy_root - else: - target_skills_root = project_root / effective_root / "skills" _, sub_deployed = self._promote_sub_skills( sub_skills_dir, target_skills_root, diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 8bfffcf44..0a99d15a1 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -20,6 +20,7 @@ from collections.abc import Callable from dataclasses import dataclass +from pathlib import Path @dataclass(frozen=True) @@ -109,7 +110,7 @@ class TargetProfile: (``~/.copilot/copilot-instructions.md``). """ - user_root_resolver: Callable[[], Path | None] | None = None # noqa: F821 + user_root_resolver: Callable[[], Path | None] | None = None """Optional callable that resolves the deploy root at runtime. When set, ``for_scope(user_scope=True)`` calls this resolver instead of @@ -121,7 +122,7 @@ class TargetProfile: staticmethod) so ``frozen=True`` is preserved. """ - resolved_deploy_root: Path | None = None # noqa: F821 + resolved_deploy_root: Path | None = None """Absolute deploy root populated by ``for_scope()`` when ``user_root_resolver`` returns a concrete ``Path``. @@ -237,22 +238,37 @@ def supports_at_user_scope(self, primitive: str) -> bool: return False return primitive in self.primitives - def deploy_path(self, project_root: Path, *parts: str) -> Path: # noqa: F821 + def deploy_path( + self, project_root: Path, *parts: str, primitive: str | None = None + ) -> Path: """Return the filesystem path for deployment. When ``resolved_deploy_root`` is set (dynamic-root targets like cowork), the path is rooted there. Otherwise falls back to the - standard ``project_root / root_dir`` pattern. + standard ``project_root / root_dir`` pattern. When ``primitive`` + names a mapping with ``deploy_root``, that primitive-specific root is + used instead of ``root_dir``. Args: project_root: Workspace or home directory root. - *parts: Additional path segments (e.g. ``"skills"``, ``"my-skill"``). + *parts: Additional path segments below the resolved deployment + root. For primitive-aware calls, pass segments below the + primitive root, e.g. ``deploy_path(root, "my-skill", + primitive="skills")`` rather than including ``"skills"`` in + ``parts``. + primitive: Optional primitive name whose mapping can override + ``root_dir`` via ``deploy_root`` and append its mapped + ``subdir``. """ + mapping = self.primitives.get(primitive) if primitive is not None else None if self.resolved_deploy_root is not None: return ( self.resolved_deploy_root.joinpath(*parts) if parts else self.resolved_deploy_root ) - base = project_root / self.root_dir + deploy_root = mapping.deploy_root if mapping is not None else None + base = project_root / (deploy_root or self.root_dir) + if mapping is not None and mapping.subdir: + base = base / mapping.subdir return base.joinpath(*parts) if parts else base def for_scope(self, user_scope: bool = False) -> TargetProfile | None: @@ -700,7 +716,7 @@ def should_use_legacy_skill_paths() -> bool: return val in ("1", "true", "yes") -def _resolve_copilot_cowork_root() -> Path | None: # noqa: F821 +def _resolve_copilot_cowork_root() -> Path | None: """Thin wrapper around ``copilot_cowork_paths.resolve_copilot_cowork_skills_dir()``. Used as the ``user_root_resolver`` callable for the cowork target. @@ -711,7 +727,7 @@ def _resolve_copilot_cowork_root() -> Path | None: # noqa: F821 return resolve_copilot_cowork_skills_dir() -def _resolve_copilot_app_root() -> Path | None: # noqa: F821 +def _resolve_copilot_app_root() -> Path | None: """Thin wrapper around ``copilot_app_db.resolve_copilot_app_root()``. Used as the ``user_root_resolver`` callable for the ``copilot-app`` diff --git a/tests/unit/_skill_integrator_target_helpers.py b/tests/unit/_skill_integrator_target_helpers.py new file mode 100644 index 000000000..fe3b0ef83 --- /dev/null +++ b/tests/unit/_skill_integrator_target_helpers.py @@ -0,0 +1,24 @@ +"""Shared target doubles for skill integrator unit tests.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock + + +def attach_skill_deploy_path(target: MagicMock) -> MagicMock: + """Attach a TargetProfile-like deploy_path implementation to *target*.""" + + def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path: + mapping = target.primitives.get(primitive) if primitive is not None else None + if target.resolved_deploy_root is not None: + base = target.resolved_deploy_root + else: + deploy_root = mapping.deploy_root if mapping is not None else None + base = project_root / (deploy_root or target.root_dir) + if mapping is not None and mapping.subdir: + base = base / mapping.subdir + return base.joinpath(*parts) if parts else base + + target.deploy_path = _deploy_path + return target diff --git a/tests/unit/integration/test_copilot_cowork_target.py b/tests/unit/integration/test_copilot_cowork_target.py index e7892b7fd..7ffff53e5 100644 --- a/tests/unit/integration/test_copilot_cowork_target.py +++ b/tests/unit/integration/test_copilot_cowork_target.py @@ -134,6 +134,11 @@ def test_deploy_path_without_resolved_root_uses_project(self, tmp_path: Path) -> result = copilot.deploy_path(tmp_path) assert result == tmp_path / ".github" + def test_deploy_path_with_primitive_deploy_root_override(self, tmp_path: Path) -> None: + copilot = KNOWN_TARGETS["copilot"] + result = copilot.deploy_path(tmp_path, "review", primitive="skills") + assert result == tmp_path / ".agents" / "skills" / "review" + # --------------------------------------------------------------------------- # TestActiveTargetsGating diff --git a/tests/unit/integration/test_skill_integrator_hermetic.py b/tests/unit/integration/test_skill_integrator_hermetic.py index a726fc230..927d40e63 100644 --- a/tests/unit/integration/test_skill_integrator_hermetic.py +++ b/tests/unit/integration/test_skill_integrator_hermetic.py @@ -29,6 +29,7 @@ should_compile_instructions, validate_skill_name, ) +from tests.unit._skill_integrator_target_helpers import attach_skill_deploy_path # --------------------------------------------------------------------------- # Helpers @@ -66,9 +67,10 @@ def _make_target( prim = MagicMock() mapping = MagicMock() mapping.deploy_root = deploy_root + mapping.subdir = "skills" prim.__getitem__ = MagicMock(return_value=mapping) target.primitives = {"skills": mapping} - return target + return attach_skill_deploy_path(target) # --------------------------------------------------------------------------- diff --git a/tests/unit/integration/test_skill_integrator_phase3w4.py b/tests/unit/integration/test_skill_integrator_phase3w4.py index 4465f27a3..7b431cb9e 100644 --- a/tests/unit/integration/test_skill_integrator_phase3w4.py +++ b/tests/unit/integration/test_skill_integrator_phase3w4.py @@ -29,6 +29,7 @@ should_compile_instructions, validate_skill_name, ) +from tests.unit._skill_integrator_target_helpers import attach_skill_deploy_path # --------------------------------------------------------------------------- # Helpers @@ -66,9 +67,10 @@ def _make_target( prim = MagicMock() mapping = MagicMock() mapping.deploy_root = deploy_root + mapping.subdir = "skills" prim.__getitem__ = MagicMock(return_value=mapping) target.primitives = {"skills": mapping} - return target + return attach_skill_deploy_path(target) # --------------------------------------------------------------------------- diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index ade166325..8f831f2c6 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -53,6 +53,29 @@ def test_ensure_config_exists_uses_utf8(self, isolated_config, monkeypatch): json.loads(isolated_config.read_bytes().decode("utf-8")) +class TestUnsetConfigHelpers: + """Unset helpers route through the shared update_config write path.""" + + @pytest.mark.parametrize( + ("unset_func", "key"), + ( + (config_mod.unset_temp_dir, "temp_dir"), + (config_mod.unset_copilot_cowork_skills_dir, "copilot_cowork_skills_dir"), + ), + ) + def test_unset_helpers_use_update_config(self, monkeypatch, unset_func, key): + calls = [] + + def fake_update_config(updates, *, remove_keys=()): + calls.append((updates, tuple(remove_keys))) + + monkeypatch.setattr(config_mod, "update_config", fake_update_config) + + unset_func() + + assert calls == [({}, (key,))] + + class TestAuditOnInstallConfig: """get/set/unset for the audit-on-install user default."""