diff --git a/CHANGELOG.md b/CHANGELOG.md index 81e83d1d6..979d9be30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Manifest contract: invalid `target:` values now raise a parse error.** Previously, an unknown token (or a CSV string like `target: opencode,claude,copilot,agents` instead of the YAML list `target: [opencode, claude, copilot, agents]`) was silently ignored, leaving `apm install` and `apm compile` to exit 0 while deploying nothing. The shared parser used by `--target` now also validates `apm.yml`'s `target:`, so the same input resolves the same way at every entry point. **Migration:** three previously-silent inputs now fail loud -- (1) unknown tokens (`target: bogus` -> fix the typo), (2) empty values (`target: ""`, `target: []` -> remove the line if you meant auto-detect), (3) `all` mixed with other targets (`target: [all, claude]` -> use `all` alone). Omitting `target:` entirely still triggers auto-detection. (#820) - Rename `DownloadStrategyManager` to `DownloadDelegate` to better reflect Facade/Delegate pattern (#918) - Fix incorrect double-checked locking in marketplace registry `_load()` -- hold lock across full check+read+set (#918) ### Fixed +- `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) ## [0.10.0] - 2026-04-27 diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index 1c0a38ceb..55107d9ea 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -113,7 +113,7 @@ policy: | **Default** | Auto-detect: `vscode` if `.github/` exists, `claude` if `.claude/` exists, `codex` if `.codex/` exists, `all` if multiple target folders exist, `minimal` if none | | **Allowed values** | `vscode` · `agents` · `copilot` · `claude` · `cursor` · `opencode` · `codex` · `all` | -Controls which output targets are generated during compilation and installation. Accepts a single string or a list of strings. When unset, a conforming resolver SHOULD auto-detect based on folder presence. Unknown values MUST be silently ignored (auto-detection takes over). +Controls which output targets are generated during compilation and installation. Accepts a single string or a list of strings. When unset, a conforming resolver SHOULD auto-detect based on folder presence. Unknown values MUST raise a parse error pointing at the offending token. Auto-detection applies only when `target:` is unset. ```yaml # Single target diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 9dda27901..d4df08241 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -58,6 +58,30 @@ my-package/ resource2.md ``` +## Manifest fields: `target:` validation contract + +The `target:` field in `apm.yml` controls which output runtimes the package +compiles and installs to. Both `apm.yml`'s `target:` and the `--target` CLI +flag share the same validator, so identical input is rejected or accepted +the same way at every entry point. Invalid values fail at parse time with a +message naming the apm.yml path and the offending token -- they do **not** +silently fall through to auto-detect. + +| Form | Behaviour | +|------|-----------| +| `target: copilot` | Single token; allowed values: `vscode`, `agents`, `copilot`, `claude`, `cursor`, `opencode`, `codex`, `all` | +| `target: [claude, copilot]` | List form; only listed targets are compiled/installed | +| `target: claude,copilot` | CSV-string form; parses identically to the list form (the shared validator splits on `,`). Before #820 was fixed, this silently produced zero deployment | +| `target:` omitted entirely | Auto-detect from project folders (`.github/`, `.claude/`, `.codex/`) | +| `target: bogus` (unknown token) | **Parse error** -- fix the typo | +| `target: ""` or `target: []` (empty) | **Parse error** -- remove the line if you meant auto-detect | +| `target: [all, claude]` (`all` mixed with other targets) | **Parse error** -- use `all` alone | + +Error messages always name the `apm.yml` path and the offending token, so the +fix point is unambiguous. The list form (`target: [a, b]`) is the recommended +shape; the CSV-string form is supported for parity with `--target a,b` on the +CLI but reads less cleanly in YAML. + ## The 7 primitive types ### 1. Instruction (`*.instructions.md`) diff --git a/packages/apm-guide/.apm/skills/apm-usage/workflow.md b/packages/apm-guide/.apm/skills/apm-usage/workflow.md index 98a41d526..efb6947d1 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/workflow.md +++ b/packages/apm-guide/.apm/skills/apm-usage/workflow.md @@ -78,6 +78,16 @@ CLI equivalent: `--target claude,copilot` (comma-separated). | Multiple target folders | `all` | | Neither exists | `minimal` (AGENTS.md only) | +Auto-detection only applies when `target:` is omitted entirely. Invalid `target:` values fail at parse time with a message naming the apm.yml path and the offending token. The same shared validator runs for both `apm.yml`'s `target:` and the `--target` CLI flag, so identical input produces identical results at every entry point. + +| Input | Result | +|-------|--------| +| `target: bogus` (unknown token) | parse error -- fix the typo | +| `target: ""` or `target: []` (empty) | parse error -- remove the line if you meant auto-detect | +| `target: [all, claude]` (`all` mixed with other targets) | parse error -- use `all` alone | +| `target: opencode,claude,copilot,agents` (CSV string in YAML) | accepted; parses identically to the list form `target: [opencode, claude, copilot, agents]` (used to silently zero-deploy before #820 was fixed) | +| `target:` line omitted | auto-detect from folders (table above) | + ## What to commit | Path | Commit? | Why | diff --git a/src/apm_cli/commands/compile/cli.py b/src/apm_cli/commands/compile/cli.py index 1b559eef2..d857eda13 100644 --- a/src/apm_cli/commands/compile/cli.py +++ b/src/apm_cli/commands/compile/cli.py @@ -379,16 +379,19 @@ def compile( # Auto-detect target if not explicitly provided from ...core.target_detection import detect_target, get_target_description - # Get config target from apm.yml if available - config_target = None - try: - from ...models.apm_package import APMPackage + # Get config target from apm.yml if available. When the file is + # absent we proceed with auto-detection; when it is present but + # malformed we let the parse error surface so users see exactly + # what is wrong (e.g. ``target: opencode,bogus`` -> a ValueError + # naming the bad token), rather than silently falling through to + # auto-detect. See #820. + from ...models.apm_package import APMPackage - apm_pkg = APMPackage.from_apm_yml(Path(APM_YML_FILENAME)) + config_target = None + apm_yml_path = Path(APM_YML_FILENAME) + if apm_yml_path.exists(): + apm_pkg = APMPackage.from_apm_yml(apm_yml_path) config_target = apm_pkg.target - except Exception: - # No apm.yml or parsing error - proceed with auto-detection - pass # Resolve list targets to compiler-understood string compile_target = _resolve_compile_target(target) @@ -475,8 +478,39 @@ def compile( # Success message for dry run already included in formatter output pass else: - # Success message for actual compilation - logger.success("Compilation completed successfully!", symbol="check") + # Defense-in-depth (#820): don't claim "completed + # successfully" when zero files were emitted. With + # parse_target_field as the upstream gatekeeper this is + # unreachable in normal flow, but silent zero-effect + # success is the worst-case package-manager DX. + # + # Pattern-based stat scan (instead of a hardcoded key + # list) so new compile-time targets pick up the guard + # automatically: any stat ending in ``_files_written`` + # or ``_files_generated`` contributes to the total. + _files_written = sum( + int(v or 0) + for k, v in result.stats.items() + if k.endswith(("_files_written", "_files_generated")) + ) + if _files_written > 0: + logger.success( + "Compilation completed successfully!", + symbol="check", + ) + else: + # Zero-output compile is the silent-success failure + # mode #820 guards against. Don't claim success; + # surface what the user can act on. The cause is + # usually one of: target dirs not present (auto- + # detect found nothing), explicit target rejected + # by policy, or no primitives in the project. + logger.warning( + "Compilation completed but produced no output " + "files. Check that target directories exist " + "(e.g. .github/, .claude/) or set 'target:' " + "in apm.yml / pass --target explicitly." + ) else: # Traditional single-file compilation - keep existing logic diff --git a/src/apm_cli/core/target_detection.py b/src/apm_cli/core/target_detection.py index 9b0878816..ac963afb3 100644 --- a/src/apm_cli/core/target_detection.py +++ b/src/apm_cli/core/target_detection.py @@ -261,12 +261,141 @@ def normalize_target_list( ) +def parse_target_field( + value: Union[str, List[str], None], + *, + source_path: Optional[Path] = None, +) -> Union[str, List[str], None]: + """Parse, validate, and normalize a target value from any entry point. + + Single source of truth for the ``target`` field, shared by the + ``--target`` CLI flag (via :class:`TargetParamType`) and ``apm.yml``'s + top-level ``target:`` (via :func:`APMPackage.from_apm_yml`). The + output may differ from the input in case (lowercased), order + (preserved but deduplicated), and shape (single-element multi-token + inputs collapse to ``str``). Aliases are resolved for multi-token + input only; see the *Returns* section below for the exact rules. + + Accepted input shapes: + + * ``None`` -> ``None`` (auto-detect at consumption time -- this is the + "field absent" path; an apm.yml without ``target:`` lands here). + * Single token (``"claude"``) -> the same lowercased token as ``str``. + Aliases are NOT resolved for solo input -- ``"copilot"`` returns + ``"copilot"`` (not the canonical ``"vscode"``) to match the + long-standing CLI contract; downstream consumers handle the alias + set explicitly. + * CSV string (``"claude,copilot"``) -> deduplicated ``List[str]`` with + aliases resolved to canonical names. Collapses to a bare ``str`` if + after dedup only one canonical token remains. + * List input (``["claude", "copilot"]``) goes through the same path as + the CSV form -- single-element lists collapse to ``str``. + * Literal ``"all"`` -> ``"all"`` (exclusive; cannot be combined). + + Args: + value: The raw value -- ``str``, ``List[str]``, or ``None``. + source_path: Optional path to the apm.yml that produced ``value``. + When supplied, ValueError messages name the file so users can + jump to it directly. + + Returns: + ``None`` for unset, a ``str`` for a single token (or ``"all"``), + or a deduplicated ``List[str]`` for multi-target input. + + Raises: + ValueError: When the value is an empty / whitespace-only / commas-only + string, an empty list, a non-string non-list type, contains a + token that is not in :data:`VALID_TARGET_VALUES`, or mixes + ``"all"`` with other targets. An empty *string* is treated as + user error (the "field absent" path is ``None``, supplied by + the YAML loader for a missing key). + """ + if value is None: + return None + + # ---- collect raw tokens ---- + if isinstance(value, str): + # Empty / whitespace-only / comma-only strings are user error -- a + # missing field comes through as ``None`` from the YAML loader, so + # an empty *string* means the user typed something invalid. + raw_parts = [v.strip().lower() for v in value.split(",") if v.strip()] + elif isinstance(value, list): + raw_parts = [] + for item in value: + if not isinstance(item, str): + raise ValueError(_target_error( + f"each entry must be a string, got {type(item).__name__}", + source_path, + )) + if item.strip(): + raw_parts.append(item.strip().lower()) + else: + raise ValueError(_target_error( + f"expected string or list of strings, got {type(value).__name__}", + source_path, + )) + + if not raw_parts: + raise ValueError(_target_error("target value must not be empty", source_path)) + + # ---- validate every token ---- + for p in raw_parts: + if p not in VALID_TARGET_VALUES: + raise ValueError(_target_error( + f"'{p}' is not a valid target. " + f"Choose from: {', '.join(sorted(VALID_TARGET_VALUES))}", + source_path, + )) + + # ---- "all" is exclusive ---- + if "all" in raw_parts: + if len(raw_parts) > 1: + raise ValueError(_target_error( + "'all' cannot be combined with other targets", + source_path, + )) + return "all" + + # Single-token input is returned as-is (no alias resolution). This + # preserves the long-standing CLI contract where ``--target copilot`` + # yields ``"copilot"`` rather than the canonical ``"vscode"``; every + # downstream consumer (active_targets, agents_compiler, + # _CROSS_TARGET_MAPS, _TARGET_PREFIXES) already accepts both alias + # spellings, so resolving here would be a visible behaviour change + # with zero functional benefit and would break the CLI test suite + # (~10 ``test_single_*`` cases). This is the one asymmetry #820's + # "shared normalization" intentionally leaves in place; collapsing it + # is an independent decision tracked separately from this fix. + if len(raw_parts) == 1: + return raw_parts[0] + + # Multi-token: resolve aliases + dedupe, preserving input order. + seen: set[str] = set() + result: List[str] = [] + for p in raw_parts: + canonical = TARGET_ALIASES.get(p, p) + if canonical not in seen: + seen.add(canonical) + result.append(canonical) + + if len(result) == 1: + return result[0] + return result + + +def _target_error(message: str, source_path: Optional[Path]) -> str: + """Format a target validation error, naming the source file when known.""" + if source_path is not None: + return f"Invalid 'target' in {source_path}: {message}" + return f"Invalid target: {message}" + + class TargetParamType(click.ParamType): """Click parameter type accepting comma-separated target values. - Single values and ``"all"`` are returned as plain strings for backward - compatibility with existing command handlers. Multiple comma-separated - targets are returned as a deduplicated ``list[str]`` of canonical names. + Delegates to :func:`parse_target_field`, which is the shared validator + used by ``apm.yml``'s ``target:`` field as well -- so ``--target X`` and + ``target: X`` always resolve identically and reject the same inputs. Examples:: @@ -284,50 +413,10 @@ def convert( param: Optional[click.Parameter], ctx: Optional[click.Context], ) -> Union[str, List[str], None]: - if value is None: - return None - # If already converted (e.g. from a default), pass through. - if isinstance(value, list): - return value - - # Split on comma, normalize whitespace & case, drop empty parts. - parts = [v.strip().lower() for v in value.split(",") if v.strip()] - if not parts: - self.fail("target value must not be empty", param, ctx) - - # Validate every token. - for p in parts: - if p not in VALID_TARGET_VALUES: - self.fail( - f"'{p}' is not a valid target. " - f"Choose from: {', '.join(sorted(VALID_TARGET_VALUES))}", - param, - ctx, - ) - - # "all" is exclusive -- reject combinations like "all,claude". - if "all" in parts: - if len(parts) > 1: - self.fail( - "'all' cannot be combined with other targets", - param, - ctx, - ) - return "all" - - # Single target -> plain string (backward compat). - if len(parts) == 1: - return parts[0] - - # Multi-target: resolve aliases and deduplicate. - seen: set[str] = set() - result: List[str] = [] - for p in parts: - canonical = TARGET_ALIASES.get(p, p) - if canonical not in seen: - seen.add(canonical) - result.append(canonical) - # If aliases collapsed everything to one target, return a string. - if len(result) == 1: - return result[0] - return result + try: + return parse_target_field(value) + except ValueError as e: + # Click idiom: route validation errors through self.fail so the + # user sees a clean "Invalid value for '--target': ..." message + # rather than a Python traceback. + self.fail(str(e), param, ctx) diff --git a/src/apm_cli/install/phases/targets.py b/src/apm_cli/install/phases/targets.py index 44f76f5ae..bfa63a5c7 100644 --- a/src/apm_cli/install/phases/targets.py +++ b/src/apm_cli/install/phases/targets.py @@ -115,21 +115,30 @@ def run(ctx: "InstallContext") -> None: ) raise SystemExit(1) - # Log target detection results - if ctx.logger and _targets: + # Log target detection results. The empty-targets branch is a defensive + # warning -- with parse_target_field as the upstream gatekeeper this + # state is unreachable in normal flow, but a silent zero-target install + # is the worst-case package-manager DX (see #820), so always emit. + if ctx.logger: _scope_label = "global" if _is_user else "project" - _target_names = ", ".join( - f"{t.name} (~/{t.root_dir}/)" if _is_user else t.name - for t in _targets - ) - ctx.logger.verbose_detail( - f"Active {_scope_label} targets: {_target_names}" - ) - if _is_user: - from apm_cli.deps.lockfile import get_lockfile_path - + if _targets: + _target_names = ", ".join( + f"{t.name} (~/{t.root_dir}/)" if _is_user else t.name + for t in _targets + ) ctx.logger.verbose_detail( - f"Lockfile: {get_lockfile_path(ctx.apm_dir)}" + f"Active {_scope_label} targets: {_target_names}" + ) + if _is_user: + from apm_cli.deps.lockfile import get_lockfile_path + + ctx.logger.verbose_detail( + f"Lockfile: {get_lockfile_path(ctx.apm_dir)}" + ) + else: + ctx.logger.warning( + f"No {_scope_label} targets resolved -- nothing will be " + f"deployed. Check 'target:' in apm.yml or use --target." ) for _t in _targets: diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index 6e70385fd..9c746dad8 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -3,6 +3,17 @@ Each target tool (Copilot, Claude, Cursor, ...) describes where APM primitives should land. Adding a new target means adding an entry to ``KNOWN_TARGETS`` -- no new classes required. + +Resolver invariant (#820): both :func:`active_targets` and +:func:`active_targets_user_scope` accept ``Union[str, List[str]]`` for +``explicit_target`` but treat the two shapes identically -- string inputs +are wrapped to a one-element list before the resolution loop. Validity +is enforced *upstream* by +:func:`apm_cli.core.target_detection.parse_target_field`, which is the +shared gatekeeper for both ``--target`` and ``apm.yml``'s ``target:`` +field. Unknown tokens never reach these functions in normal flow; if +one does, it falls through the loop without matching any profile and +the result is an empty list (no silent ``[copilot]`` fallback). """ from __future__ import annotations @@ -474,9 +485,11 @@ def active_targets_user_scope( Resolution order: - 1. **Explicit target** (``--target``): returns the matching profile - if it supports user scope. ``"all"`` returns every user-capable - target. A list of names returns all matching user-capable profiles. + 1. **Explicit target** (``--target``): returns the matching profile(s) + that support user scope. ``"all"`` returns every user-capable + target. Validity is enforced upstream by + :func:`apm_cli.core.target_detection.parse_target_field`; this + function does not silently fall back when given unknown tokens. 2. **Directory detection**: profiles whose ``effective_root(user_scope=True)`` directory exists under ``~/``. 3. **Fallback**: ``[copilot]`` -- same default as project scope. @@ -487,37 +500,22 @@ def active_targets_user_scope( # --- explicit target --- if explicit_target: - if isinstance(explicit_target, list): - profiles: list = [] - seen: set = set() - for t in explicit_target: - canonical = t - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - return [ - p for p in KNOWN_TARGETS.values() - if p.user_supported and _flag_gated(p) - ] - profile = KNOWN_TARGETS.get(canonical) - 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 [] - - # single string (existing behavior) - canonical = explicit_target - if canonical in ("copilot", "vscode", "agents"): - canonical = "copilot" - if canonical == "all": - return [ - p for p in KNOWN_TARGETS.values() - if p.user_supported and _flag_gated(p) - ] - profile = KNOWN_TARGETS.get(canonical) - if profile and profile.user_supported and _flag_gated(profile): - return [profile] - return [] + # See module docstring on the parse_target_field gate-keeping contract. + raw = [explicit_target] if isinstance(explicit_target, str) else list(explicit_target) + profiles: list = [] + seen: set = set() + for t in raw: + canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + if canonical == "all": + return [ + p for p in KNOWN_TARGETS.values() + if p.user_supported and _flag_gated(p) + ] + profile = KNOWN_TARGETS.get(canonical) + 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 # --- auto-detect by directory presence at ~/ --- # Targets with detect_by_dir=False (cowork) are never auto-detected. @@ -544,8 +542,11 @@ def active_targets( Resolution order: 1. **Explicit target** (``--target`` flag or ``apm.yml target:``): - returns only the matching profile(s). ``"all"`` returns every - known target. A list of names returns all matching profiles. + returns the matching profile(s). ``"all"`` returns every known + target. Validity is enforced upstream by + :func:`apm_cli.core.target_detection.parse_target_field`; unknown + tokens never reach here, so this branch never silently falls back + to ``[copilot]``. 2. **Directory detection**: profiles whose ``root_dir`` already exists under *project_root*. 3. **Fallback**: when nothing is detected, returns ``[copilot]`` @@ -562,35 +563,22 @@ def active_targets( # --- explicit target --- if explicit_target: - if isinstance(explicit_target, list): - profiles: list = [] - seen: set = set() - for t in explicit_target: - canonical = t - 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 _flag_gated(profile) and profile.name not in seen: - seen.add(profile.name) - profiles.append(profile) - return profiles if profiles else [KNOWN_TARGETS["copilot"]] - - # single string (existing behavior) - canonical = explicit_target - 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) - if profile and _flag_gated(profile): - return [profile] - return [] + # See module docstring on the parse_target_field gate-keeping contract. + raw = [explicit_target] if isinstance(explicit_target, str) else list(explicit_target) + profiles: list = [] + seen: set = set() + for t in raw: + canonical = "copilot" if t in ("copilot", "vscode", "agents") else t + 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 _flag_gated(profile) and profile.name not in seen: + seen.add(profile.name) + profiles.append(profile) + return profiles # --- auto-detect by directory presence --- # Targets with detect_by_dir=False (cowork) are never auto-detected. diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index eee2df4fd..34f91b071 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -27,6 +27,7 @@ ValidationResult, validate_apm_package, ) +from ..core.target_detection import parse_target_field # Re-export all moved symbols so `from apm_cli.models.apm_package import X` keeps working __all__ = [ @@ -196,6 +197,15 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": else: raise ValueError("'includes' must be 'auto' or a list of strings") + # Parse target field through the same validator as --target so a CSV + # string like ``target: "claude,copilot"`` resolves identically to + # ``--target claude,copilot`` and unknown tokens fail at parse time + # (see apm_cli.core.target_detection.parse_target_field). + target_value = parse_target_field( + data.get('target'), + source_path=apm_yml_path, + ) + result = cls( name=data['name'], version=data['version'], @@ -206,7 +216,7 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": dev_dependencies=dev_dependencies, scripts=data.get('scripts'), package_path=apm_yml_path.parent, - target=data.get('target'), + target=target_value, type=pkg_type, includes=includes, ) diff --git a/tests/test_apm_package_models.py b/tests/test_apm_package_models.py index 6f5b8abc4..643bd9f85 100644 --- a/tests/test_apm_package_models.py +++ b/tests/test_apm_package_models.py @@ -643,6 +643,152 @@ def test_has_apm_dependencies(self): pkg3 = APMPackage(name="test", version="1.0.0", dependencies={"apm": apm_deps}) assert pkg3.has_apm_dependencies() + # ------------------------------------------------------------------ + # target field parsing -- shared with --target via parse_target_field + # (regression suite for #820) + # ------------------------------------------------------------------ + + def test_csv_string_in_apm_yml_parses_like_cli(self): + """CSV string in apm.yml resolves identically to ``--target``. + + The exact value from issue #820 -- previously this returned a raw + CSV string and downstream silently produced ``[]``, leaving + ``apm install`` and ``apm compile`` to exit 0 with nothing + deployed. Now the value parses through the same validator as the + CLI flag and yields the canonical multi-target list. + """ + apm_content = { + "name": "x", + "version": "0.1.0", + "target": "opencode,claude,copilot,agents", + } + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.target == ["opencode", "claude", "vscode"] + + Path(f.name).unlink() + + def test_unknown_target_in_apm_yml_raises_with_pointer(self): + """An unknown token in ``target:`` raises a ValueError that names + the offending token AND the apm.yml path, so users can jump to + the file directly. Replaces the previous silently-ignored + contract from manifest-schema.md (see #820 spec revision).""" + apm_content = { + "name": "x", + "version": "0.1.0", + "target": "claude,bogus,copilot", + } + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + yml_path = f.name + + with pytest.raises(ValueError) as excinfo: + APMPackage.from_apm_yml(Path(yml_path)) + msg = str(excinfo.value) + assert "'bogus'" in msg + assert "not a valid target" in msg + assert yml_path in msg # apm.yml path is part of the error + + Path(yml_path).unlink() + + def test_yaml_list_target_still_parses(self): + """Native YAML list form (``target: [claude, copilot]``) keeps + working through the shared parser. Smoke test ensuring the + change didn't break the supported list shape.""" + apm_content = { + "name": "x", + "version": "0.1.0", + "target": ["claude", "copilot"], + } + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.target == ["claude", "vscode"] + + Path(f.name).unlink() + + def test_target_unset_remains_none(self): + """Omitting ``target:`` yields ``None`` -- auto-detection takes + over at consumption time (active_targets / detect_target).""" + apm_content = {"name": "x", "version": "0.1.0"} + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.target is None + + Path(f.name).unlink() + + def test_target_empty_string_raises(self): + """``target: ""`` is user error and now raises (was: silently + auto-detected before #820). See CHANGELOG migration note.""" + apm_content = {"name": "x", "version": "0.1.0", "target": ""} + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + yml_path = f.name + + with pytest.raises(ValueError, match="must not be empty"): + APMPackage.from_apm_yml(Path(yml_path)) + + Path(yml_path).unlink() + + def test_target_empty_list_raises(self): + """``target: []`` is user error and now raises (was: silently + auto-detected before #820). Empty list is "set to nothing", + which is not the same as "unset" -- to opt into auto-detection + the field must be omitted entirely.""" + apm_content = {"name": "x", "version": "0.1.0", "target": []} + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + yml_path = f.name + + with pytest.raises(ValueError, match="must not be empty"): + APMPackage.from_apm_yml(Path(yml_path)) + + Path(yml_path).unlink() + + def test_target_all_combined_with_other_raises(self): + """``all`` is exclusive -- mixing it with other targets is now + rejected at parse time, matching the existing ``--target`` flag + contract (TargetParamType test_target_combined_with_all_rejected).""" + apm_content = { + "name": "x", + "version": "0.1.0", + "target": ["all", "claude"], + } + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yml", delete=False + ) as f: + yaml.dump(apm_content, f) + f.flush() + yml_path = f.name + + with pytest.raises(ValueError, match="cannot be combined"): + APMPackage.from_apm_yml(Path(yml_path)) + + Path(yml_path).unlink() + class TestValidationResult: """Test ValidationResult functionality.""" diff --git a/tests/unit/compilation/test_compile_target_flag.py b/tests/unit/compilation/test_compile_target_flag.py index dfa89cbea..915363abe 100644 --- a/tests/unit/compilation/test_compile_target_flag.py +++ b/tests/unit/compilation/test_compile_target_flag.py @@ -556,11 +556,63 @@ def test_short_flag_t_works(self, runner, temp_project): try: os.chdir(temp_project) result = runner.invoke(cli, ["compile", "-t", "vscode", "--dry-run"]) - + assert "Invalid value for '--target'" not in result.output finally: os.chdir(original_dir) + # ----- #820 regression: apm.yml target: contract ----- + + def test_csv_target_in_apm_yml_no_longer_silent(self, runner, temp_project): + """CSV string in apm.yml's ``target:`` used to leave install/compile + with exit-0 success and zero deployment. Now the same string flows + through ``parse_target_field`` and yields a real list of targets. + """ + original_dir = os.getcwd() + try: + os.chdir(temp_project) + (temp_project / "apm.yml").write_text( + "name: test-project\nversion: 0.1.0\n" + "target: opencode,claude,copilot\n" + ) + result = runner.invoke(cli, ["compile", "--dry-run"]) + # No "Invalid value" gripe -- the string is a valid CSV now. + assert "Invalid value for '--target'" not in result.output + # And compile no longer prints success-after-zero-effect: it + # either succeeds with output (dry-run preview) or surfaces a + # real error. The pre-fix log line "Compilation completed + # successfully!" with no files written must not appear when + # zero targets resolve. + assert result.exit_code == 0 or "Invalid 'target'" in result.output + finally: + os.chdir(original_dir) + + def test_unknown_target_in_apm_yml_fails_loudly(self, runner, temp_project): + """Unknown token in apm.yml ``target:`` now fails the command with + a ValueError naming the bad token, instead of being swallowed by + the old ``except Exception: pass`` in compile/cli.py.""" + original_dir = os.getcwd() + try: + os.chdir(temp_project) + (temp_project / "apm.yml").write_text( + "name: test-project\nversion: 0.1.0\n" + "target: claude,bogus,copilot\n" + ) + result = runner.invoke(cli, ["compile", "--dry-run"]) + # Either the CLI exits non-zero with the error, or the error + # is included in the output -- both are acceptable signals + # that the silent-swallow path is gone. Normalize whitespace + # because the error message may be soft-wrapped onto multiple + # lines by the CLI logger. + combined = " ".join( + ((result.output or "") + + (str(result.exception) if result.exception else "")).split() + ) + assert "'bogus'" in combined + assert "not a valid target" in combined + finally: + os.chdir(original_dir) + class TestTargetVscodeOnlyGeneratesAgentsMd: """Tests to verify --target vscode only generates AGENTS.md.""" diff --git a/tests/unit/core/test_scope.py b/tests/unit/core/test_scope.py index 518db3b2b..0ee8f27df 100644 --- a/tests/unit/core/test_scope.py +++ b/tests/unit/core/test_scope.py @@ -297,10 +297,16 @@ def test_explicit_all_returns_all_user_capable(self): assert "cursor" in names assert "opencode" in names - def test_explicit_unknown_returns_empty(self): - from apm_cli.integration.targets import active_targets_user_scope - result = active_targets_user_scope(explicit_target="nonexistent") - assert result == [] + def test_unknown_target_raises_at_parse_time(self): + """Unknown tokens fail at the parser, not silently in the + user-scope resolver. Mirrors the project-scope contract change + from #820 -- both entry points share one validator + (:func:`apm_cli.core.target_detection.parse_target_field`).""" + import pytest + from apm_cli.core.target_detection import parse_target_field + + with pytest.raises(ValueError, match="not a valid target"): + parse_target_field("nonexistent") def test_explicit_vscode_alias(self): from apm_cli.integration.targets import active_targets_user_scope diff --git a/tests/unit/core/test_target_detection.py b/tests/unit/core/test_target_detection.py index fcd68ac0e..04b0ee2a8 100644 --- a/tests/unit/core/test_target_detection.py +++ b/tests/unit/core/test_target_detection.py @@ -408,12 +408,22 @@ def test_none_returns_none(self): """None value passes through unchanged.""" assert self.tp.convert(None, None, None) is None - # -- Already-converted list passthrough ------------------------------- + # -- List input goes through the same validator as strings ----------- - def test_list_passthrough(self): - """A list value passes through unchanged.""" - lst = ["claude", "vscode"] - assert self.tp.convert(lst, None, None) is lst + def test_list_input_is_validated(self): + """List input flows through parse_target_field: validated + deduped. + + Returned list is a fresh canonical sequence, not the input list -- + identity is no longer preserved because list and string inputs share + a single normalization path. + """ + result = self.tp.convert(["claude", "vscode"], None, None) + assert result == ["claude", "vscode"] + + def test_list_input_collapses_aliases_to_string(self): + """Multi-element list whose entries all alias to one canonical + target collapses to that single canonical name (``"vscode"``).""" + assert self.tp.convert(["copilot", "agents"], None, None) == "vscode" # -- Single target (backward compat: returns string) ------------------ diff --git a/tests/unit/integration/test_targets.py b/tests/unit/integration/test_targets.py index 29b1b7fe5..75800dff1 100644 --- a/tests/unit/integration/test_targets.py +++ b/tests/unit/integration/test_targets.py @@ -94,9 +94,18 @@ def test_explicit_overrides_detection(self): targets = active_targets(self.root, explicit_target="claude") assert [t.name for t in targets] == ["claude"] - def test_explicit_unknown_returns_empty(self): - targets = active_targets(self.root, explicit_target="nonexistent") - assert targets == [] + def test_unknown_target_raises_at_parse_time(self): + """Unknown tokens in apm.yml or --target must fail at the parser. + + Replaces the previous ``test_explicit_unknown_returns_empty`` -- + the silent-empty contract was the root cause of #820 (apm install + and apm compile exited 0 while deploying nothing). + """ + import pytest + from apm_cli.core.target_detection import parse_target_field + + with pytest.raises(ValueError, match="not a valid target"): + parse_target_field("nonexistent") # -- codex detection -- @@ -171,12 +180,22 @@ def test_explicit_list_all_mixed_returns_every_known_target(self): targets = active_targets(self.root, explicit_target=["claude", "all"]) assert len(targets) == len(KNOWN_TARGETS) - def test_explicit_list_unknown_targets_falls_back_to_copilot(self): + def test_explicit_list_all_unknown_returns_empty(self): + """When the parser is bypassed and all tokens are unknown, the + result is an empty list -- the old asymmetric ``[copilot]`` fallback + was removed in #820 because the parser + (:func:`apm_cli.core.target_detection.parse_target_field`) now + rejects unknown tokens at the entry point.""" targets = active_targets(self.root, explicit_target=["nonexistent", "bogus"]) - assert [t.name for t in targets] == ["copilot"] + assert targets == [] def test_explicit_list_mixed_known_unknown(self): - """Known targets are included, unknown ones are silently skipped.""" + """Known targets are included, unknown ones are skipped (no fallback). + + In normal use the parser rejects this input upstream; this test + exercises the post-parser invariant that the loop only adds known + profiles. + """ targets = active_targets(self.root, explicit_target=["claude", "nonexistent"]) assert [t.name for t in targets] == ["claude"] diff --git a/tests/unit/test_apm_package.py b/tests/unit/test_apm_package.py index f7cc19fd5..889224c86 100644 --- a/tests/unit/test_apm_package.py +++ b/tests/unit/test_apm_package.py @@ -225,7 +225,9 @@ def test_target_string(self, tmp_path): assert isinstance(pkg.target, str) def test_target_list(self, tmp_path): - """target: [claude, copilot] → stored as list.""" + """``target: [claude, copilot]`` is now alias-resolved through the + shared parser -- ``copilot`` collapses to its canonical name + ``vscode`` (#820). Multi-target lists stay as lists.""" yml = _write_apm_yml(tmp_path, { "name": "test-pkg", "version": "1.0.0", @@ -234,7 +236,7 @@ def test_target_list(self, tmp_path): pkg = APMPackage.from_apm_yml(yml) - assert pkg.target == ["claude", "copilot"] + assert pkg.target == ["claude", "vscode"] assert isinstance(pkg.target, list) def test_target_missing(self, tmp_path): @@ -249,7 +251,10 @@ def test_target_missing(self, tmp_path): assert pkg.target is None def test_target_single_item_list(self, tmp_path): - """target: [copilot] → stored as single-element list.""" + """A single-element list (``target: [copilot]``) collapses to a + plain string -- the shared parser canonicalizes ``str`` and + ``[str]`` to the same shape so downstream code only ever sees one + ``Union[str, List[str]]`` form per cardinality (#820).""" yml = _write_apm_yml(tmp_path, { "name": "test-pkg", "version": "1.0.0", @@ -258,8 +263,8 @@ def test_target_single_item_list(self, tmp_path): pkg = APMPackage.from_apm_yml(yml) - assert pkg.target == ["copilot"] - assert isinstance(pkg.target, list) + assert pkg.target == "copilot" + assert isinstance(pkg.target, str) def test_target_direct_construction_string(self): """APMPackage can be constructed with target as string."""