fix(install): accept YAML list form under singular target: key (#1188)

[danielmeppiel]

Fixes #1188.

TL;DR

apm install now accepts target: [copilot, claude] (YAML flow-list) and the equivalent block-list form under the canonical singular target: key. Before this PR, the install pipeline crashed with Unknown target '['copilot'' while apm install --dry-run silently accepted the same config.

Problem (WHY)

Issue #1188 reports that this perfectly valid apm.yml:

target:
  - copilot
  - claude

crashes apm install with:

[x] [x] Unknown target '['copilot''
...

Two bugs surface at once:

1. The parser bug. apm_cli.core.apm_yml.parse_targets_field does str(raw).strip() then comma-splits. When raw is a Python list, str([...]) yields the literal repr "['copilot', 'claude']", splitting on , yields "['copilot'", " 'claude']", and the first token fails validation. The targets: (plural) branch handles lists correctly — only the singular branch was broken.
2. The dry-run lies. The dry-run path goes through a different parser (core.target_detection.parse_target_field) which DOES handle lists. So apm install --dry-run silently passes on the exact apm.yml that real apm install rejects. Anyone using dry-run to validate config was misled.

The error rendering also had three coupled UX paper cuts surfaced by this same code path:

• Doubled [x] [x] prefix (renderer prepends one, logger.error prepends another).
• Garbled '['copilot'' headline with unbalanced quotes.
• The "did you mean?" suggestion is valid[0] after sorted(CANONICAL_TARGETS) = agent-skills — actively misleading when the user typed copilot/claude.

Approach (WHAT)

Surgical fix in parse_targets_field (Plan A, ratified by the devx-ux expert) plus the three tightly-coupled error-rendering fixes the cli-logging-ux expert flagged on the same code path. Eliminating the duplicate parser entirely (Plan B) is the right architectural move — core.target_detection.parse_target_field's docstring already says "Single source of truth" — but it's a refactor with non-trivial blast radius (different return-type contract, different alias-resolution behavior, different empty-list semantics) and belongs in its own PR. Filed as follow-up.

The fail-fast issue (target validation runs after 13s of dependency resolution) and a proper fuzzy-match did_you_mean helper are also follow-ups; they're feature work, not bug fixes.

Implementation (HOW)

1. src/apm_cli/core/apm_yml.py — parser

In the if has_target: branch, detect isinstance(raw, list) and tokenize the same way the targets: (plural) branch does. Empty list under the singular key falls through to auto-detect (matches target: with no value); only the plural targets: [] raises EmptyTargetsListError, which is the explicit-but-empty contradictory case.

2. src/apm_cli/core/errors.py — render_unknown_target_error

• Strip [, ], ', ", and spaces from the headline value (defense-in-depth for any future garbled-token leaks).
• Use copilot as the suggested fix (or valid[0] as fallback if copilot somehow isn't in the canonical set), instead of valid[0] which after sorted(CANONICAL_TARGETS) resolves to agent-skills.

3. src/apm_cli/install/phases/targets.py — double [x] fix

Two ctx.logger.error(str(exc)) sites (lines 212, 251) were emitting already-rendered error blocks (which start with [x]) through logger.error(), which prepends another [x] via the default symbol="error". Pass symbol=None to suppress the second prefix. The renderer's leading symbol is preserved.

Tests

• tests/unit/core/test_target_resolution_v2.py — 8 new parser cases:
  • Two-item flow list (['copilot', 'claude'])
  • Single-item list (['copilot'] parity with scalar target: copilot)
  • Whitespace tolerance in list elements
  • Empty list under singular key falls through to auto-detect
  • Unknown token in list surfaces a clean (non-list-repr) headline
  • Non-string list element coerces via str() and validates
  • 'all' mixed with canonical token is rejected (no escape hatch)
  • Duplicates preserved (parser does not dedup; matches plural branch)
• tests/unit/core/test_error_renderer.py — 2 new renderer cases:
  • Suggestion is copilot, not the alphabetically-first agent-skills
  • Headline value is sanitized when input contains bracket/quote noise
• tests/unit/install/phases/test_read_yaml_targets_list_form.py — new file, 6 cases on the install-pipeline read path:
  • Flow list (target: [a, b])
  • Block list (target:\n - a\n - b)
  • CSV form regression guard
  • Scalar form regression guard
  • Cross-parser shape agreement (regression-trap for dry-run-vs-real divergence)
  • Unknown token in YAML list surfaces a clean error headline

Validation

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
All checks passed!
706 files already formatted

$ uv run --extra dev pytest tests/unit/core/ tests/unit/install/ -q
910 passed in 4.79s

Manual repro on the issue's exact apm.yml:

$ python -c "from apm_cli.core.apm_yml import parse_targets_field; from apm_cli.utils.yaml_io import load_yaml; print(parse_targets_field(load_yaml(Path('apm.yml'))))"
['copilot', 'claude']

(Pre-fix on the same file: Unknown target '['copilot'' with unbalanced quotes and [x] [x] doubled prefix.)

Trade-offs

• Plan A vs Plan B. Plan B (eliminate the duplicate parser, have _read_yaml_targets delegate to parse_target_field) is architecturally correct but blast-radius large. Plan A is 6 lines of correctness fix in one branch of one function. Filing Plan B as tech-debt with a link back here.
• Bundled the three error-rendering fixes rather than a separate PR. Pure-cosmetic logging fixes belong in a separate PR (per the devx-ux expert), but these three are direct consequences of the parser bug on the same code path — splitting them would require this PR's tests to assert against the broken renderer, which is awkward. Trade-off accepted: the diff stays small (229 / 6) and the revert remains clean.

Follow-ups (separate issues)

• Plan B: consolidate target parsing onto core.target_detection.parse_target_field so the install path and dry-run path share a single source of truth.
• Fail-fast: target validation currently runs after dependency resolution (~13s wasted on the issue repro). Move it earlier in the install pipeline.
• Fuzzy did_you_mean: a real difflib.get_close_matches()-backed suggestion helper. Hardcoded copilot fallback is fine for now.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This pull request fixes apm install failing to parse a YAML list under the singular target: key in apm.yml, aligning install behavior with the dry-run parser and improving the unknown-target error output.

Changes:

• Extend parse_targets_field() to accept target: [a, b] / block-list forms under the singular target: key.
• Improve unknown-target error rendering (sanitize headline value; prefer copilot as the suggested fix).
• Add unit tests covering singular-key YAML list parsing and the install-pipeline read path; adjust install-phase logging to avoid double [x] prefixes.

Show a summary per file

File	Description
src/apm_cli/core/apm_yml.py	Accept YAML list values under singular target: and validate tokens correctly.
src/apm_cli/core/errors.py	Sanitize unknown-target headline and adjust suggestion selection.
src/apm_cli/install/phases/targets.py	Suppress double-prefixing when logging already-rendered [x] errors.
tests/unit/core/test_target_resolution_v2.py	Add regression tests for singular target: YAML list parsing and edge cases.
tests/unit/core/test_error_renderer.py	Add tests for improved unknown-target rendering and suggestion choice.
tests/unit/install/phases/test_read_yaml_targets_list_form.py	New tests exercising install pipeline YAML read path for list/scalar/CSV forms.
CHANGELOG.md	Add an Unreleased entry describing the fix.

Copilot's findings

• Files reviewed: 7/7 changed files
• Comments generated: 4

[danielmeppiel]

Addressed all four Copilot review comments in 71c982e1:

1. symbol=None → symbol="" (targets.py:215, :255): honors CommandLogger.error's symbol: str = "error" type contract while still suppressing the double-prefix. Empty string isn't in STATUS_SYMBOLS, so no prefix is prepended.
2. valid[0] → sorted(valid)[0] (errors.py): both the displayed valid_csv and the fallback suggestion now derive from a single valid_sorted variable. Deterministic regardless of caller input order.
3. Empty-after-strip headline: display_value = value.strip(...) or value or "<empty>" so a payload of pure noise ("[]'\"") no longer renders Unknown target ''. New test test_unknown_target_error_falls_back_when_strip_empties_value guards this.
4. CHANGELOG entry shortened to one concise line per repo convention.

Lint clean, all 62 affected unit tests still pass.