Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Stabilized `test_install_over_defer_threshold_starts_live_once` on slow CI runners by joining the deferred-start timer thread instead of relying on a 100ms grace window. (#1191)
- `triage-panel` scheduled sweep now paginates the candidate query oldest-first via the GitHub MCP `list_issues` tool instead of a single 200-issue page, so daily runs actually drain the untriaged backlog rather than processing one issue per cron tick. (#1193)
- `triage-panel` scheduled sweep switches the candidate query from `list_issues`+prose-driven pagination to `search_issues` with `-label:status/triaged sort:created-asc`, so untriaged candidates are filtered server-side; the previous approach silently noop'd because the MCP gateway DIFC filter dropped non-collaborator issues mid-page and the agent inferred a false `hasNextPage:false`.
- `apm install` now accepts the YAML list form under `target:` (e.g. `target: [copilot, claude]`); previously crashed with a garbled `Unknown target` error. (#1197)

## [0.12.4] - 2026-05-07

Expand Down
10 changes: 10 additions & 0 deletions src/apm_cli/core/apm_yml.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- 'targets: [a, b]' -> ['a', 'b'] (canonical, plural)
- 'target: a' -> ['a'] (singular sugar)
- 'target: "a,b"' -> ['a', 'b'] (CSV sugar)
- 'target: [a, b]' -> ['a', 'b'] (list sugar under singular key, #1188)
- both present -> raise ConflictingTargetsError
- neither present -> [] (empty = auto-detect upstream)

Expand Down Expand Up @@ -85,6 +86,15 @@ def parse_targets_field(yaml_data: dict) -> list[str]:
raw = yaml_data["target"]
if raw is None:
return []
if isinstance(raw, list):
# YAML list sugar: 'target: [claude, copilot]' or block list.
# Empty list under singular key falls through to auto-detect
# (consistent with 'target:' with no value).
tokens = [str(t).strip() for t in raw if str(t).strip()]
if not tokens:
return []
_validate_canonical(tokens)
return tokens
raw_str = str(raw).strip()
if not raw_str:
return []
Expand Down
18 changes: 14 additions & 4 deletions src/apm_cli/core/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,22 +116,32 @@ def render_ambiguous_error(project_root: Path | None, detected: list[str]) -> st

def render_unknown_target_error(value: str, valid: list[str]) -> str:
"""Render the 3-section error for unknown target token."""
valid_csv = ", ".join(sorted(valid))
valid_sorted = sorted(valid)
valid_csv = ", ".join(valid_sorted)
# Strip bracket/quote noise that can leak in from misparsed tokens
# (e.g. "['copilot'"). Defense-in-depth: callers should pass clean
# values, but this keeps the headline readable if they don't. Fall
# back to the raw value (or "<empty>") if stripping consumes
# everything, so the headline remains actionable.
display_value = value.strip("[]'\" ") or value or "<empty>"
suggestion = (
"copilot" if "copilot" in valid_sorted else (valid_sorted[0] if valid_sorted else "claude")
)
return (
f"[x] Unknown target '{value}'\n"
f"[x] Unknown target '{display_value}'\n"
"\n"
Comment on lines +121 to 132
f"Valid targets: {valid_csv}\n"
"\n"
"Fix with one of:\n"
"\n"
" apm targets # see all supported harnesses\n"
f" apm install <pkg> --target {valid[0] if valid else 'claude'}\n"
f" apm install <pkg> --target {suggestion}\n"
" apm install <pkg> --dry-run\n"
"\n"
"Or declare in apm.yml:\n"
"\n"
" targets:\n"
f" - {valid[0] if valid else 'claude'}"
f" - {suggestion}"
)


Expand Down
10 changes: 7 additions & 3 deletions src/apm_cli/install/phases/targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,10 @@ def run(ctx: InstallContext) -> None:
except _click.UsageError as exc:
# ConflictingTargetsError (both target: and targets: in
# apm.yml) is a user error -- surface with exit code 2.
# The renderer already emits a leading "[x]"; pass an
# empty symbol so logger.error doesn't double-prefix.
if ctx.logger:
ctx.logger.error(str(exc))
ctx.logger.error(str(exc), symbol="")
raise SystemExit(2) from exc

# Skip v2 entirely when all override targets were non-canonical
Expand Down Expand Up @@ -246,9 +248,11 @@ def run(ctx: InstallContext) -> None:
# is a false positive because v2 does not handle
# non-canonical targets. That case is already
# guarded by ``_skip_v2`` above, so it never reaches
# this except block.
# this except block. The renderer already emits a
# leading "[x]"; pass an empty symbol so logger.error
# doesn't double-prefix.
if ctx.logger:
ctx.logger.error(str(exc))
ctx.logger.error(str(exc), symbol="")
raise SystemExit(2) from exc

# Emit provenance BEFORE any mutation. Route via _rich_info so
Expand Down
31 changes: 31 additions & 0 deletions tests/unit/core/test_error_renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,37 @@ def test_unknown_target_error_lists_valid():
assert "claude" in text


def test_unknown_target_error_suggests_copilot_not_first_alphabetical():
"""Suggestion must be a sensible default (#1188), not sorted-first."""
valid = ["agent-skills", "claude", "copilot", "cursor"]
text = render_unknown_target_error("foo", valid)
# 'agent-skills' is alphabetically first but is the wrong default to
# surface to a user who typed 'foo'. Prefer 'copilot'.
assert "--target copilot" in text
assert " - copilot" in text
assert "--target agent-skills" not in text


def test_unknown_target_error_sanitizes_garbled_value():
"""Headline must not show Python list-repr noise (#1188)."""
# Simulates the pre-fix garbled token "['copilot'" leaking through.
text = render_unknown_target_error("['copilot'", ["claude", "copilot"])
headline = text.splitlines()[0]
# Bracket and quote noise is stripped from the headline value.
assert headline == "[x] Unknown target 'copilot'"


def test_unknown_target_error_falls_back_when_strip_empties_value():
"""If sanitization removes everything, fall back so headline stays actionable."""
# All-noise input: stripping yields empty string.
text = render_unknown_target_error("[]'\"", ["claude", "copilot"])
headline = text.splitlines()[0]
# Must not render `Unknown target ''`. We accept either the raw
# (un-stripped) value or a `<empty>` placeholder.
assert headline != "[x] Unknown target ''"
assert "Unknown target '" in headline


def test_conflicting_schema_error_has_three_parts():
text = render_conflicting_schema_error()
_assert_three_sections(text, ["cannot use both", "conflicting"])
Expand Down
62 changes: 62 additions & 0 deletions tests/unit/core/test_target_resolution_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,68 @@ def test_unknown_target_rejected():
assert "claude" in msg or "copilot" in msg


# ---------------------------------------------------------------------------
# YAML list under 'target:' singular key (#1188)
# ---------------------------------------------------------------------------


def test_target_singular_with_yaml_list_two_items():
"""Regression: 'target: [copilot, claude]' YAML flow-list form (#1188)."""
out = parse_targets_field({"target": ["copilot", "claude"]})
assert sorted(out) == ["claude", "copilot"]


def test_target_singular_with_yaml_list_single_item():
"""Single-item list under 'target:' must equal scalar form."""
out = parse_targets_field({"target": ["copilot"]})
assert out == ["copilot"]


def test_target_singular_with_yaml_list_whitespace_tolerated():
"""List elements with surrounding whitespace are stripped."""
out = parse_targets_field({"target": [" copilot ", "claude\t"]})
assert sorted(out) == ["claude", "copilot"]


def test_target_singular_with_empty_list_falls_through_to_autodetect():
"""'target: []' under SINGULAR key returns [] (auto-detect upstream),
matching 'target:' with no value. Only PLURAL 'targets: []' raises."""
out = parse_targets_field({"target": []})
assert out == []


def test_target_singular_with_yaml_list_unknown_token_rejected():
"""Garbled tokens from list parsing must surface a clean error."""
with pytest.raises(UnknownTargetError) as exc_info:
parse_targets_field({"target": ["nonsense"]})
msg = str(exc_info.value)
headline = msg.splitlines()[0]
# Headline must contain the bare token, not a Python list repr.
assert "'nonsense'" in headline
# No list-repr leakage: the leading "[x]" symbol is fine, but no
# "['nonsense'" or similar should appear as the value.
assert "['nonsense'" not in headline
assert '["nonsense"' not in headline


def test_target_singular_with_yaml_list_non_string_coerced():
"""Non-string list elements coerce via str() and are validated."""
with pytest.raises(UnknownTargetError):
parse_targets_field({"target": [42]})


def test_target_singular_with_all_token_in_list_rejected():
"""'all' is a CLI flag-only meta-target; must not validate inside YAML."""
with pytest.raises(UnknownTargetError):
parse_targets_field({"target": ["all", "claude"]})


def test_target_singular_with_yaml_list_preserves_duplicates():
"""Duplicates are preserved (parser does not dedup)."""
out = parse_targets_field({"target": ["copilot", "copilot"]})
assert out == ["copilot", "copilot"]


# ---------------------------------------------------------------------------
# --target all expansion
# ---------------------------------------------------------------------------
Expand Down
121 changes: 121 additions & 0 deletions tests/unit/install/phases/test_read_yaml_targets_list_form.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
"""Regression tests for #1188: install pipeline must accept YAML
list form under the singular 'target:' key.

Before the fix, the install path (``_read_yaml_targets`` ->
``parse_targets_field``) called ``str(raw)`` on a Python list, producing
the literal repr ``"['copilot', 'claude']"`` and then comma-splitting
it into garbled tokens (``"['copilot'"``, ``"'claude']"``). The dry-run
path used a different parser and silently passed, masking the bug.

These tests exercise the install-pipeline read path directly so that
any future divergence between the dry-run parser
(``target_detection.parse_target_field``) and the install parser
(``apm_yml.parse_targets_field``) is caught at unit-test level.
"""

from __future__ import annotations

from pathlib import Path
from unittest.mock import MagicMock

import pytest


def _ctx_with_apm_yml(tmp_path: Path, body: str) -> MagicMock:
project = tmp_path / "proj"
project.mkdir()
(project / "apm.yml").write_text(body, encoding="utf-8")
ctx = MagicMock()
ctx.apm_package = MagicMock()
ctx.apm_package.package_path = project
return ctx


def test_read_yaml_targets_accepts_flow_list_under_singular_key(tmp_path):
"""target: [copilot, claude] (flow style) must parse to a list."""
from apm_cli.install.phases.targets import _read_yaml_targets

ctx = _ctx_with_apm_yml(
tmp_path,
"name: demo\nversion: 0.1.0\ntarget: [copilot, claude]\n",
)
result = _read_yaml_targets(ctx)
assert result is not None
assert sorted(result) == ["claude", "copilot"]


def test_read_yaml_targets_accepts_block_list_under_singular_key(tmp_path):
"""target:\\n - copilot\\n - claude (block style) must parse to a list."""
from apm_cli.install.phases.targets import _read_yaml_targets

ctx = _ctx_with_apm_yml(
tmp_path,
"name: demo\nversion: 0.1.0\ntarget:\n - copilot\n - claude\n",
)
result = _read_yaml_targets(ctx)
assert result is not None
assert sorted(result) == ["claude", "copilot"]


def test_read_yaml_targets_csv_form_still_works(tmp_path):
"""Regression guard: CSV form 'target: copilot,claude' continues to work."""
from apm_cli.install.phases.targets import _read_yaml_targets

ctx = _ctx_with_apm_yml(
tmp_path,
'name: demo\nversion: 0.1.0\ntarget: "copilot,claude"\n',
)
result = _read_yaml_targets(ctx)
assert result is not None
assert sorted(result) == ["claude", "copilot"]


def test_read_yaml_targets_scalar_form_still_works(tmp_path):
"""Regression guard: scalar 'target: copilot' continues to work."""
from apm_cli.install.phases.targets import _read_yaml_targets

ctx = _ctx_with_apm_yml(
tmp_path,
"name: demo\nversion: 0.1.0\ntarget: copilot\n",
)
result = _read_yaml_targets(ctx)
assert result == ["copilot"]


def test_read_yaml_targets_install_and_dry_run_parsers_agree(tmp_path):
"""Both the install parser (parse_targets_field) and the dry-run
parser (parse_target_field) must ACCEPT the YAML list form without
crashing and return 2 targets. The two parsers canonicalize aliases
differently (e.g. copilot vs vscode), but that's tracked separately
as part of consolidating onto a single parser. The point of this
regression-trap is that both code paths agree on shape, not naming.
"""
from apm_cli.core.apm_yml import parse_targets_field
from apm_cli.core.target_detection import parse_target_field

yaml_value = ["copilot", "claude"]
install_result = parse_targets_field({"target": yaml_value})
dry_run_result = parse_target_field(yaml_value)

assert len(install_result) == 2
assert len(dry_run_result) == 2
# claude is canonical in both parsers; this is the stable cross-check.
assert "claude" in install_result
assert "claude" in dry_run_result


def test_read_yaml_targets_unknown_token_in_list_raises_clean_error(tmp_path):
"""Unknown token inside a YAML list must surface a readable headline,
not a Python list-repr fragment like '['copilot''.
"""
from apm_cli.core.errors import UnknownTargetError
from apm_cli.install.phases.targets import _read_yaml_targets

ctx = _ctx_with_apm_yml(
tmp_path,
"name: demo\nversion: 0.1.0\ntarget: [bogus]\n",
)
with pytest.raises(UnknownTargetError) as exc_info:
_read_yaml_targets(ctx)
headline = str(exc_info.value).splitlines()[0]
assert headline == "[x] Unknown target 'bogus'"
Loading