Refactor 922 cowork cleanups

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:

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,

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``

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

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

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)
 
 
 # ---------------------------------------------------------------------------

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)
 
 
 # ---------------------------------------------------------------------------

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."""