From 24c4891c509864241955aecc78fa021e8bf3cb4c Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Thu, 4 Jun 2026 21:56:11 +0530
Subject: [PATCH 1/6] refactor(cowork): clean up deploy path and config unset
 helpers

---
 src/apm_cli/config.py                         | 18 ++++++------
 src/apm_cli/integration/skill_integrator.py   | 23 +++------------
 src/apm_cli/integration/targets.py            | 25 ++++++++++++-----
 .../integration/test_copilot_cowork_target.py |  5 ++++
 tests/unit/test_config.py                     | 28 +++++++++++++++++++
 5 files changed, 63 insertions(+), 36 deletions(-)

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..4e747d060 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,32 @@ 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"``).
+            primitive: Optional primitive name whose mapping can override
+                ``root_dir`` via ``deploy_root``.
         """
+        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 +711,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 +722,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/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/test_config.py b/tests/unit/test_config.py
index ade166325..31da3c7d0 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -53,6 +53,34 @@ 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."""
+
+    def test_unset_temp_dir_uses_update_config(self, monkeypatch):
+        calls = []
+
+        def fake_update_config(updates, *, remove_keys=()):
+            calls.append((updates, tuple(remove_keys)))
+
+        monkeypatch.setattr(config_mod, "update_config", fake_update_config)
+
+        config_mod.unset_temp_dir()
+
+        assert calls == [({}, ("temp_dir",))]
+
+    def test_unset_copilot_cowork_skills_dir_uses_update_config(self, monkeypatch):
+        calls = []
+
+        def fake_update_config(updates, *, remove_keys=()):
+            calls.append((updates, tuple(remove_keys)))
+
+        monkeypatch.setattr(config_mod, "update_config", fake_update_config)
+
+        config_mod.unset_copilot_cowork_skills_dir()
+
+        assert calls == [({}, ("copilot_cowork_skills_dir",))]
+
+
 class TestAuditOnInstallConfig:
     """get/set/unset for the audit-on-install user default."""
 

From ed18556b491714a198eb3b3c6b9ce85046cddba1 Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Thu, 4 Jun 2026 22:21:41 +0530
Subject: [PATCH 2/6] test(cowork): update skill integrator target mocks

---
 .../integration/test_skill_integrator_hermetic.py     | 11 +++++++++++
 .../integration/test_skill_integrator_phase3w4.py     | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/tests/unit/integration/test_skill_integrator_hermetic.py b/tests/unit/integration/test_skill_integrator_hermetic.py
