fix(targets): validate apm.yml target: at parse time, share normalization with CLI (closes #820)

[edenfunf]

TL;DR

apm install and apm compile exited 0 with success messages while deploying nothing whenever target: in apm.yml was a CSV string (e.g. target: opencode,claude,copilot,agents). The CLI's --target was already validated by TargetParamType, but apm.yml bypassed it entirely — data.get('target') flowed through unchanged, and downstream resolution silently produced []. This PR introduces a single shared parser (parse_target_field) used by both entry points, removes the asymmetric [copilot] fallback, and fails loudly on unknown / empty / all-mixed inputs.

Important

This is a manifest-contract change. Three previously-silent inputs now raise at parse time (see Trade-offs and the CHANGELOG ### Changed entry).

Problem (WHY)

The bug spans three layers, each compounding the previous:

• Parser bypass: apm_package.py from_apm_yml stored target=data.get('target') untouched. The Union[str, List[str]] type hint was advisory — no validation, no normalization.
• Asymmetric resolution: integration/targets.py had two branches; the list branch fell back to [copilot] for all-unknown input, but the single-string branch returned []. A CSV string never matched any known token, hit the string branch, and silently produced [].
• Silent log gate: install/phases/targets.py:52 read if ctx.logger and _targets: — when _targets == [] the "Active targets:" line never printed, so even --verbose gave no signal.
• Swallowed parse errors: commands/compile/cli.py:389 wrapped APMPackage.from_apm_yml in try / except Exception: pass, so any future parse failure would be silently routed to auto-detect.
• [!] Unconditional success message: apm compile printed [+] Compilation completed successfully! even when zero files were emitted — the worst-case package-manager DX.

Why these matter together: the user's apm.yml had target: opencode,claude,copilot,agents (CSV); install/compile both exited 0; .opencode/, .claude/, .github/skills/ were empty. The chain is documented in the maintainer's review-panel verdict on #820 which approved the C+ direction (shared normalization + fail-hard at parse + spec revision).

Approach (WHAT)

#	Fix
1	New parse_target_field(value, *, source_path=None) next to TargetParamType — the single source of truth for the target field.
2	TargetParamType.convert becomes a thin delegator; APMPackage.from_apm_yml calls the parser with the apm.yml path so error messages name the file.
3	active_targets and active_targets_user_scope canonicalize string-or-list to one code path; the asymmetric else [copilot] fallback is removed.
4	phases/targets.py always emits — empty resolved targets routes through logger.warning instead of falling silent.
5	compile/cli.py drops the except Exception: pass swallow and gates the success message on sum(stats.*_files_written) > 0.
6	manifest-schema.md spec line revised: Unknown values MUST raise a parse error (previously: silently ignored). CHANGELOG ### Changed + migration note covers the three breaking inputs.

Implementation (HOW)

• src/apm_cli/core/target_detection.py — adds parse_target_field (same Union return shape as the old TargetParamType.convert for backward compat) and a _target_error helper that prefixes errors with Invalid 'target' in <path>: when a source path is supplied. TargetParamType.convert delegates to the parser, catching ValueError and routing through Click's self.fail().
• src/apm_cli/models/apm_package.py — wires parse_target_field(data.get('target'), source_path=apm_yml_path) into from_apm_yml. Import is at module top; verified no circular dependency.
• src/apm_cli/integration/targets.py — active_targets and active_targets_user_scope collapse string→list at the top of the explicit-target branch; the else [copilot] fallback is dead code once the parser is upstream and is removed. Module docstring captures the resolver invariant once instead of duplicating the comment in both functions.
• src/apm_cli/install/phases/targets.py — log gate becomes if ctx.logger:; non-empty path unchanged, empty path emits logger.warning.
• src/apm_cli/commands/compile/cli.py — try/except removed; replaced with explicit if Path(APM_YML_FILENAME).exists():. Success message gated on a pattern-based scan (endswith(("_files_written", "_files_generated"))) so future targets pick up the guard automatically.
• docs/src/content/docs/reference/manifest-schema.md — spec line 116 rewritten per maintainer brief.
• CHANGELOG.md — ### Changed entry with migration note covering all three breaking inputs (unknown token / empty value / all mixed).

Diagrams

Legend: how a target: value flows from either entry point through the shared parser to deployment, and where the previous bug short-circuited to silent zero-deployment.

flowchart TD
    A["apm.yml target: opencode,claude,copilot,agents"] --> B[APMPackage.from_apm_yml]
    C["--target opencode,claude,copilot,agents"] --> D[TargetParamType.convert]
    B --> E[parse_target_field]
    D --> E
    E -->|"valid: [opencode, claude, vscode]"| F[active_targets]
    E -->|"unknown / empty / all+X"| G["ValueError -> exit 1"]
    F --> H["TargetProfile list"]
    H --> I[skill_integrator deploy]
    I --> J[".claude/skills, .github/skills, .opencode/skills"]

    K["BEFORE #820: data.get('target') = raw CSV string"] -.-> L["active_targets single-string branch"]
    L -.-> M["KNOWN_TARGETS.get('opencode,claude,copilot,agents') = None -> []"]
    M -.-> N["silent zero-deployment, exit 0"]

    style G fill:#ffd6d6,stroke:#c00
    style N fill:#ffd6d6,stroke:#c00
    style J fill:#d6ffd6,stroke:#0a0

Loading

Trade-offs

• Behavior change in three previously-silent inputs. target: "", target: [], and target: [all, claude] now raise instead of falling through to auto-detect. Migration note in CHANGELOG covers the fix (omit the field for auto-detect). Pre-1.0 is the right window for this contract change per the maintainer's verdict.
• Single-token alias resolution intentionally NOT changed. --target copilot still returns "copilot" (not the canonical "vscode"). Every downstream consumer (active_targets, agents_compiler, _CROSS_TARGET_MAPS, _TARGET_PREFIXES) already accepts both spellings, and changing this would visibly break ~10 test_single_* CLI cases for zero functional benefit. This is the one asymmetry the "shared normalization" fix intentionally leaves; collapsing it is an independent decision.
• YAML line/column pinning not implemented. The maintainer brief said "when the loader gives one"; the current yaml.safe_load does not expose Mark objects. Error messages name the apm.yml path (matching the existing from_apm_yml error format at line 198 — f"Invalid 'type' field in apm.yml: {e}"). Switching to a Mark-aware loader is out of scope and a separate decision.
• active_targets_user_scope cleaned in the same PR. Not on the maintainer's explicit list, but it carried the identical asymmetric bug (mirror of active_targets), and the user-scope test test_explicit_unknown_returns_empty was on the update list — fixing only one would leave the asymmetry the PR exists to remove.

Benefits

1. apm install / apm compile no longer exit 0 when zero files deploy. Either the value parses and integration runs, or the parser raises with the offending token + apm.yml path.
2. One validation code path. --target X and target: X now resolve identically and reject the same inputs; the bug class is structurally eliminated.
3. Defense in depth on the log surface. Even if parse succeeds and resolution somehow yields zero targets, phases/targets.py warns instead of falling silent; compile only claims success when files were actually emitted.
4. Better error messages. Users see the file path, the offending token, and the valid options in one line — no need to diff against the spec.
5. Spec / code agreement restored. manifest-schema.md line 116 previously said "silently ignored"; the codebase enforced that for the string branch and a different rule (fallback to copilot) for the list branch. Both now say the same thing.

Validation

python -m pytest tests/unit tests/test_apm_package_models.py:

5707 passed, 1 skipped, 1 warning, 27 subtests passed in 39.97s

End-to-end reproduction of the original #820 scenario (CSV target with a local .apm/instructions/style.instructions.md primitive):

$ apm install --update --verbose
Active project targets: opencode, claude, copilot
Created .opencode/ (opencode target)
Created .claude/ (claude target)
  |-- 1 rule(s) integrated -> .claude/rules/
  |-- 1 instruction(s) integrated -> .github/instructions/
Deployed 2 local primitive(s) from .apm/
[*] Installed 1 APM dependency.

$ ls .claude/rules/ .github/instructions/
.claude/rules/style.md
.github/instructions/style.instructions.md

Fail-hard cases (unknown token, empty string, empty list, all+other)

$ # target: claude,bogus,copilot
$ apm install --update
[x] Failed to parse apm.yml: Invalid 'target' in C:\tmp\820-test\apm.yml: 'bogus' is not a valid target. Choose from: agents, all, claude, codex, copilot, cursor, gemini, opencode, vscode
exit 1

$ # target: ""
[x] Failed to parse apm.yml: Invalid 'target' in C:\tmp\820-test\apm.yml: target value must not be empty
exit 1

$ # target: []
[x] Failed to parse apm.yml: Invalid 'target' in C:\tmp\820-test\apm.yml: target value must not be empty
exit 1

$ # target: [all, claude]
[x] Failed to parse apm.yml: Invalid 'target' in C:\tmp\820-test\apm.yml: 'all' cannot be combined with other targets
exit 1

--target CLI flag still rejects unknown with Click's standard error (exit 2)

$ apm install --target bogus
Usage: apm install [OPTIONS] [PACKAGES]...
Try 'apm install --help' for help.

Error: Invalid value for '--target' / '-t': Invalid target: 'bogus' is not a valid target. Choose from: agents, all, claude, codex, copilot, cursor, gemini, opencode, vscode
exit 2

How to test

• In a fresh dir, write an apm.yml with target: opencode,claude,copilot,agents and one local primitive under .apm/instructions/. Run apm install --update --verbose. Observe Active project targets: opencode, claude, copilot and files deployed under .claude/rules/ and .github/instructions/.
• Replace with target: claude,bogus. Run apm install --update. Observe exit 1 with the bad token and the apm.yml path in the message.
• Replace with target: "", then target: [], then target: [all, claude] — each must exit 1 with a clear reason.
• Remove the target: line. Run apm install --update. Auto-detect from existing dirs still works (e.g. .github/ → copilot).
• python -m pytest tests/unit tests/test_apm_package_models.py — full unit suite green.

[github-actions]

APM Review Panel Verdict

Disposition: APPROVE (with two minor pre-merge fixes)

---

Per-persona findings

Python Architect:

The PR is a targeted fix with one new pure function, two call-site updates, and a fallback removal. Not a major architectural change; one class diagram + one flow diagram apply.

1. OO / class diagram

classDiagram
    direction LR
    class APMPackage {
        <<Dataclass>>
        +target Union[str, List[str], None]
        +from_apm_yml(path) APMPackage
    }
    class TargetParamType {
        <<ClickParamType>>
        +convert(value, param, ctx)
    }
    class parse_target_field {
        <<Pure>>
        +value Union[str,List,None]
        +source_path Optional[Path]
        returns Union[str,List,None]
    }
    class normalize_target_list {
        <<Pure>>
        +value Union[str,List,None]
        returns Optional[List[str]]
    }
    class detect_target {
        <<IOBoundary>>
        +project_root Path
        returns Tuple[TargetType, str]
    }
    class active_targets {
        <<Pure>>
        +project_root Path
        +explicit_target Union[str,List,None]
        returns List[TargetProfile]
    }
    class active_targets_user_scope {
        <<Pure>>
        +explicit_target Union[str,List,None]
        returns List[TargetProfile]
    }

    TargetParamType ..> parse_target_field : delegates (new)
    APMPackage ..> parse_target_field : calls at parse time (new)
    active_targets ..> normalize_target_list : uses
    active_targets_user_scope ..> normalize_target_list : uses
    detect_target ..> APMPackage : indirectly consumes via caller

    class parse_target_field:::touched
    class TargetParamType:::touched
    class APMPackage:::touched
    class active_targets:::touched
    class active_targets_user_scope:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

Note: parse_target_field is the new SSOT validator; both CLI and manifest entry points converge on it. normalize_target_list (used by the integration layer) is left unchanged -- the PR intentionally keeps the two normalization layers separate pending a follow-up cleanup of detect_target.

2. Execution flow diagram

flowchart TD
    A([User: apm install / apm compile]) --> B{Entry point?}

    B -->|--target flag| C[TargetParamType.convert\ncommands/compile/cli.py]
    B -->|apm.yml present| D[APMPackage.from_apm_yml\nmodels/apm_package.py]
    B -->|neither| E[auto-detect from folders\ncore/target_detection.detect_target]

    C --> F[parse_target_field\ncore/target_detection.py]
    D --> F

    F --> G{Valid tokens?}
    G -->|No| H[ValueError:\nInvalid target in path: bogus...\nChoose from: agents,all,claude...]
    G -->|Empty| H
    G -->|all+others| H
    H -->|compile outer except| I[logger.error: Error during compilation: ...]
    H -->|install pipeline| J[propagates as RuntimeError\ninstall/pipeline.py]

    G -->|Yes| K{Single token?}
    K -->|Yes| L[return token as-is\nno alias resolution]
    K -->|No, multi-token CSV or list| M[resolve aliases + dedupe\nreturn List or collapsed str]

    L --> N[install/phases/targets.py\nresolve_targets]
    M --> N
    E --> N

    N --> O{Targets list empty?}
    O -->|Yes| P[ctx.logger.warning:\nNo scope targets resolved --\nnothing will be deployed]
    O -->|No| Q[ctx.logger.verbose_detail:\nActive targets: ...]

    Q --> R[integrators initialized\nFS mkdir for target dirs]
    R --> S([deploy / compile output])

    S --> T{compile: files written > 0?}
    T -->|Yes| U[logger.success:\nCompilation completed successfully!]
    T -->|No| V[logger.warning:\nCompilation completed but produced no output files.\nCheck target dirs or set target:]

Loading

3. Design patterns

Pattern	Location	Notes
Pure validator (SSOT)	parse_target_field() in core/target_detection.py	3+ call sites justified extraction per "Abstract when 3+ call sites" rule
Fail-fast / whitelist	VALID_TARGET_VALUES frozenset + ValueError	Static whitelist -- correct approach
Delegation	TargetParamType.convert() -> parse_target_field()	Removed 40-line duplicated logic
Defensive warning	phases/targets.py, compile/cli.py	Surface warnings instead of silent success when output is zero

One structural concern (non-blocking): detect_target() still carries its own alias-resolution if/elif chain that is now logically superseded by parse_target_field(). The call at the bottom of phases/targets.py is explicitly marked "return values are not consumed by downstream code" yet remains in the code path. This is dead code that drifts from the SSOT it was supposed to consolidate. Should be removed or replaced in a follow-up.

One design smell (non-blocking): Single-token input is intentionally NOT alias-resolved ("copilot" stays "copilot"), while multi-token input IS alias-resolved ("copilot,claude" yields ["vscode","claude"]). The PR documents this asymmetry carefully and justifies preserving the existing CLI contract, but it is a semantic inconsistency that future callers will stumble on. Track for a dedicated follow-up.

---

CLI Logging Expert:

Output routing is correct across all new paths. Key findings:

• CommandLogger.warning() has symbol: str = "warning" as default -- the two new logger.warning() calls in compile/cli.py (line ~508) and phases/targets.py omit symbol= but will correctly emit [!]. No inconsistency.
• The success-message gate (_files_written = sum(...) over result.stats.items()) uses logger.success() and logger.warning() via CommandLogger -- never direct _rich_*. Correct.
• The _files_written stat-key heuristic (keys ending in _files_written or _files_generated) is slightly fragile: if a new stat key is added with a different suffix, the gate silently degrades. A CompilationResult.files_written_count() method would be more durable. Non-blocking for this PR.
• Warning messages are actionable: both the zero-targets warning and the zero-output warning name what to do next. Passes the "So What?" test.
• The ValueError from parse_target_field() surfaces through compile's outer except Exception as e: logger.error(f"Error during compilation: {e}") -- users see a clean formatted error, not a traceback. Good.
• The error message format from _target_error() is excellent: names the file, names the bad token, lists all valid values inline. This is the right pattern for config validation errors.

No blocking logging concerns.

---

DevX UX Expert:

The UX direction is correct -- converting silent zero-deployments into actionable errors is the right call. Specific findings:

• Error message ergonomics are excellent: "Invalid 'target' in ./apm.yml: 'bogus' is not a valid target. Choose from: agents, all, claude, codex, copilot, cursor, gemini, minimal, opencode, vscode" -- file-pointed, token-named, full valid-values list. This is npm/cargo-quality error output.
• The zero-output warning in compile is the right UX: a "completed successfully" message when nothing was written is one of the most trust-eroding failures in developer tooling.
• CHANGELOG migration guidance is clear and covers all three newly-failing inputs. First-time users hitting this on upgrade will know exactly what to fix.
• Required pre-merge action: packages/apm-guide/.apm/skills/apm-usage/package-authoring.md (and/or commands.md) describes the target: manifest field. Per repo Rule 4, the contract change ("unknown values MUST raise a parse error" vs. the old "MUST be silently ignored") must be reflected in the shipped skill resource in the same PR. The docs site (manifest-schema.md) is updated; the skill resource is not.
• The single-token alias asymmetry noted by the Python Architect is also a UX issue: apm install with --verbose may display vscode when the user typed copilot in the list form, but copilot when used alone. Low severity, warrants a follow-up issue.

---

Supply Chain Security Expert:

Clean from a supply-chain perspective. Positive security delta:

• Whitelist validation added where none existed. VALID_TARGET_VALUES is a static frozenset derived from known canonical targets and aliases. Whitelist-based input validation is correct; no network-fetched allowlist.
• Exception swallowing removed. The old except Exception: pass in compile/cli.py suppressed all parse errors including potential injection of unexpected YAML content. Removing it is a net security improvement.
• Silent fallback removed from integration layer. The old [copilot] fallback in active_targets and active_targets_user_scope meant any unknown target silently deployed to copilot. The fallback removal means no target is deployed without explicit confirmation -- this is the "fail closed" posture supply chain security requires.
• parse_target_field() operates only on plain strings from the YAML value -- no path joins, no filesystem access, no network calls. No new attack surface.
• No auth surface, lockfile, download, or signature path touched.

No blocking security concerns. This PR improves APM's fail-closed posture on config validation.

---

Auth Expert: Not activated -- all changed files (target_detection.py, targets.py, apm_package.py, compile/cli.py, install/phases/targets.py) deal exclusively with deployment-target name resolution (vscode, claude, cursor, etc.) and have no interaction with AuthResolver, token precedence, host classification, credential helpers, or HTTP authorization headers.

---

OSS Growth Hacker:

Strong trust signal worth amplifying. Findings:

• Story angle: "APM now fails loudly when your config is wrong -- no more mystery deployments." This speaks directly to a class of silent-failure bugs that cause developers to abandon new tooling. Frame in release notes as "APM validates your manifest like a real package manager."
• Friction reduction: Eliminates the most confusing class of new-user failure -- running apm install successfully while nothing deploys. This directly protects the first-5-minutes funnel.
• CHANGELOG as release material: The CHANGELOG entry is unusually good. The three-bullet migration list is reusable verbatim in the release post.
• Side-channel to CEO: This fix is a competitive signal. npm, cargo, and pip all validate their config files at parse time and fail loudly on unknown keys. APM now does too. This is worth a sentence in the next release announcement -- not as a feature, but as evidence of production-grade maturity. Consider: "We fixed the [BUG] apm fails to install or compile dependencies #820 silent zero-deploy bug reported by the community" as a trust-building beat for the 250+ star audience.
• The breaking change may generate a small wave of issues from users with malformed apm.yml files. The error messages are good enough that most will self-serve; ensure troubleshooting.md in the skill resources is updated alongside the required package-authoring.md fix (see DevX finding).

---

CEO arbitration

The panel is in strong agreement: this PR fixes a real and damaging bug (#820) correctly, using the right pattern (SSOT validator in core/), with excellent error messages and appropriate CHANGELOG communication. The supply-chain and logging reviews add net-positive findings. The only pre-merge gaps are (1) missing apm-guide skill resource update for the changed manifest contract, required by repo Rule 4 -- this is a concrete omission, not a style preference; and (2) the detect_target() dead-code call in phases/targets.py is benign but the comment "return values are not consumed by downstream code" is a maintenance red flag that should not ship without a follow-up issue. The single-token alias asymmetry is an intentional and documented design decision; accepting it as a tracked follow-up is the right pragmatic call -- expanding scope here risks destabilizing the CLI test suite. The Growth Hacker's framing note is well-taken: this fix is competitive positioning material. Disposition: APPROVE once the two required actions below are completed.

---

Required actions before merge

1. Update packages/apm-guide/.apm/skills/apm-usage/package-authoring.md (and commands.md if it describes the target: field) to document the new manifest contract: unknown target: tokens now raise a parse error; empty values, CSV strings, and all mixed with other targets are also now errors. The manifest-schema.md doc is correctly updated; the shipped skill resource that agents consume is not -- this violates repo Rule 4 and means copilot / coding agents trained on the skill will give incorrect guidance to users upgrading to this version.

2. Open a follow-up issue for removing the dead detect_target() call at the bottom of src/apm_cli/install/phases/targets.py (the call whose return values "are not consumed by downstream code") and for collapsing the now-duplicate alias-resolution logic in detect_target() with the parse_target_field() SSOT. The comment acknowledging the dead call is good; the dead call shipping in production is not.

---

Optional follow-ups

• Collapse the single-token alias asymmetry: parse_target_field("copilot") returns "copilot" while parse_target_field("copilot,claude") returns ["vscode","claude"]. Document in a dedicated issue and resolve with a semver-minor bump when the CLI test suite is ready for the change.
• Replace the _files_written stat-key heuristic in compile/cli.py with a CompilationResult.files_written_count() method so the zero-output gate does not degrade silently when new stat keys are added.
• Add troubleshooting.md entry in the apm-guide skill resource for the "I set target: in apm.yml but apm install says invalid target" error, with the three migration cases from the CHANGELOG.
• Consider surfacing the zero-output warning at INFO level in non-verbose mode but also logging it at DEBUG/verbose level with the full stat dict for agent-friendly debugging.

Generated by PR Review Panel for issue #987 · ● 1M · ◷