fix(cli): always check GitHub skills on init while skills.sh syncs#1768
Conversation
The "don't pass --skip-skills" guidance lives in SKILL.md, which ships through the laggy skills.sh registry and can't be relied on to reach the agent — so an agent that improvises `--skip-skills` silently dodges the GitHub skills freshness pull. Put the guarantee in the CLI instead (the one channel that updates promptly via `npx hyperframes@latest`): - Neuter the `--skip-skills` FLAG so it no longer skips the check; gate skipping on the HYPERFRAMES_SKIP_SKILLS=1 env var instead (the agent/user CLI path never sets it). Print a one-line notice when the ignored flag is passed. - Wire the env escape hatch into the init test helper (one place) and the CI smoke-test / windows-canary steps so they stay offline and fast. - Update the skill docs that previously told agents `--skip-skills` opts out. Temporary measure while skills.sh catches up — revert init.ts's `skipSkills` to `args["skip-skills"] === true` once it does (noted inline). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The lint extraction (#1756) made @hyperframes/lint a runtime dependency of core — core's compiled compiler/staticGuard.js imports it via the package's "node" export condition (./dist/index.js). But the Test and Studio-load-smoke jobs pre-build only @hyperframes/{parsers,studio-server} before packages/core, so loading core's dist at test / dev-server time fails with: ERR_MODULE_NOT_FOUND: Cannot find module .../@hyperframes/lint/dist/index.js imported from .../packages/core/dist/compiler/staticGuard.js Build the canonical pre-core set @hyperframes/{parsers,lint,studio-server} (the glob the root build script uses) in both jobs so it can't drift again. The SDK job is left as-is — it builds parsers+core only and passes. Reproduced locally: removing packages/lint/dist reproduces the exact ERR_MODULE_NOT_FOUND; building lint resolves it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Review: fix(cli): always check GitHub skills on init while skills.sh syncs
Reviewed at 625f204a (both commits).
LGTM — clean, well-scoped tactical fix for the skills.sh lag bootstrap problem. The reasoning is solid: the one channel that updates promptly (npx hyperframes@latest) is exactly where the guarantee belongs, and the env-var escape hatch for CI/tests is the right pattern. Two observations:
Nit: stale comment in the interactive path
The comment at the interactive-path ensureSkillsCurrent call (around line 1078) still reads:
// init is the one place the full set is pulled. Opt out with --skip-skills.
Post-PR, --skip-skills no longer opts out of anything — HYPERFRAMES_SKIP_SKILLS=1 does. The comment above the non-interactive-path call doesn't have this problem (no comment at all). Consider updating it to match reality, something like:
// init is the one place the full set is pulled. Opt out via HYPERFRAMES_SKIP_SKILLS=1.
Avoids a future reader (or agent) trusting the comment over the code.
Observation: second commit (build lint before core in CI)
The fix(ci): build @hyperframes/lint before core in Test and Studio jobs commit consolidates the two separate --filter lines into one glob @hyperframes/{parsers,lint,studio-server}, which also adds the missing lint dependency. Good catch — the ERR_MODULE_NOT_FOUND on staticGuard.js → @hyperframes/lint/dist/index.js would have been a CI-only surprise since local bun run build builds everything. The SDK job is correctly left as-is since it doesn't need lint. Clean fix.
Overall
Both commits are well-motivated, well-documented (the inline "revert to..." note is exactly what you want for temporary measures), and the test/CI coverage is thorough. The one stale comment is the only thing I'd touch before merge.
— Miga
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM on the design — neutering --skip-skills so the freshness guarantee lives in the promptly-updating CLI (npx hyperframes@latest) instead of the laggy skills.sh SKILL.md is the right fix for the bootstrap chicken-and-egg. The HYPERFRAMES_SKIP_SKILLS=1 escape for CI/tests, the inline revert instructions, the "--skip-skills temporarily ignored" note, and the CI env wiring (ci.yml / windows-render.yml / init.test.ts) are all clean.
One additive thing worth confirming (non-blocking) — offline init after the neuter. In ensureSkillsCurrent, checkSkills is try/caught (→ needsInstall=true on offline/rate-limit, per the "installs anyway" comment), but the subsequent await installAllSkills(...) is not wrapped — and installAllSkills (skills add via npx) also needs GitHub. So an offline init now goes: checkSkills fails (caught) → installAllSkills runs → if it throws offline, init breaks. And since --skip-skills no longer escapes, an offline user who relied on it has only the HYPERFRAMES_SKIP_SKILLS env var (which they won't know to set). If installAllSkills already degrades gracefully on network failure (warn + proceed), this is moot — just want to confirm the "best-effort" intent in the comment holds for the install, not only the check. (Pre-existing from #1753's default path, but #1768 removes the escape hatch, so it matters more now.)
Concur with Miga's stale-comment nit (~line 1078 still says "Opt out with --skip-skills").
— Jerrai
- Update the stale interactive-path comment that still said "Opt out with --skip-skills"; the flag is neutered, opt-out is HYPERFRAMES_SKIP_SKILLS=1. - Wrap installAllSkills in ensureSkillsCurrent with try/catch. installAllSkills is already non-strict (swallows its own failures), but since --skip-skills no longer escapes this path, every init — including offline ones that fall through to "install anyway" — runs it. The guard guarantees a skills-install failure only warns and proceeds, never breaks init. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed current head 2ca3c8c1.
The bootstrap guarantee is now at the right boundary: packages/cli/src/commands/init.ts:724-:734 neuters the user/agent-facing --skip-skills flag and gates the opt-out on HYPERFRAMES_SKIP_SKILLS=1, so stale skills.sh guidance cannot bypass the GitHub freshness path. The CI/test escape hatch is wired explicitly through packages/cli/src/commands/init.test.ts:18-:23, .github/workflows/ci.yml:575-:589 / :632-:640, and .github/workflows/windows-render.yml:200-:208.
The follow-up hardening resolves the offline-init concern: packages/cli/src/commands/init.ts:603-:617 now keeps skill installation best-effort even when the freshness check falls through to "install anyway", without masking scaffold/template failures outside that install block. The stale interactive-path comment was also corrected at packages/cli/src/commands/init.ts:1090-:1092.
The CI dependency-order fix checks out too: .github/workflows/ci.yml:230 and :363 now build @hyperframes/{parsers,lint,studio-server} before core/studio paths that resolve core's dist and therefore need @hyperframes/lint/dist.
Checks are green on the current head, including CLI smoke (required), Smoke: global install, Test: skills, Tests on windows-latest, and Render on windows-latest.
Verdict: APPROVE
Reasoning: The CLI now enforces the intended skill freshness path for normal users/agents, CI has an explicit non-user escape hatch, offline skill-install failure is safely downgraded, and the relevant smoke/test matrix is green.
— Magi
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review at 2ca3c8c1 (was 625f204a).
One new commit since the last pass — 2ca3c8c10e fix(cli): address PR #1768 review — stale comment + harden offline init. Both changes address the nit from my previous review:
-
Stale comment fixed (line 1091-1092): Now correctly says
--skip-skillsis "temporarily neutered" and points toHYPERFRAMES_SKIP_SKILLS=1for CI/tests. Matches the actual behavior at line 733. -
Defensive try/catch around
installAllSkills(lines 607-616): Since the--skip-skillsflag no longer escapes this code path, everyinitruns it — including offline invocations wherecheckSkillsthrows. The new catch prevents a skills-install failure from breakinginititself and logs a dim warning instead. Good hardening that pairs naturally with the flag-neutering decision.
Both changes are clean, scoped, and consistent with the surrounding code. CI is all green (34 checks pass, 5 skip — all expected skips). No new concerns.
LGTM ✅
— Miga
Summary
hyperframes initnow always checks the installed agent skills against GitHub — even when--skip-skillsis passed. The--skip-skillsflag is temporarily neutered; opting out is now done via theHYPERFRAMES_SKIP_SKILLS=1env var, which only CI and the test suite set.Why
#1753 made
initrefresh skills from GitHub on every run and dropped--skip-skillsfrom the creation-workflowinitcommands (product-launch-video, faceless-explainer, …) so new projects pull the latest skills. But that guidance lives inSKILL.md, which is distributed through the skills.sh registry — and skills.sh currently lags GitHubmain. So the "don't pass--skip-skills" instruction hasn't reached agents yet, and in practice an agent running e.g./product-launch-video(#1745) still scaffolds with:…which silently bypasses the GitHub freshness check —
--skip-skillsshort-circuits before the check runs.This is a bootstrap problem: the fix that makes skills fresh can't be relied on to arrive through the channel that is itself stale. The CLI is the one channel that updates promptly (
npx hyperframes@latest), so the guarantee belongs there, not in a skill doc.What changed
init—packages/cli/src/commands/init.ts--skip-skillsis neutered: it no longer skips the skills check. Whether to skip is now gated onHYPERFRAMES_SKIP_SKILLS=1, an env var the agent/user CLI path never sets.--helpdescription and its example are updated to point at the env var.Escape hatch for CI/tests (so they stay offline + fast)
init.test.ts: the env is wired into therunInitspawn helper — one place covers all 10 invocations..github/workflows/ci.yml: both CLI smoke-test steps setHYPERFRAMES_SKIP_SKILLS=1..github/workflows/windows-render.yml: the canary scaffold step sets it.Docs
--skip-skillsnow say the flag is temporarily ignored and point at the env var.skills-manifest.jsonwas regenerated by the pre-commit hook to match.What this does not change
The source of skills is unchanged —
initalready installs via a--full-depthgit clone ofmainHEAD (#1753, #1744), which bypasses the laggy skills.sh blob. This PR only ensures the check/pull always runs; it does not alter where the skills come from.Reverting
Temporary measure. Once skills.sh catches up with
mainand the #1753 doc change propagates, revertinit.ts'sback to
args["skip-skills"] === trueand drop the notice. The CI/test env vars are harmless to leave in place. The intent is documented inline at the change site.Test plan
bun run --filter @hyperframes/cli test -- init.test.ts— 20/20 pass (offline, via the env escape hatch).--skip-skillsno longer skips. Ran the exact agent traceinit … --non-interactive --skip-skills --example=blankwith no env set → output:HYPERFRAMES_SKIP_SKILLS=1→ zero skills-check output,initcompletes normally (exit 0).oxlint+oxfmtclean on changed files; lefthook pre-commit (lint / format / typecheck / commitlint / skills-manifest) passed.yaml.safe_load).🤖 Generated with Claude Code