index a726fc230..43745051e 100644
--- a/tests/unit/integration/test_skill_integrator_hermetic.py
+++ b/tests/unit/integration/test_skill_integrator_hermetic.py
@@ -68,6 +68,17 @@ def _make_target(
     mapping.deploy_root = deploy_root
     prim.__getitem__ = MagicMock(return_value=mapping)
     target.primitives = {"skills": mapping}
+
+    def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path:
+        if target.resolved_deploy_root is not None:
+            base = target.resolved_deploy_root
+        elif primitive == "skills":
+            base = project_root / (deploy_root or root_dir) / "skills"
+        else:
+            base = project_root / root_dir
+        return base.joinpath(*parts) if parts else base
+
+    target.deploy_path = _deploy_path
     return target
 
 
diff --git a/tests/unit/integration/test_skill_integrator_phase3w4.py b/tests/unit/integration/test_skill_integrator_phase3w4.py
index 4465f27a3..57f745e6c 100644
--- a/tests/unit/integration/test_skill_integrator_phase3w4.py
+++ b/tests/unit/integration/test_skill_integrator_phase3w4.py
@@ -68,6 +68,17 @@ def _make_target(
     mapping.deploy_root = deploy_root
     prim.__getitem__ = MagicMock(return_value=mapping)
     target.primitives = {"skills": mapping}
+
+    def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path:
+        if target.resolved_deploy_root is not None:
+            base = target.resolved_deploy_root
+        elif primitive == "skills":
+            base = project_root / (deploy_root or root_dir) / "skills"
+        else:
+            base = project_root / root_dir
+        return base.joinpath(*parts) if parts else base
+
+    target.deploy_path = _deploy_path
     return target
 
 

From ea31942d63c408483c4e0410fb9a92c7719a6696 Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Thu, 4 Jun 2026 22:39:41 +0530
Subject: [PATCH 3/6] test(cowork): address deploy path review comments

---
 src/apm_cli/integration/targets.py            |  9 +++++--
 .../unit/_skill_integrator_target_helpers.py  | 24 +++++++++++++++++++
 .../test_skill_integrator_hermetic.py         | 15 +++---------
 .../test_skill_integrator_phase3w4.py         | 15 +++---------
 4 files changed, 37 insertions(+), 26 deletions(-)
 create mode 100644 tests/unit/_skill_integrator_target_helpers.py

diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py
index 4e747d060..0a99d15a1 100644
--- a/src/apm_cli/integration/targets.py
+++ b/src/apm_cli/integration/targets.py
@@ -251,9 +251,14 @@ def deploy_path(
 
         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``.
+                ``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:
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_skill_integrator_hermetic.py b/tests/unit/integration/test_skill_integrator_hermetic.py
index 43745051e..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,20 +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}
-
-    def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path:
-        if target.resolved_deploy_root is not None:
-            base = target.resolved_deploy_root
-        elif primitive == "skills":
-            base = project_root / (deploy_root or root_dir) / "skills"
-        else:
-            base = project_root / root_dir
-        return base.joinpath(*parts) if parts else base
-
-    target.deploy_path = _deploy_path
-    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 57f745e6c..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,20 +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}
-
-    def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path:
-        if target.resolved_deploy_root is not None:
-            base = target.resolved_deploy_root
-        elif primitive == "skills":
-            base = project_root / (deploy_root or root_dir) / "skills"
-        else:
-            base = project_root / root_dir
-        return base.joinpath(*parts) if parts else base
-
-    target.deploy_path = _deploy_path
-    return target
+    return attach_skill_deploy_path(target)
 
 
 # ---------------------------------------------------------------------------

From 1e4ea61c267f9a0b29650a7a215d73d2401301dc Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Thu, 4 Jun 2026 22:44:03 +0530
Subject: [PATCH 4/6] test(config): parameterize unset helper coverage

---
 tests/unit/test_config.py | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index 31da3c7d0..8f831f2c6 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -56,7 +56,14 @@ def test_ensure_config_exists_uses_utf8(self, isolated_config, monkeypatch):
 class TestUnsetConfigHelpers:
     """Unset helpers route through the shared update_config write path."""
 
-    def test_unset_temp_dir_uses_update_config(self, monkeypatch):
+    @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=()):
@@ -64,21 +71,9 @@ def fake_update_config(updates, *, remove_keys=()):
 
         monkeypatch.setattr(config_mod, "update_config", fake_update_config)
 
-        config_mod.unset_temp_dir()
+        unset_func()
 
-        assert calls == [({}, ("temp_dir",))]
-
-    def test_unset_copilot_cowork_skills_dir_uses_update_config(self, monkeypatch):
-        calls = []
-
-        def fake_update_config(updates, *, remove_keys=()):
-            calls.append((updates, tuple(remove_keys)))
-
-        monkeypatch.setattr(config_mod, "update_config", fake_update_config)
-
-        config_mod.unset_copilot_cowork_skills_dir()
-
-        assert calls == [({}, ("copilot_cowork_skills_dir",))]
+        assert calls == [({}, (key,))]
 
 
 class TestAuditOnInstallConfig:

From 5ac0545dcbbff80cc6e7530712137a920b2b64d4 Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Sat, 20 Jun 2026 07:39:16 +0530
Subject: [PATCH 5/6] fix(cowork): address deploy path review follow-ups

