APM integration — proposed target architecture (follow-up to #1015)

TL;DR

Re-opening from #1015. The five contract drifts I flagged are still verbatim in v0.9.4's packages/squad-cli/src/cli/commands/skill.ts (PR #1023 promoted PR #876 unchanged from insider to main; CHANGELOG [0.9.4] lists three unrelated fixes). This issue is paired with a PR I'm opening alongside it that mechanically realigns the existing squad skill ... integration to APM's documented contract — same command surface, same UX intent as PR #876, just routed through apm install / apm pack instead of a parallel reimplementation.

Problem (WHY)

Two commits have ever touched packages/squad-cli/src/cli/commands/skill.ts in the v0.9.4 history (gh api repos/bradygaster/squad/commits?path=packages/squad-cli/src/cli/commands/skill.ts&sha=v0.9.4):

PR #1023 ("promote insider to official") added skill.ts to main with diff +569/-0 — a verbatim copy of the insider file. CHANGELOG.md [0.9.4] lists three fixes (monorepo git init, ADO az shell on Windows, nap budget); none touch APM.

Each finding from #1015 against v0.9.4 skill.ts:

1. Invalid top-level apm.yml keys still emitted: skills: (L194), instructions: (L207), prompts: (L212). APM's manifest schema does not recognise these — they are silently ignored.
2. Custom gh api + regex YAML parser still present: gh api calls at L317, L379, L422, L454; "Simple YAML parse" comment at L174; if (line.trim() === 'skills:') regex at L339. 81f84b5 only added shell: IS_WINDOWS to those calls — it did not replace them with apm install.
3. Hand-rolled apm.yml regen instead of apm pack --format plugin — publishSkill still emits apm.yml directly (L187–225); no apm pack invocation anywhere.
4. References to non-existent registry / apm publish still verbatim — L5, L162, L227, L539, L545. APM is git-native: "Any git repository is a valid APM package. Publish by pushing to a git remote — no registry required."
5. .copilot/skills/ as source-of-truth — resolveSkillsDir defaults to .copilot/skills/ (L82, L90); .apm/skills/ is not referenced. APM's primitive-types doc says .apm/<primitive>/ is the source location; .copilot/, .github/, .claude/, .cursor/ are deploy outputs that apm install populates.

Approach (WHAT)

Replace the parallel reimplementation with a thin adapter over the apm CLI. Same command surface as today plus a one-shot migrate for the legacy layout — no new flags, no new mental model for users.

apm becomes an opt-in runtime dependency of the four squad skill ... verbs only. squad init, squad watch, and the rest work unchanged. Users who never run squad skill ... never see apm. A one-line install hint in the missing-binary error path covers first-run.

Diagrams

Architecture before — squad skill install owner/repo reimplements parts of APM:

flowchart TD
    A[squad skill install] --> B["gh api repos/owner/repo/contents/apm.yml"]
    B --> C["regex parse skills section"]
    C --> D["for each skill: gh api contents"]
    D --> E["write to .copilot/skills/n/skill.md"]
    E --> F["write .apm-source.json"]

Architecture after — Squad delegates to apm and inherits its lockfile, integrity verification, path-security, and security scanning for free:

flowchart TD
    A[squad skill install] --> B[ApmCli.ensureAvailable]
    B -->|missing| X["fatal: apm not on PATH (install hint)"]
    B -->|ok| C["execFile apm install source --target copilot"]
    C --> D["stream stdout/stderr with apm prefix"]
    D --> E["apm writes .copilot/, updates apm.lock.yaml"]

Module split — collapse the 569-line god-file into a small adapter plus per-verb files:

classDiagram
    class SkillCommand {
        +install(source)
        +publish(name)
        +list()
        +migrate()
    }
    class ApmCli {
        +ensureAvailable()
        +install(source, target)
        +pack(format)
        +version()
    }
    class LegacySkillsLayout {
        +detect()
        +planMigration()
        +execute()
    }
    class ApmCliError {
        kind: missing | exit | versionTooOld
    }
    SkillCommand --> ApmCli : delegates
    SkillCommand --> LegacySkillsLayout : opt-in migrate
    ApmCli --> ApmCliError : throws

Trade-offs

• Adds an opt-in runtime dependency on the apm CLI for the four skill verbs. Mitigation: squad init / watch / etc. unaffected; missing-binary error includes the exact install command. Squad users who never publish or install skills never see apm.
• Replaces a working bespoke YAML parser with a subprocess call. Mitigation: APM's parser is schema-validated and gets supply-chain fixes upstream; the bespoke parser was flagged in #963 anyway.
• One-minor backward-compat window for legacy .copilot/skills/<n>/skill.md source layout. Removed in the next minor. Consistent with APM's CHANGELOG philosophy of favouring shipping fast over long deprecation cycles.
• One PR, not split. The five findings are tightly coupled (the manifest schema fix and the publish-path fix only make sense together). Splitting would create an awkward half-correct intermediate state.

Benefits

1. Net-negative LOC. Estimated ~150 LOC removed (regex YAML parser, four gh api call sites, .apm-source.json writer, hand-rolled apm.yml emitter) vs ~80 LOC added (adapter, per-verb files, migrate command).
2. Closes four security surfaces with no new ones: bespoke parser, raw-blob trust without integrity check, path-traversal exposure on upstream-controlled paths, bypassed dependency security scanning. APM enforces lockfile content-hash verification, path-segment validation, and a security gate; Squad inherits all four by delegating.
3. Truthful user-facing strings. No more "APM registry" / "Run apm publish" pointing at things that don't exist. APM is git-native — any git repo is a valid package.
4. Lockfile + transitive deps + provenance for free. .apm-source.json becomes redundant; apm.lock.yaml already tracks origin and integrity per dependency.
5. Cross-platform apm shim handling done once in the adapter — no shell: IS_WINDOWS sprinkled across call sites.

What ships in the paired PR

• New module: packages/squad-cli/src/cli/commands/skill/{index.ts, install.ts, publish.ts, list.ts, migrate.ts, apm-cli.ts} replacing today's single 569-line file.
• Delete: regex YAML parser, four gh api content-fetch sites, .apm-source.json writer, hand-rolled apm.yml emitter.
• Replace: every "APM registry" / apm publish string with git-native phrasing.
• Add: squad skill migrate (one-shot, prompts for confirmation, dry-run summary first).
• Tests: replace gh api mocks with apm subprocess mocks via the same execFile patterns Squad already uses (core/gh-cli.ts); regression test asserts "APM registry" / "apm publish" absent from skill/ output.
• CHANGELOG.md entry under [Unreleased] ### Fixed.
• Backward-compat: detect legacy .copilot/skills/<n>/skill.md source layout; print a one-time deprecation pointer at squad skill migrate. Reader removed in the next minor.

How to engage

• The paired PR is opening alongside this issue and references it. Review either; happy to iterate or close if the direction doesn't fit your roadmap.
• Comment with concerns about scope, naming, or backward-compat — I'll adjust.

— Daniel

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