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/config.py b/src/apm_cli/config.py index c4f858890..1f4bf26f2 100644 --- a/src/apm_cli/config.py +++ b/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: diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index f98fb873e..c595fa1c1 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/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() diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index c89ab7b28..489fd1101 100644 --- a/src/apm_cli/integration/targets.py +++ b/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: 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 new file mode 100644 index 000000000..d83e854cc --- /dev/null +++ b/tests/unit/_skill_integrator_target_helpers.py @@ -0,0 +1,18 @@ +"""Shared target doubles for skill integrator unit tests.""" + +from __future__ import annotations + +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 the production ``TargetProfile.deploy_path`` method to *target*.""" + + def _deploy_path(project_root: Path, *parts: str, primitive: str | None = None) -> Path: + return TargetProfile.deploy_path(target, project_root, *parts, primitive=primitive) + + 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..f07a63053 100644 --- a/tests/unit/integration/test_copilot_cowork_target.py +++ b/tests/unit/integration/test_copilot_cowork_target.py @@ -134,6 +134,24 @@ 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" + + 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 diff --git a/tests/unit/integration/test_skill_integrator_hermetic.py b/tests/unit/integration/test_skill_integrator_hermetic.py index d83f868e4..b4dc22e00 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 f7030d94f..38c98879f 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..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 @@ -53,6 +54,51 @@ 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_allow_protocol_fallback, "allow_protocol_fallback"), + (config_mod.unset_prefer_ssh, "prefer_ssh"), + (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,))] + + 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."""