---
 src/apm_cli/config.py                         |  8 +-
 src/apm_cli/integration/skill_integrator.py   |  2 +
 src/apm_cli/integration/targets.py            |  8 +-
 .../test_skill_integrator_hermetic.py         | 83 +++++++------------
 .../test_skill_integrator_phase3w4.py         | 83 +++++++------------
 .../unit/_skill_integrator_target_helpers.py  | 14 +---
 tests/unit/test_config.py                     | 23 +++++
 7 files changed, 101 insertions(+), 120 deletions(-)

diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py
index 09fd0c469..1f4bf26f2 100644
--- a/src/apm_cli/config.py
+++ b/src/apm_cli/config.py
@@ -71,6 +71,9 @@ def update_config(updates, *, remove_keys=()):
     """
     _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)
@@ -151,8 +154,9 @@ 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.  Routes through ``update_config()``
-    so all config writes share the same read-modify-write path.
+    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``.
diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py
index f82734f1d..925c3543d 100644
--- a/src/apm_cli/integration/skill_integrator.py
+++ b/src/apm_cli/integration/skill_integrator.py
@@ -926,6 +926,8 @@ def _integrate_native_skill(
                 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, target_skills_root)
 
diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py
index 0a99d15a1..50e9d161c 100644
--- a/src/apm_cli/integration/targets.py
+++ b/src/apm_cli/integration/targets.py
@@ -238,16 +238,16 @@ 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, primitive: str | None = None
-    ) -> 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.  When ``primitive``
         names a mapping with ``deploy_root``, that primitive-specific root is
-        used instead of ``root_dir``.
+        used instead of ``root_dir``.  ``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.
diff --git a/tests/integration/test_skill_integrator_hermetic.py b/tests/integration/test_skill_integrator_hermetic.py
index ebcf5b2c3..d69688fb2 100644
--- a/tests/integration/test_skill_integrator_hermetic.py
+++ b/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,
diff --git a/tests/integration/test_skill_integrator_phase3w4.py b/tests/integration/test_skill_integrator_phase3w4.py
index 5df01baf9..44d80cc32 100644
--- a/tests/integration/test_skill_integrator_phase3w4.py
+++ b/tests/integration/test_skill_integrator_phase3w4.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,
diff --git a/tests/unit/_skill_integrator_target_helpers.py b/tests/unit/_skill_integrator_target_helpers.py
index fe3b0ef83..d83e854cc 100644
--- a/tests/unit/_skill_integrator_target_helpers.py
+++ b/tests/unit/_skill_integrator_target_helpers.py
@@ -5,20 +5,14 @@
 from pathlib import Path
 from unittest.mock import MagicMock
 
+from apm_cli.integration.targets import TargetProfile
+
 
 def attach_skill_deploy_path(target: MagicMock) -> MagicMock:
-    """Attach a TargetProfile-like deploy_path implementation to *target*."""
+    """Attach the production ``TargetProfile.deploy_path`` method 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
+        return TargetProfile.deploy_path(target, project_root, *parts, primitive=primitive)
 
     target.deploy_path = _deploy_path
     return target
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index 8f831f2c6..17038ad70 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -5,6 +5,7 @@
 bugs on Windows when ``open()`` is called without an explicit encoding.
 """
 
+import builtins
 import json
 
 import pytest
@@ -60,6 +61,8 @@ class TestUnsetConfigHelpers:
         ("unset_func", "key"),
         (
             (config_mod.unset_temp_dir, "temp_dir"),
+            (config_mod.unset_allow_protocol_fallback, "allow_protocol_fallback"),
+            (config_mod.unset_prefer_ssh, "prefer_ssh"),
             (config_mod.unset_copilot_cowork_skills_dir, "copilot_cowork_skills_dir"),
         ),
     )
@@ -75,6 +78,26 @@ def fake_update_config(updates, *, remove_keys=()):
 
         assert calls == [({}, (key,))]
 
+    def test_update_config_absent_remove_key_does_not_write(self, isolated_config, monkeypatch):
+        config_mod.get_config()
+        real_open = builtins.open
+
+        def guarded_open(file, mode="r", *args, **kwargs):
+            if "w" in mode:
+                raise AssertionError("absent-key removal should not rewrite config")
+            return real_open(file, mode, *args, **kwargs)
+
+        monkeypatch.setattr(builtins, "open", guarded_open)
+
+        config_mod.update_config({}, remove_keys=("missing_key",))
+
+    def test_update_config_present_remove_key_writes_removal(self, isolated_config):
+        config_mod.update_config({"temp_dir": "/tmp/apm"})
+
+        config_mod.update_config({}, remove_keys=("temp_dir",))
+
+        assert "temp_dir" not in config_mod.get_config()
+
 
 class TestAuditOnInstallConfig:
     """get/set/unset for the audit-on-install user default."""

From 5abe33eeffe730171d436f099be670a346de10ad Mon Sep 17 00:00:00 2001
From: abhinavgautam01 <abgautam1017@gmail.com>
Date: Mon, 22 Jun 2026 21:11:57 +0530
Subject: [PATCH 6/6] fix(cowork): address review follow-ups

apm-spec-waiver: internal cowork path refactor only; no OpenAPM observable behavior change
---
 CHANGELOG.md                                  |  2 ++
 src/apm_cli/integration/skill_integrator.py   | 21 ++++++++++---------
 src/apm_cli/integration/targets.py            | 10 +++++----
 .../integration/test_copilot_cowork_target.py | 13 ++++++++++++
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1a517122c..8f5fe6a95 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py
index 599e77cf4..c595fa1c1 100644
--- a/src/apm_cli/integration/skill_integrator.py
+++ b/src/apm_cli/integration/skill_integrator.py
@@ -938,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.
@@ -970,13 +980,6 @@ def _integrate_native_skill(
             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"
@@ -1156,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()
diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py
index 7cc1ab4b3..489fd1101 100644
--- a/src/apm_cli/integration/targets.py
+++ b/src/apm_cli/integration/targets.py
@@ -310,9 +310,11 @@ def deploy_path(self, project_root: Path, *parts: str, primitive: str | None = N
         cowork), the path is rooted there.  Otherwise falls back to the
         standard ``project_root / root_dir`` pattern.  When ``primitive``
         names a mapping with ``deploy_root``, that primitive-specific root is
-        used instead of ``root_dir``.  ``primitive`` is not consulted when
-        ``resolved_deploy_root`` is set; the dynamic root already identifies
-        the complete deployment root.
+        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.
@@ -325,7 +327,7 @@ def deploy_path(self, project_root: Path, *parts: str, primitive: str | None = N
                 ``root_dir`` via ``deploy_root`` and append its mapped
                 ``subdir``.
         """
