From ebe322e532e0178981dfc5b7f79b76adc599eac4 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Wed, 8 Apr 2026 19:51:46 +0200 Subject: [PATCH 1/2] feat: add configurable temp directory (temp-dir) Add `temp-dir` configuration key so users can override the system temporary directory used during apm install. This resolves [WinError 5] Access is denied errors in corporate Windows environments where endpoint security restricts %TEMP%. - Add get_temp_dir(), set_temp_dir(), get_apm_temp_dir() to config module - Resolution order: APM_TEMP_DIR env var > config file > system default - Wire all tempfile.mkdtemp/TemporaryDirectory call sites - Catch PermissionError/OSError in download path with actionable message - Add 51 new tests covering config, CLI, and error handling - Update CLI reference docs, skills resource, and CHANGELOG Fixes #624 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + .../content/docs/reference/cli-commands.md | 27 +++ .../.apm/skills/apm-usage/commands.md | 4 +- src/apm_cli/bundle/unpacker.py | 3 +- src/apm_cli/commands/config.py | 35 ++- src/apm_cli/commands/update.py | 4 +- src/apm_cli/config.py | 51 ++++ src/apm_cli/deps/github_downloader.py | 27 ++- src/apm_cli/runtime/manager.py | 3 +- tests/unit/test_config_command.py | 220 ++++++++++++++++++ tests/unit/test_github_downloader_temp_dir.py | 134 +++++++++++ 11 files changed, 496 insertions(+), 13 deletions(-) create mode 100644 tests/unit/test_github_downloader_temp_dir.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e4cae5e7a..0ad3b1cfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `apm install` now automatically discovers and deploys local `.apm/` primitives (skills, instructions, agents, prompts, hooks, commands) to target directories, with local content taking priority over dependencies on collision (#626, #644) +- Add `temp-dir` configuration key (`apm config set temp-dir PATH`) to override the system temporary directory, resolving `[WinError 5] Access is denied` in corporate Windows environments (#624) ### Fixed diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 438ded4f3..73079b669 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -1345,6 +1345,7 @@ apm config - Global configuration - APM CLI version - `auto-integrate` setting + - `temp-dir` setting (when configured) **Examples:** ```bash @@ -1363,6 +1364,7 @@ apm config get [KEY] **Arguments:** - `KEY` (optional) - Configuration key to retrieve. Supported keys: - `auto-integrate` - Whether to automatically integrate `.prompt.md` files into AGENTS.md + - `temp-dir` - Custom temporary directory for clone/download operations If `KEY` is omitted, displays all configuration values. @@ -1386,6 +1388,7 @@ apm config set KEY VALUE **Arguments:** - `KEY` - Configuration key to set. Supported keys: - `auto-integrate` - Enable/disable automatic integration of `.prompt.md` files + - `temp-dir` - Set a custom temporary directory path - `VALUE` - Value to set. For boolean keys, use: `true`, `false`, `yes`, `no`, `1`, `0` **Configuration Keys:** @@ -1411,6 +1414,30 @@ apm config set auto-integrate yes apm config set auto-integrate 1 ``` +**`temp-dir`** - Override the system temporary directory +- **Type:** String (directory path) +- **Default:** System temp directory (not stored) +- **Description:** Set a custom temporary directory for clone and download operations. Useful in corporate Windows environments where endpoint security software restricts access to `%TEMP%`, causing `[WinError 5] Access is denied`. +- **Resolution order:** `APM_TEMP_DIR` environment variable > `temp_dir` in `~/.apm/config.json` > system default. +- **Use Cases:** + - Set when the default system temp directory is restricted or unavailable + - Use the `APM_TEMP_DIR` environment variable for CI pipelines or per-session overrides + +**Examples:** +```bash +# Set a custom temp directory (Windows) +apm config set temp-dir C:\apm-temp + +# Set a custom temp directory (macOS/Linux) +apm config set temp-dir /tmp/apm-work + +# Check the current temp-dir setting +apm config get temp-dir + +# Or use the environment variable instead +export APM_TEMP_DIR=/tmp/apm-work +``` + ## Runtime Management (Experimental) ### `apm runtime` (Experimental) - Manage AI runtimes diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 029891bba..479451253 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -81,6 +81,6 @@ | Command | Purpose | Key flags | |---------|---------|-----------| | `apm config` | Show current configuration | -- | -| `apm config get [KEY]` | Get a config value | -- | -| `apm config set KEY VALUE` | Set a config value | -- | +| `apm config get [KEY]` | Get a config value (`auto-integrate`, `temp-dir`) | -- | +| `apm config set KEY VALUE` | Set a config value (`auto-integrate`, `temp-dir`) | -- | | `apm update` | Update APM itself | `--check` only check | diff --git a/src/apm_cli/bundle/unpacker.py b/src/apm_cli/bundle/unpacker.py index 908f4ddda..787b1a925 100644 --- a/src/apm_cli/bundle/unpacker.py +++ b/src/apm_cli/bundle/unpacker.py @@ -56,7 +56,8 @@ def unpack_bundle( # 1. If archive, extract to temp dir cleanup_temp = False if bundle_path.is_file() and bundle_path.name.endswith(".tar.gz"): - temp_dir = Path(tempfile.mkdtemp(prefix="apm-unpack-")) + from ..config import get_apm_temp_dir + temp_dir = Path(tempfile.mkdtemp(prefix="apm-unpack-", dir=get_apm_temp_dir())) cleanup_temp = True try: with tarfile.open(bundle_path, "r:gz") as tar: diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index 9108f2fea..74c36234d 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -84,6 +84,12 @@ def config(ctx): config_table.add_row("Global", "APM CLI Version", get_version()) + from ..config import get_temp_dir as _get_temp_dir + + _temp_dir_val = _get_temp_dir() + if _temp_dir_val: + config_table.add_row("", "Temp Directory", _temp_dir_val) + console.print(config_table) except (ImportError, NameError): @@ -105,6 +111,12 @@ def config(ctx): click.echo(f"\n{HIGHLIGHT}Global:{RESET}") click.echo(f" APM CLI Version: {get_version()}") + from ..config import get_temp_dir as _get_temp_dir_fb + + _temp_dir_fb = _get_temp_dir_fb() + if _temp_dir_fb: + click.echo(f" Temp Directory: {_temp_dir_fb}") + @config.command(help="Set a configuration value") @click.argument("key") @@ -116,7 +128,7 @@ def set(key, value): apm config set auto-integrate false apm config set auto-integrate true """ - from ..config import set_auto_integrate + from ..config import set_auto_integrate, set_temp_dir logger = CommandLogger("config set") if key == "auto-integrate": @@ -129,9 +141,16 @@ def set(key, value): else: logger.error(f"Invalid value '{value}'. Use 'true' or 'false'.") sys.exit(1) + elif key == "temp-dir": + try: + set_temp_dir(value) + logger.success(f"Temporary directory set to: {value}") + except ValueError as exc: + logger.error(str(exc)) + sys.exit(1) else: logger.error(f"Unknown configuration key: '{key}'") - logger.progress("Valid keys: auto-integrate") + logger.progress("Valid keys: auto-integrate, temp-dir") logger.progress( "This error may indicate a bug in command routing. Please report this issue." ) @@ -147,16 +166,22 @@ def get(key): apm config get auto-integrate apm config get """ - from ..config import get_auto_integrate + from ..config import get_auto_integrate, get_temp_dir logger = CommandLogger("config get") if key: if key == "auto-integrate": value = get_auto_integrate() click.echo(f"auto-integrate: {value}") + elif key == "temp-dir": + value = get_temp_dir() + if value is None: + click.echo("temp-dir: Not set (using system default)") + else: + click.echo(f"temp-dir: {value}") else: logger.error(f"Unknown configuration key: '{key}'") - logger.progress("Valid keys: auto-integrate") + logger.progress("Valid keys: auto-integrate, temp-dir") logger.progress( "This error may indicate a bug in command routing. Please report this issue." ) @@ -167,3 +192,5 @@ def get(key): # have not been written yet (e.g. auto_integrate on a fresh install). logger.progress("APM Configuration:") click.echo(f" auto-integrate: {get_auto_integrate()}") + temp_dir = get_temp_dir() + click.echo(f" temp-dir: {temp_dir if temp_dir is not None else 'Not set (using system default)'}") diff --git a/src/apm_cli/commands/update.py b/src/apm_cli/commands/update.py index 569c88b35..8011cf72d 100644 --- a/src/apm_cli/commands/update.py +++ b/src/apm_cli/commands/update.py @@ -117,8 +117,10 @@ def update(check): response.raise_for_status() # Create temporary file for install script + from ..config import get_apm_temp_dir with tempfile.NamedTemporaryFile( - mode="w", suffix=_get_update_installer_suffix(), delete=False + mode="w", suffix=_get_update_installer_suffix(), delete=False, + dir=get_apm_temp_dir() ) as f: temp_script = f.name f.write(response.text) diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index 0d4532215..214ec6a3e 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -92,3 +92,54 @@ def set_auto_integrate(enabled: bool) -> None: enabled: Whether to enable auto-integration. """ update_config({"auto_integrate": enabled}) + + +def get_temp_dir() -> Optional[str]: + """Get the configured temporary directory. + + Returns: + The stored temp_dir config value, or None if not set. + """ + return get_config().get("temp_dir") + + +def set_temp_dir(path: str) -> None: + """Set the temporary directory after validating it exists and is writable. + + The path is normalised (``~`` expansion + absolute) before validation and + storage so that relative or home-relative paths work predictably. + + Args: + path: Filesystem path to use as temporary directory. + + Raises: + ValueError: If the directory does not exist or is not writable. + """ + resolved = os.path.abspath(os.path.expanduser(path)) + if not os.path.isdir(resolved): + raise ValueError(f"Directory does not exist: {resolved}") + if not os.access(resolved, os.W_OK): + raise ValueError(f"Directory is not writable: {resolved}") + update_config({"temp_dir": resolved}) + + +def get_apm_temp_dir() -> Optional[str]: + """Return the effective temporary directory for APM operations. + + Resolution order: + 1. ``APM_TEMP_DIR`` environment variable (escape-hatch override) + 2. ``temp_dir`` value from ``~/.apm/config.json`` + 3. ``None`` (caller falls back to the system default) + + Empty or whitespace-only values are treated as unset and skipped. + + Returns: + Directory path string, or None when the system default should be used. + """ + env_val = os.environ.get("APM_TEMP_DIR", "").strip() + if env_val: + return env_val + config_val = (get_temp_dir() or "").strip() + if config_val: + return config_val + return None diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 1b1287863..37e502f11 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -195,7 +195,9 @@ def _setup_git_environment(self) -> Dict[str, Any]: if sys.platform == 'win32': # 'NUL' fails on some Windows git versions; use an empty temp file. import tempfile - empty_cfg = os.path.join(tempfile.gettempdir(), '.apm_empty_gitconfig') + from ..config import get_apm_temp_dir + temp_base = get_apm_temp_dir() or tempfile.gettempdir() + empty_cfg = os.path.join(temp_base, '.apm_empty_gitconfig') with open(empty_cfg, 'w') as f: pass env['GIT_CONFIG_GLOBAL'] = empty_cfg @@ -954,7 +956,8 @@ def resolve_git_reference(self, repo_ref: Union[str, "DependencyReference"]) -> # Create a temporary directory for Git operations temp_dir = None try: - temp_dir = Path(tempfile.mkdtemp()) + from ..config import get_apm_temp_dir + temp_dir = Path(tempfile.mkdtemp(dir=get_apm_temp_dir())) if is_likely_commit: # For commit SHAs, clone full repository first, then checkout the commit @@ -1775,7 +1778,8 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat # tempfile.TemporaryDirectory().__exit__ calls shutil.rmtree without our # retry logic, which raises WinError 32 when git processes still hold # handles at the end of the with-block. - temp_dir = tempfile.mkdtemp() + from ..config import get_apm_temp_dir + temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) try: # Sparse checkout always targets "repo/". If it fails we clone into # "repo_clone/" so we never have to rmtree a directory that may still @@ -1886,6 +1890,20 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat if progress_obj and progress_task_id is not None: progress_obj.update(progress_task_id, completed=90, total=100) + except PermissionError: + raise RuntimeError( + f"Access denied in temporary directory '{temp_dir}'. " + "Corporate security may restrict this path. " + "Fix: apm config set temp-dir " + ) from None + except OSError as exc: + if getattr(exc, 'errno', None) == 13 or getattr(exc, 'winerror', None) == 5: + raise RuntimeError( + f"Access denied in temporary directory '{temp_dir}'. " + "Corporate security may restrict this path. " + "Fix: apm config set temp-dir " + ) from None + raise finally: _rmtree(temp_dir) @@ -1938,6 +1956,7 @@ def _download_subdirectory_from_artifactory( ) -> PackageInfo: """Download an archive from Artifactory and extract a subdirectory.""" import tempfile + from ..config import get_apm_temp_dir ref = dep_ref.reference or "main" subdir_path = dep_ref.virtual_path repo_parts = dep_ref.repo_url.split('/') @@ -1947,7 +1966,7 @@ def _download_subdirectory_from_artifactory( if progress_obj and progress_task_id is not None: progress_obj.update(progress_task_id, completed=10, total=100) - with tempfile.TemporaryDirectory() as temp_dir: + with tempfile.TemporaryDirectory(dir=get_apm_temp_dir()) as temp_dir: temp_path = Path(temp_dir) / "full_pkg" self._download_artifactory_archive(host, prefix, owner, repo, ref, temp_path, scheme=scheme) if progress_obj and progress_task_id is not None: diff --git a/src/apm_cli/runtime/manager.py b/src/apm_cli/runtime/manager.py index bc78b79dd..d6b13c624 100644 --- a/src/apm_cli/runtime/manager.py +++ b/src/apm_cli/runtime/manager.py @@ -105,7 +105,8 @@ def run_embedded_script(self, script_content: str, common_content: str, """Execute an embedded setup script with common utilities.""" script_args = script_args or [] - with tempfile.TemporaryDirectory() as temp_dir: + from ..config import get_apm_temp_dir + with tempfile.TemporaryDirectory(dir=get_apm_temp_dir()) as temp_dir: temp_path = Path(temp_dir) if self._is_windows: diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index 6fff74106..e845b94c2 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -131,6 +131,42 @@ def test_config_show_fallback_inside_project(self): os.chdir(self.original_dir) assert result.exit_code == 0 + def test_config_show_displays_temp_dir_in_global_section(self): + """Fallback display includes Temp Directory row when temp-dir is configured.""" + import rich.table + + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + try: + with ( + patch("apm_cli.commands.config.get_version", return_value="1.2.3"), + patch("apm_cli.config.get_temp_dir", return_value="/custom/tmp"), + patch.object(rich.table, "Table", side_effect=ImportError("no rich")), + ): + result = self.runner.invoke(config, []) + finally: + os.chdir(self.original_dir) + assert result.exit_code == 0 + assert "Temp Directory: /custom/tmp" in result.output + + def test_config_show_omits_temp_dir_when_not_configured(self): + """Fallback display omits Temp Directory row when temp-dir is not configured.""" + import rich.table + + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + try: + with ( + patch("apm_cli.commands.config.get_version", return_value="1.2.3"), + patch("apm_cli.config.get_temp_dir", return_value=None), + patch.object(rich.table, "Table", side_effect=ImportError("no rich")), + ): + result = self.runner.invoke(config, []) + finally: + os.chdir(self.original_dir) + assert result.exit_code == 0 + assert "Temp Directory" not in result.output + class TestConfigSet: """Tests for `apm config set `.""" @@ -274,3 +310,187 @@ def test_set_auto_integrate_false_calls_update_config(self): with patch.object(cfg_module, "update_config") as mock_update: cfg_module.set_auto_integrate(False) mock_update.assert_called_once_with({"auto_integrate": False}) + + +class TestTempDirFunctions: + """Tests for get_temp_dir, set_temp_dir, and get_apm_temp_dir in apm_cli.config.""" + + def test_get_temp_dir_default_is_none(self): + """Returns None when temp_dir is not set.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "get_config", return_value={}): + assert cfg_module.get_temp_dir() is None + + def test_get_temp_dir_returns_stored_value(self): + """Returns stored temp_dir value.""" + import apm_cli.config as cfg_module + + with patch.object( + cfg_module, "get_config", return_value={"temp_dir": "/custom/tmp"} + ): + assert cfg_module.get_temp_dir() == "/custom/tmp" + + def test_set_temp_dir_validates_and_stores(self): + """set_temp_dir normalises path and stores via update_config.""" + import apm_cli.config as cfg_module + + with tempfile.TemporaryDirectory() as tmp: + with patch.object(cfg_module, "update_config") as mock_update: + cfg_module.set_temp_dir(tmp) + resolved = os.path.abspath(os.path.expanduser(tmp)) + mock_update.assert_called_once_with({"temp_dir": resolved}) + + def test_set_temp_dir_rejects_nonexistent_directory(self): + """Raises ValueError when path does not exist.""" + import apm_cli.config as cfg_module + + with pytest.raises(ValueError, match="does not exist"): + cfg_module.set_temp_dir("/nonexistent/path/xyz") + + def test_set_temp_dir_rejects_file_path(self): + """Raises ValueError when path is a file, not a directory.""" + import apm_cli.config as cfg_module + + with tempfile.NamedTemporaryFile() as f: + with pytest.raises(ValueError, match="does not exist"): + cfg_module.set_temp_dir(f.name) + + def test_set_temp_dir_normalises_home_path(self): + """Tilde paths are expanded before storage.""" + import apm_cli.config as cfg_module + + home = os.path.expanduser("~") + with patch.object(cfg_module, "update_config") as mock_update: + cfg_module.set_temp_dir("~") + mock_update.assert_called_once_with({"temp_dir": home}) + + def test_get_apm_temp_dir_prefers_env(self): + """Env var takes precedence over config value.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value="/from/config"), + patch.dict(os.environ, {"APM_TEMP_DIR": "/from/env"}), + ): + assert cfg_module.get_apm_temp_dir() == "/from/env" + + def test_get_apm_temp_dir_falls_back_to_config(self): + """Falls back to config when env var is not set.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value="/from/config"), + patch.dict(os.environ, {}, clear=False), + ): + os.environ.pop("APM_TEMP_DIR", None) + assert cfg_module.get_apm_temp_dir() == "/from/config" + + def test_get_apm_temp_dir_returns_none_when_unset(self): + """Returns None when neither config nor env var is set.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value=None), + patch.dict(os.environ, {}, clear=False), + ): + os.environ.pop("APM_TEMP_DIR", None) + assert cfg_module.get_apm_temp_dir() is None + + def test_get_apm_temp_dir_ignores_empty_env(self): + """Empty APM_TEMP_DIR is treated as unset.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value="/from/config"), + patch.dict(os.environ, {"APM_TEMP_DIR": ""}), + ): + assert cfg_module.get_apm_temp_dir() == "/from/config" + + def test_get_apm_temp_dir_ignores_whitespace_env(self): + """Whitespace-only APM_TEMP_DIR is treated as unset.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value=None), + patch.dict(os.environ, {"APM_TEMP_DIR": " "}), + ): + assert cfg_module.get_apm_temp_dir() is None + + def test_get_apm_temp_dir_ignores_empty_config(self): + """Empty config temp_dir is treated as unset.""" + import apm_cli.config as cfg_module + + with ( + patch.object(cfg_module, "get_temp_dir", return_value=""), + patch.dict(os.environ, {}, clear=False), + ): + os.environ.pop("APM_TEMP_DIR", None) + assert cfg_module.get_apm_temp_dir() is None + + +class TestConfigSetTempDir: + """Tests for `apm config set temp-dir `.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_set_temp_dir_success(self): + """Set a valid temp-dir.""" + with patch("apm_cli.config.set_temp_dir") as mock_set: + result = self.runner.invoke(config, ["set", "temp-dir", "/tmp/apm"]) + assert result.exit_code == 0 + mock_set.assert_called_once_with("/tmp/apm") + + def test_set_temp_dir_validation_error(self): + """Exit 1 when set_temp_dir raises ValueError.""" + with patch( + "apm_cli.config.set_temp_dir", + side_effect=ValueError("Directory does not exist: /bad"), + ): + result = self.runner.invoke(config, ["set", "temp-dir", "/bad"]) + assert result.exit_code == 1 + + def test_set_unknown_key_includes_temp_dir_in_valid_keys(self): + """Error message lists temp-dir as a valid key.""" + result = self.runner.invoke(config, ["set", "nonexistent", "value"]) + assert result.exit_code == 1 + assert "temp-dir" in result.output + + +class TestConfigGetTempDir: + """Tests for `apm config get temp-dir`.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_get_temp_dir_when_set(self): + """Display the configured temp-dir.""" + with patch("apm_cli.config.get_temp_dir", return_value="/custom/tmp"): + result = self.runner.invoke(config, ["get", "temp-dir"]) + assert result.exit_code == 0 + assert "temp-dir: /custom/tmp" in result.output + + def test_get_temp_dir_when_unset(self): + """Display fallback message when temp-dir is not configured.""" + with patch("apm_cli.config.get_temp_dir", return_value=None): + result = self.runner.invoke(config, ["get", "temp-dir"]) + assert result.exit_code == 0 + assert "Not set (using system default)" in result.output + + def test_get_unknown_key_includes_temp_dir_in_valid_keys(self): + """Error message lists temp-dir as a valid key.""" + result = self.runner.invoke(config, ["get", "nonexistent"]) + assert result.exit_code == 1 + assert "temp-dir" in result.output + + def test_get_all_config_maps_temp_dir_key(self): + """All-config listing maps internal temp_dir to display temp-dir.""" + fake_config = { + "auto_integrate": True, + "temp_dir": "/my/temp", + } + with patch("apm_cli.config.get_config", return_value=fake_config): + result = self.runner.invoke(config, ["get"]) + assert result.exit_code == 0 + assert "temp-dir: /my/temp" in result.output diff --git a/tests/unit/test_github_downloader_temp_dir.py b/tests/unit/test_github_downloader_temp_dir.py new file mode 100644 index 000000000..3662f68c1 --- /dev/null +++ b/tests/unit/test_github_downloader_temp_dir.py @@ -0,0 +1,134 @@ +"""Tests for PermissionError / OSError handling in GitHubPackageDownloader. + +Covers the temp-dir configuration feature requirement that PermissionError +(and OSError errno=13 / winerror=5) raised during download_subdirectory_package +are converted to RuntimeError with an actionable 'apm config set temp-dir' +suggestion, while other OSError variants propagate unchanged. +""" + +import errno +import os +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from apm_cli.deps.github_downloader import GitHubPackageDownloader + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_downloader(): + """Create a GitHubPackageDownloader with stubbed auth (no network needed).""" + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ): + return GitHubPackageDownloader() + + +def _make_virtual_dep_ref(subdir="src/agent", ref="main"): + """Return a MagicMock dep_ref that passes is_virtual subdirectory checks.""" + dep_ref = MagicMock() + dep_ref.is_virtual = True + dep_ref.virtual_path = subdir + dep_ref.is_virtual_subdirectory.return_value = True + dep_ref.reference = ref + return dep_ref + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + +class TestDownloadSubdirectoryPermissionError: + """PermissionError / OSError handling in download_subdirectory_package.""" + + def _invoke_with_error(self, downloader, dep_ref, side_effect): + """Invoke download_subdirectory_package with a mocked sparse-checkout + that raises *side_effect*, returning whatever exception propagates.""" + with ( + patch( + "apm_cli.deps.github_downloader.tempfile.mkdtemp", + return_value=tempfile.mkdtemp(), + ), + patch.object( + downloader, + "_try_sparse_checkout", + side_effect=side_effect, + ), + patch("apm_cli.deps.github_downloader._rmtree"), + ): + return downloader.download_subdirectory_package( + dep_ref, Path(tempfile.mkdtemp()) + ) + + def test_permission_error_raises_runtime_error_with_suggestion(self): + """PermissionError is converted to RuntimeError with temp-dir hint.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + msg = str(exc_info.value) + assert "apm config set temp-dir" in msg + + def test_permission_error_message_mentions_access_denied(self): + """RuntimeError message explains that access was denied.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + assert "Access denied" in str(exc_info.value) + + def test_oserror_errno13_raises_runtime_error_with_suggestion(self): + """OSError with errno=13 (EACCES) is converted to RuntimeError with temp-dir hint.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + exc = OSError("Permission denied") + exc.errno = 13 + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_error(downloader, dep_ref, exc) + assert "apm config set temp-dir" in str(exc_info.value) + + def test_oserror_winerror5_raises_runtime_error_with_suggestion(self): + """OSError with winerror=5 (ERROR_ACCESS_DENIED) is converted to RuntimeError.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + exc = OSError("Access is denied") + exc.winerror = 5 + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_error(downloader, dep_ref, exc) + assert "apm config set temp-dir" in str(exc_info.value) + + def test_other_oserror_is_reraised(self): + """OSError with unrelated errno is NOT caught and propagates unchanged.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + exc = OSError("Disk full") + exc.errno = errno.ENOSPC + with pytest.raises(OSError) as exc_info: + self._invoke_with_error(downloader, dep_ref, exc) + assert exc_info.value is exc + + def test_permission_error_chain_is_suppressed(self): + """RuntimeError suppress_context is True (raised with 'from None').""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + # 'raise ... from None' sets __suppress_context__ so the original + # PermissionError is hidden in tracebacks even though __context__ is set. + assert exc_info.value.__cause__ is None + assert exc_info.value.__suppress_context__ is True + + def test_invalid_dep_ref_not_virtual_raises_value_error(self): + """Passing a non-virtual dep_ref raises ValueError before any git ops.""" + downloader = _make_downloader() + dep_ref = MagicMock() + dep_ref.is_virtual = False + dep_ref.virtual_path = None + with pytest.raises(ValueError, match="virtual subdirectory package"): + downloader.download_subdirectory_package(dep_ref, Path("/tmp/target")) From 1c1c5bcc55bdaadb4380471169c1f236c2443419 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Thu, 9 Apr 2026 10:19:48 +0200 Subject: [PATCH 2/2] fix: address PR review feedback on temp-dir feature - Move mkdtemp() inside try block so PermissionError is caught (#629) - Narrow error handler to check failing path is under temp_dir - Differentiate "does not exist" vs "not a directory" in set_temp_dir - Log resolved path in success message, not raw input - Fix tests to trigger PermissionError from mkdtemp (realistic path) - Fix test temp dir leaks using tmp_path fixture - Update CHANGELOG to reference PR #629 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/apm_cli/commands/config.py | 3 +- src/apm_cli/config.py | 7 +- src/apm_cli/deps/github_downloader.py | 35 +++++--- tests/unit/test_config_command.py | 2 +- tests/unit/test_github_downloader_temp_dir.py | 89 +++++++++++++------ 6 files changed, 96 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ad3b1cfb..942671f8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `apm install` now automatically discovers and deploys local `.apm/` primitives (skills, instructions, agents, prompts, hooks, commands) to target directories, with local content taking priority over dependencies on collision (#626, #644) -- Add `temp-dir` configuration key (`apm config set temp-dir PATH`) to override the system temporary directory, resolving `[WinError 5] Access is denied` in corporate Windows environments (#624) +- Add `temp-dir` configuration key (`apm config set temp-dir PATH`) to override the system temporary directory, resolving `[WinError 5] Access is denied` in corporate Windows environments (#629) ### Fixed diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index 74c36234d..2ddfbfc6c 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -144,7 +144,8 @@ def set(key, value): elif key == "temp-dir": try: set_temp_dir(value) - logger.success(f"Temporary directory set to: {value}") + from ..config import get_temp_dir + logger.success(f"Temporary directory set to: {get_temp_dir()}") except ValueError as exc: logger.error(str(exc)) sys.exit(1) diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index 214ec6a3e..d0f8d93e7 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -113,11 +113,14 @@ def set_temp_dir(path: str) -> None: path: Filesystem path to use as temporary directory. Raises: - ValueError: If the directory does not exist or is not writable. + ValueError: If the path does not exist, is not a directory, or is not + writable. """ resolved = os.path.abspath(os.path.expanduser(path)) - if not os.path.isdir(resolved): + if not os.path.exists(resolved): raise ValueError(f"Directory does not exist: {resolved}") + if not os.path.isdir(resolved): + raise ValueError(f"Path is not a directory: {resolved}") if not os.access(resolved, os.W_OK): raise ValueError(f"Directory is not writable: {resolved}") update_config({"temp_dir": resolved}) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 37e502f11..f9887e6e4 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1779,8 +1779,9 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat # retry logic, which raises WinError 32 when git processes still hold # handles at the end of the with-block. from ..config import get_apm_temp_dir - temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) + temp_dir = None try: + temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) # Sparse checkout always targets "repo/". If it fails we clone into # "repo_clone/" so we never have to rmtree a directory that may still # have live git handles from the failed subprocess. @@ -1890,22 +1891,32 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat if progress_obj and progress_task_id is not None: progress_obj.update(progress_task_id, completed=90, total=100) - except PermissionError: - raise RuntimeError( - f"Access denied in temporary directory '{temp_dir}'. " - "Corporate security may restrict this path. " - "Fix: apm config set temp-dir " - ) from None - except OSError as exc: - if getattr(exc, 'errno', None) == 13 or getattr(exc, 'winerror', None) == 5: + except PermissionError as exc: + exc_path = getattr(exc, 'filename', None) + # If temp_dir wasn't created (mkdtemp failed) or the error is within + # the temp tree, this is likely a restricted temp directory issue. + if temp_dir is None or (exc_path and str(exc_path).startswith(str(temp_dir))): raise RuntimeError( - f"Access denied in temporary directory '{temp_dir}'. " - "Corporate security may restrict this path. " + "Access denied in temporary directory" + + (f" '{temp_dir}'" if temp_dir else "") + + ". Corporate security may restrict this path. " "Fix: apm config set temp-dir " ) from None raise + except OSError as exc: + if getattr(exc, 'errno', None) == 13 or getattr(exc, 'winerror', None) == 5: + exc_path = getattr(exc, 'filename', None) + if temp_dir is None or (exc_path and str(exc_path).startswith(str(temp_dir))): + raise RuntimeError( + "Access denied in temporary directory" + + (f" '{temp_dir}'" if temp_dir else "") + + ". Corporate security may restrict this path. " + "Fix: apm config set temp-dir " + ) from None + raise finally: - _rmtree(temp_dir) + if temp_dir: + _rmtree(temp_dir) # Validate the extracted package (after temp dir is cleaned up) validation_result = validate_apm_package(target_path) diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index e845b94c2..bf89df6ff 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -353,7 +353,7 @@ def test_set_temp_dir_rejects_file_path(self): import apm_cli.config as cfg_module with tempfile.NamedTemporaryFile() as f: - with pytest.raises(ValueError, match="does not exist"): + with pytest.raises(ValueError, match="not a directory"): cfg_module.set_temp_dir(f.name) def test_set_temp_dir_normalises_home_path(self): diff --git a/tests/unit/test_github_downloader_temp_dir.py b/tests/unit/test_github_downloader_temp_dir.py index 3662f68c1..b6789c30a 100644 --- a/tests/unit/test_github_downloader_temp_dir.py +++ b/tests/unit/test_github_downloader_temp_dir.py @@ -8,7 +8,6 @@ import errno import os -import tempfile from pathlib import Path from unittest.mock import MagicMock, patch @@ -47,83 +46,123 @@ def _make_virtual_dep_ref(subdir="src/agent", ref="main"): class TestDownloadSubdirectoryPermissionError: """PermissionError / OSError handling in download_subdirectory_package.""" - def _invoke_with_error(self, downloader, dep_ref, side_effect): - """Invoke download_subdirectory_package with a mocked sparse-checkout - that raises *side_effect*, returning whatever exception propagates.""" + def _invoke_with_mkdtemp_error(self, downloader, dep_ref, side_effect, tmp_path): + """Invoke download_subdirectory_package with a mocked mkdtemp that + raises *side_effect*, returning whatever exception propagates.""" with ( patch( "apm_cli.deps.github_downloader.tempfile.mkdtemp", - return_value=tempfile.mkdtemp(), - ), - patch.object( - downloader, - "_try_sparse_checkout", side_effect=side_effect, ), patch("apm_cli.deps.github_downloader._rmtree"), ): return downloader.download_subdirectory_package( - dep_ref, Path(tempfile.mkdtemp()) + dep_ref, tmp_path / "target" ) - def test_permission_error_raises_runtime_error_with_suggestion(self): - """PermissionError is converted to RuntimeError with temp-dir hint.""" + def test_permission_error_raises_runtime_error_with_suggestion(self, tmp_path): + """PermissionError from mkdtemp is converted to RuntimeError with temp-dir hint.""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() with pytest.raises(RuntimeError) as exc_info: - self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + self._invoke_with_mkdtemp_error( + downloader, dep_ref, PermissionError("denied"), tmp_path + ) msg = str(exc_info.value) assert "apm config set temp-dir" in msg - def test_permission_error_message_mentions_access_denied(self): + def test_permission_error_message_mentions_access_denied(self, tmp_path): """RuntimeError message explains that access was denied.""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() with pytest.raises(RuntimeError) as exc_info: - self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + self._invoke_with_mkdtemp_error( + downloader, dep_ref, PermissionError("denied"), tmp_path + ) assert "Access denied" in str(exc_info.value) - def test_oserror_errno13_raises_runtime_error_with_suggestion(self): - """OSError with errno=13 (EACCES) is converted to RuntimeError with temp-dir hint.""" + def test_oserror_errno13_raises_runtime_error_with_suggestion(self, tmp_path): + """OSError with errno=13 (EACCES) from mkdtemp is converted to RuntimeError.""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() exc = OSError("Permission denied") exc.errno = 13 with pytest.raises(RuntimeError) as exc_info: - self._invoke_with_error(downloader, dep_ref, exc) + self._invoke_with_mkdtemp_error(downloader, dep_ref, exc, tmp_path) assert "apm config set temp-dir" in str(exc_info.value) - def test_oserror_winerror5_raises_runtime_error_with_suggestion(self): - """OSError with winerror=5 (ERROR_ACCESS_DENIED) is converted to RuntimeError.""" + def test_oserror_winerror5_raises_runtime_error_with_suggestion(self, tmp_path): + """OSError with winerror=5 (ERROR_ACCESS_DENIED) from mkdtemp is converted to RuntimeError.""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() exc = OSError("Access is denied") exc.winerror = 5 with pytest.raises(RuntimeError) as exc_info: - self._invoke_with_error(downloader, dep_ref, exc) + self._invoke_with_mkdtemp_error(downloader, dep_ref, exc, tmp_path) assert "apm config set temp-dir" in str(exc_info.value) - def test_other_oserror_is_reraised(self): + def test_other_oserror_is_reraised(self, tmp_path): """OSError with unrelated errno is NOT caught and propagates unchanged.""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() exc = OSError("Disk full") exc.errno = errno.ENOSPC with pytest.raises(OSError) as exc_info: - self._invoke_with_error(downloader, dep_ref, exc) + self._invoke_with_mkdtemp_error(downloader, dep_ref, exc, tmp_path) assert exc_info.value is exc - def test_permission_error_chain_is_suppressed(self): + def test_permission_error_chain_is_suppressed(self, tmp_path): """RuntimeError suppress_context is True (raised with 'from None').""" downloader = _make_downloader() dep_ref = _make_virtual_dep_ref() with pytest.raises(RuntimeError) as exc_info: - self._invoke_with_error(downloader, dep_ref, PermissionError("denied")) + self._invoke_with_mkdtemp_error( + downloader, dep_ref, PermissionError("denied"), tmp_path + ) # 'raise ... from None' sets __suppress_context__ so the original # PermissionError is hidden in tracebacks even though __context__ is set. assert exc_info.value.__cause__ is None assert exc_info.value.__suppress_context__ is True + def test_permission_error_from_mkdtemp_omits_path_in_message(self, tmp_path): + """When mkdtemp itself fails, temp_dir is None so message omits a path.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + with pytest.raises(RuntimeError) as exc_info: + self._invoke_with_mkdtemp_error( + downloader, dep_ref, PermissionError("denied"), tmp_path + ) + msg = str(exc_info.value) + assert "Access denied in temporary directory." in msg + # No quoted path when mkdtemp never created the directory + assert "'" not in msg + + def test_permission_error_on_target_path_is_reraised(self, tmp_path): + """PermissionError whose filename is outside temp_dir re-raises.""" + downloader = _make_downloader() + dep_ref = _make_virtual_dep_ref() + fake_temp = str(tmp_path / "faketemp") + target_exc = PermissionError("denied") + target_exc.filename = "/other/path/outside" + + with ( + patch( + "apm_cli.deps.github_downloader.tempfile.mkdtemp", + return_value=fake_temp, + ), + patch.object( + downloader, + "_try_sparse_checkout", + side_effect=target_exc, + ), + patch("apm_cli.deps.github_downloader._rmtree"), + ): + with pytest.raises(PermissionError) as exc_info: + downloader.download_subdirectory_package( + dep_ref, tmp_path / "target" + ) + assert exc_info.value is target_exc + def test_invalid_dep_ref_not_virtual_raises_value_error(self): """Passing a non-virtual dep_ref raises ValueError before any git ops.""" downloader = _make_downloader()