fix: cross-target path mapping for skills/agents in apm pack

[Copilot]

Description

apm pack --target claude produces an empty bundle when skills/agents are installed under .github/ (the default path when .claude/ doesn't exist). The filter strictly matches .claude/ prefix, dropping all .github/skills/ and .github/agents/ paths.

Root cause: Install deploys to .github/ when .claude/ doesn't exist, but pack filters by the target's prefix only — so --target claude sees zero matching files.

Fix: Cross-target path mapping at pack time. skills/ and agents/ are semantically identical across targets, so the packer remaps them when the lockfile paths don't match the pack target. Commands, instructions, and hooks are target-specific and are never mapped.

• lockfile_enrichment.py — _CROSS_TARGET_MAPS table + _filter_files_by_target() returns Tuple[List[str], Dict[str, str]] (filtered files + bundle-path→disk-path mappings)
• packer.py — PackResult gains mapped_count/path_mappings; copy loop reads from disk path, writes to mapped bundle path; verification checks source existence
• pack.py — UX: [i] Mapped N file(s) to match target with verbose per-file detail
• enrich_lockfile_for_pack() — enriched lockfile uses mapped paths; mapped_from recorded in pack: section
• Docs — cross-target mapping section in pack-distribute.md

# Before: empty bundle
result = pack_bundle(project, out, target="claude", dry_run=True)
result.files  # [] — BUG

# After: skills remapped .github/skills/ → .claude/skills/
result.files  # ['.claude/skills/my-plugin/SKILL.md', ...]
result.mapped_count  # 3

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

17 new tests across test_packer.py and test_lockfile_enrichment.py covering: github→claude, claude→vscode, cursor, opencode, mixed direct+mapped, dedup when both paths exist, dry-run, enriched lockfile mapped_from, commands/instructions NOT mapped. Full suite: 2638 passed.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Copilot]

Pull request overview

Fixes apm pack --target claude producing empty bundles when skills/agents were installed under .github/ by adding cross-target path remapping during pack/enrichment, so semantically equivalent skills/ and agents/ content is bundled under the requested target prefix.

Changes:

• Add cross-target mapping support to target filtering and lockfile enrichment, returning both filtered paths and bundle-path -> disk-path mappings.
• Update the packer to copy from original on-disk paths while writing to remapped bundle paths, and to surface mapping metadata in PackResult.
• Add tests, docs, and a changelog entry covering the new mapping behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/test_packer.py	Expands unit tests for target filtering + pack behavior with cross-target remaps.
tests/unit/test_lockfile_enrichment.py	Adds lockfile enrichment tests to validate mapped deployed paths + pack.mapped_from.
src/apm_cli/commands/pack.py	Adds user-facing logging for mapped paths and adjusts dry-run file listing.
src/apm_cli/bundle/packer.py	Implements bundle-path -> disk-path copy behavior and exposes mapping info via PackResult.
src/apm_cli/bundle/lockfile_enrichment.py	Introduces mapping table and updates filtering/enrichment to support remaps and mapped_from.
docs/src/content/docs/guides/pack-distribute.md	Documents cross-target mapping behavior and how it affects the bundled lockfile.
CHANGELOG.md	Adds an Unreleased Fixed entry for the empty bundle bug.

Comments suppressed due to low confidence (1)

src/apm_cli/commands/pack.py:76

• CommandLogger has no info() method, so this mapping notice will crash non-dry-run apm pack executions. Replace with an existing logging method (or add info() to CommandLogger).

        if result.mapped_count:
            logger.info(
                f"Mapped {result.mapped_count} file(s) to match target"
            )

[Copilot]

cursor is included in _CROSS_TARGET_MAPS, but pack_bundle() derives effective_target via detect_target(), which currently never returns "cursor" (and ignores an explicit --target cursor). As a result, these cursor remap rules (and .cursor/ prefix filtering) are effectively unreachable from apm pack. Either extend detect_target() to support cursor for pack, or normalize/handle explicit pack targets before calling detect_target().

[Copilot]

CommandLogger does not define an info() method, so this will raise an AttributeError at runtime when apm pack --dry-run hits the mapping message. Use an existing logger method (e.g., progress(..., symbol="info")), or add an info() helper to CommandLogger and use it consistently.

This issue also appears on line 73 of the same file.

[danielmeppiel]

Expert Panel Review: Architecture + UX

Reviewed by python-architect and cli-logging-ux agents. Tests verified locally (43/43 pass). Found 1 critical runtime crash, 1 security hardening gap, and 4 important improvements.

---

🔴 Critical — logger.info() does not exist: runtime crash