-        mapping = self.primitives.get(primitive) if primitive is not None else None
+        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
diff --git a/tests/unit/integration/test_copilot_cowork_target.py b/tests/unit/integration/test_copilot_cowork_target.py
index 7ffff53e5..f07a63053 100644
--- a/tests/unit/integration/test_copilot_cowork_target.py
+++ b/tests/unit/integration/test_copilot_cowork_target.py
@@ -139,6 +139,19 @@ def test_deploy_path_with_primitive_deploy_root_override(self, tmp_path: Path) -
         result = copilot.deploy_path(tmp_path, "review", primitive="skills")
         assert result == tmp_path / ".agents" / "skills" / "review"
 
+    def test_deploy_path_cowork_known_primitive_uses_resolved_root(self, tmp_path: Path) -> None:
+        cowork = replace(
+            KNOWN_TARGETS["copilot-cowork"],
+            resolved_deploy_root=tmp_path,
+        )
+        result = cowork.deploy_path(Path("/unused"), "review", primitive="skills")
+        assert result == tmp_path / "review"
+
+    def test_deploy_path_unknown_primitive_raises(self, tmp_path: Path) -> None:
+        copilot = KNOWN_TARGETS["copilot"]
+        with pytest.raises(KeyError):
+            copilot.deploy_path(tmp_path, primitive="unknown")
+
 
 # ---------------------------------------------------------------------------
 # TestActiveTargetsGating