fix(cli,skills): install workflow skills on demand instead of re-pulling the full set

[kiritowoo]

Users report every init re-pulls all 20 skills into ~/.agents/skills whenever anything is stale or missing - heavy, noisy, and it re-expands deliberate partial installs.

Split the set into two tiers:

• core: the /hyperframes router + hyperframes-* domain skills + media-use, which every workflow references structurally. init and bare 'skills update' keep these (plus anything already installed) fresh, and never expand the install.
• on demand: the end-user workflow skills (and figma). They install at trigger time via 'skills update <name...>' - positional names are the only way update expands an install: one targeted 'skills add --skill ' covering only stale/missing targets, a fast no-op when current, presence-verified after install, exit 1 on unknown names, and a presence-only degrade when GitHub is unreachable.

The /hyperframes router now runs 'skills update ' after routing and before reading the workflow skill, so a routed workflow is guaranteed present even on a machine that only has the core set.

skills check still lists every skill, but exits non-zero only for stale installed skills, an incomplete core set, or removed leftovers - workflow skills not yet installed are reported as available on demand. Bare 'hyperframes skills' (and 'skills add --all') remain the explicit full-set installs.

Verified end-to-end with a sandboxed $HOME: fresh init installs the 8 core skills only; 'skills update slideshow' adds exactly that skill (no-op on re-run, exit 1 on unknown names); bare update refreshes without expanding; and a live Claude Code run routed PR-to-video, executed the router's update step, and the workflow skill appeared before use.

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

[miga-heygen]

Review: LGTM with nits ✅

Clean, well-architected change that solves a real user pain point. The core/on-demand tier split is single-sourced (isCoreSkill is the single classifier), the offline degradation is thoughtful, and the test coverage is thorough.

Single-source-of-truth audit

• isCoreSkill — single classifier for the tier decision. No duplication. ✓
• FALLBACK_CORE_SKILLS — only used in the offline path, pinned by a test against the repo tree. ✓
• updateSkills — single engine behind both init and skills update. No duplicated install logic. ✓
• Workflow skill preambles — self-contained bootstrap instruction in each on-demand SKILL.md. Decision lives in one place (the skill file itself). ✓

No SSOT violations found.

Verified

• Tier classification: Core skills (hyperframes, hyperframes-core, hyperframes-animation) always refreshed by init/update. On-demand workflow skills installed at trigger time only.
• init behavior: Best-effort, non-strict. Won't fail on skill issues. Partial installs stay partial — refreshed but never expanded.
• update <name> behavior: Strict. Installs the named skill + refreshes installed + always includes core. verifyInstalled throws on failure.
• check exit code: Now exits 0 when only on-demand skills are missing. check || update CI contract correctly updated — on-demand gaps don't trigger a full pull.
• Offline degradation: Presence-only guarantee with FALLBACK_CORE_SKILLS. Correctly skips the network fetch and uses local copy.
• SKILL.md preambles: All 13 on-demand workflow skills got the "keep this skill fresh" line. Core skills correctly have no preamble.

Nits (non-blocking)

1. UpdateSkillsResult.unknown contract (skills.ts:~320) — Suggestion. When non-strict with unknown names, the function warns but proceeds. The exported unknown field could mislead a future caller who doesn't check it. Consider documenting: "callers SHOULD check result.unknown.length when non-strict."

2. "Stays partial" comment (skills.ts:~640) — Nit. The comment "stays partial" could momentarily confuse since refreshInstalled: true is set. A clause like "stays partial — refreshed but never expanded" would clarify.

3. verifyInstalled non-strict behavior (skills.ts:~363) — Suggestion. In non-strict mode, it warns but doesn't set process.exitCode. This is correct (init is best-effort), but a one-line comment in verifyInstalled itself would make the intent explicit.

4. Test beforeEach duplication (skills.test.ts:~234,477) — Nit. Two describe blocks have nearly identical beforeEach blocks. A shared helper would remove the duplication.

Ship it.

— Miga

[jrusso1020]

