This was generated by AI from an Architecture Review (arch-review).
What to build
The installer's install() interleaves the same six-step dance twice — once for the skill body, once for the marketplace manifest: resolve target → validate source exists → (skill: canonical exists) → produce a Plan → route stream out/err → print → conditional write → return an exit code. The pure Plan dataclass is a clean seam but never escapes install(). install_all_skills just loops install() and prints — it returns no structured plans, so "what would change across all skills?" can't be inspected before any write. Verifying an abort (e.g. body-parity failure) today means calling install() with a full repo and scraping stderr.
Make the Install Plan the unit of work: planning returns a list[Plan] for both single and --all installs; a thin renderer prints them and an executor writes them. install() collapses to roughly plans = plan_all(...); render(plans); if not check_only: execute(plans). Skill vs marketplace differ only in which planner feeds the list. --check becomes "render, skip execute" with no branch duplication.
Behavior (exit codes, printed actions, dry-run prefix, idempotent/overlay/abort semantics) is unchanged — only the orchestration shape moves.
Acceptance criteria
Blocked by
What to build
The installer's
install()interleaves the same six-step dance twice — once for the skill body, once for the marketplace manifest: resolve target → validate source exists → (skill: canonical exists) → produce aPlan→ route stream out/err → print → conditional write → return an exit code. The purePlandataclass is a clean seam but never escapesinstall().install_all_skillsjust loopsinstall()and prints — it returns no structured plans, so "what would change across all skills?" can't be inspected before any write. Verifying an abort (e.g. body-parity failure) today means callinginstall()with a full repo and scraping stderr.Make the Install Plan the unit of work: planning returns a
list[Plan]for both single and--allinstalls; a thin renderer prints them and an executor writes them.install()collapses to roughlyplans = plan_all(...); render(plans); if not check_only: execute(plans). Skill vs marketplace differ only in which planner feeds the list.--checkbecomes "render, skip execute" with no branch duplication.Behavior (exit codes, printed actions, dry-run prefix, idempotent/overlay/abort semantics) is unchanged — only the orchestration shape moves.
Acceptance criteria
list[Plan]for both single-target and--allinstalls (skill and marketplace).--check/ check-only is "render plans, do not execute" with no separate branch.Planobjects (action / reason / target) rather than captured stdout/stderr; the abort cases are covered without a full write.Blocked by