Files: src/apm_cli/commands/pack.py lines 58, 74

CommandLogger has no info() method. The PR calls logger.info(...) in both the dry-run and normal paths for the mapping message. This will raise AttributeError at runtime whenever cross-target mapping occurs — meaning the bug is "fixed" but the fix itself crashes.

Available methods: start(), progress(), success(), warning(), error(), verbose_detail(), tree_item(), dry_run_notice().

Fix:

# Line 58 (dry-run): use dry_run_notice (adds [dry-run] prefix automatically)
logger.dry_run_notice(f"Would remap {result.mapped_count} file(s) to match target")

# Line 74 (normal): use progress (produces [i] prefix)
logger.progress(f"Mapped {result.mapped_count} file(s) to match target")

Also: line 58 currently has a manual [dry-run] prefix inside the message string. If switched to dry_run_notice(), that manual prefix must be removed to avoid double [dry-run].

---

🔴 Security Hardening — Bundle write path not validated against escape

File: src/apm_cli/bundle/packer.py lines 131-144

The security validation checks rel_path (mapped bundle path) for .. and checks disk_path (on-disk source) for project-root escape. But it never validates that the mapped destination can't escape the bundle directory. Currently safe because mapping prefixes are hardcoded constants, but defense-in-depth says both read and write paths should be independently validated.

Suggested addition (after line 211, before writing):

bundle_dest = (bundle_dir / rel_path).resolve()
if not bundle_dest.is_relative_to(bundle_dir.resolve()):
    raise ValueError(f"Refusing to write outside bundle: {rel_path!r}")

---

⚠️ Important — Mapping message is vague: "to match target"

File: src/apm_cli/commands/pack.py lines 58, 74

"Mapped 3 file(s) to match target" — which target? From where? Fails the newspaper test. Users (and CI grep) need the direction.

Suggested:

[i] Mapped 3 file(s): .github/ -> .claude/

The source/dest prefixes are available from result.path_mappings.values() — extract the common prefix.

---

⚠️ Important — Empty bundle message still has no actionable fix

When mapping produces zero results AND no direct matches exist, the message is still:

[!] No deployed files found -- empty bundle created

Should name the target and suggest alternatives:

[!] No files for target 'claude' -- installed files are under .github/
    Use '--target vscode' or '--target all'

---

⚠️ Important — _CROSS_TARGET_MAPS is asymmetric

File: src/apm_cli/bundle/lockfile_enrichment.py lines 22-39

The map only handles .github/ → other targets (and .claude/ → .github/). If someone has skills under .cursor/skills/ and packs with --target claude, it's the same empty-bundle bug for a different source.

Should either:

1. Make it a full mesh (each target maps from all others), or
2. Document .github/ as the canonical interop source and only map from that (removing the .claude/ → .github/ entry in vscode too for consistency)

Option 2 is cleaner — .github/ is the standard root.

---

🟡 Minor — Stale comment and dead import

1. lockfile_enrichment.py:9 — Comment says "Must stay in sync with packer._TARGET_PREFIXES" but _TARGET_PREFIXES lives here, not in packer. Misleading.
2. packer.py:13 — Imports _TARGET_PREFIXES but never uses it. Dead import.
3. lockfile_enrichment.py:127-131 — mapped_from prefix extraction splits paths by / and takes first 2 segments. Fragile if path structure ever changes. Could record the source prefix directly from _CROSS_TARGET_MAPS during mapping instead.

---

🟡 Minor — cursor target exists in pack CLI but not in target_detection.py

pack.py accepts --target cursor and _CROSS_TARGET_MAPS has cursor entries, but detect_target() in target_detection.py has no cursor handling. The explicit flag is silently ignored and auto-detection takes over. Pre-existing issue, not introduced by this PR, but worth noting.

---

✅ What's done well

• Core algorithm is clean: direct-match-first with fallback cross-mapping, dedup via set
• PackResult extension with mapped_count + path_mappings is a good pattern for caller introspection
• Security: disk-path resolution correctly uses path_mappings.get(rel_path, rel_path) consistently across all on-disk operations (validation, scanning, copying)
• Test coverage: 20 new tests covering mapping, reverse mapping, dedup, commands-not-mapped, instructions-not-mapped, all targets, enriched lockfile, dry-run — solid
• Bundle self-consistency: enriched lockfile uses mapped paths, so unpack needs no changes
• Performance: O(N*M) with M=2 per target — negligible
• Backward compatible: no lockfile format changes, existing lockfiles just work
• Docs + changelog: pack-distribute.md updated, changelog entry present

---

Verdict

