Refactor 922 cowork cleanups

CHANGELOG.md

@@ -35,6 +35,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `apm install -g --target codex` now honors `CODEX_HOME` for user-scope
   Codex MCP config writes, falling back to `~/.codex/config.toml` when unset.
   (closes #1861) (#1863)
+- Config unset helpers no longer rewrite `~/.apm/config.json` when the key is
+  already absent, preserving true no-op behavior for unset calls. (#1667)
 
 ## [0.21.0] - 2026-06-19
 

src/apm_cli/config.py

@@ -61,14 +61,21 @@ 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()
+    remove_keys = tuple(remove_keys)
+    if not updates and all(key not in config for key in remove_keys):
+        return
+    for key in remove_keys:
+        config.pop(key, None)
     config.update(updates)
 
     with open(CONFIG_FILE, "w", encoding="utf-8") as f:
@@ -147,19 +154,14 @@ 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, including avoiding a config-file
+    rewrite.  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

@@ -831,13 +831,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()
@@ -944,6 +938,16 @@ def _integrate_native_skill(
                 except ImportError:
                     pass  # CLI not available in tests
 
+        # Validate before target path construction so no invalid segment reaches
+        # deploy_path(), including cowork's dynamic-root branch.
+        from apm_cli.utils.path_security import (
+            PathTraversalError,
+            ensure_path_within,
+            validate_path_segments,
+        )
+
+        validate_path_segments(skill_name, context="skill name")
+
         # Deploy to all active targets that support skills.
         # When *targets* is provided (from --target), use it directly.
         # Otherwise auto-detect with copilot as the fallback.
@@ -972,28 +976,18 @@ 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 (
-                PathTraversalError,
-                ensure_path_within,
-                validate_path_segments,
-            )
-
-            validate_path_segments(skill_name, context="skill name")
             if target_skill_dir.is_symlink():
                 raise PathTraversalError(
                     f"Skill destination {target_skill_dir} is a symlink -- refusing to deploy"
                 )
+            # Cowork targets resolve to an explicit user-configured skills root;
+            # deploy_path() joins only the validated skill name below that root.
             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()
@@ -1072,10 +1066,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,
@@ -1169,9 +1159,7 @@ def _integrate_skill_bundle(
                 continue
 
             is_primary = idx == 0
-            skills_mapping = target.primitives["skills"]
-            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()

src/apm_cli/integration/targets.py

@@ -303,22 +303,39 @@ 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:
+    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``.  Unknown primitive names raise
+        ``KeyError`` instead of falling back to ``root_dir``.  For known
+        primitives, ``primitive`` is not consulted when ``resolved_deploy_root``
+        is set; the dynamic root already identifies the complete deployment
+        root.
 
         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[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:

tests/integration/test_skill_integrator_hermetic.py

@@ -21,6 +21,7 @@
 )
 from apm_cli.models.apm_package import APMPackage, PackageInfo
 from apm_cli.models.validation import PackageType
+from tests.unit._skill_integrator_target_helpers import attach_skill_deploy_path
 
 # ---------------------------------------------------------------------------
 # Helpers
@@ -44,6 +45,25 @@ def _make_skill_source(tmp_path: Path, name: str = "my-skill") -> Path:
     return src
 
 
+def _make_skill_target(
+    *,
+    supports_skills: bool = True,
+    root_dir: str | Path = ".github",
+    deploy_root: str | None = None,
+    resolved_deploy_root: Path | None = None,
+    auto_create: bool = True,
+) -> MagicMock:
+    target = MagicMock()
+    target.name = "copilot"
+    target.supports.return_value = supports_skills
+    target.user_root_resolver = None
+    target.primitives = {"skills": MagicMock(deploy_root=deploy_root, subdir="skills")}
+    target.root_dir = root_dir
+    target.resolved_deploy_root = resolved_deploy_root
+    target.auto_create = auto_create
+    return attach_skill_deploy_path(target)
+
+
 # ---------------------------------------------------------------------------
 # validate_skill_name -- line 129 (fallback invalid name)
 # ---------------------------------------------------------------------------
@@ -128,11 +148,7 @@ def test_symlink_skill_dir_raises(self, tmp_path):
         link_target.mkdir()
         dest.symlink_to(link_target)
 
-        target = MagicMock()
-        target.supports.return_value = True
-        target.primitives = {"skills": MagicMock(deploy_root=None)}
-        target.root_dir = Path(".github")
-        target.auto_create = True
+        target = _make_skill_target()
 
         with patch(
             "apm_cli.integration.skill_integrator.should_install_skill",
@@ -155,8 +171,7 @@ def test_target_not_support_skills_skipped(self, tmp_path):
         target_base = tmp_path / "project"
         target_base.mkdir()
 
-        target = MagicMock()
-        target.supports.return_value = False
+        target = _make_skill_target(supports_skills=False)
 
         with patch("apm_cli.integration.skill_integrator.should_install_skill", return_value=True):
             result = copy_skill_to_target(
@@ -174,11 +189,7 @@ def test_auto_create_false_missing_dir_skipped(self, tmp_path):
         target_base = tmp_path / "project"
         target_base.mkdir()
 
-        target = MagicMock()
-        target.supports.return_value = True
-        target.primitives = {"skills": MagicMock(deploy_root=None)}
-        target.root_dir = Path(".missing-dir")
-        target.auto_create = False
+        target = _make_skill_target(root_dir=".missing-dir", auto_create=False)
 
         with patch("apm_cli.integration.skill_integrator.should_install_skill", return_value=True):
             result = copy_skill_to_target(
@@ -200,12 +211,7 @@ def test_dedup_same_resolved_path_only_deploys_once(self, tmp_path):
         skills_root = target_base / ".github" / "skills"
 
         def _make_target():
-            t = MagicMock()
-            t.supports.return_value = True
-            t.primitives = {"skills": MagicMock(deploy_root=None)}
-            t.root_dir = Path(".github")
-            t.auto_create = True
-            return t
+            return _make_skill_target()
 
         t1 = _make_target()
         t2 = _make_target()
@@ -565,12 +571,7 @@ def test_invalid_name_normalized_with_diagnostics(self, tmp_path):
         integrator = SkillIntegrator()
         diag = MagicMock()
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
-        fake_target.resolved_deploy_root = None
-        fake_target.auto_create = True
+        fake_target = _make_skill_target()
 
         with (
             patch(
@@ -603,12 +604,7 @@ def test_invalid_name_normalized_with_logger(self, tmp_path):
         integrator = SkillIntegrator()
         logger = MagicMock()
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
-        fake_target.resolved_deploy_root = None
-        fake_target.auto_create = True
+        fake_target = _make_skill_target()
 
         with (
             patch(
@@ -654,11 +650,7 @@ def test_managed_files_removes_tracked_skill_dir(self, tmp_path):
         managed = {".github/skills/orphan-skill"}
 
         # We need a target that maps to .github/skills prefix
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.user_root_resolver = None
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
+        fake_target = _make_skill_target()
 
         stats = integrator.sync_integration(
             apm_pkg,
@@ -678,11 +670,7 @@ def test_managed_files_dotdot_path_skipped(self, tmp_path):
         apm_pkg = self._make_apm_package()
         managed = {".github/skills/../../../etc/passwd"}
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.user_root_resolver = None
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
+        fake_target = _make_skill_target()
 
         stats = integrator.sync_integration(
             apm_pkg,
@@ -703,11 +691,7 @@ def test_managed_files_outside_project_root_skipped(self, tmp_path):
         # This path is valid-looking but resolves outside
         managed = {".github/skills/../../../outside"}
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.user_root_resolver = None
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
+        fake_target = _make_skill_target()
 
         stats = integrator.sync_integration(
             apm_pkg,
@@ -731,11 +715,7 @@ def test_legacy_orphan_detection_removes_unknown_skill(self, tmp_path):
 
         integrator = SkillIntegrator()
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = True
-        fake_target.user_root_resolver = None
-        fake_target.primitives = {"skills": MagicMock(deploy_root=None)}
-        fake_target.root_dir = Path(".github")
+        fake_target = _make_skill_target()
 
         stats = integrator.sync_integration(
             apm_pkg,
@@ -756,8 +736,7 @@ def test_legacy_target_without_skills_support_skipped(self, tmp_path):
 
         integrator = SkillIntegrator()
 
-        fake_target = MagicMock()
-        fake_target.supports.return_value = False
+        fake_target = _make_skill_target(supports_skills=False)
 
         stats = integrator.sync_integration(
             apm_pkg,