Additive review — COMMENT (external author; stamp = James's call)

Audited: delta topology (core vs on-demand tier split), the update engine (skills.ts updateSkills), the init→engine call path (init.ts:592-628), the router SKILL.md instruction change, and the FALLBACK-pin invariant.

Trusting: Miga's single-source-of-truth audit (isCoreSkill / FALLBACK_CORE_SKILLS / updateSkills / preambles) and her test-coverage scan — both hold on inspection. Not duplicating those; my angle is additive only.

Additive findings

• (nit / PR body vs code) PR description says "fresh init installs the 9 core skills only" — actual is 8. The isCoreSkill pattern (hyperframes router + hyperframes-* domain + media-use) matches 8 entries in skills-manifest.json (hyperframes, hyperframes-animation, hyperframes-cli, hyperframes-core, hyperframes-creative, hyperframes-keyframes, hyperframes-registry, media-use), and FALLBACK_CORE_SKILLS at packages/cli/src/utils/skillsManifest.ts:143-152 is that same 8 verbatim. The "9" is off-by-one — worth fixing in the description before merge so future readers don't come away with the wrong number.

• (strength / drift-detection) The FALLBACK_CORE_SKILLS list is a real footgun (it duplicates the truth that isCoreSkill computes) and the PR handles it correctly: packages/cli/src/utils/skillsManifest.test.ts:90-108 pins the list to readdirSync(skills/).filter(isCoreSkill) — so a future core addition that doesn't update the FALLBACK breaks CI immediately, not silently at offline-install time. This is exactly the kind of invariant test that keeps the parallel-truth pattern honest. Nice.

• (strength / init behavior) Verified the fresh-init path at packages/cli/src/commands/init.ts:600-601: updateSkills({refreshInstalled: true, cwd: destDir}) with no requested. On an empty machine, manifestSkills all have status missing, targets filter keeps only isCoreSkill(name) matches (requested is empty, refreshInstalled + missing is excluded), so the fresh install is exactly the 8-skill core set — not the full 20. This is the correct "never expand" shape for init, matches the PR-body claim (modulo the 9→8 count).

• (strength / router UX) skills/hyperframes/SKILL.md:80-88 handles the mid-run failure case explicitly: "If the command fails, surface its error to the user instead of improvising the workflow from memory. Manual fallback (no HyperFrames CLI available): npx skills add ... --skill <workflow-name>." This is the right shape — an agent hitting a network failure during skills update <workflow> won't silently attempt the workflow without the skill.

• (nit / concurrency, out of scope) Two parallel hyperframes skills update <workflow-A> and <workflow-B> runs (e.g. two Claude Code sessions concurrently routing) both spawn npx skills add ... writing into the same ~/.claude/skills / ~/.agents/skills tree. The upstream vercel-labs/skills CLI has no cross-process lock. Not a regression this PR introduces (bare hyperframes skills had the same shape), and unlikely to hit in normal use — but as on-demand installs become more frequent (every workflow trigger, per this PR), the concurrency exposure widens. Worth a future lockfile or flock if we start seeing lock-file corruption in the wild.

• (nit / docs, non-blocking) The UpdateSkillsResult.unknown field is documented as "requested names the latest manifest doesn't ship (typo, or renamed upstream)" (packages/cli/src/commands/skills.ts:270-273), but callers may want to gate on it (e.g. router-level "did I typo the workflow name?"). Consider a one-line note on the callsite contract: unknown.length > 0 means the caller should treat this as user-facing error, not a network hiccup. Miga already flagged this in her doc-suggestion trio.

Verdict

COMMENT — architecture is clean, the "never expand a partial install" invariant is enforceable and enforced (unit-pinned), the mid-run router UX is thoughtful, and CI is green on d2eca631a113. Stamp = @jrusso1020's call (external author kiritowoo, requester Miao is allowlisted but not a trusted-stamper, per delegation rule).

— Rames Jusso

[jrusso1020]

Re-review — COMMENT (external author; stamp still James's call)

Audited: delta since my prior review head d2eca631 (2 files: packages/cli/src/commands/skills.ts +14/-3, docs/guides/skills.mdx +18/0). Zero code-behavior change; pure docs polish addressing prior review nits.

Delta:

• UpdateSkillsResult.unknown doc — clarified with the strict-vs-non-strict contract (a strict run throws on unknown names before returning, so strict callers never observe this). Addresses both Miga's and my prior nit.
• verifyInstalled — new doc block explicitly stating "strict throws so the failure is loud; non-strict warns and proceeds, since a skills hiccup must never break scaffolding." Addresses Miga's nit.
• updateCommand "stays partial" comment — now reads "a deliberate partial install stays partial (refreshed, but never expanded)". Addresses Miga's nit.
• docs/guides/skills.mdx — new "Keeping skills current" section documenting the core vs on-demand model + the four commands (skills check / skills update / skills update <name> / skills). External user-facing docs, matches the SKILL.md and README wording.
• PR body — the "9 core skills" count claim I flagged is now correctly "8 core skills". Nit resolved.

Substance preserved: the updateSkills engine, isCoreSkill pattern, FALLBACK_CORE_SKILLS pin test, prune-in-scope, router SKILL.md instruction, and CI wiring are all unchanged from d2eca631. Nothing to re-audit at the invariant layer.

CI: green on 7554ceec5971 — Analyze / Build / CLI smoke / CodeQL / Format / Lint / Fallow audit / Skills: manifest in sync / Test: skills / Preflight / Mintlify all pass.

Verdict

COMMENT — docs-only re-push cleanly addresses the prior-round nits (mine + Miga's) with zero code-behavior drift. As before, stamp = @jrusso1020's call (external author kiritowoo; requester Miao is allowlisted but not a trusted-stamper, per delegation rule).

— Rames Jusso

[miga-heygen]

Re-review: LGTM ✅

New commit (7554ceec) cleanly addresses 3 of 4 previous nits:

1. ✅ UpdateSkillsResult.unknown contract — JSDoc now explains unknown is only non-empty on non-strict runs; strict throws before returning.
2. ✅ "stays partial" comment — Now reads "stays partial (refreshed, but never expanded)" — exact clarification suggested.
3. ✅ verifyInstalled non-strict behavior — Explicit comment: "a non-strict run (init) only warns and proceeds, since a skills hiccup must never break scaffolding."
4. ⏭️ Test beforeEach duplication — Not addressed, but trivially non-blocking. Semantically distinct describe blocks.

Bonus: New "Keeping skills current" section in docs/guides/skills.mdx with clear user-facing docs for the core-eager / workflow-on-demand model.

Merge commit (d2eca631) is mechanical — regenerated skills-manifest.json hashes after conflict resolution with main. No logic changes.

SSOT audit: no new violations. Architecture unchanged — isCoreSkill still single-sources the tier classification, FALLBACK pin test still guards against drift.

Ship it.

— Miga

[miga-heygen]

Re-review (3rd): LGTM ✅

Pure merge conflict resolution — zero substantive changes since the last LGTM.

Three new commits are all mechanical merges from main: skills-manifest.json hashes regenerated and 5 workflow SKILL.md conflicts resolved by accepting merged content. The entire diff is identical in substance to the second review.

All 38 CI checks green. MERGEABLE, no conflicts. Previous LGTM verdict stands. Ship it.

— Miga

[jrusso1020]

Re-review — COMMENT (external author; stamp still James's call)

Audited: delta between prior-reviewed 7554ceec5971 and current head f49f767a3246. Compare = 17 commits ahead / 0 behind; the 3 new commits on the branch are all merge commits from origin/main — no new author work by kiritowoo or Miao since the prior head.

PR contribution is byte-identical to my prior review. SHA-equality check on the load-bearing PR-scope files:

• packages/cli/src/commands/skills.ts — SAME
• packages/cli/src/commands/init.ts — SAME
• packages/cli/src/utils/skillsManifest.ts — SAME
• packages/cli/src/commands/skills.test.ts — SAME

Files that show DIFF between 7554ceec and f49f767a (skills-manifest.json, skills/hyperframes/SKILL.md, skills/hyperframes-animation/adapters/gsap-transforms-and-perf.md, CLAUDE.md, README.md, docs/guides/skills.mdx) are all downstream of main-side edits pulled in via the merges — most obviously the skills-manifest hash regeneration (matches the 12 hash changes I'd expect from HF#2058 having landed on main), not PR-owned. The PR's own contribution to the tree is stable.

CI green on new head: Analyze / Build / CLI smoke / CodeQL / Format / Lint / Fallow audit / Skills: manifest in sync / Test: skills / Preflight / Mintlify all pass.

Verdict

COMMENT — merge-only re-push, PR contribution unchanged from prior review. My prior COMMENT verdict on this PR stands (architecture clean, FALLBACK_CORE_SKILLS drift-detection real + pinned, init path correctly installs 8-skill core, router mid-run-failure UX solid). Stamp = @jrusso1020's call (external author kiritowoo; requester Miao is allowlisted but not a trusted-stamper).

— Rames Jusso