Block on: Critical #1 (runtime crash). The rest can be addressed in this PR or as fast-follow.

Architecture: Sound. The mapping table in lockfile_enrichment.py is correctly placed — it's tightly coupled to _filter_files_by_target() and only consumed by the bundle layer. The (bundle_path, disk_path) pair pattern through path_mappings is clean and avoids working-tree mutation.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (2)

src/apm_cli/bundle/packer.py:168

• Non-ASCII em dash character appears in this comment string ("warn only" line). Per encoding rules, keep Python source and CLI output within printable ASCII; replace the em dash with ASCII (e.g., " -- "). See .github/instructions/encoding.instructions.md:10-16.

    # 5b. Scan files for hidden characters before bundling.
    # Intentionally non-blocking (warn only) — pack is an authoring tool.
    # Critical findings here mean the author's own source files contain

docs/src/content/docs/guides/pack-distribute.md:250

• The lockfile enrichment example shows pack.target: copilot, but the implementation currently writes the internal canonical target ("vscode") when packing for Copilot/GitHub. Update this example (or adjust the implementation) so bundle metadata matches what apm pack actually produces.

```yaml
pack:
  format: apm
  target: copilot
  packed_at: '2025-07-14T09:30:00+00:00'

[Copilot]

This tree output uses Unicode box-drawing characters ("\u2514\u2500"). Per encoding rules, CLI output must be printable ASCII; replace with an ASCII prefix to avoid Windows cp1252 issues. See .github/instructions/encoding.instructions.md:10-16.

See below for a potential fix:

                logger.tree_item(f"    - {f}")
    else:
        for f in result.files:
            logger.tree_item(f"  - {f}")

[Copilot]

The CLI advertises --target cursor, but detect_target() does not accept the explicit value "cursor" (and doesn't detect a .cursor/ folder), so apm pack --target cursor will not reliably filter to .cursor/ files. Either extend target detection to handle "cursor" or bypass detect_target normalization when an explicit pack target is provided.

Suggested change

	type=click.Choice(["copilot", "vscode", "claude", "cursor", "opencode", "all"]),
	type=click.Choice(["copilot", "vscode", "claude", "opencode", "all"]),

[Copilot]

This warning surfaces internal target names (e.g., "vscode") even though the CLI/docs present "copilot" as the user-facing name. Consider normalizing targets for display in this message so users see consistent terminology (while still comparing using the internal canonical values).

[Copilot]

This example shows project target "copilot", but detect_target() currently returns the internal canonical value "vscode" for .github/ projects and _log_bundle_meta() prints that raw value. Either update the example or normalize targets to user-facing names in CLI output.

This issue also appears on line 246 of the same file.

Suggested change

	[!] Bundle target 'claude' differs from project target 'copilot'
	[!] Bundle target 'claude' differs from project target 'vscode'

[Copilot]

Test name says "copilot" but passes target="vscode". Since the codebase treats these as aliases, consider using the same term in both the test name and the input to avoid confusion (either rename the test or pass "copilot").

Suggested change

	result, mappings = _filter_files_by_target(files, "vscode")
	result, mappings = _filter_files_by_target(files, "copilot")

[Copilot]

This test docstring says "--target copilot" but the test actually calls enrich_lockfile_for_pack(..., target="vscode"). Align the wording with the actual value used (or pass "copilot") to keep the test intent unambiguous.

Suggested change

	"""Pack with --target copilot should exclude .claude/ files from lockfile."""
	"""Pack with --target vscode should exclude .claude/ files from lockfile."""

[Copilot]

pack_meta can become non-dict if the lockfile contains an unexpected pack: value (e.g., string/list). Downstream code assumes .get(...) exists; coerce pack_meta to {} unless it is a dict to avoid AttributeError during apm unpack logging.

Suggested change

	pack_meta = raw.get("pack", {})
	pack_meta = raw.get("pack", {})
	if not isinstance(pack_meta, dict):
	# Defensive: malformed lockfile with non-dict "pack" value
	pack_meta = {}

[Copilot]

Docstring says the effective pack target is "copilot", but pack_bundle() passes the internal canonical target from detect_target() (currently "vscode" for Copilot/GitHub). Either update this docstring (and related docs) to reflect the internal value, or change pack: metadata to store the user-facing name ("copilot") for consistency.

Suggested change

	target: Effective target used for packing (``"copilot"``, ``"claude"``, ``"all"``).
	target: Canonical pack target name (e.g. ``"vscode"`` for GitHub/Copilot),
	such as ``"vscode"``, ``"copilot"``, ``"claude"``, ``"cursor"``,
	``"opencode"``, or ``"all"``.