diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d525b0dc..d007d98bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Gemini CLI** as a supported APM target (`--target gemini`): auto-detects `.gemini/`, writes MCP config to `.gemini/settings.json`, and adds `apm runtime setup|remove gemini`. (#917) +- Experimental `cowork` target for Microsoft 365 Copilot Cowork custom-skill deployment via OneDrive (`apm experimental enable cowork`; `apm install --target cowork --global`; persisted via `apm config set cowork-skills-dir`). (#913) - `apm experimental` command group (`list` / `enable` / `disable` / `reset`) lets you opt into new behaviour before it graduates to default. Ships with the `verbose-version` flag. (#849) - `apm audit --ci` now verifies hash integrity of locally deployed `.apm/` files so hand-edits and config drift fail CI instead of slipping through. (#887) - `includes:` manifest field (`auto` or list) gives you explicit control over which local `.apm/` files are deployed; pair with `policy.manifest.require_explicit_includes` to block silent expansion. Audit raises an `includes-consent` advisory while you migrate. (#887) diff --git a/docs/astro.config.mjs b/docs/astro.config.mjs index f558c74fa..2aa97ebf3 100644 --- a/docs/astro.config.mjs +++ b/docs/astro.config.mjs @@ -105,6 +105,7 @@ export default defineConfig({ { label: 'CI/CD Pipelines', slug: 'integrations/ci-cd' }, { label: 'GitHub Agentic Workflows', slug: 'integrations/gh-aw' }, { label: 'IDE & Tool Integration', slug: 'integrations/ide-tool-integration' }, + { label: 'Microsoft 365 Copilot Cowork (Experimental)', slug: 'integrations/copilot-cowork' }, { label: 'AI Runtime Compatibility', slug: 'integrations/runtime-compatibility' }, { label: 'GitHub Rulesets', slug: 'integrations/github-rulesets' }, ], diff --git a/docs/src/content/docs/integrations/copilot-cowork.md b/docs/src/content/docs/integrations/copilot-cowork.md new file mode 100644 index 000000000..1114e414c --- /dev/null +++ b/docs/src/content/docs/integrations/copilot-cowork.md @@ -0,0 +1,145 @@ +--- +title: "Microsoft 365 Copilot Cowork (Experimental)" +description: "Deploy APM skills to Microsoft 365 Copilot Cowork through a OneDrive-synchronised skills folder." +sidebar: + order: 4 +--- + +:::caution[Frontier preview] +This integration is experimental and off by default. You must enable the `copilot-cowork` flag before using it. + +```bash +apm experimental enable copilot-cowork +``` + +Until the flag is enabled, the `copilot-cowork` target stays inert: it is hidden from active target detection, and explicit `--target copilot-cowork` installs fail cleanly instead of deploying anything. +::: + +## What it does + +When the `copilot-cowork` flag is enabled, APM can deploy package skills to Microsoft 365 Copilot Cowork at user scope. APM writes each deployed skill to Cowork's fixed OneDrive convention: + +```text +/Documents/Cowork/skills//SKILL.md +``` + +## Enable the flag + +```bash +apm experimental enable copilot-cowork +apm experimental list +apm experimental disable copilot-cowork +``` + +Use `apm experimental list` to confirm whether `copilot-cowork` is enabled on the current machine. + +## OneDrive auto-detection + +Resolution is first match wins: + +1. If `APM_COPILOT_COWORK_SKILLS_DIR` is set, APM uses that path as-is. +2. Otherwise if `apm config set copilot-cowork-skills-dir` has stored a path, APM uses that persisted value. +3. Otherwise APM falls back to platform-specific detection. + +| Platform | Resolution | +|----------|------------| +| macOS | Search `~/Library/CloudStorage/OneDrive*`. One match is used. No matches means Cowork is unavailable. Two or more matches fail with an actionable error that lists the candidates and recommends `APM_COPILOT_COWORK_SKILLS_DIR`. | +| Windows | Use `%ONEDRIVECOMMERCIAL%`, then `%ONEDRIVE%`. | +| Linux | No default lookup. Set `APM_COPILOT_COWORK_SKILLS_DIR` or persist the path with `apm config set copilot-cowork-skills-dir ...`. | + +When APM finds a OneDrive root, it always deploys to `Documents/Cowork/skills/` under that root. + +## APM_COPILOT_COWORK_SKILLS_DIR override + +Set `APM_COPILOT_COWORK_SKILLS_DIR` when you need to bypass auto-detection, such as: + +- a non-standard OneDrive install +- a multi-tenant macOS machine +- Linux, where there is no platform default + +Example: + +```bash +export APM_COPILOT_COWORK_SKILLS_DIR="$HOME/Library/CloudStorage/OneDrive - Contoso/Documents/Cowork/skills" +``` + +## Persisting the skills directory + +Use `apm config` when you want the Cowork skills path to persist across shells. This is especially useful on Linux, where there is no auto-detection and you would otherwise need to export `APM_COPILOT_COWORK_SKILLS_DIR` in every shell. + +Set a persisted path: + +```bash +apm experimental enable copilot-cowork +apm config set copilot-cowork-skills-dir "$HOME/OneDrive/Documents/Cowork/skills" +``` + +`apm config set copilot-cowork-skills-dir` requires the `copilot-cowork` experimental flag. APM expands `~`, rejects empty or whitespace-only values, and rejects relative paths. The path does not need to exist yet, which is useful while OneDrive is still synchronising. + +Inspect the stored value: + +```bash +apm config get copilot-cowork-skills-dir +``` + +`apm config get copilot-cowork-skills-dir` works whether or not the `copilot-cowork` flag is enabled, and prints the stored path or `Not set`. + +Clear the persisted path: + +```bash +apm config unset copilot-cowork-skills-dir +``` + +`apm config unset copilot-cowork-skills-dir` also works whether or not the `copilot-cowork` flag is enabled. + +## Install + +Cowork is user-scope only. Use `--global`, and add `--target copilot-cowork` when you want to target Cowork explicitly. + +```bash +apm install --global +apm install --target copilot-cowork --global +``` + +Cowork deployments are skills only: + +```text +.apm/skills//SKILL.md +-> /Documents/Cowork/skills//SKILL.md +``` + +If you try project scope, APM stops with a clean error that tells you to rerun with `--global`. + +## Skills-only behaviour + +Cowork deploys only `SKILL.md` content. Instructions, agents, prompts, hooks, commands, chatmodes, and MCP material are skipped for this target. + +If any selected package contains non-skill primitives, APM emits one `[!]` summary warning for the whole install run. The install still succeeds, and the skill content still deploys. + +## Caps + +Cowork limits are warn-only. They never block install: + +- More than 50 skills in the Cowork directory after install -> one `[!]` warning recommending review. +- Any individual `SKILL.md` larger than 1 MiB -> one `[!]` warning for that file. + +## Lockfile representation + +In `apm.lock.yaml`, Cowork-deployed paths are recorded as synthetic URIs such as: + +```text +cowork://skills/my-skill/SKILL.md +``` + +This keeps the lockfile portable across machines, users, and OneDrive tenants. APM translates between `cowork://skills/...` and absolute filesystem paths only at I/O boundaries; internal install logic still works with absolute `Path` objects. + +## Troubleshooting + +- Cowork unavailable or no OneDrive detected: confirm OneDrive is installed and synchronising, then set `APM_COPILOT_COWORK_SKILLS_DIR`. +- macOS multi-tenant error: set `APM_COPILOT_COWORK_SKILLS_DIR` to the account you want to use. +- Linux: set `APM_COPILOT_COWORK_SKILLS_DIR` or persist the path with `apm config set copilot-cowork-skills-dir ...`. +- Path still persists after disabling `copilot-cowork`: run `apm config unset copilot-cowork-skills-dir` to remove the stored value. +- Project-scope error: rerun with `--global`. +- Non-skill primitives skipped: expected behaviour. Cowork only deploys skills. + +See also [IDE and Tool Integration](../ide-tool-integration/) and [apm experimental](../../reference/experimental/). diff --git a/docs/src/content/docs/integrations/ide-tool-integration.md b/docs/src/content/docs/integrations/ide-tool-integration.md index 8de94b0dd..2add1fd12 100644 --- a/docs/src/content/docs/integrations/ide-tool-integration.md +++ b/docs/src/content/docs/integrations/ide-tool-integration.md @@ -51,7 +51,7 @@ apm compile For running agentic workflows locally, see the [Agent Workflows guide](../../guides/agent-workflows/). -> **User-scope deployment**: `apm install -g` deploys primitives to user-level directories (`~/.copilot/`, `~/.claude/`, etc.), making packages available across all projects. See [Global Installation](../../guides/dependencies/#global-user-scope-installation) for per-target coverage. +> **User-scope deployment**: `apm install -g` deploys primitives to user-level directories (`~/.copilot/`, `~/.claude/`, etc.), making packages available across all projects. See [Global Installation](../../guides/dependencies/#global-user-scope-installation) for per-target coverage. For Microsoft 365 Copilot Cowork custom skills, enable `copilot-cowork` with `apm experimental enable copilot-cowork` and use `apm install --target copilot-cowork --global`. See [Microsoft 365 Copilot Cowork](../copilot-cowork/). ## VS Code Integration @@ -619,4 +619,4 @@ The following IDE integrations are planned for future releases: - **[CLI Reference](../../reference/cli-commands/)** -- Complete command documentation - Review the [VSCode Copilot Customization Guide](https://code.visualstudio.com/docs/copilot/copilot-customization) for VSCode-specific features - Check the [Spec-kit documentation](https://github.com/github/spec-kit) for SDD integration details -- Explore [MCP servers](https://modelcontextprotocol.io/servers) for tool integration options \ No newline at end of file +- Explore [MCP servers](https://modelcontextprotocol.io/servers) for tool integration options diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index d70d3e6bd..260d4f403 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -87,7 +87,8 @@ apm install [PACKAGES...] [OPTIONS] - `--runtime TEXT` - Target specific runtime only (copilot, codex, gemini, vscode) - `--exclude TEXT` - Exclude specific runtime from installation - `--only [apm|mcp]` - Install only specific dependency type -- `--target [copilot|claude|cursor|codex|opencode|gemini|all]` - Force deployment to specific target(s). Accepts comma-separated values for multiple targets (e.g., `-t claude,copilot`). Overrides auto-detection +- `--target [copilot|claude|cursor|codex|opencode|gemini|copilot-cowork|all]` - Force deployment to specific target(s). Accepts comma-separated values for multiple targets (e.g., `-t claude,copilot`). Overrides auto-detection + - `copilot-cowork` - Microsoft 365 Copilot Cowork skills (user scope only, requires `copilot-cowork` experimental flag) - `--update` - Update dependencies to latest Git references - `--force` - Overwrite locally-authored files on collision; bypass security scan blocks - `--dry-run` - Show what would be installed without installing @@ -1572,6 +1573,7 @@ apm config get [KEY] - `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 + - `copilot-cowork-skills-dir` - Override the resolved Cowork OneDrive skills directory If `KEY` is omitted, displays all configuration values. @@ -1596,6 +1598,7 @@ apm config set KEY VALUE - `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 + - `copilot-cowork-skills-dir` - Override the resolved Cowork OneDrive skills directory - `VALUE` - Value to set. For boolean keys, use: `true`, `false`, `yes`, `no`, `1`, `0` **Configuration Keys:** @@ -1641,6 +1644,30 @@ apm config get temp-dir export APM_TEMP_DIR=/tmp/apm-work ``` +**`copilot-cowork-skills-dir`** - Override the resolved Cowork OneDrive skills directory +- **Type:** String (absolute directory path) +- **Default:** Auto-detected Cowork skills directory (not stored) +- **Description:** Override the resolved Cowork OneDrive skills directory. Gated on the `copilot-cowork` experimental flag for `set`; `get` and `unset` are always available for cleanup. +- **Resolution order:** `APM_COPILOT_COWORK_SKILLS_DIR` environment variable > `copilot_cowork_skills_dir` in `~/.apm/config.json` > platform auto-detection. +- **Use Cases:** + - Set a specific OneDrive-backed Cowork skills directory instead of relying on auto-detection + - Clear the override with `apm config unset copilot-cowork-skills-dir` when returning to auto-detection + +**Examples:** +```bash +# Enable the experimental flag, then set an explicit Cowork skills directory +apm experimental enable copilot-cowork +apm config set copilot-cowork-skills-dir ~/Library/CloudStorage/OneDrive-Contoso/Documents/Cowork/skills + +# Check the current copilot-cowork-skills-dir setting +apm config get copilot-cowork-skills-dir + +# Remove the override and return to auto-detection +apm config unset copilot-cowork-skills-dir +``` + +See also: [Cowork integration](../integrations/copilot-cowork/). + ## Runtime Management (Experimental) ### `apm runtime` (Experimental) - Manage AI runtimes diff --git a/docs/src/content/docs/reference/experimental.md b/docs/src/content/docs/reference/experimental.md index 132441ff8..d34dd34df 100644 --- a/docs/src/content/docs/reference/experimental.md +++ b/docs/src/content/docs/reference/experimental.md @@ -170,8 +170,10 @@ apm experimental reset verbose-version | Name | Description | |-------------------|----------------------------------------------------------------------------------| | `verbose-version` | Show Python version, platform, and install path in `apm --version`. | +| `copilot-cowork` | Deploy APM skills to Microsoft 365 Copilot Cowork via OneDrive. | New flags are proposed via [CONTRIBUTING.md](https://github.com/microsoft/apm/blob/main/CONTRIBUTING.md#how-to-add-an-experimental-feature-flag) and graduate to default when stable. See the contributor recipe for the full lifecycle. +See also: [Cowork integration](../integrations/copilot-cowork/). ## Storage and scope diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 6a587e711..393bdbcf4 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -10,7 +10,7 @@ | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from SKILL_BUNDLE (repeatable; persisted in apm.yml; `'*'` resets to all), `--mcp NAME` add MCP entry, `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | +| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from SKILL_BUNDLE (repeatable; persisted in apm.yml; `'*'` resets to all), `--mcp NAME` add MCP entry, `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | | `apm uninstall PKGS...` | Remove packages | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | | `apm deps list` | List installed packages | `-g` global, `--all` both scopes, `--insecure` | @@ -92,6 +92,8 @@ Set `MCP_REGISTRY_URL` (default `https://api.mcp.github.com`) to point all `apm | `apm experimental disable NAME` | Disable an opt-in experimental flag | `-v` verbose | | `apm experimental reset [NAME]` | Reset one flag or all flags to defaults; also cleans malformed overrides during bulk reset | `-y` skip confirm, `-v` verbose | +Use `apm experimental enable copilot-cowork` to turn on Microsoft 365 Copilot Cowork skill deployment. Once enabled, deploy skills with `apm install --target copilot-cowork --global`. + Experimental flags MUST NOT gate security-critical behaviour (content scanning, path validation, lockfile integrity, token handling, MCP trust, collision detection). Flags are ergonomic/UX toggles only. ## Configuration and updates @@ -99,6 +101,9 @@ Experimental flags MUST NOT gate security-critical behaviour (content scanning, | Command | Purpose | Key flags | |---------|---------|-----------| | `apm config` | Show current configuration | -- | -| `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 config get [KEY]` | Get a config value (`auto-integrate`, `temp-dir`, `copilot-cowork-skills-dir`) | -- | +| `apm config set KEY VALUE` | Set a config value (`auto-integrate`, `temp-dir`; `copilot-cowork-skills-dir` requires `apm experimental enable copilot-cowork`) | -- | +| `apm config unset KEY` | Remove a stored config value (`temp-dir`, `copilot-cowork-skills-dir`) | -- | | `apm update` | Update APM itself (or show distributor guidance when self-update is disabled at build time) | `--check` only check | + +`apm config set copilot-cowork-skills-dir ` persists the Cowork skills directory across shells. `apm config get copilot-cowork-skills-dir` and `apm config unset copilot-cowork-skills-dir` remain available even when the `copilot-cowork` flag is disabled so leftover state can still be inspected or cleared. In `apm config` and bare `apm config get`, the `copilot-cowork-skills-dir` entry is shown only when the `copilot-cowork` flag is enabled. diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index 3291f7a1d..da3b4172b 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -19,6 +19,7 @@ _CONFIG_KEY_DISPLAY_NAMES = { "auto_integrate": "auto-integrate", "temp_dir": "temp-dir", + "copilot_cowork_skills_dir": "copilot-cowork-skills-dir", } @@ -52,7 +53,7 @@ def _get_config_getters(): def _valid_config_keys() -> str: """Return valid config keys for messages.""" - return ", ".join(["auto-integrate", "temp-dir"]) + return ", ".join(["auto-integrate", "temp-dir", "copilot-cowork-skills-dir"]) @click.group(help="Configure APM CLI", invoke_without_command=True) @@ -130,6 +131,18 @@ def config(ctx): if _temp_dir_val: config_table.add_row("", "Temp Directory", _temp_dir_val) + from ..core.experimental import is_enabled as _is_enabled + + if _is_enabled("copilot_cowork"): + from ..config import get_copilot_cowork_skills_dir as _get_csd + + _csd_val = _get_csd() + config_table.add_row( + "", + "Cowork Skills Dir", + _csd_val if _csd_val else "Not set (using auto-detection)", + ) + console.print(config_table) except (ImportError, NameError): @@ -157,6 +170,17 @@ def config(ctx): if _temp_dir_fb: click.echo(f" Temp Directory: {_temp_dir_fb}") + from ..core.experimental import is_enabled as _is_enabled_fb + + if _is_enabled_fb("copilot_cowork"): + from ..config import get_copilot_cowork_skills_dir as _get_csd_fb + + _csd_fb = _get_csd_fb() + click.echo( + f" Cowork Skills Dir: " + f"{_csd_fb if _csd_fb else 'Not set (using auto-detection)'}" + ) + @config.command(help="Set a configuration value") @click.argument("key") @@ -171,6 +195,27 @@ def set(key, value): from ..config import get_temp_dir, set_temp_dir logger = CommandLogger("config set") + if key == "copilot-cowork-skills-dir": + from ..core.experimental import is_enabled + + if not is_enabled("copilot_cowork"): + logger.error( + "copilot-cowork-skills-dir requires the copilot-cowork experimental flag. " + "Run: apm experimental enable copilot-cowork" + ) + sys.exit(1) + from ..config import get_copilot_cowork_skills_dir, set_copilot_cowork_skills_dir + + try: + set_copilot_cowork_skills_dir(value) + logger.success( + f"Cowork skills directory set to: {get_copilot_cowork_skills_dir()}" + ) + except ValueError as exc: + logger.error(str(exc)) + sys.exit(1) + return + if key == "temp-dir": try: set_temp_dir(value) @@ -218,6 +263,18 @@ def get(key): logger = CommandLogger("config get") getters = _get_config_getters() if key: + if key == "copilot-cowork-skills-dir": + from ..config import get_copilot_cowork_skills_dir + + value = get_copilot_cowork_skills_dir() + if value is None: + click.echo( + "copilot-cowork-skills-dir: Not set (using auto-detection)" + ) + else: + click.echo(f"copilot-cowork-skills-dir: {value}") + return + if key == "temp-dir": value = get_temp_dir() if value is None: @@ -244,3 +301,44 @@ def get(key): 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)'}") + + from ..core.experimental import is_enabled as _is_enabled_get + + if _is_enabled_get("copilot_cowork"): + from ..config import get_copilot_cowork_skills_dir as _get_csd_get + + csd = _get_csd_get() + click.echo( + f" copilot-cowork-skills-dir: " + f"{csd if csd is not None else 'Not set (using auto-detection)'}" + ) + + +@config.command(help="Unset a configuration value") +@click.argument("key") +def unset(key): + """Unset (remove) a configuration value. + + Examples: + apm config unset temp-dir + apm config unset copilot-cowork-skills-dir + """ + logger = CommandLogger("config unset") + + if key == "temp-dir": + from ..config import unset_temp_dir + + unset_temp_dir() + logger.success("Temporary directory configuration removed") + return + + if key == "copilot-cowork-skills-dir": + from ..config import unset_copilot_cowork_skills_dir + + unset_copilot_cowork_skills_dir() + logger.success("Cowork skills directory configuration removed") + return + + logger.error(f"Unknown configuration key: '{key}'") + logger.progress(f"Valid keys: {_valid_config_keys()}") + sys.exit(1) diff --git a/src/apm_cli/commands/uninstall/engine.py b/src/apm_cli/commands/uninstall/engine.py index 13e264127..5d3d4fa9e 100644 --- a/src/apm_cli/commands/uninstall/engine.py +++ b/src/apm_cli/commands/uninstall/engine.py @@ -298,11 +298,43 @@ def _sync_integrations_after_uninstall(apm_package, project_root, all_deployed_f if (project_root / er / "skills").exists(): _skill_dirs_exist = True break - if _skill_dirs_exist: + + # Scan sync_managed DIRECTLY for cowork:// entries. + # partition_managed_files() uses resolved_deploy_root to detect + # dynamic-root targets, but the static KNOWN_TARGETS["copilot-cowork"] + # always has resolved_deploy_root=None (it is only set after for_scope() + # resolves the OneDrive path at install time). As a result, cowork:// + # paths are never routed into _buckets["skills"] by the partition, so + # the bucket-based _has_cowork_skills check in the previous fix always + # returned False. Bypassing the bucket and scanning sync_managed + # directly is the correct approach: no partition logic is involved. + _cowork_skill_files: "set" = set() + if sync_managed: + from ...integration.copilot_cowork_paths import COWORK_URI_SCHEME + _cowork_skill_files = { + p for p in sync_managed if p.startswith(COWORK_URI_SCHEME) + } + _has_cowork_skills = bool(_cowork_skill_files) + + if _skill_dirs_exist or _has_cowork_skills: + # Merge cowork entries into the skills bucket so sync_integration + # receives them via managed_files. + if _has_cowork_skills and _buckets is not None: + _buckets.setdefault("skills", set()).update(_cowork_skill_files) + elif _has_cowork_skills: + _buckets = {"skills": _cowork_skill_files, "hooks": set()} + + # When cowork entries are present, pass targets=None so + # sync_integration builds skill_prefix_tuple from KNOWN_TARGETS + # (which includes the copilot-cowork target with user_root_resolver + # set). Using _resolved_targets alone would yield only the local + # prefix (.copilot/skills/) and cowork:// paths would be silently + # skipped by the startswith() guard inside sync_integration. + _sync_targets = None if _has_cowork_skills else _resolved_targets result = _integrators["skills"].sync_integration( apm_package, project_root, managed_files=_buckets["skills"] if _buckets else None, - targets=_resolved_targets, + targets=_sync_targets, ) counts["skills"] = result.get("files_removed", 0) diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index d0f8d93e7..af2dd8aae 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -126,6 +126,69 @@ def set_temp_dir(path: str) -> None: update_config({"temp_dir": resolved}) +def unset_temp_dir() -> None: + """Remove the ``temp_dir`` key from the config file. + + No-op if the key is not present. + """ + _invalidate_config_cache() + config = get_config() + if "temp_dir" in config: + del config["temp_dir"] + with open(CONFIG_FILE, "w") as f: + json.dump(config, f, indent=2) + _invalidate_config_cache() + + +# --------------------------------------------------------------------------- +# Cowork skills directory +# --------------------------------------------------------------------------- + +def get_copilot_cowork_skills_dir() -> Optional[str]: + """Get the configured cowork skills directory. + + Returns: + The stored ``copilot_cowork_skills_dir`` config value, or ``None`` if not set. + """ + return get_config().get("copilot_cowork_skills_dir") + + +def set_copilot_cowork_skills_dir(path: str) -> None: + """Set the cowork skills directory after validation. + + The path is expanded (``~``) and verified to be absolute. The + directory does **not** need to exist on disk (OneDrive may not yet + be synced). + + Args: + path: Filesystem path to use as the cowork skills directory. + + Raises: + ValueError: If *path* is empty, whitespace-only, or relative + after expansion. + """ + if not path or not path.strip(): + raise ValueError("Path cannot be empty") + expanded = os.path.expanduser(path) + if not os.path.isabs(expanded): + raise ValueError(f"Path must be absolute: {expanded}") + update_config({"copilot_cowork_skills_dir": expanded}) + + +def unset_copilot_cowork_skills_dir() -> None: + """Remove the ``copilot_cowork_skills_dir`` key from the config file. + + No-op if the key is not present. + """ + _invalidate_config_cache() + config = get_config() + if "copilot_cowork_skills_dir" in config: + del config["copilot_cowork_skills_dir"] + with open(CONFIG_FILE, "w") as f: + json.dump(config, f, indent=2) + _invalidate_config_cache() + + def get_apm_temp_dir() -> Optional[str]: """Return the effective temporary directory for APM operations. diff --git a/src/apm_cli/core/experimental.py b/src/apm_cli/core/experimental.py index 15939df4f..b8e9faa83 100644 --- a/src/apm_cli/core/experimental.py +++ b/src/apm_cli/core/experimental.py @@ -61,6 +61,15 @@ class ExperimentalFlag: default=False, hint="Run 'apm --version' to see the new output.", ), + "copilot_cowork": ExperimentalFlag( + name="copilot_cowork", + description="Enable Microsoft 365 Copilot Cowork skills deployment via OneDrive.", + default=False, + hint=( + "Use '--target copilot-cowork --global' to deploy skills. " + "See https://microsoft.github.io/apm/integrations/copilot-cowork/" + ), + ), } diff --git a/src/apm_cli/core/target_detection.py b/src/apm_cli/core/target_detection.py index 7f1b12cd4..9b0878816 100644 --- a/src/apm_cli/core/target_detection.py +++ b/src/apm_cli/core/target_detection.py @@ -196,6 +196,12 @@ def get_target_description(target: UserTargetType) -> str: #: "minimal" is intentionally excluded -- it is a fallback pseudo-target. ALL_CANONICAL_TARGETS = frozenset({"vscode", "claude", "cursor", "opencode", "codex", "gemini"}) +#: Targets that the parser must accept but that are gated at runtime by +#: ``is_enabled()`` in ``core/experimental.py`` and ``_flag_gated()`` in +#: ``integration/targets.py``. They are NOT included in the +#: ``parse_target_arg("all")`` expansion -- explicit opt-in only. +EXPERIMENTAL_TARGETS: frozenset[str] = frozenset({"copilot-cowork"}) + #: Alias mapping: user-facing name -> canonical internal name. TARGET_ALIASES: dict[str, str] = { "copilot": "vscode", @@ -251,7 +257,7 @@ def normalize_target_list( #: All values accepted by the ``--target`` CLI option. #: Derived from canonical targets, alias keys, and the ``"all"`` keyword. VALID_TARGET_VALUES: frozenset[str] = ( - ALL_CANONICAL_TARGETS | frozenset(TARGET_ALIASES) | frozenset({"all"}) + ALL_CANONICAL_TARGETS | EXPERIMENTAL_TARGETS | frozenset(TARGET_ALIASES) | frozenset({"all"}) ) diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index 012f08df2..41f262b9b 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -133,3 +133,8 @@ class InstallContext: old_local_deployed: List[str] = field(default_factory=list) # pipeline setup local_deployed_files: List[str] = field(default_factory=list) # integrate (root) local_content_errors_before: int = 0 # integrate (pre-root) + + # ------------------------------------------------------------------ + # Cowork integration state + # ------------------------------------------------------------------ + cowork_nonsupported_warned: bool = False # integrate (once-per-run guard) diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index efadd2d61..c5e8e921a 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -227,6 +227,7 @@ def _integrate_root_project( diagnostics=diagnostics, logger=logger, scope=ctx.scope, + ctx=ctx, ) # Track deployed files for the post-deps-local phase (stale @@ -270,6 +271,72 @@ def _integrate_root_project( return None +# ====================================================================== +# Cowork cap checks (Amendment 7) +# ====================================================================== + +_COWORK_MAX_SKILLS: int = 50 +"""Warn when the cowork skills directory contains more than this many skills.""" + +_COWORK_MAX_SKILL_SIZE: int = 1_048_576 # 1 MB +"""Warn when any source SKILL.md exceeds this size in bytes.""" + + +def _check_cowork_caps(ctx: "InstallContext") -> None: + """Emit warn-only diagnostics for cowork skill count and size caps. + + Walks ``/skills/*/SKILL.md`` (existing + just-installed) + and checks against ``_COWORK_MAX_SKILLS`` and ``_COWORK_MAX_SKILL_SIZE``. + Install still succeeds regardless. + """ + if not ctx.targets: + return + + cowork_root = None + for t in ctx.targets: + if t.name == "copilot-cowork" and t.resolved_deploy_root is not None: + cowork_root = t.resolved_deploy_root + break + if cowork_root is None: + return + if not cowork_root.is_dir(): + return + + skill_dirs = sorted( + d for d in cowork_root.iterdir() + if d.is_dir() and (d / "SKILL.md").exists() + ) + + # --- count cap --- + if len(skill_dirs) > _COWORK_MAX_SKILLS: + msg = ( + f"Cowork skills directory contains {len(skill_dirs)} skills " + f"(cap: {_COWORK_MAX_SKILLS}). Consider removing unused skills." + ) + if ctx.logger: + ctx.logger.warning(msg, symbol="warning") + if ctx.diagnostics: + ctx.diagnostics.warn(msg, package="cowork") + + # --- per-file size cap --- + for skill_dir in skill_dirs: + skill_md = skill_dir / "SKILL.md" + try: + size = skill_md.stat().st_size + except OSError: + continue + if size > _COWORK_MAX_SKILL_SIZE: + size_mb = size / (1024 * 1024) + msg = ( + f"Skill '{skill_dir.name}/SKILL.md' is {size_mb:.1f} MB " + f"(cap: 1 MB). Large skills may degrade Copilot performance." + ) + if ctx.logger: + ctx.logger.warning(msg, symbol="warning") + if ctx.diagnostics: + ctx.diagnostics.warn(msg, package="cowork") + + # ====================================================================== # Public phase entry point # ====================================================================== @@ -435,3 +502,10 @@ def run(ctx: "InstallContext") -> None: ctx.total_commands_integrated = total_commands_integrated ctx.total_hooks_integrated = total_hooks_integrated ctx.total_links_resolved = total_links_resolved + + # ------------------------------------------------------------------ + # Amendment 7: cowork 50-skill / 1 MB cap check (warn-only). + # Runs once per install, after all packages integrate, only when + # a cowork target with a resolved_deploy_root is active. + # ------------------------------------------------------------------ + _check_cowork_caps(ctx) diff --git a/src/apm_cli/install/phases/targets.py b/src/apm_cli/install/phases/targets.py index 8283e8731..44f76f5ae 100644 --- a/src/apm_cli/install/phases/targets.py +++ b/src/apm_cli/install/phases/targets.py @@ -30,6 +30,7 @@ def run(ctx: "InstallContext") -> None: from apm_cli.integration.hook_integrator import HookIntegrator from apm_cli.integration.instruction_integrator import InstructionIntegrator from apm_cli.integration.skill_integrator import SkillIntegrator + from apm_cli.integration.copilot_cowork_paths import CoworkResolutionError from apm_cli.integration.targets import resolve_targets as _resolve_targets # Get config target from apm.yml if available @@ -42,11 +43,77 @@ def run(ctx: "InstallContext") -> None: # the user's choice wins. Otherwise auto-detect from existing dirs, # falling back to copilot when nothing is found. _is_user = ctx.scope is InstallScope.USER - _targets = _resolve_targets( - ctx.project_root, - user_scope=_is_user, - explicit_target=_explicit, - ) + try: + _targets = _resolve_targets( + ctx.project_root, + user_scope=_is_user, + explicit_target=_explicit, + ) + except CoworkResolutionError as exc: + if ctx.logger: + ctx.logger.error(str(exc), symbol="cross") + raise SystemExit(1) from exc + + # ------------------------------------------------------------------ + # Fix 2: explicit --target copilot-cowork with flag OFF must error. + # Fix 3: explicit --target copilot-cowork with flag ON but unresolvable + # OneDrive must error. + # Only fire when the user explicitly asked for cowork. Auto-detect + # silently omits cowork when unavailable. + # ------------------------------------------------------------------ + _user_asked_cowork = False + if _explicit: + if isinstance(_explicit, list): + _user_asked_cowork = "copilot-cowork" in _explicit + else: + _user_asked_cowork = _explicit == "copilot-cowork" + + if _user_asked_cowork: + _cowork_resolved = any(t.name == "copilot-cowork" for t in _targets) + if not _cowork_resolved: + from apm_cli.core.experimental import is_enabled as _is_flag_on + + if not _is_flag_on("copilot_cowork"): + # Flag is OFF — no-op with a targeted enable hint. + if ctx.logger: + ctx.logger.progress( + "The 'copilot-cowork' target requires an experimental flag. " + "Run: apm experimental enable copilot-cowork", + symbol="info", + ) + else: + # Fix 3: flag is ON but resolver returned None + import sys as _sys + if _sys.platform.startswith("linux"): + _cowork_msg = ( + "Cowork has no auto-detection on Linux.\n" + "Set APM_COPILOT_COWORK_SKILLS_DIR or run: " + "apm config set copilot-cowork-skills-dir " + ) + else: + _cowork_msg = ( + "Cowork: no OneDrive path detected.\n" + "Set APM_COPILOT_COWORK_SKILLS_DIR or run: " + "apm config set copilot-cowork-skills-dir " + ) + if ctx.logger: + ctx.logger.error(_cowork_msg, symbol="cross") + raise SystemExit(1) + + # ------------------------------------------------------------------ + # Amendment 5: project-scope gate for cowork target. + # `--target copilot-cowork` without `--global` is an error -- cowork is + # user-scope only. Abort before any filesystem activity. + # ------------------------------------------------------------------ + if not _is_user: + _cowork_in_set = any(t.name == "copilot-cowork" for t in _targets) + if _cowork_in_set: + if ctx.logger: + ctx.logger.error( + "The 'copilot-cowork' target requires --global (user scope). " + "Run: apm install --target copilot-cowork --global" + ) + raise SystemExit(1) # Log target detection results if ctx.logger and _targets: @@ -72,6 +139,11 @@ def run(ctx: "InstallContext") -> None: # claude` would silently no-op when .claude/ doesn't exist (#763). if not _t.auto_create and not _explicit: continue + # Dynamic-root targets (cowork): the integrator creates the + # directory lazily via resolved_deploy_root. Do not attempt to + # create project_root / root_dir (the placeholder "copilot-cowork" dir). + if _t.resolved_deploy_root is not None: + continue _root = _t.root_dir _target_dir = ctx.project_root / _root if not _target_dir.exists(): diff --git a/src/apm_cli/install/services.py b/src/apm_cli/install/services.py index 31ac7bfc7..b03466e1e 100644 --- a/src/apm_cli/install/services.py +++ b/src/apm_cli/install/services.py @@ -25,6 +25,7 @@ if TYPE_CHECKING: from ..core.command_logger import InstallLogger from ..core.scope import InstallScope + from ..install.context import InstallContext from ..utils.diagnostics import DiagnosticCollector @@ -37,6 +38,41 @@ dict = builtins.dict +def _deployed_path_entry( + target_path: Path, + project_root: Path, + targets: Any, +) -> str: + """Return the lockfile-safe path string for a deployed file. + + For standard targets the entry is ``project_root``-relative. For + cowork (dynamic-root) targets the entry uses the synthetic + ``cowork://`` URI scheme so the lockfile pipeline does not attempt + a ``Path.relative_to(project_root)`` that would crash. + + Raises + ------ + RuntimeError + If the path is outside the project tree and cannot be + translated to a ``cowork://`` URI via any available target. + """ + try: + return target_path.relative_to(project_root).as_posix() + except ValueError: + # Path is outside the project tree -- must be a dynamic-root + # target. Find the matching target and translate. + if targets: + for _t in targets: + if _t.resolved_deploy_root is not None: + from apm_cli.integration.copilot_cowork_paths import to_lockfile_path + return to_lockfile_path(target_path, _t.resolved_deploy_root) + raise RuntimeError( + f"Cannot translate {target_path!r} to a lockfile path: " + f"path is outside the project tree and no dynamic-root " + f"target matched. This is a bug — please report it." + ) + + def integrate_package_primitives( package_info: Any, project_root: Path, @@ -55,6 +91,7 @@ def integrate_package_primitives( logger: Optional["InstallLogger"] = None, scope: Optional["InstallScope"] = None, skill_subset: "Optional[tuple]" = None, + ctx: Optional["InstallContext"] = None, ) -> dict: """Run the full integration pipeline for a single package. @@ -66,6 +103,10 @@ def integrate_package_primitives( When *scope* is ``InstallScope.USER``, targets and primitives that do not support user-scope deployment are silently skipped. + When *ctx* is provided, the cowork non-skill primitive warning + (Amendment 6) is emitted once per install run for packages that + contain non-skill primitives when the cowork target is active. + Returns a dict with integration counters and the list of deployed file paths. """ from apm_cli.integration.dispatch import get_dispatch_table @@ -88,6 +129,37 @@ def integrate_package_primitives( if not targets: return result + # --- Amendment 6: cowork non-skill primitive warning (once per run) --- + _cowork_active = any(t.name == "copilot-cowork" for t in targets) + if _cowork_active and ctx is not None and not ctx.cowork_nonsupported_warned: + _apm_dir = Path(package_info.install_path) / ".apm" + _NON_SKILL_DIRS = { + "agents": "agents", + "prompts": "prompts", + "instructions": "instructions", + "hooks": "hooks", + # Commands live under ``.apm/prompts/`` and cannot be + # distinguished from general prompts at directory level + # without inspecting frontmatter. Omitted to avoid + # misleading duplicate warnings. + } + _found_types = [ + ptype for ptype, subdir in _NON_SKILL_DIRS.items() + if (_apm_dir / subdir).is_dir() and any((_apm_dir / subdir).iterdir()) + ] + if _found_types: + _pkg_label = package_name or getattr(package_info, "name", "unknown") + _types_str = ", ".join(sorted(builtins.set(_found_types))) + _warn_msg = ( + f"copilot-cowork target only supports skills; " + f"non-skill primitives in {_pkg_label} " + f"({_types_str}) will not deploy to cowork" + ) + if logger: + logger.warning(_warn_msg, symbol="warning") + diagnostics.warn(_warn_msg) + ctx.cowork_nonsupported_warned = True + def _log_integration(msg): if logger: logger.tree_item(msg) @@ -137,7 +209,7 @@ def _log_integration(msg): ) result["links_resolved"] += _int_result.links_resolved for tp in _int_result.target_paths: - deployed.append(tp.relative_to(project_root).as_posix()) + deployed.append(_deployed_path_entry(tp, project_root, targets)) skill_result = skill_integrator.integrate_package_skill( package_info, project_root, @@ -146,9 +218,13 @@ def _log_integration(msg): ) _skill_target_dirs: set = builtins.set() for tp in skill_result.target_paths: - rel = tp.relative_to(project_root) - if rel.parts: - _skill_target_dirs.add(rel.parts[0]) + try: + rel = tp.relative_to(project_root) + if rel.parts: + _skill_target_dirs.add(rel.parts[0]) + except ValueError: + # Dynamic-root target (copilot-cowork) -- path is outside project tree. + _skill_target_dirs.add("copilot-cowork") _skill_targets = sorted(_skill_target_dirs) _skill_target_str = ", ".join(f"{d}/skills/" for d in _skill_targets) or "skills/" if skill_result.skill_created: @@ -158,7 +234,7 @@ def _log_integration(msg): result["sub_skills"] += skill_result.sub_skills_promoted _log_integration(f" |-- {skill_result.sub_skills_promoted} skill(s) integrated -> {_skill_target_str}") for tp in skill_result.target_paths: - deployed.append(tp.relative_to(project_root).as_posix()) + deployed.append(_deployed_path_entry(tp, project_root, targets)) return result @@ -178,6 +254,7 @@ def integrate_local_content( diagnostics: "DiagnosticCollector", logger: Optional["InstallLogger"] = None, scope: Optional["InstallScope"] = None, + ctx: Optional["InstallContext"] = None, ) -> dict: """Integrate primitives from the project's own .apm/ directory. @@ -222,6 +299,7 @@ def integrate_local_content( package_name="_local", logger=logger, scope=scope, + ctx=ctx, ) diff --git a/src/apm_cli/install/template.py b/src/apm_cli/install/template.py index b5903a804..ccffbcd90 100644 --- a/src/apm_cli/install/template.py +++ b/src/apm_cli/install/template.py @@ -98,6 +98,7 @@ def _integrate_materialization( tuple(dep_ref.skill_subset) if dep_ref.skill_subset else None ) ), + ctx=ctx, ) for k in ( "prompts", "agents", "skills", "sub_skills", diff --git a/src/apm_cli/integration/base_integrator.py b/src/apm_cli/integration/base_integrator.py index 1fc42f91e..7a5316f8a 100644 --- a/src/apm_cli/integration/base_integrator.py +++ b/src/apm_cli/integration/base_integrator.py @@ -132,12 +132,35 @@ def validate_deploy_path( Checks: 1. No path-traversal components (``..``) 2. Starts with an allowed integration prefix - 3. Resolves within *project_root* + 3. Resolves within *project_root* (or within the cowork root + for ``cowork://`` paths) """ + from apm_cli.integration.copilot_cowork_paths import COWORK_URI_SCHEME + if allowed_prefixes is None: allowed_prefixes = BaseIntegrator._get_integration_prefixes(targets=targets) if ".." in rel_path: return False + + # --- cowork:// paths: validate against cowork root --- + if rel_path.startswith(COWORK_URI_SCHEME): + if not rel_path.startswith(allowed_prefixes): + return False + # Resolve to absolute and validate containment against cowork root. + try: + from apm_cli.integration.copilot_cowork_paths import ( + from_lockfile_path, + resolve_copilot_cowork_skills_dir, + ) + cowork_root = resolve_copilot_cowork_skills_dir() + if cowork_root is None: + return False + # from_lockfile_path internally calls ensure_path_within. + from_lockfile_path(rel_path, cowork_root) + return True + except Exception: + return False + if not rel_path.startswith(allowed_prefixes): return False target = project_root / rel_path @@ -208,6 +231,12 @@ def partition_managed_files( for target in source: for prim_name, mapping in target.primitives.items(): + # Dynamic-root targets (cowork) use cowork:// URI prefix. + if target.resolved_deploy_root is not None: + if prim_name == "skills": + from apm_cli.integration.copilot_cowork_paths import COWORK_LOCKFILE_PREFIX + skill_prefixes.append(COWORK_LOCKFILE_PREFIX) + continue effective_root = mapping.deploy_root or target.root_dir prefix = f"{effective_root}/{mapping.subdir}/" if mapping.subdir else f"{effective_root}/" if prim_name == "skills": @@ -357,6 +386,7 @@ def sync_remove_files( legacy_glob_dir: Optional[Path] = None, legacy_glob_pattern: Optional[str] = None, targets=None, + logger=None, ) -> Dict[str, int]: """Remove APM-managed files matching *prefix* from *managed_files*. @@ -373,6 +403,7 @@ def sync_remove_files( targets: Optional target profiles for path validation. Passed through to ``validate_deploy_path()`` so user-scope prefixes are recognised. + logger: Optional logger for diagnostic messages. Returns: ``{"files_removed": int, "errors": int}`` @@ -380,19 +411,59 @@ def sync_remove_files( stats: Dict[str, int] = {"files_removed": 0, "errors": 0} if managed_files is not None: + # Lazy-resolve cowork root at most once per invocation. + _cowork_root_resolved: bool = False + _cowork_root_cached: Optional[Path] = None + _cowork_orphans_skipped: int = 0 + for rel_path in managed_files: # managed_files is pre-normalized -- no .replace() needed if not rel_path.startswith(prefix): continue if not BaseIntegrator.validate_deploy_path(rel_path, project_root, targets=targets): continue - target = project_root / rel_path + # Resolve cowork:// paths to absolute before filesystem ops. + from apm_cli.integration.copilot_cowork_paths import COWORK_URI_SCHEME + if rel_path.startswith(COWORK_URI_SCHEME): + try: + if not _cowork_root_resolved: + from apm_cli.integration.copilot_cowork_paths import ( + resolve_copilot_cowork_skills_dir, + ) + _cowork_root_cached = resolve_copilot_cowork_skills_dir() + _cowork_root_resolved = True + if _cowork_root_cached is None: + _cowork_orphans_skipped += 1 + continue + from apm_cli.integration.copilot_cowork_paths import ( + from_lockfile_path, + ) + target = from_lockfile_path(rel_path, _cowork_root_cached) + except Exception: + continue + else: + target = project_root / rel_path if target.exists(): try: target.unlink() stats["files_removed"] += 1 except Exception: stats["errors"] += 1 + + # Emit a one-time warning when cowork orphans were skipped. + if _cowork_orphans_skipped > 0: + _orphan_msg = ( + f"Cowork: skipping {_cowork_orphans_skipped} orphaned lockfile " + f"{'entry' if _cowork_orphans_skipped == 1 else 'entries'}" + " -- OneDrive path not detected.\n" + "Run: apm config set copilot-cowork-skills-dir " + "(or set APM_COPILOT_COWORK_SKILLS_DIR)\n" + "to clean up these entries on the next install/uninstall." + ) + if logger: + logger.warning(_orphan_msg, symbol="warning") + else: + _rich_warning(_orphan_msg, symbol="warning") elif legacy_glob_dir and legacy_glob_pattern and legacy_glob_dir.exists(): for f in legacy_glob_dir.glob(legacy_glob_pattern): try: diff --git a/src/apm_cli/integration/cleanup.py b/src/apm_cli/integration/cleanup.py index 7811b02df..93a34a574 100644 --- a/src/apm_cli/integration/cleanup.py +++ b/src/apm_cli/integration/cleanup.py @@ -115,15 +115,68 @@ def remove_stale_deployed_files( result = CleanupResult() recorded_hashes = recorded_hashes or {} + # Lazy-resolve cowork root at most once per invocation (same + # pattern as sync_remove_files in base_integrator.py -- PR #926 P4). + _cowork_root_resolved: bool = False + _cowork_root_cached: Optional[Path] = None + _cowork_orphans_skipped: int = 0 + _cowork_resolve_errors: int = 0 + for stale_path in sorted(stale_paths): - # Gate 1: path validation (traversal, allowed prefix, in-tree). - if not BaseIntegrator.validate_deploy_path( - stale_path, project_root, targets=targets - ): - result.skipped_unmanaged.append(stale_path) - continue + # ── Cowork:// paths ────────────────────────────────────────── + # Handled BEFORE validate_deploy_path because that method + # hard-rejects cowork:// when the OneDrive root is unavailable + # (returning False ⇒ skipped_unmanaged). For cleanup we want + # the gentler "retain in *failed* for retry" behaviour, so we + # do equivalent security checks (no traversal, known prefix, + # containment via from_lockfile_path) ourselves. + from .copilot_cowork_paths import COWORK_URI_SCHEME + if stale_path.startswith(COWORK_URI_SCHEME): + # Basic security: reject path-traversal components. + if ".." in stale_path: + result.skipped_unmanaged.append(stale_path) + continue + # Verify the path starts with a known integration prefix. + from .targets import get_integration_prefixes + if not stale_path.startswith( + get_integration_prefixes(targets=targets) + ): + result.skipped_unmanaged.append(stale_path) + continue + # Resolve the cowork:// URI to a real filesystem path. + try: + if not _cowork_root_resolved: + from .copilot_cowork_paths import ( + resolve_copilot_cowork_skills_dir, + ) + _cowork_root_cached = resolve_copilot_cowork_skills_dir() + _cowork_root_resolved = True + if _cowork_root_cached is None: + # OneDrive unavailable -- retain lockfile entry so a + # later install with a configured root can clean up. + _cowork_orphans_skipped += 1 + result.failed.append(stale_path) + continue + from .copilot_cowork_paths import from_lockfile_path + stale_target = from_lockfile_path( + stale_path, _cowork_root_cached + ) + except Exception: + # Containment violation or malformed path -- retain in + # lockfile for manual inspection. + _cowork_resolve_errors += 1 + result.failed.append(stale_path) + continue + else: + # ── Non-cowork paths ───────────────────────────────────── + # Gate 1: path validation (traversal, allowed prefix, in-tree). + if not BaseIntegrator.validate_deploy_path( + stale_path, project_root, targets=targets + ): + result.skipped_unmanaged.append(stale_path) + continue + stale_target = project_root / stale_path - stale_target = project_root / stale_path if not stale_target.exists(): # File already gone -- treat as cleaned (no-op success). continue @@ -208,4 +261,28 @@ def remove_stale_deployed_files( package=dep_key, ) + # One-time warnings for cowork edge cases (mirrors sync_remove_files). + if _cowork_orphans_skipped > 0: + diagnostics.warn( + ( + f"Cowork: skipping {_cowork_orphans_skipped} stale lockfile " + f"{'entry' if _cowork_orphans_skipped == 1 else 'entries'}" + " -- OneDrive path not detected.\n" + "Run: apm config set copilot-cowork-skills-dir " + "(or set APM_COPILOT_COWORK_SKILLS_DIR)\n" + "to clean up these entries on the next install/uninstall." + ), + package=dep_key, + ) + if _cowork_resolve_errors > 0: + diagnostics.warn( + ( + f"Cowork: {_cowork_resolve_errors} lockfile " + f"{'entry' if _cowork_resolve_errors == 1 else 'entries'}" + " failed path resolution (containment violation or " + "malformed path). Paths retained for manual inspection." + ), + package=dep_key, + ) + return result diff --git a/src/apm_cli/integration/copilot_cowork_paths.py b/src/apm_cli/integration/copilot_cowork_paths.py new file mode 100644 index 000000000..8d2af8b94 --- /dev/null +++ b/src/apm_cli/integration/copilot_cowork_paths.py @@ -0,0 +1,241 @@ +"""OneDrive-backed Cowork skills directory resolution and lockfile path translation. + +Cowork skills are deployed to the user's OneDrive ``Documents/Cowork/skills/`` +directory so that Microsoft 365 Copilot can discover them. This module owns: + +1. **Resolution** -- locating the OneDrive mount point on macOS and Windows, + and mapping it to ``/Documents/Cowork/skills/``. The + ``APM_COPILOT_COWORK_SKILLS_DIR`` environment variable overrides automatic detection + for CI or non-standard layouts. + +2. **Lockfile translation** -- APM's lockfile pipeline expects paths relative + to ``project_root``. Cowork paths are absolute (outside the project tree), + so we encode them as ``cowork://skills/`` in the lockfile and + translate back to absolute paths at filesystem-I/O boundaries. + +All filesystem input passes through ``validate_path_segments`` and +``ensure_path_within`` from ``apm_cli.utils.path_security``. No ad-hoc +traversal checks. + +Design note +----------- +This module is pure-stdlib and does **not** import any third-party library. +It is always importable but functionally inert until the ``cowork`` +experimental flag is enabled by the caller. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +COWORK_URI_SCHEME: str = "cowork://" +"""Synthetic URI prefix used in lockfile entries for cowork deployments.""" + +COWORK_LOCKFILE_PREFIX: str = "cowork://skills/" +"""Full prefix for skill entries in the lockfile (scheme + skills segment).""" + +_ONEDRIVE_GLOB: str = "OneDrive*" +"""Glob pattern for OneDrive mount directories under ``~/Library/CloudStorage/``. + +macOS creates directories named ``OneDrive - TenantName`` (with spaces), so +the glob must NOT restrict to ``OneDrive-*``. +""" + +_COWORK_SUBDIR: str = "Documents/Cowork" +"""Relative path from the OneDrive mount root to the Cowork directory.""" + +_COPILOT_COWORK_SKILLS_SUBDIR: str = "Documents/Cowork/skills" +"""Relative path from the OneDrive mount root to the skills directory.""" + + +# --------------------------------------------------------------------------- +# Exceptions +# --------------------------------------------------------------------------- + +class CoworkResolutionError(Exception): + """Raised when OneDrive resolution fails with an actionable diagnostic. + + Callers should format the ``str(err)`` via ``CommandLogger.error()`` + so the user sees the message with an ``[x]`` symbol. + """ + + +# --------------------------------------------------------------------------- +# Resolution +# --------------------------------------------------------------------------- + +def resolve_copilot_cowork_skills_dir() -> Path | None: + """Locate the Cowork skills directory on the current machine. + + Resolution order: + + 1. ``APM_COPILOT_COWORK_SKILLS_DIR`` environment variable (highest priority). + 2. ``copilot_cowork_skills_dir`` from ``~/.apm/config.json`` (via ``apm config``). + 3. Platform auto-detection: + - macOS: ``~/Library/CloudStorage/OneDrive*/``. + - Windows: ``%ONEDRIVECOMMERCIAL%``, then ``%ONEDRIVE%``. + - Linux: no default lookup (returns ``None``). + + Returns ``None`` when no OneDrive mount is found (target unavailable). + + Raises: + CoworkResolutionError: When multiple OneDrive tenants are detected + on macOS and ``APM_COPILOT_COWORK_SKILLS_DIR`` is not set. The exception + message lists the candidates and instructs the user to set the + env var. + """ + # --- env-var override --- + env_override = os.environ.get("APM_COPILOT_COWORK_SKILLS_DIR") + if env_override: + from apm_cli.utils.path_security import ( + PathTraversalError, + validate_path_segments, + ) + try: + validate_path_segments(env_override, context="APM_COPILOT_COWORK_SKILLS_DIR") + except PathTraversalError as exc: + raise CoworkResolutionError( + f"APM_COPILOT_COWORK_SKILLS_DIR contains a traversal sequence: {exc}" + ) from exc + return Path(env_override).expanduser().resolve() + + # --- persisted config value --- + from apm_cli.config import get_copilot_cowork_skills_dir + + config_value = get_copilot_cowork_skills_dir() + if config_value: + from apm_cli.utils.path_security import ( + PathTraversalError, + validate_path_segments, + ) + try: + validate_path_segments(config_value, context="copilot_cowork_skills_dir config") + except PathTraversalError as exc: + raise CoworkResolutionError( + f"copilot_cowork_skills_dir config contains a traversal sequence: {exc}" + ) from exc + return Path(config_value).expanduser().resolve() + + # --- Windows auto-detection --- + if sys.platform == "win32": + from apm_cli.utils.path_security import ( + PathTraversalError, + validate_path_segments, + ) + + for _env_name in ("ONEDRIVECOMMERCIAL", "ONEDRIVE"): + _win_root = os.environ.get(_env_name, "") + if _win_root: + _win_skills = Path(_win_root) / _COPILOT_COWORK_SKILLS_SUBDIR + try: + validate_path_segments( + str(_win_skills), context=f"{_env_name} env var" + ) + except PathTraversalError as exc: + raise CoworkResolutionError( + f"{_env_name} contains a traversal sequence: {exc}" + ) from exc + return _win_skills.resolve() + return None + + # --- macOS auto-detection --- + cloud_storage = Path.home() / "Library" / "CloudStorage" + if not cloud_storage.is_dir(): + return None + + candidates = sorted(cloud_storage.glob(_ONEDRIVE_GLOB)) + if not candidates: + return None + + if len(candidates) > 1: + listing = "\n".join(f" - {c}" for c in candidates) + raise CoworkResolutionError( + f"Multiple OneDrive mounts detected:\n{listing}\n" + f"Set APM_COPILOT_COWORK_SKILLS_DIR to the desired skills directory, e.g.:\n" + f" export APM_COPILOT_COWORK_SKILLS_DIR=" + f'"{candidates[0] / _COPILOT_COWORK_SKILLS_SUBDIR}"' + ) + + return candidates[0] / _COPILOT_COWORK_SKILLS_SUBDIR + + +# --------------------------------------------------------------------------- +# Lockfile translation +# --------------------------------------------------------------------------- + +def to_lockfile_path(absolute: Path, cowork_root: Path) -> str: + """Encode an absolute cowork path as a ``cowork://`` lockfile entry. + + Args: + absolute: Absolute path to a deployed file or directory inside + the cowork skills tree. + cowork_root: The resolved cowork skills root (from + ``resolve_copilot_cowork_skills_dir()``). + + Returns: + A string like ``cowork://skills/my-skill/SKILL.md``. + + Raises: + ``PathTraversalError`` if *absolute* escapes *cowork_root*. + """ + from apm_cli.utils.path_security import ensure_path_within + + # Validate containment -- raises PathTraversalError on violation. + resolved = ensure_path_within(absolute, cowork_root) + rel = resolved.relative_to(cowork_root.resolve()) + return f"{COWORK_URI_SCHEME}skills/{rel.as_posix()}" + + +def from_lockfile_path(lockfile_path: str, cowork_root: Path) -> Path: + """Decode a ``cowork://`` lockfile entry to an absolute ``Path``. + + Args: + lockfile_path: A string like ``cowork://skills/my-skill/SKILL.md``. + cowork_root: The resolved cowork skills root. + + Returns: + Absolute ``Path`` under *cowork_root*. + + Raises: + ``PathTraversalError`` if the decoded path escapes *cowork_root*. + ``ValueError`` if *lockfile_path* does not start with the cowork + URI scheme. + """ + from apm_cli.utils.path_security import ( + ensure_path_within, + validate_path_segments, + ) + + if not lockfile_path.startswith(COWORK_URI_SCHEME): + raise ValueError( + f"Not a cowork lockfile path: {lockfile_path!r}" + ) + + # Strip scheme to get the relative portion (e.g. "skills/my-skill/SKILL.md"). + rel_posix = lockfile_path[len(COWORK_URI_SCHEME):] + + # Pre-parse traversal rejection. + validate_path_segments(rel_posix, context="cowork lockfile path") + + # The lockfile stores "skills//..." but cowork_root already + # points to the skills directory, so we must strip the leading + # "skills/" segment to avoid double-nesting. + _skills_prefix = "skills/" + if rel_posix.startswith(_skills_prefix): + rel_posix = rel_posix[len(_skills_prefix):] + + candidate = cowork_root / rel_posix + # Re-validate containment after path construction. + return ensure_path_within(candidate, cowork_root) + + +def is_cowork_path(lockfile_path: str) -> bool: + """Return ``True`` if *lockfile_path* uses the ``cowork://`` scheme.""" + return lockfile_path.startswith(COWORK_URI_SCHEME) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index 3e829a5c0..aa9fed76e 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -1,7 +1,7 @@ """Skill integration functionality for APM packages (Claude Code & Cursor support).""" from pathlib import Path -from typing import List, Dict +from typing import Dict, List, Optional from dataclasses import dataclass from datetime import datetime import filecmp @@ -504,6 +504,8 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n try: rel_prefix = target_skills_root.relative_to(project_root).as_posix() except ValueError: + # Dynamic-root targets (cowork): use synthetic prefix + # when the skills root lives outside the project tree. rel_prefix = target_skills_root.name else: rel_prefix = target_skills_root.name @@ -676,8 +678,12 @@ def _promote_sub_skills_standalone( is_primary = (idx == 0) # first active target owns diagnostics skills_mapping = target.primitives["skills"] - effective_root = skills_mapping.deploy_root or target.root_dir - target_skills_root = project_root / effective_root / "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.mkdir(parents=True, exist_ok=True) n, deployed = self._promote_sub_skills( @@ -784,8 +790,12 @@ def _integrate_native_skill( is_primary = (idx == 0) # first active target owns diagnostics skills_mapping = target.primitives["skills"] - effective_root = skills_mapping.deploy_root or target.root_dir - target_skill_dir = project_root / effective_root / "skills" / skill_name + # 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 if is_primary: skill_created = not target_skill_dir.exists() @@ -806,6 +816,8 @@ def _integrate_native_skill( try: rel_prefix = target_skill_dir.parent.relative_to(project_root).as_posix() except ValueError: + # Dynamic-root targets (cowork): directory is + # outside the project tree. rel_prefix = "skills" rel_path = f"{rel_prefix}/{skill_name}" # Issue 1: package= should identify the package causing the @@ -850,7 +862,10 @@ def _ignore_symlinks_and_apm(directory, contents): files_copied = sum(1 for _ in target_skill_dir.rglob('*') if _.is_file()) # Promote sub-skills for this target - target_skills_root = project_root / effective_root / "skills" + 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, skill_name, warn=is_primary, @@ -1099,6 +1114,12 @@ def sync_integration(self, apm_package, project_root: Path, for t in source: if not t.supports("skills"): continue + # Dynamic-root targets (cowork) use cowork:// URI prefix. + if t.user_root_resolver is not None: + from apm_cli.integration.copilot_cowork_paths import COWORK_LOCKFILE_PREFIX + if COWORK_LOCKFILE_PREFIX not in skill_prefixes: + skill_prefixes.append(COWORK_LOCKFILE_PREFIX) + continue sm = t.primitives["skills"] effective_root = sm.deploy_root or t.root_dir skill_prefixes.append(f"{effective_root}/skills/") @@ -1107,20 +1128,68 @@ def sync_integration(self, apm_package, project_root: Path, if managed_files is not None: # Manifest-based removal -- only remove tracked skill directories project_root_resolved = project_root.resolve() + + # Lazy-resolve cowork root at most once per invocation + # (mirrors the pattern in cleanup.py and sync_remove_files). + _cowork_root_resolved: bool = False + _cowork_root_cached: Optional[Path] = None + _cowork_skipped: int = 0 + for rel_path in managed_files: if not rel_path.startswith(skill_prefix_tuple): continue if ".." in rel_path: continue - target = project_root / rel_path - if not str(target.resolve()).startswith(str(project_root_resolved)): - continue - if target.exists() and target.is_dir(): + + # ── Cowork:// paths ────────────────────────────────── + from apm_cli.integration.copilot_cowork_paths import COWORK_URI_SCHEME + if rel_path.startswith(COWORK_URI_SCHEME): try: - shutil.rmtree(target) - stats['files_removed'] += 1 + if not _cowork_root_resolved: + from apm_cli.integration.copilot_cowork_paths import ( + resolve_copilot_cowork_skills_dir, + ) + _cowork_root_cached = resolve_copilot_cowork_skills_dir() + _cowork_root_resolved = True + if _cowork_root_cached is None: + _cowork_skipped += 1 + continue + from apm_cli.integration.copilot_cowork_paths import from_lockfile_path + target = from_lockfile_path(rel_path, _cowork_root_cached) except Exception: stats['errors'] += 1 + continue + else: + target = project_root / rel_path + if not str(target.resolve()).startswith(str(project_root_resolved)): + continue + + if not target.exists(): + continue + + try: + if target.is_dir(): + shutil.rmtree(target) + else: + target.unlink() + stats['files_removed'] += 1 + except Exception: + stats['errors'] += 1 + + # One-time warning when cowork entries were skipped + # because the OneDrive path is unavailable. + if _cowork_skipped > 0: + from apm_cli.utils.console import _rich_warning + _rich_warning( + f"Cowork: skipping {_cowork_skipped} skill " + f"{'entry' if _cowork_skipped == 1 else 'entries'}" + " -- OneDrive path not detected.\n" + "Run: apm config set copilot-cowork-skills-dir " + "(or set APM_COPILOT_COWORK_SKILLS_DIR)\n" + "to clean up these entries on the next install/uninstall.", + symbol="warning", + ) + return stats # Legacy fallback: npm-style orphan detection diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 554b6c59a..6e70385fd 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -8,7 +8,7 @@ from __future__ import annotations from dataclasses import dataclass, field -from typing import Dict, List, Optional, Tuple, Union +from typing import Callable, Dict, List, Optional, Tuple, Union @dataclass(frozen=True) @@ -84,6 +84,33 @@ class TargetProfile: target itself is partially supported (e.g. Copilot CLI cannot deploy prompts at user scope).""" + user_root_resolver: Optional[Callable[[], Optional["Path"]]] = None + """Optional callable that resolves the deploy root at runtime. + + When set, ``for_scope(user_scope=True)`` calls this resolver instead of + using a static ``user_root_dir``. If the resolver returns ``None`` + the target is unavailable in the current environment (same semantics + as ``user_supported=False``). + + The callable must be hashable by reference (plain function or + staticmethod) so ``frozen=True`` is preserved. + """ + + resolved_deploy_root: Optional["Path"] = None + """Absolute deploy root populated by ``for_scope()`` when + ``user_root_resolver`` returns a concrete ``Path``. + + Downstream code uses ``deploy_path()`` to route filesystem I/O + through this root instead of ``project_root / root_dir``. + """ + + requires_flag: Optional[str] = None + """When set, the target is only returned by ``active_targets`` / + ``active_targets_user_scope`` / ``resolve_targets`` when the named + experimental flag is enabled. The target entry is always visible + in ``KNOWN_TARGETS`` for tooling introspection. + """ + @property def prefix(self) -> str: """Return the path prefix for this target (e.g. ``".github/"``). @@ -114,12 +141,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": + """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. + + Args: + project_root: Workspace or home directory root. + *parts: Additional path segments (e.g. ``"skills"``, ``"my-skill"``). + """ + 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 + return base.joinpath(*parts) if parts else base + def for_scope(self, user_scope: bool = False) -> "TargetProfile | None": """Return a scope-resolved copy of this profile. When *user_scope* is ``False``, returns ``self`` unchanged. When *user_scope* is ``True``: + - If ``user_root_resolver`` is set, calls it. Returns ``None`` + when the resolver returns ``None`` (target unavailable). + Otherwise returns a copy with ``resolved_deploy_root`` set and + primitives filtered for user scope. - Returns ``None`` if this target does not support user scope. - Otherwise returns a frozen copy with ``root_dir`` set to ``user_root_dir`` (or left unchanged when ``user_root_dir`` @@ -131,11 +178,30 @@ def for_scope(self, user_scope: bool = False) -> "TargetProfile | None": """ if not user_scope: return self - if not self.user_supported: - return None from dataclasses import replace + # --- dynamic-root resolver path (cowork) --- + if self.user_root_resolver is not None: + resolved_root = self.user_root_resolver() + if resolved_root is None: + return None + if self.unsupported_user_primitives: + filtered = { + k: v for k, v in self.primitives.items() + if k not in self.unsupported_user_primitives + } + else: + filtered = self.primitives + return replace( + self, + primitives=filtered, + resolved_deploy_root=resolved_root, + ) + + if not self.user_supported: + return None + new_root = self.user_root_dir or self.root_dir if self.unsupported_user_primitives: filtered = { @@ -307,9 +373,54 @@ def for_scope(self, user_scope: bool = False) -> "TargetProfile | None": auto_create=False, detect_by_dir=True, ), + # Microsoft 365 Copilot (Cowork) -- experimental, user-scope only. + # Skills are deployed to /Documents/Cowork/skills/. + # The deploy root is resolved dynamically at runtime via + # copilot_cowork_paths.resolve_copilot_cowork_skills_dir(). + # Non-skill primitives are not supported. + "copilot-cowork": TargetProfile( + name="copilot-cowork", + root_dir="copilot-cowork", # display grouping placeholder only + primitives={ + "skills": PrimitiveMapping( + "skills", "/SKILL.md", "skill_standard", + ), + }, + auto_create=False, + detect_by_dir=False, + user_supported=True, + user_root_resolver=lambda: _resolve_copilot_cowork_root(), + requires_flag="copilot_cowork", + ), } +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. + Exceptions propagate to the caller (``for_scope`` / install pipeline). + """ + from apm_cli.integration.copilot_cowork_paths import resolve_copilot_cowork_skills_dir + return resolve_copilot_cowork_skills_dir() + + +def _is_flag_enabled(flag_name: str) -> bool: + """Check whether an experimental flag is enabled. + + Lazy import to avoid config I/O at module load time. + """ + from apm_cli.core.experimental import is_enabled + return is_enabled(flag_name) + + +def _flag_gated(profile: TargetProfile) -> bool: + """Return ``True`` if *profile* passes its flag gate (or has none).""" + if profile.requires_flag is None: + return True + return _is_flag_enabled(profile.requires_flag) + + def get_integration_prefixes(targets=None) -> tuple: """Return all known target root prefixes as a tuple. @@ -327,6 +438,20 @@ def get_integration_prefixes(targets=None) -> tuple: prefixes: list[str] = [] seen: set[str] = set() for t in source: + # Dynamic-root targets (cowork) use cowork:// prefix in lockfile. + # Check the *capability* (user_root_resolver is not None) rather + # than the *run-time state* (resolved_deploy_root is not None). + # The static KNOWN_TARGETS registry always has resolved_deploy_root + # = None (the resolver fires only on per-install copies created by + # for_scope()), but cleanup code passes targets=None which falls + # back to the static registry. Using the capability flag ensures + # cowork:// entries pass prefix validation during cleanup/uninstall. + if t.user_root_resolver is not None: + from apm_cli.integration.copilot_cowork_paths import COWORK_LOCKFILE_PREFIX + if COWORK_LOCKFILE_PREFIX not in seen: + seen.add(COWORK_LOCKFILE_PREFIX) + prefixes.append(COWORK_LOCKFILE_PREFIX) + continue if t.prefix not in seen: seen.add(t.prefix) prefixes.append(t.prefix) @@ -372,10 +497,10 @@ def active_targets_user_scope( if canonical == "all": return [ p for p in KNOWN_TARGETS.values() - if p.user_supported + if p.user_supported and _flag_gated(p) ] profile = KNOWN_TARGETS.get(canonical) - if profile and profile.user_supported and profile.name not in seen: + if profile and profile.user_supported and _flag_gated(profile) and profile.name not in seen: seen.add(profile.name) profiles.append(profile) return profiles if profiles else [] @@ -387,17 +512,20 @@ def active_targets_user_scope( if canonical == "all": return [ p for p in KNOWN_TARGETS.values() - if p.user_supported + if p.user_supported and _flag_gated(p) ] profile = KNOWN_TARGETS.get(canonical) - if profile and profile.user_supported: + if profile and profile.user_supported and _flag_gated(profile): return [profile] return [] # --- auto-detect by directory presence at ~/ --- + # Targets with detect_by_dir=False (cowork) are never auto-detected. detected = [ p for p in KNOWN_TARGETS.values() - if p.user_supported and (home / p.effective_root(user_scope=True)).is_dir() + if p.user_supported and p.detect_by_dir + and _flag_gated(p) + and (home / p.effective_root(user_scope=True)).is_dir() ] if detected: return detected @@ -442,9 +570,12 @@ def active_targets( if canonical in ("copilot", "vscode", "agents"): canonical = "copilot" if canonical == "all": + # Return all targets regardless of flag gating. + # The project-scope gate in phases/targets.py and + # for_scope() handle user-observable blocking. return list(KNOWN_TARGETS.values()) profile = KNOWN_TARGETS.get(canonical) - if profile and profile.name not in seen: + if profile and _flag_gated(profile) and profile.name not in seen: seen.add(profile.name) profiles.append(profile) return profiles if profiles else [KNOWN_TARGETS["copilot"]] @@ -454,14 +585,19 @@ def active_targets( if canonical in ("copilot", "vscode", "agents"): canonical = "copilot" if canonical == "all": + # Return all targets regardless of flag gating. return list(KNOWN_TARGETS.values()) profile = KNOWN_TARGETS.get(canonical) - return [profile] if profile else [] + if profile and _flag_gated(profile): + return [profile] + return [] # --- auto-detect by directory presence --- + # Targets with detect_by_dir=False (cowork) are never auto-detected. detected = [ p for p in KNOWN_TARGETS.values() - if (root / p.root_dir).is_dir() + if p.detect_by_dir and _flag_gated(p) + and (root / p.root_dir).is_dir() ] if detected: return detected diff --git a/tests/unit/core/test_experimental.py b/tests/unit/core/test_experimental.py index 788aa018c..69aaa3763 100644 --- a/tests/unit/core/test_experimental.py +++ b/tests/unit/core/test_experimental.py @@ -504,3 +504,68 @@ def test_reset_bulk_returns_count(self, isolated_config: Any) -> None: result = reset(None) assert result == 1 assert isinstance(result, int) + + +# --------------------------------------------------------------------------- +# Cowork flag registration +# --------------------------------------------------------------------------- + + +class TestCoworkFlagRegistration: + """Tests for the 'cowork' experimental flag registration.""" + + def test_cowork_flag_is_registered(self) -> None: + from apm_cli.core.experimental import FLAGS + + assert "copilot_cowork" in FLAGS + + def test_cowork_flag_default_is_false(self) -> None: + from apm_cli.core.experimental import FLAGS + + assert FLAGS["copilot_cowork"].default is False + + def test_cowork_flag_is_disabled_by_default( + self, inject_config: Any + ) -> None: + inject_config({}) + from apm_cli.core.experimental import is_enabled + + assert is_enabled("copilot_cowork") is False + + def test_cowork_flag_can_be_enabled( + self, isolated_config: Any + ) -> None: + from apm_cli.core.experimental import enable, is_enabled + + enable("copilot_cowork") + assert is_enabled("copilot_cowork") is True + + def test_cowork_flag_hint_contains_docs_url(self) -> None: + """Verify the hint URL is a valid https URL using urlparse.""" + from urllib.parse import urlparse + from apm_cli.core.experimental import FLAGS + + hint = FLAGS["copilot_cowork"].hint + assert hint is not None + # Extract URL portion from the hint string + import re as _re + + urls = _re.findall(r"https?://\S+", hint) + assert urls, "hint must contain at least one URL" + parsed = urlparse(urls[0]) + assert parsed.scheme == "https" + assert parsed.hostname is not None + assert parsed.path != "" + + def test_cowork_flag_description_is_printable_ascii(self) -> None: + import string + from apm_cli.core.experimental import FLAGS + + desc = FLAGS["copilot_cowork"].description + assert len(desc) <= 80 + assert all(c in string.printable for c in desc) + + def test_cowork_key_equals_name(self) -> None: + from apm_cli.core.experimental import FLAGS + + assert FLAGS["copilot_cowork"].name == "copilot_cowork" diff --git a/tests/unit/core/test_scope.py b/tests/unit/core/test_scope.py index 6d5fa92ab..518db3b2b 100644 --- a/tests/unit/core/test_scope.py +++ b/tests/unit/core/test_scope.py @@ -158,7 +158,7 @@ class TestTargetProfileUserScope: """Validate user-scope metadata on TargetProfile in KNOWN_TARGETS.""" def test_all_known_targets_present(self): - expected = {"copilot", "claude", "cursor", "opencode", "codex", "gemini"} + expected = {"copilot", "claude", "cursor", "opencode", "codex", "gemini", "copilot-cowork"} assert set(KNOWN_TARGETS.keys()) == expected def test_each_target_has_user_supported(self): diff --git a/tests/unit/core/test_target_detection.py b/tests/unit/core/test_target_detection.py index f2527c07d..fcd68ac0e 100644 --- a/tests/unit/core/test_target_detection.py +++ b/tests/unit/core/test_target_detection.py @@ -1,7 +1,10 @@ """Tests for target detection module.""" from apm_cli.core.target_detection import ( + ALL_CANONICAL_TARGETS, + EXPERIMENTAL_TARGETS, detect_target, + normalize_target_list, should_compile_agents_md, should_compile_claude_md, should_compile_gemini_md, @@ -549,3 +552,105 @@ def test_only_commas_rejected(self): """Only commas (no actual values) is rejected.""" with pytest.raises(click.exceptions.BadParameter, match="must not be empty"): self.tp.convert(",,,", None, None) + + +# --------------------------------------------------------------------------- +# Cowork parser-layer regression tests (2f96dd5 / #926) +# --------------------------------------------------------------------------- + + +class TestCoworkParserLayer: + """Regression guard for the parser-level EXPERIMENTAL_TARGETS fix. + + These tests are DELIBERATELY flag-agnostic -- the parser accepts or + rejects tokens based solely on VALID_TARGET_VALUES, independent of + the experimental flag state in ~/.apm/config.json. + + Ref: commit 2f96dd5 -- fix(cli): accept cowork target at parser layer + via EXPERIMENTAL_TARGETS. + """ + + def setup_method(self): + self.tp = TargetParamType() + + # -- Case 1: single "copilot-cowork" accepted --------------------------------- + + def test_convert_cowork_single_returns_string(self): + """TargetParamType.convert('copilot-cowork') returns the string 'copilot-cowork'.""" + result = self.tp.convert("copilot-cowork", None, None) + assert result == "copilot-cowork" + assert isinstance(result, str) + + # -- Case 2: "copilot-cowork,claude" accepted as multi-target list ----------- + + def test_convert_cowork_multi_returns_list_with_both(self): + """TargetParamType.convert('copilot-cowork,claude') returns a list containing both.""" + result = self.tp.convert("copilot-cowork,claude", None, None) + assert isinstance(result, list) + assert "copilot-cowork" in result + assert "claude" in result + + def test_convert_cowork_multi_preserves_input_order(self): + """'copilot-cowork,claude' preserves the parser's natural (input) order.""" + result = self.tp.convert("copilot-cowork,claude", None, None) + assert result == ["copilot-cowork", "claude"] + + # -- Case 3: membership in VALID_TARGET_VALUES ----------------------- + + def test_cowork_in_valid_target_values(self): + """'copilot-cowork' must be accepted by the --target parser.""" + assert "copilot-cowork" in VALID_TARGET_VALUES + + # -- Case 4: NOT in ALL_CANONICAL_TARGETS (constant-split guard) ----- + + def test_cowork_not_in_all_canonical_targets(self): + """'copilot-cowork' must NOT bleed into ALL_CANONICAL_TARGETS (regression guard). + + ALL_CANONICAL_TARGETS drives the 'all' expansion at the parser layer. + Experimental targets are opt-in only and must live in EXPERIMENTAL_TARGETS. + """ + assert "copilot-cowork" not in ALL_CANONICAL_TARGETS + + # -- Case 5: in EXPERIMENTAL_TARGETS -------------------------------- + + def test_cowork_in_experimental_targets(self): + """'copilot-cowork' must appear in EXPERIMENTAL_TARGETS.""" + assert "copilot-cowork" in EXPERIMENTAL_TARGETS + + # -- Case 6: exact membership lock ----------------------------------- + + def test_experimental_targets_exact_membership(self): + """EXPERIMENTAL_TARGETS must equal frozenset({'copilot-cowork'}) exactly. + + This locks the constant so that adding a new experimental target + requires an intentional test update. + """ + assert EXPERIMENTAL_TARGETS == frozenset({"copilot-cowork"}) + + # -- Case 7: "all" expansion does NOT include "copilot-cowork" --------------- + + def test_all_expansion_excludes_cowork(self): + """parse_target_arg('all') at the parser layer must NOT include 'copilot-cowork'. + + 'all' must expand only to ALL_CANONICAL_TARGETS. Experimental + targets are explicitly excluded -- they require opt-in. + """ + # TargetParamType.convert("all") returns the string "all" for + # backward compat. The expansion to a list happens in + # normalize_target_list(); test both surfaces. + result_str = self.tp.convert("all", None, None) + assert result_str == "all" + + result_list = normalize_target_list("all") + assert isinstance(result_list, list) + assert "copilot-cowork" not in result_list + + # -- Case 8: invalid target still rejected (sanity check) ------------ + + def test_invalid_target_still_rejected(self): + """'nonsense' must still raise BadParameter after adding copilot-cowork.""" + with pytest.raises( + click.exceptions.BadParameter, + match="'nonsense' is not a valid target", + ): + self.tp.convert("nonsense", None, None) diff --git a/tests/unit/install/phases/__init__.py b/tests/unit/install/phases/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/install/phases/test_integrate_phase.py b/tests/unit/install/phases/test_integrate_phase.py new file mode 100644 index 000000000..4f65fe81a --- /dev/null +++ b/tests/unit/install/phases/test_integrate_phase.py @@ -0,0 +1,179 @@ +"""Tests for _check_cowork_caps in apm_cli.install.phases.integrate.""" +from __future__ import annotations + +from dataclasses import replace +from pathlib import Path +from typing import Any, Dict +from unittest.mock import MagicMock + +import pytest + +from apm_cli.install.phases.integrate import _check_cowork_caps +from apm_cli.integration.targets import KNOWN_TARGETS + + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def _reset_config_cache(): + """Reset the in-process config cache before and after every test.""" + from apm_cli.config import _invalidate_config_cache + + _invalidate_config_cache() + yield + _invalidate_config_cache() + + +def _make_cowork_target(cowork_root: Path) -> Any: + """Return a frozen TargetProfile with resolved_deploy_root for cowork. + + Args: + cowork_root: The resolved cowork skills root directory. + + Returns: + A frozen TargetProfile suitable for cowork tests. + """ + return replace(KNOWN_TARGETS["copilot-cowork"], resolved_deploy_root=cowork_root) + + +def _make_ctx( + cowork_root: Path | None = None, + include_copilot: bool = False, +) -> MagicMock: + """Build a minimal ctx mock for cap check tests. + + Args: + cowork_root: If set, adds a cowork target with this root. + include_copilot: If True, also adds the copilot target. + + Returns: + A MagicMock configured as an InstallContext. + """ + ctx = MagicMock() + ctx.targets = [] + if cowork_root is not None: + ctx.targets.append(_make_cowork_target(cowork_root)) + if include_copilot: + ctx.targets.append(KNOWN_TARGETS["copilot"]) + ctx.logger = MagicMock() + ctx.diagnostics = MagicMock() + return ctx + + +def _create_skills( + cowork_root: Path, count: int, size: int = 100 +) -> None: + """Create N skill directories with SKILL.md files. + + Args: + cowork_root: Root directory for skills. + count: Number of skill dirs to create. + size: Size of each SKILL.md in bytes. + """ + for i in range(count): + skill_dir = cowork_root / f"skill-{i:04d}" + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_bytes(b"x" * size) + + +# --------------------------------------------------------------------------- +# TestCheckCoworkCaps +# --------------------------------------------------------------------------- + + +class TestCheckCoworkCaps: + """Tests for _check_cowork_caps capacity checks.""" + + def test_count_cap_warning_fires_at_51_skills( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork" + cowork_root.mkdir() + _create_skills(cowork_root, 51) + ctx = _make_ctx(cowork_root) + _check_cowork_caps(ctx) + warning_calls = ctx.logger.warning.call_args_list + assert len(warning_calls) >= 1 + msg = str(warning_calls[0]) + assert "51" in msg + assert "50" in msg + + def test_count_cap_no_warning_at_50_skills( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork" + cowork_root.mkdir() + _create_skills(cowork_root, 50) + ctx = _make_ctx(cowork_root) + _check_cowork_caps(ctx) + warning_calls = ctx.logger.warning.call_args_list + # No warning about count + count_warnings = [ + c for c in warning_calls + if "50" in str(c) and "cap" in str(c).lower() + ] + assert len(count_warnings) == 0 + + def test_size_cap_warning_fires_for_oversized_skill_md( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork" + cowork_root.mkdir() + _create_skills(cowork_root, 1, size=1_048_577) + ctx = _make_ctx(cowork_root) + _check_cowork_caps(ctx) + warning_calls = ctx.logger.warning.call_args_list + assert len(warning_calls) >= 1 + msg = str(warning_calls[0]) + assert "MB" in msg + + def test_size_cap_no_warning_at_exactly_1mb( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork" + cowork_root.mkdir() + _create_skills(cowork_root, 1, size=1_048_576) + ctx = _make_ctx(cowork_root) + _check_cowork_caps(ctx) + size_warnings = [ + c for c in ctx.logger.warning.call_args_list + if "MB" in str(c) + ] + assert len(size_warnings) == 0 + + def test_cap_check_skipped_when_no_cowork_target( + self, tmp_path: Path + ) -> None: + ctx = _make_ctx(cowork_root=None, include_copilot=True) + _check_cowork_caps(ctx) + ctx.logger.warning.assert_not_called() + + def test_cap_check_skipped_when_cowork_root_nonexistent( + self, tmp_path: Path + ) -> None: + nonexistent = tmp_path / "nonexistent" + ctx = _make_ctx(cowork_root=nonexistent) + _check_cowork_caps(ctx) + ctx.logger.warning.assert_not_called() + + def test_package_100_skills_all_deploy_cap_warns_but_completes( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork" + cowork_root.mkdir() + _create_skills(cowork_root, 100) + ctx = _make_ctx(cowork_root) + # Should warn but NOT raise + _check_cowork_caps(ctx) + warning_calls = ctx.logger.warning.call_args_list + assert len(warning_calls) >= 1 + + def test_cap_check_skipped_when_targets_empty(self) -> None: + ctx = MagicMock() + ctx.targets = [] + ctx.logger = MagicMock() + _check_cowork_caps(ctx) + ctx.logger.warning.assert_not_called() diff --git a/tests/unit/install/phases/test_targets_phase.py b/tests/unit/install/phases/test_targets_phase.py new file mode 100644 index 000000000..375998d02 --- /dev/null +++ b/tests/unit/install/phases/test_targets_phase.py @@ -0,0 +1,352 @@ +"""Tests for apm_cli.install.phases.targets (project-scope gate, auto-create).""" +from __future__ import annotations + +from dataclasses import dataclass, field, replace +from pathlib import Path +from typing import Any, Dict, List, Optional +from unittest.mock import MagicMock, patch + +import pytest + +from apm_cli.core.scope import InstallScope +from apm_cli.integration.copilot_cowork_paths import CoworkResolutionError +from apm_cli.integration.targets import KNOWN_TARGETS, TargetProfile + + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def _reset_config_cache(): + """Reset the in-process config cache before and after every test.""" + from apm_cli.config import _invalidate_config_cache + + _invalidate_config_cache() + yield + _invalidate_config_cache() + + +@pytest.fixture +def inject_config(monkeypatch: pytest.MonkeyPatch): + """Directly inject a dict into the config cache -- no disk I/O.""" + import apm_cli.config as _conf + + def _set(cfg: Dict[str, Any]) -> None: + monkeypatch.setattr(_conf, "_config_cache", cfg) + + return _set + + +def _make_cowork_target(cowork_root: Path) -> TargetProfile: + """Return a frozen TargetProfile with resolved_deploy_root for cowork. + + Args: + cowork_root: The resolved cowork skills root directory. + + Returns: + A frozen TargetProfile suitable for cowork tests. + """ + return replace(KNOWN_TARGETS["copilot-cowork"], resolved_deploy_root=cowork_root) + + +def _make_ctx( + tmp_path: Path, + scope: InstallScope = InstallScope.PROJECT, + target_override: Optional[str] = None, +) -> MagicMock: + """Build a minimal ctx mock for phase tests. + + Args: + tmp_path: Base temp directory for project_root. + scope: Install scope (PROJECT or USER). + target_override: CLI --target value. + + Returns: + A MagicMock configured as an InstallContext. + """ + ctx = MagicMock() + ctx.project_root = tmp_path / "project" + ctx.project_root.mkdir(parents=True, exist_ok=True) + ctx.scope = scope + ctx.target_override = target_override + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + ctx.targets = [] + ctx.integrators = {} + return ctx + + +# --------------------------------------------------------------------------- +# TestProjectScopeGateForCowork +# --------------------------------------------------------------------------- + + +class TestProjectScopeGateForCowork: + """Tests for the project-scope cowork gate in phases/targets.py.""" + + def test_project_scope_with_cowork_raises_system_exit( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + cowork_target = _make_cowork_target(tmp_path / "cowork") + ctx = _make_ctx(tmp_path, scope=InstallScope.PROJECT) + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[cowork_target], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit): + from apm_cli.install.phases.targets import run + run(ctx) + + def test_project_scope_with_cowork_logs_error_before_exit( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + cowork_target = _make_cowork_target(tmp_path / "cowork") + ctx = _make_ctx(tmp_path, scope=InstallScope.PROJECT) + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[cowork_target], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit): + from apm_cli.install.phases.targets import run + run(ctx) + # Check that the error was logged with --global hint + error_calls = ctx.logger.error.call_args_list + assert len(error_calls) >= 1 + msg = str(error_calls[0]) + assert "--global" in msg + + def test_project_scope_with_cowork_no_mkdir_before_exit( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + cowork_target = _make_cowork_target(tmp_path / "cowork") + ctx = _make_ctx(tmp_path, scope=InstallScope.PROJECT) + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[cowork_target], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit): + from apm_cli.install.phases.targets import run + run(ctx) + assert not (ctx.project_root / "copilot-cowork").exists() + + def test_user_scope_with_cowork_does_not_raise( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + cowork_target = _make_cowork_target(tmp_path / "cowork") + ctx = _make_ctx(tmp_path, scope=InstallScope.USER) + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[cowork_target], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + from apm_cli.install.phases.targets import run + run(ctx) # Should not raise + + def test_project_scope_non_cowork_target_unaffected( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({}) + copilot = KNOWN_TARGETS["copilot"] + ctx = _make_ctx(tmp_path, scope=InstallScope.PROJECT) + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[copilot], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + from apm_cli.install.phases.targets import run + run(ctx) # Should not raise + + +# --------------------------------------------------------------------------- +# TestAutoCreateSkipForDynamicRoot +# --------------------------------------------------------------------------- + + +class TestAutoCreateSkipForDynamicRoot: + """Tests for auto-create directory skipping with dynamic-root targets.""" + + def test_dynamic_root_target_skips_mkdir( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + cowork_target = _make_cowork_target(tmp_path / "cowork") + ctx = _make_ctx(tmp_path, scope=InstallScope.USER) + ctx.target_override = "copilot-cowork" + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[cowork_target], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + from apm_cli.install.phases.targets import run + run(ctx) + assert not (ctx.project_root / "copilot-cowork").exists() + + def test_static_root_target_does_mkdir( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({}) + copilot = KNOWN_TARGETS["copilot"] + ctx = _make_ctx(tmp_path, scope=InstallScope.PROJECT) + ctx.target_override = "copilot" + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[copilot], + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + from apm_cli.install.phases.targets import run + run(ctx) + assert (ctx.project_root / ".github").exists() + + +# --------------------------------------------------------------------------- +# TestCoworkResolutionErrorHandling +# --------------------------------------------------------------------------- + + +class TestCoworkResolutionErrorHandling: + """Tests for CoworkResolutionError catch in phases/targets.py run().""" + + def test_resolution_error_raises_system_exit( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + ctx = _make_ctx(tmp_path, scope=InstallScope.USER, target_override="copilot-cowork") + + with patch( + "apm_cli.integration.targets.resolve_targets", + side_effect=CoworkResolutionError("Multiple OneDrive mounts detected"), + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit) as exc_info: + from apm_cli.install.phases.targets import run + run(ctx) + assert exc_info.value.code == 1 + + def test_resolution_error_logs_message_no_traceback( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + ctx = _make_ctx(tmp_path, scope=InstallScope.USER, target_override="copilot-cowork") + error_msg = "Multiple OneDrive mounts detected:\n - /a\n - /b" + + with patch( + "apm_cli.integration.targets.resolve_targets", + side_effect=CoworkResolutionError(error_msg), + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit): + from apm_cli.install.phases.targets import run + run(ctx) + + ctx.logger.error.assert_called_once_with(error_msg, symbol="cross") + + def test_resolution_error_no_logger_still_exits( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + ctx = _make_ctx(tmp_path, scope=InstallScope.USER, target_override="copilot-cowork") + ctx.logger = None + + with patch( + "apm_cli.integration.targets.resolve_targets", + side_effect=CoworkResolutionError("test"), + ), patch( + "apm_cli.core.target_detection.detect_target", + ): + with pytest.raises(SystemExit) as exc_info: + from apm_cli.install.phases.targets import run + run(ctx) + assert exc_info.value.code == 1 + + +# --------------------------------------------------------------------------- +# TestCoworkLinuxSpecificMessage (P3) +# --------------------------------------------------------------------------- + + +class TestCoworkLinuxSpecificMessage: + """P3: Linux users see a Linux-specific error; others see the generic one.""" + + def _run_cowork_no_onedrive( + self, tmp_path: Path, inject_config, platform_value: str + ) -> MagicMock: + """Run the targets phase with cowork flag ON but resolver returning None. + + Returns the ctx mock so callers can inspect logger calls. + """ + inject_config({"experimental": {"copilot_cowork": True}}) + ctx = _make_ctx( + tmp_path, + scope=InstallScope.USER, + target_override="copilot-cowork", + ) + + # resolve_targets returns NO cowork target (resolver returned None + # during target resolution) -- this triggers the flag-ON-but-no-path branch. + from apm_cli.integration.targets import KNOWN_TARGETS + + non_cowork = [KNOWN_TARGETS["copilot"]] + + with patch( + "apm_cli.integration.targets.resolve_targets", + return_value=non_cowork, + ), patch( + "apm_cli.core.target_detection.detect_target", + ), patch( + "sys.platform", platform_value + ): + with pytest.raises(SystemExit): + from apm_cli.install.phases.targets import run + + run(ctx) + return ctx + + def test_linux_message_contains_no_auto_detection( + self, tmp_path: Path, inject_config + ) -> None: + ctx = self._run_cowork_no_onedrive(tmp_path, inject_config, "linux") + msg = ctx.logger.error.call_args[0][0] + assert "no auto-detection on Linux" in msg + assert "APM_COPILOT_COWORK_SKILLS_DIR" in msg + + def test_darwin_message_does_not_contain_linux_phrase( + self, tmp_path: Path, inject_config + ) -> None: + ctx = self._run_cowork_no_onedrive(tmp_path, inject_config, "darwin") + msg = ctx.logger.error.call_args[0][0] + assert "no auto-detection on Linux" not in msg + assert "no OneDrive path detected" in msg + + def test_win32_message_does_not_contain_linux_phrase( + self, tmp_path: Path, inject_config + ) -> None: + ctx = self._run_cowork_no_onedrive(tmp_path, inject_config, "win32") + msg = ctx.logger.error.call_args[0][0] + assert "no auto-detection on Linux" not in msg + assert "no OneDrive path detected" in msg diff --git a/tests/unit/install/test_install_target_copilot_cowork_e2e.py b/tests/unit/install/test_install_target_copilot_cowork_e2e.py new file mode 100644 index 000000000..fc91ed13e --- /dev/null +++ b/tests/unit/install/test_install_target_copilot_cowork_e2e.py @@ -0,0 +1,562 @@ +"""E2E regression tests for 'apm install --target copilot-cowork --global'. + +These tests exercise the real Click parser to guard against the bug fixed in +commit 2f96dd5: 'cowork' was not in VALID_TARGET_VALUES, so the CLI rejected +the flag with "is not a valid target" at *parse time*, before the install +pipeline even ran. + +Two mandatory scenarios: + 1. Flag OFF -> reaches phases/targets.py, prints enable-hint, exits 0. + 2. Flag ON -> reaches phases/targets.py, resolver finds no OneDrive, exits 1 + with "no OneDrive path detected" message. + 3. (Optional) No --global -> project-scope gate rejects with --global hint. + +Design notes +------------ +* ``CONFIG_DIR`` and ``CONFIG_FILE`` in ``apm_cli.config`` are module-level + strings computed from ``~`` at import time. We must monkeypatch them + directly -- changing the HOME env var after import has no effect. +* ``Path.home()`` is used by ``apm_cli.core.scope`` functions that build + user-scope paths at call time. We monkeypatch the classmethod so that + every call inside the installed pipeline returns our temp directory. +* ``APM_E2E_TESTS=1`` silences the version-check background side effect. +* A minimal ``apm.yml`` (no deps) avoids all network calls: the resolve + phase creates an empty dependency graph and the download phase is a no-op. +""" + +from __future__ import annotations + +import json +import os +import tempfile +from pathlib import Path +from typing import Any, Dict +from unittest.mock import patch + +import pytest + +from click.testing import CliRunner + +from apm_cli.cli import cli + + +# --------------------------------------------------------------------------- +# Shared helpers +# --------------------------------------------------------------------------- + +_MINIMAL_APM_YML = "name: test\ndescription: test\nversion: 0.0.1\n" + +# Env additions applied to every CliRunner.invoke call in this module. +_BASE_ENV: Dict[str, str] = {"APM_E2E_TESTS": "1"} + + +def _write_minimal_apm_yml(apm_dir: Path) -> None: + """Write a minimal apm.yml with no dependencies into *apm_dir*.""" + (apm_dir / "apm.yml").write_text(_MINIMAL_APM_YML, encoding="ascii") + + +def _write_config_json(apm_dir: Path, cfg: Dict[str, Any]) -> None: + """Write *cfg* as JSON to ``/config.json``.""" + (apm_dir / "config.json").write_text(json.dumps(cfg), encoding="ascii") + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture() +def fake_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Isolated home directory wired into every APM config lookup. + + Sets up: + - ``tmp_path/home/.apm/`` directory + - Monkeypatches ``Path.home`` so scope helpers use the fake home + - Monkeypatches ``apm_cli.config.CONFIG_DIR`` and ``CONFIG_FILE`` + (computed at import time, not re-evaluated from HOME at runtime) + - Resets ``_config_cache`` before/after so stale cached state never + leaks between tests + + Returns the fake home root (``tmp_path/home``). + """ + home = tmp_path / "home" + apm_dir = home / ".apm" + apm_dir.mkdir(parents=True) + + # -- apm.yml (required -- bare install with no apm.yml exits 1) ----- + _write_minimal_apm_yml(apm_dir) + + # -- Path.home() ------------------------------------------------------- + # scope.py calls Path.home() at *call time* (not import time) so + # patching the classmethod is enough. + monkeypatch.setattr(Path, "home", staticmethod(lambda: home)) + + # -- apm_cli.config constants (evaluated at import time) ------------- + import apm_cli.config as _conf + + monkeypatch.setattr(_conf, "CONFIG_DIR", str(apm_dir)) + monkeypatch.setattr(_conf, "CONFIG_FILE", str(apm_dir / "config.json")) + monkeypatch.setattr(_conf, "_config_cache", None) + yield home + # Reset after the test so no cached state bleeds out. + monkeypatch.setattr(_conf, "_config_cache", None) + + +# --------------------------------------------------------------------------- +# TestCoworkParserE2E -- core regression tests +# --------------------------------------------------------------------------- + + +class TestCoworkParserE2E: + """CliRunner regression tests for 'apm install --target copilot-cowork --global'. + + Before the fix in 2f96dd5, both tests below would have failed at Click + *parse time* with: + Error: Invalid value for '--target': 'cowork' is not a valid target. ... + and exited with code 2 (Click's UsageError exit code). + """ + + # ------------------------------------------------------------------ # + # Case 1: Flag OFF -- parser accepts cowork, targets phase emits hint # + # ------------------------------------------------------------------ # + + def test_flag_off_parser_accepts_cowork_and_emits_hint( + self, fake_home: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """apm install --target copilot-cowork --global with flag OFF: + - Click must NOT reject 'copilot-cowork' ("is not a valid target" must be absent). + - The command must exit 0 (enable-hint path). + - Output must contain 'apm experimental enable copilot-cowork'. + """ + # Ensure cowork flag is OFF (no config.json, or explicit false). + # With no config.json the config module creates a default one that + # does NOT include the copilot_cowork key, so is_enabled("copilot_cowork") == False. + config_file = fake_home / ".apm" / "config.json" + if config_file.exists(): + config_file.unlink() + + # Ensure APM_COPILOT_COWORK_SKILLS_DIR is unset so no accidental OneDrive hit. + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + + runner = CliRunner() + result = runner.invoke( + cli, + ["install", "--target", "copilot-cowork", "--global"], + env={**_BASE_ENV}, + catch_exceptions=False, + ) + + # Regression: Click parse-time rejection used exit code 2. + # The fix means we reach the install pipeline instead. + assert result.exit_code == 0, ( + f"Expected exit 0 from enable-hint path, got {result.exit_code}.\n" + f"Output:\n{result.output}" + ) + + combined = result.output or "" + + # Old bug: Click rejected at parse time. + assert "is not a valid target" not in combined, ( + "Parser still rejecting 'copilot-cowork' -- fix may have been reverted.\n" + f"Output:\n{combined}" + ) + + # Phases/targets.py must have emitted the enable hint. + # Normalize whitespace to handle terminal line-wrapping. + normalized = " ".join(combined.split()) + assert "apm experimental enable copilot-cowork" in normalized, ( + "Enable hint not found in output -- targets phase may not have run.\n" + f"Output:\n{combined}" + ) + + # ------------------------------------------------------------------ # + # Case 2: Flag ON -- parser accepts cowork, resolver error emitted # + # ------------------------------------------------------------------ # + + def test_flag_on_parser_accepts_cowork_resolver_error( + self, fake_home: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """apm install --target copilot-cowork --global with flag ON but no OneDrive: + - Click must NOT reject 'copilot-cowork'. + - phases/targets.py must emit the 'no OneDrive path detected' error. + - The command exits non-zero (cowork resolver failure). + + The exit code is 1 (sys.exit(1) in phases/targets.py run()). + """ + import apm_cli.config as _conf + + # Enable the cowork experimental flag via direct cache injection. + monkeypatch.setattr( + _conf, + "_config_cache", + {"experimental": {"copilot_cowork": True}}, + ) + + # Ensure no OneDrive path is available in the sandbox. + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + + # Patch the cowork root resolver to return None (no OneDrive found). + # Patch at the point-of-use in integration.targets so that the + # resolve_targets() call in phases/targets.py hits our stub. + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ): + runner = CliRunner() + result = runner.invoke( + cli, + ["install", "--target", "copilot-cowork", "--global"], + env={**_BASE_ENV}, + catch_exceptions=True, # SystemExit is expected + ) + + combined = result.output or "" + + # Regression guard: no parse-time "is not a valid target" rejection. + assert "is not a valid target" not in combined, ( + "Parser still rejecting 'copilot-cowork' -- fix may have been reverted.\n" + f"Output:\n{combined}" + ) + + # The resolver error message from phases/targets.py must appear. + # Linux emits "Cowork has no auto-detection on Linux." while macOS + # emits "no OneDrive path detected" — accept either variant. + assert ( + "no OneDrive path detected" in combined + or "Cowork has no auto-detection on Linux" in combined + ), ( + "Expected cowork resolver error in output.\n" + f"Output:\n{combined}" + ) + + # The command must have failed (sys.exit(1) in targets phase). + # Note: CliRunner wraps SystemExit -- exit_code reflects the code. + assert result.exit_code != 0, ( + "Expected non-zero exit when OneDrive resolver returns None.\n" + f"Output:\n{combined}" + ) + + # ------------------------------------------------------------------ # + # Case 3: No --global -- project-scope gate must reject # + # ------------------------------------------------------------------ # + + def test_no_global_flag_project_scope_rejected( + self, fake_home: Path, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """apm install --target copilot-cowork (no --global) must error with --global hint. + + The project-scope gate in phases/targets.py checks that cowork is + only valid with --global (user scope). + """ + import apm_cli.config as _conf + + # Flag ON so cowork passes the flag gate and reaches the scope gate. + monkeypatch.setattr( + _conf, + "_config_cache", + {"experimental": {"copilot_cowork": True}}, + ) + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + + # For project scope, CWD must have an apm.yml. + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / "apm.yml").write_text(_MINIMAL_APM_YML, encoding="ascii") + + # Patch cowork root resolver so user-scope path (not triggered here) + # would return a valid dir -- the project-scope gate fires first. + with ( + patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ), + ): + runner = CliRunner() + result = runner.invoke( + cli, + ["install", "--target", "copilot-cowork"], + env={**_BASE_ENV}, + catch_exceptions=True, + # Provide the project dir as CWD via CliRunner. + ) + + combined = result.output or "" + + # Parser must NOT reject at parse time. + assert "is not a valid target" not in combined, ( + f"Parser rejected 'cowork' -- fix may have been reverted.\n" + f"Output:\n{combined}" + ) + + # The project-scope error from phases/targets.py should mention --global. + assert "--global" in combined, ( + "Expected '--global' hint in project-scope error output.\n" + f"Output:\n{combined}" + ) + + +# --------------------------------------------------------------------------- +# TestCoworkCleanupSyncRemove -- regression test for PR #926 +# --------------------------------------------------------------------------- + + +class TestCoworkCleanupSyncRemove: + """Regression test: sync_remove_files must delete cowork:// entries + when called with targets=None (the cleanup/uninstall call site). + + Before the fix, get_integration_prefixes(targets=None) omitted the + cowork:// prefix because it checked resolved_deploy_root (always None + on the static KNOWN_TARGETS registry) instead of user_root_resolver + (a capability flag). This caused validate_deploy_path to reject + every cowork:// path, silently skipping deletion. + """ + + def test_cowork_skill_deleted_via_sync_remove_with_targets_none( + self, tmp_path: Path + ) -> None: + """The exact scenario that triggers the regression: + + 1. A lockfile has a cowork://skills/foo entry. + 2. The cowork skills dir has a foo/SKILL.md file. + 3. sync_remove_files is called with targets=None (cleanup path). + 4. The file MUST be deleted (was silently skipped before the fix). + """ + from apm_cli.integration.base_integrator import BaseIntegrator + + # -- setup: cowork skills dir with a skill file --- + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + skill_dir = cowork_root / "foo" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text("---\nname: foo\n---\n# Foo skill\n", encoding="ascii") + assert skill_md.exists() + + # -- setup: project root (unrelated to cowork) --- + project_root = tmp_path / "project" + project_root.mkdir() + + # -- exercise: sync_remove with targets=None --- + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + stats = BaseIntegrator.sync_remove_files( + project_root, + managed_files={"cowork://skills/foo/SKILL.md"}, + prefix="cowork://skills/", + targets=None, + ) + + # -- verify: the file was deleted --- + assert not skill_md.exists(), ( + "SKILL.md still exists -- cowork:// path was not cleaned up. " + "This is the PR #926 regression." + ) + assert stats["files_removed"] == 1 + assert stats["errors"] == 0 + + +class TestCoworkCleanupOrphanFlow: + """Integration-style regression test simulating the uninstall flow. + + Exercises remove_stale_deployed_files (the orphan cleanup path) + with a cowork:// bearing package and a real temp cowork root. + Before the fix, the cowork file would silently survive because + the cleanup helper computed ``project_root / "cowork://skills/..."`` + instead of resolving the URI to the actual OneDrive path. + """ + + def test_orphan_cleanup_deletes_cowork_skill_directory( + self, tmp_path: Path + ) -> None: + """Simulate uninstalling a package that deployed a cowork skill: + + 1. A lockfile has cowork://skills/demo-skill entries. + 2. The cowork skills dir has demo-skill/SKILL.md. + 3. remove_stale_deployed_files (orphan path) is called with + targets=None. + 4. The skill file MUST be deleted. + """ + from apm_cli.integration.cleanup import remove_stale_deployed_files + from apm_cli.utils.diagnostics import DiagnosticCollector + + # -- setup: cowork skills dir with a skill --- + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + skill_dir = cowork_root / "demo-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + "---\nname: demo-skill\n---\n# Demo\n", encoding="ascii" + ) + assert skill_md.exists() + + project_root = tmp_path / "project" + project_root.mkdir() + + diagnostics = DiagnosticCollector(verbose=False) + + # The lockfile would have recorded these deployed files. + stale_entries = ["cowork://skills/demo-skill/SKILL.md"] + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = remove_stale_deployed_files( + stale_entries, + project_root, + dep_key="some-org/skill-pack", + targets=None, + diagnostics=diagnostics, + failed_path_retained=False, # orphan cleanup path + ) + + # -- verify: the skill file was deleted --- + assert not skill_md.exists(), ( + "SKILL.md still exists in cowork root -- " + "remove_stale_deployed_files did not resolve the " + "cowork:// URI. This is the cleanup half of the PR #926 " + "regression." + ) + assert "cowork://skills/demo-skill/SKILL.md" in result.deleted + assert not result.failed + assert not result.skipped_unmanaged + + +# --------------------------------------------------------------------------- +# TestCoworkUninstallSyncIntegration -- regression test for PR #926 +# --------------------------------------------------------------------------- + + +class TestCoworkUninstallSyncIntegration: + """Regression test: SkillIntegrator.sync_integration must delete cowork:// + entries during uninstall (the _sync_integrations_after_uninstall flow). + + Before the fix, sync_integration built a skill_prefix_tuple from only + local directory prefixes (.github/skills/, .copilot/skills/, etc.) and + never included cowork://skills/. Cowork entries were silently skipped, + leaving orphaned skill directories on disk in OneDrive forever. + """ + + def test_uninstall_deletes_cowork_skill_directory( + self, tmp_path: Path + ) -> None: + """Simulate the uninstall flow via SkillIntegrator.sync_integration: + + 1. A cowork://skills/demo-skill entry is in managed_files. + 2. The cowork skills dir has demo-skill/ with SKILL.md. + 3. sync_integration is called with targets including the cowork target. + 4. The skill directory MUST be deleted (was silently skipped before). + """ + from unittest.mock import MagicMock + from apm_cli.integration.skill_integrator import SkillIntegrator + from apm_cli.integration.targets import TargetProfile, PrimitiveMapping + + # -- setup: cowork skills dir with a skill --- + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + skill_dir = cowork_root / "demo-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text( + "---\nname: demo-skill\n---\n# Demo\n", encoding="ascii" + ) + assert skill_md.exists() + + # -- setup: project root (unrelated to cowork) --- + project_root = tmp_path / "project" + project_root.mkdir() + + # -- setup: cowork target profile --- + cowork_target = TargetProfile( + name="copilot-cowork", + root_dir="copilot-cowork", + primitives={ + "skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard"), + }, + auto_create=False, + detect_by_dir=False, + user_supported=True, + user_root_resolver=lambda: cowork_root, + ) + + # -- setup: minimal apm_package --- + apm_package = MagicMock() + apm_package.get_apm_dependencies.return_value = [] + + integrator = SkillIntegrator() + + # -- exercise: sync_integration with cowork entry --- + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = integrator.sync_integration( + apm_package, + project_root, + managed_files={"cowork://skills/demo-skill"}, + targets=[cowork_target], + ) + + # -- verify: the skill directory was deleted --- + assert not skill_dir.exists(), ( + "demo-skill/ still exists in cowork root -- " + "sync_integration did not handle the cowork:// entry. " + "This is the PR #926 uninstall regression." + ) + assert result["files_removed"] == 1 + assert result["errors"] == 0 + + def test_uninstall_cowork_with_resolver_none_skips_gracefully( + self, tmp_path: Path + ) -> None: + """When OneDrive is unavailable, cowork entries are skipped (not error).""" + from unittest.mock import MagicMock + from apm_cli.integration.skill_integrator import SkillIntegrator + from apm_cli.integration.targets import TargetProfile, PrimitiveMapping + + project_root = tmp_path / "project" + project_root.mkdir() + + cowork_target = TargetProfile( + name="copilot-cowork", + root_dir="copilot-cowork", + primitives={ + "skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard"), + }, + auto_create=False, + detect_by_dir=False, + user_supported=True, + user_root_resolver=lambda: None, + ) + + apm_package = MagicMock() + apm_package.get_apm_dependencies.return_value = [] + + integrator = SkillIntegrator() + + with ( + patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ), + patch( + "apm_cli.utils.console._rich_warning", + ) as mock_warn, + ): + result = integrator.sync_integration( + apm_package, + project_root, + managed_files={"cowork://skills/demo-skill"}, + targets=[cowork_target], + ) + + # Entry skipped, not counted as error. + assert result["files_removed"] == 0 + assert result["errors"] == 0 + + # Warning must have been emitted. + mock_warn.assert_called_once() + diff --git a/tests/unit/install/test_services.py b/tests/unit/install/test_services.py new file mode 100644 index 000000000..725d24355 --- /dev/null +++ b/tests/unit/install/test_services.py @@ -0,0 +1,534 @@ +"""Unit tests for apm_cli.install.services (_deployed_path_entry and Amendment 6 warning).""" +from __future__ import annotations + +from dataclasses import replace +from pathlib import Path +from typing import Any, Dict +from unittest.mock import MagicMock, patch, call + +import pytest + +from apm_cli.install.services import _deployed_path_entry +from apm_cli.integration.targets import KNOWN_TARGETS + + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def _reset_config_cache(): + """Reset the in-process config cache before and after every test.""" + from apm_cli.config import _invalidate_config_cache + + _invalidate_config_cache() + yield + _invalidate_config_cache() + + +@pytest.fixture +def inject_config(monkeypatch: pytest.MonkeyPatch): + """Directly inject a dict into the config cache -- no disk I/O.""" + import apm_cli.config as _conf + + def _set(cfg: Dict[str, Any]) -> None: + monkeypatch.setattr(_conf, "_config_cache", cfg) + + return _set + + +def _make_cowork_target(cowork_root: Path) -> Any: + """Return a TargetProfile with resolved_deploy_root set for cowork. + + Args: + cowork_root: The resolved cowork skills root directory. + + Returns: + A frozen TargetProfile suitable for cowork tests. + """ + return replace(KNOWN_TARGETS["copilot-cowork"], resolved_deploy_root=cowork_root) + + +# --------------------------------------------------------------------------- +# TestDeployedPathEntry +# --------------------------------------------------------------------------- + + +class TestDeployedPathEntry: + """Tests for _deployed_path_entry lockfile path generation.""" + + def test_relative_path_for_project_target(self, tmp_path: Path) -> None: + project_root = tmp_path / "project" + project_root.mkdir() + target_path = project_root / ".github" / "skills" / "foo" / "SKILL.md" + result = _deployed_path_entry(target_path, project_root, targets=[]) + assert result == ".github/skills/foo/SKILL.md" + + def test_cowork_uri_for_out_of_tree_path(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + project_root = tmp_path / "project" + project_root.mkdir() + target_path = cowork_root / "my-skill" / "SKILL.md" + cowork_target = _make_cowork_target(cowork_root) + + with patch( + "apm_cli.integration.copilot_cowork_paths.to_lockfile_path", + return_value="cowork://skills/my-skill/SKILL.md", + ): + result = _deployed_path_entry( + target_path, project_root, targets=[cowork_target] + ) + assert result == "cowork://skills/my-skill/SKILL.md" + + def test_runtime_error_when_no_matching_target( + self, tmp_path: Path + ) -> None: + """Out-of-tree path with no dynamic-root target must raise, not silently store an absolute path.""" + project_root = tmp_path / "project" + project_root.mkdir() + target_path = tmp_path / "outside" / "file.md" + with pytest.raises(RuntimeError, match="This is a bug"): + _deployed_path_entry( + target_path, project_root, targets=[] + ) + + def test_path_traversal_error_propagates_from_cowork_translation( + self, tmp_path: Path + ) -> None: + """PathTraversalError from to_lockfile_path must propagate, never be swallowed.""" + from apm_cli.utils.path_security import PathTraversalError + + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + project_root = tmp_path / "project" + project_root.mkdir() + # target_path is deliberately outside the cowork_root + target_path = tmp_path / "evil" / "escape.md" + cowork_target = _make_cowork_target(cowork_root) + + with pytest.raises(PathTraversalError): + _deployed_path_entry( + target_path, project_root, targets=[cowork_target] + ) + + @pytest.mark.parametrize( + "dir_prefix", + [".github", ".claude", ".cursor", ".codex"], + ) + def test_deployed_path_entry_non_cowork_lockfile_unchanged_parametrised( + self, dir_prefix: str, tmp_path: Path + ) -> None: + project_root = tmp_path / "project" + project_root.mkdir() + target_path = project_root / dir_prefix / "sub" / "file.md" + result = _deployed_path_entry( + target_path, project_root, targets=[] + ) + expected = f"{dir_prefix}/sub/file.md" + assert result == expected + + +# --------------------------------------------------------------------------- +# TestAmendment6Warning +# --------------------------------------------------------------------------- + + +class TestAmendment6Warning: + """Tests for the cowork non-skill primitive warning in integrate_package_primitives.""" + + def _make_ctx(self, cowork_active: bool = True) -> MagicMock: + """Build a minimal ctx mock for Amendment 6 testing. + + Args: + cowork_active: Whether cowork_nonsupported_warned starts False. + + Returns: + A MagicMock configured as an InstallContext. + """ + ctx = MagicMock() + ctx.cowork_nonsupported_warned = False + return ctx + + def _make_pkg_info(self, tmp_path: Path, non_skill_dirs: list[str] | None = None) -> MagicMock: + """Create a package info mock with optional non-skill directories. + + Args: + tmp_path: Base temp directory. + non_skill_dirs: Subdirectory names under .apm/ to create. + + Returns: + A MagicMock configured as PackageInfo. + """ + pkg_dir = tmp_path / "pkg" + pkg_dir.mkdir(parents=True, exist_ok=True) + apm_dir = pkg_dir / ".apm" + apm_dir.mkdir(exist_ok=True) + if non_skill_dirs: + for d in non_skill_dirs: + sub = apm_dir / d + sub.mkdir(exist_ok=True) + (sub / "placeholder.md").write_text("# content") + pkg = MagicMock() + pkg.install_path = pkg_dir + pkg.name = "test-pkg" + return pkg + + def test_warning_fires_once_per_run_with_non_skill_primitives( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + copilot = KNOWN_TARGETS["copilot"] + targets = [copilot, cowork_target] + + pkg_info = self._make_pkg_info(tmp_path, ["agents"]) + logger = MagicMock() + ctx = self._make_ctx() + + # Mock all integrators to avoid real dispatch + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + # Make skill_integrator.integrate_package_skill return a result + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + # Mock dispatch table to skip integration loops + mock_dispatch = {} + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value=mock_dispatch, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + package_name="test-pkg", + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + + # Warning should have fired + warning_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls) == 1 + assert ctx.cowork_nonsupported_warned is True + + # Second call should NOT fire again + pkg_info2 = self._make_pkg_info(tmp_path / "pkg2", ["prompts"]) + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value=mock_dispatch, + ): + integrate_package_primitives( + pkg_info2, tmp_path, + targets=targets, + diagnostics=MagicMock(), + package_name="test-pkg2", + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + warning_calls_after = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls_after) == 1 # still just 1 + + def test_warning_does_not_fire_when_only_skills( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + targets = [cowork_target] + + # Package has only skills dir (no non-skill dirs) + pkg_info = self._make_pkg_info(tmp_path, ["skills"]) + logger = MagicMock() + ctx = self._make_ctx() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + warning_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls) == 0 + + def test_warning_does_not_fire_when_cowork_not_active( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({}) + from apm_cli.install.services import integrate_package_primitives + + copilot = KNOWN_TARGETS["copilot"] + targets = [copilot] + + pkg_info = self._make_pkg_info(tmp_path, ["agents"]) + logger = MagicMock() + ctx = self._make_ctx() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + warning_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls) == 0 + + def test_warning_does_not_fire_when_ctx_is_none( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + targets = [cowork_target] + + pkg_info = self._make_pkg_info(tmp_path, ["agents"]) + logger = MagicMock() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + # ctx=None should not raise + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + logger=logger, + ctx=None, + **integrators, + force=False, + managed_files=None, + ) + # No exception is the assertion + + def test_warning_msg_text_includes_package_name_and_primitive_types( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + targets = [cowork_target] + + pkg_info = self._make_pkg_info(tmp_path, ["agents"]) + logger = MagicMock() + ctx = self._make_ctx() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + package_name="my-awesome-pkg", + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + warning_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls) == 1 + msg = str(warning_calls[0]) + assert "my-awesome-pkg" in msg + assert "agents" in msg + + def test_warning_also_emitted_to_diagnostics_warn( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + targets = [cowork_target] + + pkg_info = self._make_pkg_info(tmp_path, ["agents"]) + logger = MagicMock() + ctx = self._make_ctx() + diagnostics = MagicMock() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=diagnostics, + package_name="diag-pkg", + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + + # logger.warning should have been called once + logger_warn_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(logger_warn_calls) == 1 + + # diagnostics.warn should also have been called once with same message + diagnostics.warn.assert_called_once() + diag_msg = diagnostics.warn.call_args[0][0] + assert "cowork" in diag_msg + assert "diag-pkg" in diag_msg + + def test_warning_with_prompts_only_does_not_mention_commands( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Package with only prompts/ dir: warning says 'prompts', not 'commands'.""" + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.services import integrate_package_primitives + + cowork_target = _make_cowork_target(tmp_path / "cowork") + targets = [cowork_target] + + # Package has only prompts dir (no agents, instructions, hooks, etc.) + pkg_info = self._make_pkg_info(tmp_path, ["prompts"]) + logger = MagicMock() + ctx = self._make_ctx() + + integrators = {k: MagicMock() for k in [ + "prompt_integrator", "agent_integrator", + "skill_integrator", "instruction_integrator", + "command_integrator", "hook_integrator", + ]} + skill_result = MagicMock() + skill_result.target_paths = [] + skill_result.skill_created = False + skill_result.sub_skills_promoted = 0 + integrators["skill_integrator"].integrate_package_skill.return_value = skill_result + + with patch( + "apm_cli.integration.dispatch.get_dispatch_table", + return_value={}, + ): + integrate_package_primitives( + pkg_info, tmp_path, + targets=targets, + diagnostics=MagicMock(), + package_name="prompts-only-pkg", + logger=logger, + ctx=ctx, + **integrators, + force=False, + managed_files=None, + ) + + warning_calls = [ + c for c in logger.warning.call_args_list + if "cowork" in str(c).lower() + ] + assert len(warning_calls) == 1 + msg = str(warning_calls[0]) + assert "prompts" in msg + assert "commands" not in msg diff --git a/tests/unit/integration/test_base_integrator.py b/tests/unit/integration/test_base_integrator.py index 6e5a95500..3a3fc843b 100644 --- a/tests/unit/integration/test_base_integrator.py +++ b/tests/unit/integration/test_base_integrator.py @@ -631,3 +631,515 @@ def test_uses_install_path_when_not_home( bi.init_link_resolver(pkg_info, tmp_path) mock_discover.assert_called_once_with(tmp_path) + +# Cowork additive tests +# --------------------------------------------------------------------------- + +from dataclasses import replace +from apm_cli.integration.targets import KNOWN_TARGETS + + +def _make_cowork_target(cowork_root: Path) -> "TargetProfile": + """Return a frozen TargetProfile with resolved_deploy_root for cowork. + + Args: + cowork_root: Absolute path to the cowork skills directory. + + Returns: + A frozen TargetProfile suitable for cowork tests. + """ + return replace(KNOWN_TARGETS["copilot-cowork"], resolved_deploy_root=cowork_root) + + +class TestValidateDeployPathCowork: + """Tests for validate_deploy_path with cowork:// paths.""" + + def test_cowork_valid_skill_md_validates(self, tmp_path: Path) -> None: + skill_md = tmp_path / "my-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + cowork_target = _make_cowork_target(tmp_path) + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=tmp_path, + ): + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/my-skill/SKILL.md", + tmp_path, + targets=[cowork_target], + ) + assert result is True + + def test_cowork_traversal_rejected(self, tmp_path: Path) -> None: + cowork_target = _make_cowork_target(tmp_path) + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/../../escape.md", + tmp_path, + targets=[cowork_target], + ) + assert result is False + + def test_cowork_no_resolver_result_returns_false( + self, tmp_path: Path + ) -> None: + cowork_target = _make_cowork_target(tmp_path) + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ): + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/my-skill/SKILL.md", + tmp_path, + targets=[cowork_target], + ) + assert result is False + + def test_cowork_prefix_not_in_allowed_prefixes_rejected( + self, tmp_path: Path + ) -> None: + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/my-skill/SKILL.md", + tmp_path, + allowed_prefixes=(".github/",), + ) + assert result is False + + def test_non_cowork_paths_unaffected(self, tmp_path: Path) -> None: + prompt = tmp_path / ".github" / "prompts" / "foo.prompt.md" + prompt.parent.mkdir(parents=True) + prompt.touch() + result = BaseIntegrator.validate_deploy_path( + ".github/prompts/foo.prompt.md", + tmp_path, + ) + assert result is True + + # -- Regression tests for cleanup with targets=None (PR #926) ---------- + + def test_validate_deploy_path_accepts_cowork_uri_during_cleanup_with_targets_none( + self, tmp_path: Path + ) -> None: + """Simulate the cleanup call site: targets=None, cowork:// URI. + The static KNOWN_TARGETS registry has resolved_deploy_root=None + but the fix ensures the cowork prefix is still included via the + user_root_resolver capability check. + """ + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=tmp_path, + ): + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/my-skill/SKILL.md", + tmp_path, + targets=None, + ) + assert result is True + + def test_validate_deploy_path_rejects_cowork_uri_when_resolver_returns_none( + self, tmp_path: Path + ) -> None: + """Even with the cowork prefix in the allowed list, validation + must still reject when the resolver returns None (no OneDrive + available). This preserves the safe-default behaviour. + """ + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ): + result = BaseIntegrator.validate_deploy_path( + "cowork://skills/my-skill/SKILL.md", + tmp_path, + targets=None, + ) + assert result is False + + +class TestPartitionManagedFilesCowork: + """Tests for partition_managed_files with cowork targets.""" + + def test_cowork_skills_go_to_skills_bucket( + self, tmp_path: Path + ) -> None: + cowork_target = _make_cowork_target(tmp_path) + managed = {"cowork://skills/my-skill/SKILL.md"} + result = BaseIntegrator.partition_managed_files( + managed, targets=[cowork_target] + ) + assert "cowork://skills/my-skill/SKILL.md" in result["skills"] + + def test_cowork_entries_absent_from_other_buckets( + self, tmp_path: Path + ) -> None: + cowork_target = _make_cowork_target(tmp_path) + managed = {"cowork://skills/my-skill/SKILL.md"} + result = BaseIntegrator.partition_managed_files( + managed, targets=[cowork_target] + ) + for key, entries in result.items(): + if key != "skills": + assert "cowork://skills/my-skill/SKILL.md" not in entries + + def test_non_cowork_entries_unaffected_in_partitioned_result( + self, tmp_path: Path + ) -> None: + copilot = KNOWN_TARGETS["copilot"] + cowork_target = _make_cowork_target(tmp_path) + managed = { + ".github/prompts/foo.prompt.md", + "cowork://skills/my-skill/SKILL.md", + } + result = BaseIntegrator.partition_managed_files( + managed, targets=[copilot, cowork_target] + ) + assert ".github/prompts/foo.prompt.md" in result["prompts"] + assert "cowork://skills/my-skill/SKILL.md" in result["skills"] + + def test_relative_paths_partitioned_identically_with_cowork_target_present( + self, tmp_path: Path + ) -> None: + copilot = KNOWN_TARGETS["copilot"] + cowork_target = _make_cowork_target(tmp_path) + managed = {".github/prompts/foo.prompt.md"} + result = BaseIntegrator.partition_managed_files( + managed, targets=[copilot, cowork_target] + ) + assert ".github/prompts/foo.prompt.md" in result["prompts"] + + +class TestSyncRemoveFilesCowork: + """Tests for sync_remove_files with cowork:// entries.""" + + def test_cowork_entry_deleted_when_file_exists( + self, tmp_path: Path + ) -> None: + skill_md = tmp_path / "my-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.write_text("# Skill") + cowork_target = _make_cowork_target(tmp_path) + project_root = tmp_path / "project" + project_root.mkdir() + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=tmp_path, + ): + stats = BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/my-skill/SKILL.md"}, + "cowork://", + targets=[cowork_target], + ) + assert not skill_md.exists() + assert stats["files_removed"] == 1 + + def test_stale_cowork_entry_does_not_error( + self, tmp_path: Path + ) -> None: + cowork_target = _make_cowork_target(tmp_path) + project_root = tmp_path / "project" + project_root.mkdir() + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=tmp_path, + ): + stats = BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/nonexistent/SKILL.md"}, + "cowork://", + targets=[cowork_target], + ) + assert stats["files_removed"] == 0 + assert stats["errors"] == 0 + + def test_cowork_entry_skipped_when_resolver_returns_none( + self, tmp_path: Path + ) -> None: + cowork_target = _make_cowork_target(tmp_path) + project_root = tmp_path / "project" + project_root.mkdir() + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ): + stats = BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/my-skill/SKILL.md"}, + "cowork://", + targets=[cowork_target], + ) + assert stats["files_removed"] == 0 + assert stats["errors"] == 0 + + def test_relative_path_entries_unaffected(self, tmp_path: Path) -> None: + target_file = tmp_path / ".github" / "prompts" / "foo.prompt.md" + target_file.parent.mkdir(parents=True) + target_file.write_text("# Prompt") + stats = BaseIntegrator.sync_remove_files( + tmp_path, + {".github/prompts/foo.prompt.md"}, + ".github/prompts/", + ) + assert not target_file.exists() + assert stats["files_removed"] == 1 + + +class TestCleanupEmptyParentsCowork: + """Tests for cleanup_empty_parents with cowork root boundary.""" + + def test_walk_up_stops_at_cowork_root(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-root" + skill_dir = cowork_root / "my-skill" + skill_dir.mkdir(parents=True) + # Simulate file deletion -- dir is now empty + deleted_file = skill_dir / "SKILL.md" + BaseIntegrator.cleanup_empty_parents( + [deleted_file], stop_at=cowork_root + ) + assert not skill_dir.exists(), "empty my-skill/ should be removed" + assert cowork_root.exists(), "cowork_root itself must remain" + + def test_walk_up_does_not_reach_home(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "deep" / "cowork-root" + skill_dir = cowork_root / "my-skill" + skill_dir.mkdir(parents=True) + deleted_file = skill_dir / "SKILL.md" + BaseIntegrator.cleanup_empty_parents( + [deleted_file], stop_at=cowork_root + ) + assert (tmp_path / "deep").exists(), "ancestors above stop_at must survive" + + +# --------------------------------------------------------------------------- +# P2: cowork resolver called at most once per sync_remove_files invocation +# --------------------------------------------------------------------------- + + +class TestSyncRemoveFilesCoworkResolverCalledOnce: + """P2: resolve_copilot_cowork_skills_dir must be invoked at most once + even when multiple cowork:// paths are processed.""" + + def test_resolver_called_once_for_five_cowork_paths( + self, tmp_path: Path + ) -> None: + """With 5 cowork:// entries the resolver is called exactly once + inside sync_remove_files' cowork branch (validate_deploy_path is + stubbed so it doesn't contribute extra calls).""" + cowork_root = tmp_path / "cowork-skills" + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(cowork_root) + + # Create 5 skill files so they exist on disk + paths = set() + for i in range(5): + skill_dir = cowork_root / f"skill-{i}" + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text(f"# Skill {i}") + paths.add(f"cowork://skills/skill-{i}/SKILL.md") + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ) as mock_resolve, patch.object( + BaseIntegrator, "validate_deploy_path", return_value=True, + ): + stats = BaseIntegrator.sync_remove_files( + project_root, + paths, + "cowork://", + targets=[cowork_target], + ) + + mock_resolve.assert_called_once() + assert stats["files_removed"] == 5 + + def test_resolver_called_once_when_returns_none( + self, tmp_path: Path + ) -> None: + """When resolver returns None the call still happens only once.""" + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(tmp_path) + + paths = { + f"cowork://skills/skill-{i}/SKILL.md" for i in range(3) + } + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ) as mock_resolve, patch.object( + BaseIntegrator, "validate_deploy_path", return_value=True, + ): + BaseIntegrator.sync_remove_files( + project_root, + paths, + "cowork://", + targets=[cowork_target], + ) + + mock_resolve.assert_called_once() + + def test_resolver_not_called_without_cowork_paths( + self, tmp_path: Path + ) -> None: + """No cowork:// paths means the resolver is never invoked.""" + project_root = tmp_path / "project" + project_root.mkdir() + (project_root / ".github" / "prompts").mkdir(parents=True) + (project_root / ".github" / "prompts" / "a.prompt.md").write_text("x") + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + ) as mock_resolve: + BaseIntegrator.sync_remove_files( + project_root, + {".github/prompts/a.prompt.md"}, + ".github/prompts/", + ) + + mock_resolve.assert_not_called() + + +# --------------------------------------------------------------------------- +# P4: orphan-visibility diagnostic in sync_remove_files +# --------------------------------------------------------------------------- + + +class TestSyncRemoveFilesOrphanWarning: + """P4: when cowork resolver returns None the function must emit a + one-time warning with the count of skipped orphan entries.""" + + def test_orphan_warning_emitted_with_logger( + self, tmp_path: Path + ) -> None: + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(tmp_path) + logger = MagicMock() + + paths = { + f"cowork://skills/skill-{i}/SKILL.md" for i in range(3) + } + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ), patch.object( + BaseIntegrator, "validate_deploy_path", return_value=True, + ): + BaseIntegrator.sync_remove_files( + project_root, + paths, + "cowork://", + targets=[cowork_target], + logger=logger, + ) + + logger.warning.assert_called_once() + msg = logger.warning.call_args[0][0] + assert "3" in msg + assert "orphaned lockfile" in msg + assert "APM_COPILOT_COWORK_SKILLS_DIR" in msg + assert "apm config set copilot-cowork-skills-dir" in msg + + def test_orphan_warning_singular_for_one_entry( + self, tmp_path: Path + ) -> None: + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(tmp_path) + logger = MagicMock() + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ), patch.object( + BaseIntegrator, "validate_deploy_path", return_value=True, + ): + BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/only-one/SKILL.md"}, + "cowork://", + targets=[cowork_target], + logger=logger, + ) + + logger.warning.assert_called_once() + msg = logger.warning.call_args[0][0] + assert "1 orphaned lockfile entry" in msg + + def test_orphan_warning_fallback_to_rich_warning( + self, tmp_path: Path + ) -> None: + """Without a logger the warning routes through _rich_warning.""" + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(tmp_path) + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ), patch.object( + BaseIntegrator, "validate_deploy_path", return_value=True, + ), patch( + "apm_cli.integration.base_integrator._rich_warning", + ) as mock_warn: + BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/a/SKILL.md", "cowork://skills/b/SKILL.md"}, + "cowork://", + targets=[cowork_target], + ) + + mock_warn.assert_called_once() + msg = mock_warn.call_args[0][0] + assert "2" in msg + assert "orphaned lockfile" in msg + + def test_no_orphan_warning_when_resolver_succeeds( + self, tmp_path: Path + ) -> None: + """No warning emitted when the cowork root resolves successfully.""" + cowork_root = tmp_path / "cowork-skills" + project_root = tmp_path / "project" + project_root.mkdir() + cowork_target = _make_cowork_target(cowork_root) + logger = MagicMock() + + skill_dir = cowork_root / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill") + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + BaseIntegrator.sync_remove_files( + project_root, + {"cowork://skills/my-skill/SKILL.md"}, + "cowork://", + targets=[cowork_target], + logger=logger, + ) + + logger.warning.assert_not_called() + + def test_no_orphan_warning_without_cowork_paths( + self, tmp_path: Path + ) -> None: + """No warning emitted when no cowork:// paths are present.""" + project_root = tmp_path / "project" + project_root.mkdir() + (project_root / ".github" / "prompts").mkdir(parents=True) + (project_root / ".github" / "prompts" / "a.prompt.md").write_text("x") + logger = MagicMock() + + BaseIntegrator.sync_remove_files( + project_root, + {".github/prompts/a.prompt.md"}, + ".github/prompts/", + logger=logger, + ) + + logger.warning.assert_not_called() + diff --git a/tests/unit/integration/test_cleanup_helper.py b/tests/unit/integration/test_cleanup_helper.py index 2cb754625..ecb6b8448 100644 --- a/tests/unit/integration/test_cleanup_helper.py +++ b/tests/unit/integration/test_cleanup_helper.py @@ -355,3 +355,140 @@ def test_result_dataclass_defaults(): assert r.skipped_user_edit == [] assert r.skipped_unmanaged == [] assert r.deleted_targets == [] + + +# --------------------------------------------------------------------------- +# Cowork cleanup tests (PR #926 -- remove_stale_deployed_files) +# --------------------------------------------------------------------------- + + +def test_cowork_stale_entry_deletes_real_file(tmp_path, diagnostics): + """Happy path: cowork:// stale entry with a real file in a temp cowork + root -> file deleted, lockfile entry removed (in result.deleted).""" + from unittest.mock import patch + + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + skill_md = cowork_root / "my-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.write_text("# My Skill\n", encoding="ascii") + assert skill_md.exists() + + project_root = tmp_path / "project" + project_root.mkdir() + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = remove_stale_deployed_files( + ["cowork://skills/my-skill/SKILL.md"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + + assert not skill_md.exists(), "File should have been deleted" + assert "cowork://skills/my-skill/SKILL.md" in result.deleted + assert not result.failed + assert not result.skipped_unmanaged + + +def test_cowork_stale_entry_resolver_returns_none(tmp_path, diagnostics): + """Resolver returns None -> file NOT deleted, lockfile entry retained + in result.failed, one-time warning emitted.""" + from unittest.mock import patch + + project_root = tmp_path / "project" + project_root.mkdir() + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ): + result = remove_stale_deployed_files( + ["cowork://skills/my-skill/SKILL.md"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + + assert result.deleted == [] + assert "cowork://skills/my-skill/SKILL.md" in result.failed + # One-time warning about missing OneDrive path. + msgs = [d.message for d in diagnostics._diagnostics] + assert any("OneDrive path not detected" in m for m in msgs) + assert any("APM_COPILOT_COWORK_SKILLS_DIR" in m for m in msgs) + + +def test_cowork_stale_entry_file_already_gone(tmp_path, diagnostics): + """Cowork root resolves but file is already missing -> lockfile entry + is removed (not in failed), no error. Idempotent cleanup.""" + from unittest.mock import patch + + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + # Do NOT create the skill file -- it is already gone. + + project_root = tmp_path / "project" + project_root.mkdir() + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = remove_stale_deployed_files( + ["cowork://skills/my-skill/SKILL.md"], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + + # Not in deleted (nothing was on disk), not in failed (no error), + # not in skipped_unmanaged (path was valid). + assert result.deleted == [] + assert result.failed == [] + assert result.skipped_unmanaged == [] + + +def test_cowork_stale_entry_from_lockfile_error_retains_in_failed( + tmp_path, diagnostics +): + """from_lockfile_path raises after validation passes -> entry + retained in result.failed, warning emitted.""" + from unittest.mock import patch + + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + + project_root = tmp_path / "project" + project_root.mkdir() + + # Use a valid-looking cowork path so it passes validate_deploy_path. + stale = "cowork://skills/bad-skill/SKILL.md" + + def _boom(_path, _root): + raise ValueError("simulated resolution failure") + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ), patch( + "apm_cli.integration.copilot_cowork_paths.from_lockfile_path", + side_effect=_boom, + ): + result = remove_stale_deployed_files( + [stale], + project_root, + dep_key="pkg", + targets=None, + diagnostics=diagnostics, + ) + + assert result.deleted == [] + assert stale in result.failed + msgs = [d.message for d in diagnostics._diagnostics] + assert any("failed path resolution" in m for m in msgs) diff --git a/tests/unit/integration/test_copilot_cowork_paths.py b/tests/unit/integration/test_copilot_cowork_paths.py new file mode 100644 index 000000000..dd685ec75 --- /dev/null +++ b/tests/unit/integration/test_copilot_cowork_paths.py @@ -0,0 +1,436 @@ +"""Unit tests for apm_cli.integration.copilot_cowork_paths.""" +from __future__ import annotations + +import os +from pathlib import Path +from unittest.mock import patch, MagicMock + +import pytest + +from apm_cli.integration.copilot_cowork_paths import ( + COWORK_LOCKFILE_PREFIX, + COWORK_URI_SCHEME, + CoworkResolutionError, + from_lockfile_path, + is_cowork_path, + resolve_copilot_cowork_skills_dir, + to_lockfile_path, +) +from apm_cli.utils.path_security import PathTraversalError + + +# --------------------------------------------------------------------------- +# TestResolveCoworkSkillsDir +# --------------------------------------------------------------------------- + + +class TestResolveCoworkSkillsDir: + """Tests for resolve_copilot_cowork_skills_dir auto-detection and env override.""" + + def test_env_override_returns_expanded_path( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + target = tmp_path / "my-skills" + target.mkdir() + monkeypatch.setenv("APM_COPILOT_COWORK_SKILLS_DIR", str(target)) + result = resolve_copilot_cowork_skills_dir() + assert isinstance(result, Path) + assert result.name == "my-skills" + + def test_env_override_wins_over_glob( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + target = tmp_path / "env-skills" + target.mkdir() + monkeypatch.setenv("APM_COPILOT_COWORK_SKILLS_DIR", str(target)) + # Even if home has cloud storage dirs, env should win: + cloud = tmp_path / "Library" / "CloudStorage" + (cloud / "OneDrive - TenantA").mkdir(parents=True) + (cloud / "OneDrive - TenantB").mkdir(parents=True) + result = resolve_copilot_cowork_skills_dir() + assert result is not None + assert result.name == "env-skills" + + def test_env_override_traversal_raises( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("APM_COPILOT_COWORK_SKILLS_DIR", "../escape") + with pytest.raises(CoworkResolutionError, match="traversal"): + resolve_copilot_cowork_skills_dir() + + def test_env_override_embedded_traversal_raises( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("APM_COPILOT_COWORK_SKILLS_DIR", "/valid/../invalid") + with pytest.raises(CoworkResolutionError, match="traversal"): + resolve_copilot_cowork_skills_dir() + + def test_macos_single_tenant_returns_skills_dir( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + cloud_dir = tmp_path / "Library" / "CloudStorage" + tenant_dir = cloud_dir / "OneDrive - Tenant" + tenant_dir.mkdir(parents=True) + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + result = resolve_copilot_cowork_skills_dir() + expected = tenant_dir / "Documents" / "Cowork" / "skills" + assert result == expected + + def test_macos_zero_tenant_returns_none( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + cloud_dir = tmp_path / "Library" / "CloudStorage" + cloud_dir.mkdir(parents=True) + # No OneDrive dirs + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + result = resolve_copilot_cowork_skills_dir() + assert result is None + + def test_macos_no_cloud_storage_dir_returns_none( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + # No Library/CloudStorage at all + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + result = resolve_copilot_cowork_skills_dir() + assert result is None + + def test_macos_multi_tenant_raises_cowork_resolution_error( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + cloud_dir = tmp_path / "Library" / "CloudStorage" + (cloud_dir / "OneDrive - TenantA").mkdir(parents=True) + (cloud_dir / "OneDrive - TenantB").mkdir(parents=True) + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + with pytest.raises(CoworkResolutionError): + resolve_copilot_cowork_skills_dir() + + def test_multi_tenant_error_message_lists_candidates( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + cloud_dir = tmp_path / "Library" / "CloudStorage" + (cloud_dir / "OneDrive - TenantA").mkdir(parents=True) + (cloud_dir / "OneDrive - TenantB").mkdir(parents=True) + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + with pytest.raises(CoworkResolutionError) as exc_info: + resolve_copilot_cowork_skills_dir() + msg = str(exc_info.value) + assert "TenantA" in msg + assert "TenantB" in msg + + def test_multi_tenant_error_message_hint_contains_env_var_name( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + cloud_dir = tmp_path / "Library" / "CloudStorage" + (cloud_dir / "OneDrive - TenantA").mkdir(parents=True) + (cloud_dir / "OneDrive - TenantB").mkdir(parents=True) + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + with pytest.raises(CoworkResolutionError) as exc_info: + resolve_copilot_cowork_skills_dir() + assert "APM_COPILOT_COWORK_SKILLS_DIR" in str(exc_info.value) + + def test_windows_env_var_returns_path( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv( + "APM_COPILOT_COWORK_SKILLS_DIR", "/tmp/fake-onedrive/skills" + ) + result = resolve_copilot_cowork_skills_dir() + assert isinstance(result, Path) + + def test_linux_no_env_returns_none( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + with patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ): + result = resolve_copilot_cowork_skills_dir() + assert result is None + + # ----------------------------------------------------------------------- + # Resolution precedence: config layer + # ----------------------------------------------------------------------- + + def test_config_beats_macos_auto_detect( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Config value is used instead of macOS auto-detection when env is unset.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + # Set up a cloud storage directory that auto-detect would find. + cloud = tmp_path / "Library" / "CloudStorage" + (cloud / "OneDrive - Tenant").mkdir(parents=True) + with ( + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value="/config/skills"), + patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ), + ): + result = resolve_copilot_cowork_skills_dir() + # Config path should win over auto-detected tenant directory. + assert result == Path("/config/skills").expanduser().resolve() + + def test_env_beats_config_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Env var takes precedence over the persisted config value.""" + monkeypatch.setenv("APM_COPILOT_COWORK_SKILLS_DIR", "/env/override/skills") + with patch("apm_cli.config.get_copilot_cowork_skills_dir") as mock_get_cfg: + result = resolve_copilot_cowork_skills_dir() + # Config should not be consulted when the env var is present. + mock_get_cfg.assert_not_called() + assert result == Path("/env/override/skills").expanduser().resolve() + + def test_auto_detect_used_when_both_env_and_config_absent( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Falls through to macOS auto-detection when env and config are both absent.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + tenant = tmp_path / "Library" / "CloudStorage" / "OneDrive - EPAM" + tenant.mkdir(parents=True) + with ( + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ), + ): + result = resolve_copilot_cowork_skills_dir() + assert result == tenant / "Documents" / "Cowork" / "skills" + + def test_config_path_traversal_raises_cowork_resolution_error( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A traversal sequence in the config value raises CoworkResolutionError.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + with patch( + "apm_cli.config.get_copilot_cowork_skills_dir", + return_value="/valid/../invalid", + ): + with pytest.raises(CoworkResolutionError, match="traversal"): + resolve_copilot_cowork_skills_dir() + + def test_config_none_falls_through_cleanly_to_next_branch( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None from config is silently skipped; no exception is raised.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + # No CloudStorage directory -- auto-detect returns None. + with ( + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + patch( + "apm_cli.integration.copilot_cowork_paths.Path.home", + return_value=tmp_path, + ), + ): + result = resolve_copilot_cowork_skills_dir() + assert result is None + + # ----------------------------------------------------------------------- + # Windows auto-detection + # ----------------------------------------------------------------------- + + def test_windows_onedrivecommercial_autodetect( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """ONEDRIVECOMMERCIAL is used first on Windows.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + od_root = tmp_path / "OneDrive - Contoso" + od_root.mkdir() + monkeypatch.setenv("ONEDRIVECOMMERCIAL", str(od_root)) + monkeypatch.delenv("ONEDRIVE", raising=False) + with ( + patch("apm_cli.integration.copilot_cowork_paths.sys.platform", "win32"), + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + ): + result = resolve_copilot_cowork_skills_dir() + expected = (od_root / "Documents" / "Cowork" / "skills").resolve() + assert result == expected + + def test_windows_onedrive_fallback( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """ONEDRIVE is used when ONEDRIVECOMMERCIAL is absent.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + od_root = tmp_path / "OneDrive" + od_root.mkdir() + monkeypatch.delenv("ONEDRIVECOMMERCIAL", raising=False) + monkeypatch.setenv("ONEDRIVE", str(od_root)) + with ( + patch("apm_cli.integration.copilot_cowork_paths.sys.platform", "win32"), + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + ): + result = resolve_copilot_cowork_skills_dir() + expected = (od_root / "Documents" / "Cowork" / "skills").resolve() + assert result == expected + + def test_windows_neither_env_returns_none( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Neither ONEDRIVECOMMERCIAL nor ONEDRIVE set returns None.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + monkeypatch.delenv("ONEDRIVECOMMERCIAL", raising=False) + monkeypatch.delenv("ONEDRIVE", raising=False) + with ( + patch("apm_cli.integration.copilot_cowork_paths.sys.platform", "win32"), + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + ): + result = resolve_copilot_cowork_skills_dir() + assert result is None + + def test_windows_onedrivecommercial_empty_falls_through( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Empty ONEDRIVECOMMERCIAL falls through to ONEDRIVE.""" + monkeypatch.delenv("APM_COPILOT_COWORK_SKILLS_DIR", raising=False) + od_root = tmp_path / "OneDrive" + od_root.mkdir() + monkeypatch.setenv("ONEDRIVECOMMERCIAL", "") + monkeypatch.setenv("ONEDRIVE", str(od_root)) + with ( + patch("apm_cli.integration.copilot_cowork_paths.sys.platform", "win32"), + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None), + ): + result = resolve_copilot_cowork_skills_dir() + expected = (od_root / "Documents" / "Cowork" / "skills").resolve() + assert result == expected + + +# --------------------------------------------------------------------------- +# TestToLockfilePath +# --------------------------------------------------------------------------- + + +class TestToLockfilePath: + """Tests for to_lockfile_path encoding.""" + + def test_round_trip_absolute_macos_path(self, tmp_path: Path) -> None: + skill_md = tmp_path / "my-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + result = to_lockfile_path(skill_md, tmp_path) + assert result == "cowork://skills/my-skill/SKILL.md" + + def test_round_trip_path_with_spaces(self, tmp_path: Path) -> None: + skill_md = tmp_path / "my skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + result = to_lockfile_path(skill_md, tmp_path) + assert "my skill" in result + assert result.startswith("cowork://") + + def test_escape_attempt_raises_path_traversal_error( + self, tmp_path: Path + ) -> None: + outside = tmp_path.parent / "outside.md" + with pytest.raises(PathTraversalError): + to_lockfile_path(outside, tmp_path) + + def test_result_starts_with_cowork_scheme(self, tmp_path: Path) -> None: + skill_md = tmp_path / "foo" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + result = to_lockfile_path(skill_md, tmp_path) + assert result.startswith("cowork://") + + +# --------------------------------------------------------------------------- +# TestFromLockfilePath +# --------------------------------------------------------------------------- + + +class TestFromLockfilePath: + """Tests for from_lockfile_path decoding.""" + + def test_decode_skills_prefix(self, tmp_path: Path) -> None: + expected = tmp_path / "my-skill" / "SKILL.md" + expected.parent.mkdir(parents=True) + expected.touch() + result = from_lockfile_path( + "cowork://skills/my-skill/SKILL.md", tmp_path + ) + assert result == expected.resolve() + + def test_round_trip_macos_path(self, tmp_path: Path) -> None: + skill_md = tmp_path / "round-trip-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + encoded = to_lockfile_path(skill_md, tmp_path) + decoded = from_lockfile_path(encoded, tmp_path) + assert decoded == skill_md.resolve() + + def test_round_trip_posix_on_windows_style(self, tmp_path: Path) -> None: + skill_md = tmp_path / "win-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.touch() + encoded = to_lockfile_path(skill_md, tmp_path) + decoded = from_lockfile_path(encoded, tmp_path) + assert decoded.as_posix().endswith("win-skill/SKILL.md") + + def test_traversal_rejected(self, tmp_path: Path) -> None: + with pytest.raises((PathTraversalError, CoworkResolutionError)): + from_lockfile_path( + "cowork://skills/../../etc/passwd", tmp_path + ) + + def test_non_cowork_uri_raises_value_error(self, tmp_path: Path) -> None: + with pytest.raises(ValueError, match="Not a cowork lockfile path"): + from_lockfile_path("relative/path.md", tmp_path) + + def test_traversal_via_url_encoding_rejected( + self, tmp_path: Path + ) -> None: + # URL-encoded ".." (%2e%2e) -- the implementation does NOT decode + # URL-encoded sequences so the literal "%2e%2e" segment is not ".." + # and is not rejected by validate_path_segments. Document current + # behavior: it returns a path (no exception). + result = from_lockfile_path( + "cowork://skills/%2e%2e/etc/passwd", tmp_path + ) + # Current behavior: the literal %2e%2e is treated as a dir name. + assert isinstance(result, Path) + # NOTE: potential security gap -- URL-encoded traversal sequences + # are not decoded/rejected. Reported as implementation observation. + + +# --------------------------------------------------------------------------- +# TestIsCoworkPath +# --------------------------------------------------------------------------- + + +class TestIsCoworkPath: + """Tests for is_cowork_path predicate.""" + + def test_cowork_uri_returns_true(self) -> None: + assert is_cowork_path("cowork://skills/foo/SKILL.md") is True + + def test_relative_path_returns_false(self) -> None: + assert is_cowork_path("relative/path.md") is False + + def test_empty_string_returns_false(self) -> None: + assert is_cowork_path("") is False diff --git a/tests/unit/integration/test_copilot_cowork_target.py b/tests/unit/integration/test_copilot_cowork_target.py new file mode 100644 index 000000000..4f2747ce3 --- /dev/null +++ b/tests/unit/integration/test_copilot_cowork_target.py @@ -0,0 +1,572 @@ +"""Unit tests for cowork target gating in apm_cli.integration.targets.""" +from __future__ import annotations + +from dataclasses import FrozenInstanceError, replace +from pathlib import Path +from typing import Any, Dict +from unittest.mock import patch, MagicMock + +import pytest + +from apm_cli.integration.targets import ( + KNOWN_TARGETS, + TargetProfile, + active_targets, + active_targets_user_scope, + get_integration_prefixes, + resolve_targets, +) + + +# --------------------------------------------------------------------------- +# Shared fixtures (same pattern as test_experimental.py) +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def _reset_config_cache(): + """Reset the in-process config cache before and after every test.""" + from apm_cli.config import _invalidate_config_cache + + _invalidate_config_cache() + yield + _invalidate_config_cache() + + +@pytest.fixture +def inject_config(monkeypatch: pytest.MonkeyPatch): + """Directly inject a dict into the config cache -- no disk I/O.""" + import apm_cli.config as _conf + + def _set(cfg: Dict[str, Any]) -> None: + monkeypatch.setattr(_conf, "_config_cache", cfg) + + return _set + + +# --------------------------------------------------------------------------- +# TestTargetProfileForScope +# --------------------------------------------------------------------------- + + +class TestTargetProfileForScope: + """Tests for TargetProfile.for_scope().""" + + def test_for_scope_false_returns_self(self) -> None: + profile = KNOWN_TARGETS["copilot"] + result = profile.for_scope(user_scope=False) + assert result is profile + + def test_for_scope_user_scope_resolver_returns_path( + self, tmp_path: Path + ) -> None: + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=tmp_path, + ): + result = KNOWN_TARGETS["copilot-cowork"].for_scope(user_scope=True) + assert result is not None + assert result.resolved_deploy_root == tmp_path + + def test_for_scope_user_scope_resolver_returns_none(self) -> None: + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ): + result = KNOWN_TARGETS["copilot-cowork"].for_scope(user_scope=True) + assert result is None + + def test_for_scope_result_is_frozen(self, tmp_path: Path) -> None: + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=tmp_path, + ): + result = KNOWN_TARGETS["copilot-cowork"].for_scope(user_scope=True) + assert result is not None + with pytest.raises(FrozenInstanceError): + result.name = "changed" # type: ignore[misc] + + def test_for_scope_non_resolver_user_supported_returns_profile( + self, + ) -> None: + copilot = KNOWN_TARGETS["copilot"] + result = copilot.for_scope(user_scope=True) + assert result is not None + assert result.name == "copilot" + + def test_for_scope_non_resolver_user_unsupported_returns_none( + self, + ) -> None: + codex = KNOWN_TARGETS["codex"] + result = codex.for_scope(user_scope=True) + assert result is None + + +# --------------------------------------------------------------------------- +# TestDeployPath +# --------------------------------------------------------------------------- + + +class TestDeployPath: + """Tests for TargetProfile.deploy_path().""" + + def test_deploy_path_with_resolved_root_and_parts( + self, tmp_path: Path + ) -> None: + cowork = replace( + KNOWN_TARGETS["copilot-cowork"], + resolved_deploy_root=tmp_path, + ) + result = cowork.deploy_path(Path("/unused"), "sub", "file.md") + assert result == tmp_path / "sub" / "file.md" + + def test_deploy_path_with_resolved_root_no_parts( + self, tmp_path: Path + ) -> None: + cowork = replace( + KNOWN_TARGETS["copilot-cowork"], + resolved_deploy_root=tmp_path, + ) + result = cowork.deploy_path(Path("/unused")) + assert result == tmp_path + + def test_deploy_path_without_resolved_root_uses_project( + self, tmp_path: Path + ) -> None: + copilot = KNOWN_TARGETS["copilot"] + result = copilot.deploy_path(tmp_path) + assert result == tmp_path / ".github" + + +# --------------------------------------------------------------------------- +# TestActiveTargetsGating +# --------------------------------------------------------------------------- + + +class TestActiveTargetsGating: + """Tests for cowork gating in active_targets / resolve_targets.""" + + def test_cowork_absent_when_flag_off_auto_detect( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + (tmp_path / "copilot-cowork").mkdir() + results = active_targets(tmp_path) + names = [t.name for t in results] + assert "copilot-cowork" not in names + + def test_cowork_absent_when_flag_off_explicit_cowork( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + results = active_targets(tmp_path, explicit_target="copilot-cowork") + assert results == [] + + def test_cowork_absent_from_all_when_flag_off( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + results = active_targets(tmp_path, explicit_target="all") + names = [t.name for t in results] + # "all" returns all targets regardless of flag gating + # but explicit_target="copilot-cowork" with flag off returns [] + # The "all" path returns list(KNOWN_TARGETS.values()) which + # includes cowork. This is documented: "all" bypasses flag gate. + # So cowork IS in the "all" set even when flag is off. + # This matches the implementation comment: + # "Return all targets regardless of flag gating." + assert "copilot-cowork" in names + + def test_cowork_absent_when_flag_on_resolver_returns_none( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ): + results = resolve_targets( + tmp_path, + user_scope=True, + explicit_target="copilot-cowork", + ) + names = [t.name for t in results] + assert "copilot-cowork" not in names + + def test_cowork_never_auto_detected( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + (tmp_path / "copilot-cowork").mkdir() + results = active_targets(tmp_path) + names = [t.name for t in results] + assert "copilot-cowork" not in names + + def test_cowork_present_when_flag_on_explicit( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + results = active_targets(tmp_path, explicit_target="copilot-cowork") + assert len(results) == 1 + assert results[0].name == "copilot-cowork" + + def test_all_user_scope_includes_cowork_when_flag_on_resolver_succeeds( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": True}}) + user_profiles = active_targets_user_scope(explicit_target="all") + names = [t.name for t in user_profiles] + assert "copilot-cowork" in names + # Now resolve via resolve_targets with resolver returning a path + with patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=tmp_path, + ): + resolved = resolve_targets( + tmp_path, + user_scope=True, + explicit_target="all", + ) + resolved_names = [t.name for t in resolved] + assert "copilot-cowork" in resolved_names + + def test_all_user_scope_excludes_cowork_when_flag_off( + self, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + results = active_targets_user_scope(explicit_target="all") + names = [t.name for t in results] + assert "copilot-cowork" not in names + + def test_other_targets_unaffected_when_flag_off( + self, tmp_path: Path, inject_config: Any + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + results = active_targets(tmp_path) + names = [t.name for t in results] + assert "copilot" in names + + @pytest.mark.parametrize( + "target_name", + ["copilot", "claude", "cursor", "codex", "opencode"], + ) + def test_existing_target_active_targets_unchanged_when_cowork_flag_off( + self, + target_name: str, + tmp_path: Path, + inject_config: Any, + ) -> None: + inject_config({"experimental": {"copilot_cowork": False}}) + assert target_name in KNOWN_TARGETS + + +# --------------------------------------------------------------------------- +# TestGetIntegrationPrefixes +# --------------------------------------------------------------------------- + + +class TestGetIntegrationPrefixes: + """Tests for get_integration_prefixes with cowork targets.""" + + def test_cowork_prefix_present_when_resolved_root_set( + self, tmp_path: Path + ) -> None: + cowork = replace( + KNOWN_TARGETS["copilot-cowork"], + resolved_deploy_root=tmp_path, + ) + prefixes = get_integration_prefixes([cowork]) + assert "cowork://skills/" in prefixes + + def test_cowork_prefix_absent_when_no_resolved_root(self) -> None: + copilot = KNOWN_TARGETS["copilot"] + prefixes = get_integration_prefixes([copilot]) + assert all( + not p.startswith("cowork://") for p in prefixes + ) + + def test_standard_prefixes_unchanged_when_cowork_absent(self) -> None: + copilot = KNOWN_TARGETS["copilot"] + prefixes = get_integration_prefixes([copilot]) + assert ".github/" in prefixes + + # -- Regression tests for cleanup with targets=None (PR #926) ---------- + + def test_get_integration_prefixes_includes_cowork_with_targets_none( + self, + ) -> None: + """When targets=None, KNOWN_TARGETS is iterated. The static + copilot-cowork entry has resolved_deploy_root=None but DOES have + a user_root_resolver. The cowork prefix must be included so + cleanup/uninstall can validate cowork:// lockfile entries. + """ + prefixes = get_integration_prefixes(targets=None) + assert "cowork://skills/" in prefixes + + def test_get_integration_prefixes_includes_cowork_with_explicit_static_targets( + self, + ) -> None: + """Passing the static KNOWN_TARGETS['copilot-cowork'] instance + (resolved_deploy_root=None, user_root_resolver is set) must + include the cowork prefix -- same scenario as targets=None but + with an explicit list containing only the static entry. + """ + static_cowork = KNOWN_TARGETS["copilot-cowork"] + # Confirm this is the unresolved static instance. + assert static_cowork.resolved_deploy_root is None + assert static_cowork.user_root_resolver is not None + prefixes = get_integration_prefixes([static_cowork]) + assert "cowork://skills/" in prefixes + + def test_get_integration_prefixes_resolved_target_still_works( + self, tmp_path: Path + ) -> None: + """A fully-resolved per-install target (resolved_deploy_root set) + must still produce the cowork prefix -- regression guard for the + normal install path. + """ + resolved_cowork = replace( + KNOWN_TARGETS["copilot-cowork"], + resolved_deploy_root=tmp_path, + ) + prefixes = get_integration_prefixes([resolved_cowork]) + assert "cowork://skills/" in prefixes + + +# --------------------------------------------------------------------------- +# TestExplicitCoworkFlagOff (Fix 2) +# --------------------------------------------------------------------------- + + +class TestExplicitCoworkFlagOff: + """When the user explicitly requests --target copilot-cowork and the flag is OFF, + the targets phase must emit an info hint and be a no-op.""" + + def test_user_scope_explicit_cowork_flag_off_is_noop( + self, tmp_path: Path, inject_config: Any + ) -> None: + """User-scope + explicit cowork + flag OFF -> info hint, no error.""" + inject_config({"experimental": {"copilot_cowork": False}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = "copilot-cowork" + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with patch("apm_cli.core.target_detection.detect_target"): + run(ctx) # Should not raise + + hint_msg = ctx.logger.progress.call_args[0][0] + assert "experimental flag" in hint_msg + assert "apm experimental enable copilot-cowork" in hint_msg + + def test_project_scope_explicit_cowork_flag_off_is_noop( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Project-scope + explicit cowork + flag OFF -> info hint, no error.""" + inject_config({"experimental": {"copilot_cowork": False}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.PROJECT + ctx.target_override = "copilot-cowork" + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with patch("apm_cli.core.target_detection.detect_target"): + run(ctx) # Should not raise + + hint_msg = ctx.logger.progress.call_args[0][0] + assert "experimental flag" in hint_msg + + def test_auto_detect_silent_when_flag_off( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Auto-detect path (no explicit target) stays silent when flag OFF.""" + inject_config({"experimental": {"copilot_cowork": False}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = None + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with patch("apm_cli.core.target_detection.detect_target"): + run(ctx) # Should not raise + + # logger.error should NOT have been called with cowork-related message + for c in ctx.logger.error.call_args_list: + assert "cowork" not in str(c).lower() + + def test_multi_target_cowork_copilot_flag_off_copilot_proceeds( + self, tmp_path: Path, inject_config: Any + ) -> None: + """cowork + copilot targets, flag OFF: cowork dropped, copilot proceeds.""" + inject_config({"experimental": {"copilot_cowork": False}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = ["copilot-cowork", "copilot"] + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + copilot = KNOWN_TARGETS["copilot"].for_scope(user_scope=True) + with ( + patch( + "apm_cli.integration.targets.resolve_targets", + return_value=[copilot], + ), + patch("apm_cli.core.target_detection.detect_target"), + ): + run(ctx) # Should not raise + + # Cowork hint was logged + hint_calls = [ + c for c in ctx.logger.progress.call_args_list + if "experimental flag" in str(c) + ] + assert len(hint_calls) == 1 + # Copilot target proceeds + assert any(t.name == "copilot" for t in ctx.targets) + + +# --------------------------------------------------------------------------- +# TestExplicitCoworkUnresolvable (Fix 3) +# --------------------------------------------------------------------------- + + +class TestExplicitCoworkUnresolvable: + """When the user explicitly requests --target copilot-cowork, flag is ON, but + OneDrive path cannot be resolved, the targets phase must error.""" + + def test_linux_flag_on_explicit_cowork_no_env_no_config_errors( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Linux + flag ON + explicit cowork + no env + no config -> error.""" + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = "copilot-cowork" + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with ( + patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ), + patch("apm_cli.core.target_detection.detect_target"), + ): + with pytest.raises(SystemExit) as exc_info: + run(ctx) + assert exc_info.value.code == 1 + + error_msg = ctx.logger.error.call_args[0][0] + # Linux emits "Cowork has no auto-detection on Linux." while macOS + # emits "no OneDrive path detected" — accept either variant. + assert ( + "no OneDrive path detected" in error_msg + or "Cowork has no auto-detection on Linux" in error_msg + ), f"Expected cowork resolver error in output. Got: {error_msg}" + assert "APM_COPILOT_COWORK_SKILLS_DIR" in error_msg + + def test_linux_flag_on_explicit_cowork_env_set_succeeds( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Linux + flag ON + explicit cowork + env var set -> success.""" + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = "copilot-cowork" + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with ( + patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=cowork_root, + ), + patch("apm_cli.core.target_detection.detect_target"), + ): + run(ctx) # Should not raise + + def test_linux_flag_off_explicit_cowork_hint_message( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Linux + flag OFF + explicit cowork -> info hint (not error).""" + inject_config({"experimental": {"copilot_cowork": False}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = "copilot-cowork" + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with patch("apm_cli.core.target_detection.detect_target"): + run(ctx) # Should not raise + + # Should be the flag hint, not an error + hint_msg = ctx.logger.progress.call_args[0][0] + assert "experimental flag" in hint_msg + assert "OneDrive" not in hint_msg + + def test_auto_detect_flag_on_no_resolution_silent( + self, tmp_path: Path, inject_config: Any + ) -> None: + """Auto-detect + flag ON + no resolution -> still silent.""" + inject_config({"experimental": {"copilot_cowork": True}}) + from apm_cli.install.phases.targets import run + from apm_cli.core.scope import InstallScope + + ctx = MagicMock() + ctx.project_root = tmp_path + ctx.scope = InstallScope.USER + ctx.target_override = None + ctx.apm_package = MagicMock() + ctx.apm_package.target = None + ctx.logger = MagicMock() + + with ( + patch( + "apm_cli.integration.targets._resolve_copilot_cowork_root", + return_value=None, + ), + patch("apm_cli.core.target_detection.detect_target"), + ): + run(ctx) # Should not raise + + # No error about cowork + for c in ctx.logger.error.call_args_list: + assert "cowork" not in str(c).lower() diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index 4643bc7ed..2fd07f24d 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -3619,3 +3619,223 @@ def test_copy_skill_to_target_fallback_without_targets(self): assert len(deployed) == 1 assert (self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md").exists() + + +# --------------------------------------------------------------------------- +# Cowork additive tests +# --------------------------------------------------------------------------- + +from dataclasses import replace as _dc_replace +from unittest.mock import MagicMock +from apm_cli.integration.targets import KNOWN_TARGETS + + +def _make_resolved_cowork_target(cowork_root: Path) -> "TargetProfile": + """Return a frozen TargetProfile with resolved_deploy_root set for cowork. + + Args: + cowork_root: The resolved cowork skills root directory. + + Returns: + A frozen TargetProfile suitable for cowork deployment tests. + """ + from apm_cli.integration.targets import TargetProfile + return _dc_replace(KNOWN_TARGETS["copilot-cowork"], resolved_deploy_root=cowork_root) + + +def _make_package_info(install_path: Path) -> MagicMock: + """Create a minimal PackageInfo mock for skill integration tests. + + Args: + install_path: The package install directory. + + Returns: + A MagicMock configured as a PackageInfo. + """ + pkg = MagicMock() + pkg.install_path = install_path + pkg.dependency_ref = None + pkg.content_type = PackageContentType.SKILL + pkg.apm_yml = {} + pkg.package_type = PackageType.CLAUDE_SKILL + pkg.package = MagicMock() + pkg.package.name = install_path.name + return pkg + + +class TestIntegrateNativeSkillCowork: + """Tests for _integrate_native_skill with cowork target.""" + + def test_deploys_to_resolved_deploy_root(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "my-skill" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("# My Skill") + + pkg_info = _make_package_info(pkg_dir) + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + with patch.object( + integrator, "_build_ownership_maps", return_value=({}, {}) + ): + result = integrator._integrate_native_skill( + pkg_info, + project_root, + pkg_dir / "SKILL.md", + targets=[cowork_target], + ) + deployed_skill = cowork_root / "my-skill" / "SKILL.md" + assert deployed_skill.exists() + + def test_does_not_deploy_under_project_root( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "my-skill" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("# My Skill") + + pkg_info = _make_package_info(pkg_dir) + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + with patch.object( + integrator, "_build_ownership_maps", return_value=({}, {}) + ): + integrator._integrate_native_skill( + pkg_info, + project_root, + pkg_dir / "SKILL.md", + targets=[cowork_target], + ) + assert not (project_root / "copilot-cowork").exists() + + def test_result_target_paths_contain_absolute_path( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "my-skill" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("# My Skill") + + pkg_info = _make_package_info(pkg_dir) + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + with patch.object( + integrator, "_build_ownership_maps", return_value=({}, {}) + ): + result = integrator._integrate_native_skill( + pkg_info, + project_root, + pkg_dir / "SKILL.md", + targets=[cowork_target], + ) + assert any(p.is_absolute() for p in result.target_paths) + assert any( + str(p).startswith(str(cowork_root)) + for p in result.target_paths + ) + + +class TestPromoteSubSkillsCowork: + """Tests for sub-skill promotion with cowork target.""" + + def test_promote_sub_skills_deploys_to_cowork_root( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "parent-pkg" + sub_skill = pkg_dir / ".apm" / "skills" / "my-sub" + sub_skill.mkdir(parents=True) + (sub_skill / "SKILL.md").write_text("# Sub Skill") + + pkg_info = _make_package_info(pkg_dir) + # Package without root SKILL.md -> INSTRUCTIONS type + pkg_info.package_type = PackageType.APM_PACKAGE + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + count, deployed = integrator._promote_sub_skills_standalone( + pkg_info, + project_root, + targets=[cowork_target], + ) + assert count >= 1 + assert (cowork_root / "my-sub" / "SKILL.md").exists() + + def test_promote_sub_skills_rel_prefix_no_relative_to_crash( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "parent-pkg" + sub_skill = pkg_dir / ".apm" / "skills" / "my-sub" + sub_skill.mkdir(parents=True) + (sub_skill / "SKILL.md").write_text("# Sub Skill") + + pkg_info = _make_package_info(pkg_dir) + pkg_info.package_type = PackageType.APM_PACKAGE + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + # Should NOT raise ValueError from relative_to + integrator = SkillIntegrator() + count, deployed = integrator._promote_sub_skills_standalone( + pkg_info, + project_root, + targets=[cowork_target], + ) + assert count >= 1 + + def test_skill_only_agents_skipped_on_cowork( + self, tmp_path: Path + ) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + pkg_dir = tmp_path / "src" / "my-skill" + pkg_dir.mkdir(parents=True) + (pkg_dir / "SKILL.md").write_text("# My Skill") + agents_dir = pkg_dir / ".apm" / "agents" + agents_dir.mkdir(parents=True) + (agents_dir / "foo.agent.md").write_text("# Agent") + + pkg_info = _make_package_info(pkg_dir) + cowork_target = _make_resolved_cowork_target(cowork_root) + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + with patch.object( + integrator, "_build_ownership_maps", return_value=({}, {}) + ): + result = integrator._integrate_native_skill( + pkg_info, + project_root, + pkg_dir / "SKILL.md", + targets=[cowork_target], + ) + deployed_skill = cowork_root / "my-skill" / "SKILL.md" + assert deployed_skill.exists() + # .apm dir is excluded via shutil.ignore_patterns('.apm') + assert not (cowork_root / "my-skill" / ".apm").exists() diff --git a/tests/unit/integration/test_skill_integrator_cowork.py b/tests/unit/integration/test_skill_integrator_cowork.py new file mode 100644 index 000000000..43ac2c767 --- /dev/null +++ b/tests/unit/integration/test_skill_integrator_cowork.py @@ -0,0 +1,287 @@ +"""Unit tests for SkillIntegrator.sync_integration handling cowork:// entries. + +Covers the fix for PR #926: cowork://skills/... lockfile entries were silently +skipped during uninstall because sync_integration's prefix tuple only included +local directory prefixes (e.g. .github/skills/) and never matched the +cowork:// URI scheme. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch, MagicMock + +import pytest + +from apm_cli.integration.skill_integrator import SkillIntegrator + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_cowork_target_profile(): + """Return a minimal TargetProfile that mimics the copilot-cowork target.""" + from apm_cli.integration.targets import TargetProfile, PrimitiveMapping + return TargetProfile( + name="copilot-cowork", + root_dir="copilot-cowork", + primitives={ + "skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard"), + }, + auto_create=False, + detect_by_dir=False, + user_supported=True, + user_root_resolver=lambda: None, # will be patched per-test + ) + + +def _make_copilot_target_profile(project_root: Path): + """Return a minimal copilot TargetProfile (local, non-cowork).""" + from apm_cli.integration.targets import TargetProfile, PrimitiveMapping + return TargetProfile( + name="copilot", + root_dir=".github", + primitives={ + "skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard"), + }, + ) + + +def _stub_apm_package(): + """Return a minimal APMPackage mock for sync_integration.""" + pkg = MagicMock() + pkg.get_apm_dependencies.return_value = [] + return pkg + + +# --------------------------------------------------------------------------- +# Tests: cowork entry in managed_files -> file/dir deleted +# --------------------------------------------------------------------------- + + +class TestSyncIntegrationCoworkDeletion: + """sync_integration must resolve cowork:// entries and delete them.""" + + def test_cowork_skill_directory_deleted(self, tmp_path: Path) -> None: + """A cowork://skills/foo entry pointing to a directory is rmtree'd.""" + cowork_root = tmp_path / "cowork-skills" + skill_dir = cowork_root / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Foo\n", encoding="ascii") + (skill_dir / "extra.md").write_text("ref\n", encoding="ascii") + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + targets = [_make_cowork_target_profile()] + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files={"cowork://skills/foo"}, + targets=targets, + ) + + assert not skill_dir.exists(), ( + "Cowork skill directory still exists after sync_integration." + ) + assert result["files_removed"] == 1 + assert result["errors"] == 0 + + def test_cowork_skill_file_deleted(self, tmp_path: Path) -> None: + """A cowork://skills/bar/SKILL.md entry pointing to a file is unlinked.""" + cowork_root = tmp_path / "cowork-skills" + skill_dir = cowork_root / "bar" + skill_dir.mkdir(parents=True) + skill_md = skill_dir / "SKILL.md" + skill_md.write_text("# Bar\n", encoding="ascii") + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + targets = [_make_cowork_target_profile()] + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files={"cowork://skills/bar/SKILL.md"}, + targets=targets, + ) + + assert not skill_md.exists(), ( + "Cowork SKILL.md still exists after sync_integration." + ) + assert result["files_removed"] == 1 + assert result["errors"] == 0 + + +# --------------------------------------------------------------------------- +# Tests: cowork resolver returns None -> graceful skip + warning +# --------------------------------------------------------------------------- + + +class TestSyncIntegrationCoworkResolverNone: + """When cowork root resolver returns None, entries are skipped with a warning.""" + + def test_resolver_none_skips_entry_and_warns(self, tmp_path: Path) -> None: + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + targets = [_make_cowork_target_profile()] + + with ( + patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=None, + ), + patch( + "apm_cli.utils.console._rich_warning", + ) as mock_warn, + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files={"cowork://skills/baz"}, + targets=targets, + ) + + # Entry is skipped, not an error. + assert result["files_removed"] == 0 + assert result["errors"] == 0 + + # A one-time warning must have been emitted. + mock_warn.assert_called_once() + warn_msg = mock_warn.call_args[0][0] + assert "OneDrive" in warn_msg or "cowork" in warn_msg.lower() + + +# --------------------------------------------------------------------------- +# Tests: translation error -> graceful skip (counted as error) +# --------------------------------------------------------------------------- + + +class TestSyncIntegrationCoworkTranslationError: + """When from_lockfile_path raises, the entry is skipped and counted as error.""" + + def test_translation_error_skips_entry(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + targets = [_make_cowork_target_profile()] + + with ( + patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ), + patch( + "apm_cli.integration.copilot_cowork_paths.from_lockfile_path", + side_effect=ValueError("bad path"), + ), + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files={"cowork://skills/malformed-entry"}, + targets=targets, + ) + + assert result["files_removed"] == 0 + assert result["errors"] == 1 + + +# --------------------------------------------------------------------------- +# Tests: idempotent -- missing file = success +# --------------------------------------------------------------------------- + + +class TestSyncIntegrationCoworkIdempotent: + """If the cowork file/dir is already gone, sync_integration succeeds silently.""" + + def test_missing_cowork_entry_is_noop(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_root.mkdir() + # No skill directory on disk -- it's already been removed. + + project_root = tmp_path / "project" + project_root.mkdir() + + integrator = SkillIntegrator() + targets = [_make_cowork_target_profile()] + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files={"cowork://skills/already-gone"}, + targets=targets, + ) + + assert result["files_removed"] == 0 + assert result["errors"] == 0 + + +# --------------------------------------------------------------------------- +# Tests: mixed cowork + local entries +# --------------------------------------------------------------------------- + + +class TestSyncIntegrationMixed: + """Both cowork:// and local entries are handled in one call.""" + + def test_mixed_entries_all_deleted(self, tmp_path: Path) -> None: + cowork_root = tmp_path / "cowork-skills" + cowork_skill = cowork_root / "remote-skill" + cowork_skill.mkdir(parents=True) + (cowork_skill / "SKILL.md").write_text("# Remote\n", encoding="ascii") + + project_root = tmp_path / "project" + local_skill = project_root / ".github" / "skills" / "local-skill" + local_skill.mkdir(parents=True) + (local_skill / "SKILL.md").write_text("# Local\n", encoding="ascii") + + integrator = SkillIntegrator() + targets = [ + _make_copilot_target_profile(project_root), + _make_cowork_target_profile(), + ] + + managed = { + ".github/skills/local-skill", + "cowork://skills/remote-skill", + } + + with patch( + "apm_cli.integration.copilot_cowork_paths.resolve_copilot_cowork_skills_dir", + return_value=cowork_root, + ): + result = integrator.sync_integration( + _stub_apm_package(), + project_root, + managed_files=managed, + targets=targets, + ) + + assert not local_skill.exists(), "Local skill dir should be removed." + assert not cowork_skill.exists(), "Cowork skill dir should be removed." + assert result["files_removed"] == 2 + assert result["errors"] == 0 diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index ba610cd1a..459fdd036 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -494,3 +494,411 @@ def test_get_all_config_maps_temp_dir_key(self): assert result.exit_code == 0 assert "temp-dir: /my/temp" in result.output + +# --------------------------------------------------------------------------- +# Isolation fixture used by storage-layer tests that perform real disk writes. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def isolated_config(tmp_path, monkeypatch): + """Redirect CONFIG_FILE to a temp dir so mutator tests never touch ~/.apm. + + Returns the Path to the config.json file for post-write inspection. + The cache is invalidated before and after the test body. + """ + import apm_cli.config as _conf + + _conf._invalidate_config_cache() + config_dir = tmp_path / ".apm" + config_file = config_dir / "config.json" + monkeypatch.setattr(_conf, "CONFIG_DIR", str(config_dir)) + monkeypatch.setattr(_conf, "CONFIG_FILE", str(config_file)) + yield config_file + _conf._invalidate_config_cache() + + +# --------------------------------------------------------------------------- +# Storage layer -- copilot_cowork_skills_dir +# --------------------------------------------------------------------------- + + +class TestCoworkSkillsDirFunctions: + """Tests for get_copilot_cowork_skills_dir, set_copilot_cowork_skills_dir, unset_copilot_cowork_skills_dir.""" + + def test_get_copilot_cowork_skills_dir_default_is_none(self): + """Returns None when copilot_cowork_skills_dir key is absent from the config.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "get_config", return_value={}): + assert cfg_module.get_copilot_cowork_skills_dir() is None + + def test_get_copilot_cowork_skills_dir_returns_stored_value(self): + """Returns the stored copilot_cowork_skills_dir value from config.""" + import apm_cli.config as cfg_module + + with patch.object( + cfg_module, + "get_config", + return_value={"copilot_cowork_skills_dir": "/stored/path"}, + ): + assert cfg_module.get_copilot_cowork_skills_dir() == "/stored/path" + + def test_set_copilot_cowork_skills_dir_stores_absolute_path(self): + """set_copilot_cowork_skills_dir persists the absolute path via update_config.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "update_config") as mock_update: + cfg_module.set_copilot_cowork_skills_dir("/absolute/skills") + mock_update.assert_called_once_with( + {"copilot_cowork_skills_dir": "/absolute/skills"} + ) + + def test_set_copilot_cowork_skills_dir_expands_tilde_before_storing(self): + """Tilde in path is expanded to an absolute path 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_copilot_cowork_skills_dir("~/myskills") + expected = os.path.join(home, "myskills") + mock_update.assert_called_once_with({"copilot_cowork_skills_dir": expected}) + + def test_set_copilot_cowork_skills_dir_raises_for_empty_string(self): + """Raises ValueError when path is an empty string.""" + import apm_cli.config as cfg_module + + with pytest.raises(ValueError): + cfg_module.set_copilot_cowork_skills_dir("") + + def test_set_copilot_cowork_skills_dir_raises_for_whitespace_only(self): + """Raises ValueError when path is whitespace only.""" + import apm_cli.config as cfg_module + + with pytest.raises(ValueError): + cfg_module.set_copilot_cowork_skills_dir(" ") + + def test_set_copilot_cowork_skills_dir_raises_for_relative_path(self): + """Raises ValueError when path is relative after tilde expansion.""" + import apm_cli.config as cfg_module + + with pytest.raises(ValueError, match="absolute"): + cfg_module.set_copilot_cowork_skills_dir("relative/path") + + def test_set_copilot_cowork_skills_dir_accepts_nonexistent_absolute_path(self): + """Non-existent absolute path is accepted; OneDrive may not yet be synced.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "update_config"): + # Should not raise even when the path does not exist on disk. + cfg_module.set_copilot_cowork_skills_dir("/nonexistent/absolute/path/xyz") + + def test_unset_copilot_cowork_skills_dir_removes_key(self, isolated_config): + """unset_copilot_cowork_skills_dir removes the key; subsequent get returns None.""" + import apm_cli.config as cfg_module + + cfg_module.set_copilot_cowork_skills_dir("/absolute/skills/path") + assert cfg_module.get_copilot_cowork_skills_dir() == "/absolute/skills/path" + + cfg_module.unset_copilot_cowork_skills_dir() + assert cfg_module.get_copilot_cowork_skills_dir() is None + + def test_unset_copilot_cowork_skills_dir_noop_when_absent(self, isolated_config): + """unset_copilot_cowork_skills_dir is a no-op when the key was never set.""" + import apm_cli.config as cfg_module + + # Should not raise even though the key does not exist. + cfg_module.unset_copilot_cowork_skills_dir() + assert cfg_module.get_copilot_cowork_skills_dir() is None + + +# --------------------------------------------------------------------------- +# Storage layer -- unset_temp_dir (new function added in the same commit) +# --------------------------------------------------------------------------- + + +class TestUnsetTempDir: + """Tests for the new unset_temp_dir function in apm_cli.config.""" + + def test_unset_temp_dir_removes_key(self, isolated_config, tmp_path): + """unset_temp_dir removes temp_dir; subsequent get_temp_dir returns None.""" + import apm_cli.config as cfg_module + + # Use tmp_path itself as the real temp directory to satisfy set_temp_dir + # validation (must exist and be writable). + cfg_module.set_temp_dir(str(tmp_path)) + assert cfg_module.get_temp_dir() == str(tmp_path) + + cfg_module.unset_temp_dir() + assert cfg_module.get_temp_dir() is None + + def test_unset_temp_dir_noop_when_absent(self, isolated_config): + """unset_temp_dir is a no-op when temp_dir was never set.""" + import apm_cli.config as cfg_module + + # Should not raise. + cfg_module.unset_temp_dir() + assert cfg_module.get_temp_dir() is None + + +# --------------------------------------------------------------------------- +# CLI -- apm config set copilot-cowork-skills-dir +# --------------------------------------------------------------------------- + + +class TestConfigSetCoworkSkillsDir: + """Tests for `apm config set copilot-cowork-skills-dir `.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_set_copilot_cowork_skills_dir_flag_enabled_returns_exit_0(self): + """Valid absolute path with the cowork flag enabled succeeds.""" + with ( + patch("apm_cli.core.experimental.is_enabled", return_value=True), + patch("apm_cli.config.set_copilot_cowork_skills_dir") as mock_set, + patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value="/tmp/foo"), + ): + result = self.runner.invoke( + config, ["set", "copilot-cowork-skills-dir", "/tmp/foo"] + ) + assert result.exit_code == 0 + mock_set.assert_called_once_with("/tmp/foo") + + def test_set_copilot_cowork_skills_dir_flag_disabled_returns_exit_1(self): + """Attempting to set copilot-cowork-skills-dir without the cowork flag exits 1.""" + with patch("apm_cli.core.experimental.is_enabled", return_value=False): + result = self.runner.invoke( + config, ["set", "copilot-cowork-skills-dir", "/tmp/foo"] + ) + assert result.exit_code == 1 + # The phrase may be line-wrapped in terminal output; check for the + # key parts that appear on the same output line. + assert "experimental" in result.output + assert "enable copilot-cowork" in result.output + + def test_set_copilot_cowork_skills_dir_relative_path_exits_1(self): + """Relative path is rejected with exit code 1 and an absolute-path hint.""" + with ( + patch("apm_cli.core.experimental.is_enabled", return_value=True), + patch( + "apm_cli.config.set_copilot_cowork_skills_dir", + side_effect=ValueError("Path must be absolute: relative/path"), + ), + ): + result = self.runner.invoke( + config, ["set", "copilot-cowork-skills-dir", "relative/path"] + ) + assert result.exit_code == 1 + assert "absolute" in result.output + + def test_set_copilot_cowork_skills_dir_empty_string_exits_1(self): + """Empty string is rejected with exit code 1.""" + with ( + patch("apm_cli.core.experimental.is_enabled", return_value=True), + patch( + "apm_cli.config.set_copilot_cowork_skills_dir", + side_effect=ValueError("Path cannot be empty"), + ), + ): + result = self.runner.invoke(config, ["set", "copilot-cowork-skills-dir", ""]) + assert result.exit_code == 1 + + +# --------------------------------------------------------------------------- +# CLI -- apm config get copilot-cowork-skills-dir +# --------------------------------------------------------------------------- + + +class TestConfigGetCoworkSkillsDir: + """Tests for `apm config get copilot-cowork-skills-dir`.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_get_copilot_cowork_skills_dir_displays_stored_value(self): + """Displays the configured copilot-cowork-skills-dir path.""" + with patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value="/my/skills"): + result = self.runner.invoke(config, ["get", "copilot-cowork-skills-dir"]) + assert result.exit_code == 0 + assert "/my/skills" in result.output + + def test_get_copilot_cowork_skills_dir_when_unset_shows_not_set(self): + """Displays a 'Not set' message when copilot-cowork-skills-dir has not been configured.""" + with patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None): + result = self.runner.invoke(config, ["get", "copilot-cowork-skills-dir"]) + assert result.exit_code == 0 + assert "Not set" in result.output + + def test_get_copilot_cowork_skills_dir_requires_no_flag(self): + """get copilot-cowork-skills-dir does not require the copilot-cowork experimental flag.""" + with patch("apm_cli.config.get_copilot_cowork_skills_dir", return_value=None): + # No patch on is_enabled -- the real function must not gate the get path. + result = self.runner.invoke(config, ["get", "copilot-cowork-skills-dir"]) + assert result.exit_code == 0 + + +# --------------------------------------------------------------------------- +# CLI -- apm config unset copilot-cowork-skills-dir / temp-dir +# --------------------------------------------------------------------------- + + +class TestConfigUnsetSubcommand: + """Tests for `apm config unset `.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_unset_copilot_cowork_skills_dir_exits_0(self): + """apm config unset copilot-cowork-skills-dir exits 0 and prints success message.""" + with patch("apm_cli.config.unset_copilot_cowork_skills_dir") as mock_unset: + result = self.runner.invoke(config, ["unset", "copilot-cowork-skills-dir"]) + assert result.exit_code == 0 + mock_unset.assert_called_once() + + def test_unset_copilot_cowork_skills_dir_idempotent(self): + """Unsetting an absent copilot-cowork-skills-dir key is safe and exits 0.""" + with patch( + "apm_cli.config.unset_copilot_cowork_skills_dir" + ): # real no-op behaviour tested in storage tests + result = self.runner.invoke(config, ["unset", "copilot-cowork-skills-dir"]) + assert result.exit_code == 0 + + def test_unset_temp_dir_exits_0(self): + """apm config unset temp-dir exits 0.""" + with patch("apm_cli.config.unset_temp_dir") as mock_unset: + result = self.runner.invoke(config, ["unset", "temp-dir"]) + assert result.exit_code == 0 + mock_unset.assert_called_once() + + def test_unset_unknown_key_exits_1(self): + """Unsetting an unknown key exits 1 with an informative error.""" + result = self.runner.invoke(config, ["unset", "unknown-key"]) + assert result.exit_code == 1 + + +# --------------------------------------------------------------------------- +# CLI -- flag-gated listing (apm config and apm config get with no key) +# --------------------------------------------------------------------------- + + +class TestConfigListingFlagGating: + """Tests that copilot-cowork-skills-dir appears in listings only when the flag is enabled.""" + + def setup_method(self): + self.runner = CliRunner() + self.original_dir = os.getcwd() + + def teardown_method(self): + try: + os.chdir(self.original_dir) + except (FileNotFoundError, OSError): + pass + + def test_config_get_shows_copilot_cowork_skills_dir_when_flag_enabled(self): + """apm config get (no key) includes copilot-cowork-skills-dir when the flag is on.""" + fake_config = {"auto_integrate": True} + with ( + patch("apm_cli.config.get_config", return_value=fake_config), + patch("apm_cli.core.experimental.is_enabled", return_value=True), + patch( + "apm_cli.config.get_copilot_cowork_skills_dir", + return_value="/enabled/path", + ), + ): + result = self.runner.invoke(config, ["get"]) + assert result.exit_code == 0 + assert "copilot-cowork-skills-dir" in result.output + + def test_config_get_hides_copilot_cowork_skills_dir_when_flag_disabled(self): + """apm config get (no key) omits copilot-cowork-skills-dir when the flag is off.""" + fake_config = {"auto_integrate": True} + with ( + patch("apm_cli.config.get_config", return_value=fake_config), + patch("apm_cli.core.experimental.is_enabled", return_value=False), + ): + result = self.runner.invoke(config, ["get"]) + assert result.exit_code == 0 + assert "copilot-cowork-skills-dir" not in result.output + + def test_config_show_includes_copilot_cowork_skills_dir_when_flag_enabled(self): + """apm config (no subcommand) includes copilot-cowork-skills-dir when the flag is on.""" + 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.0.0"), + patch("apm_cli.config.get_temp_dir", return_value=None), + patch("apm_cli.core.experimental.is_enabled", return_value=True), + patch( + "apm_cli.config.get_copilot_cowork_skills_dir", + return_value="/cowork/skills", + ), + 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 "Cowork Skills Dir" in result.output + + def test_config_show_omits_copilot_cowork_skills_dir_when_flag_disabled(self): + """apm config (no subcommand) omits copilot-cowork-skills-dir when the flag is off.""" + 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.0.0"), + patch("apm_cli.config.get_temp_dir", return_value=None), + patch("apm_cli.core.experimental.is_enabled", return_value=False), + 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 "Cowork Skills Dir" not in result.output + + +# --------------------------------------------------------------------------- +# Flag-gating regression -- only copilot-cowork-skills-dir should be gated +# --------------------------------------------------------------------------- + + +class TestFlagGatingRegression: + """Regression checks: only copilot-cowork-skills-dir is gated on the copilot-cowork flag.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_auto_integrate_set_is_not_gated(self): + """apm config set auto-integrate does not require the cowork flag.""" + with patch("apm_cli.config.set_auto_integrate"): + result = self.runner.invoke(config, ["set", "auto-integrate", "true"]) + assert result.exit_code == 0 + + def test_temp_dir_set_is_not_gated(self): + """apm config set temp-dir does not require the cowork flag.""" + with ( + patch("apm_cli.config.set_temp_dir"), + patch("apm_cli.config.get_temp_dir", return_value="/tmp/foo"), + ): + result = self.runner.invoke(config, ["set", "temp-dir", "/tmp/foo"]) + assert result.exit_code == 0 + + def test_copilot_cowork_skills_dir_set_is_gated(self): + """apm config set copilot-cowork-skills-dir exits 1 when the copilot-cowork flag is off.""" + with patch("apm_cli.core.experimental.is_enabled", return_value=False): + result = self.runner.invoke( + config, ["set", "copilot-cowork-skills-dir", "/some/path"] + ) + assert result.exit_code == 1 +