feat(cli): add skills version check, update, and freshness manifest

[WaterrrForever]

Give the HyperFrames skill bundle a content fingerprint so agents and users can tell whether installed skills are the latest version, on any platform that can run the CLI.

• skills-manifest.json (repo root): per-skill sha256 over the whole skill directory; minimal {source, skills}, no version/timestamp so it is fully deterministic. Generated by scripts/gen-skills-manifest.ts.
• hyperframes skills check [--json]: compares installed skills to the manifest; exits non-zero when something is outdated (agent/CI gate).
• hyperframes skills update: thin wrapper over npx skills update.
• Passive nudge on render/lint/validate when skills are stale (24h cache, same opt-out as the CLI self-update notice).
• "latest" resolved via git ls-remote + SHA-pinned raw URL to dodge GitHub raw-CDN lag, falling back to the main branch URL.
• CI job + lefthook hook keep skills-manifest.json in sync with skills/.

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: skills version check, update, and freshness manifest

SHA reviewed: 825577435c3d3684d59ec1de73933fa90dfdc850

Overall, this is a well-structured feature. The deterministic content-hash approach (no version/timestamp) is the right call — it means the manifest only drifts when actual skill content changes, which makes the CI guard (--check) churn-free. The architecture mirrors the existing updateCheck.ts pattern cleanly, and the test coverage on the hashing/diffing core is solid. Five commits, each well-scoped.

A few things worth discussing:

---

1. hashInstalled only hashes manifest-listed skills — local-only skills are invisible to the diff

hashInstalled() iterates skillNames (from the manifest), so any installed skill whose name isn't in the manifest is silently skipped. The diff union in diffSkills then only sees skills passed to it, so a locally-installed skill that exists on disk but wasn't in the installed record never appears as local-only.

This is fine for the "is an update available?" question (local-only doesn't affect updateAvailable), but the --json output's skills array and the human-readable renderCheck both advertise a local-only status that can never actually appear in the checkSkills end-to-end flow. If local-only is dead code in practice, consider either:

• scanning installed directories for skills not in the manifest (so the status works), or
• removing local-only from the public types/rendering to avoid confusion.

Not a blocker — just a fidelity gap in the reported output.

---

2. listFilesSorted double-sorts

function listFilesSorted(dir: string): string[] {
  const out: string[] = [];
  const walk = (d: string): void => {
    for (const name of readdirSync(d).sort()) {
      // ...
      else out.push(p);
    }
  };
  walk(dir);
  return out.sort(); // second sort on the full result
}

readdirSync(d).sort() sorts each directory's entries, but the final out.sort() re-sorts the accumulated absolute paths. Since walk visits directories depth-first with sorted entries, the output is already in sorted order for most filesystem layouts — but the final .sort() is the one that actually guarantees determinism (it sorts by full path string). The per-directory .sort() inside walk is then redundant. Nit, not a bug.

---

3. TOCTOU in gen-skills-manifest.ts (matching the CodeQL flag)

const inSync = committed !== null && signature(committed.skills) === signature(fresh.skills);
// ...
writeFileSync(outPath, JSON.stringify(fresh, null, 2) + "\n");

CodeQL flagged the existsSync → writeFileSync as a potential TOCTOU race. In practice this script runs in a lefthook pre-commit hook or CI — single-writer, no concurrent mutation risk. The CodeQL finding is a false positive for this use case. Mentioning it so it doesn't surprise anyone reviewing the security tab.

---

4. resolveLocalManifest path detection is fragile

if (source && (source.startsWith(".") || source.startsWith("/"))) {
  return resolveLocalManifest(source);
}

This heuristic treats anything starting with . or / as a local path. A Windows absolute path (C:\...) would fall through to the remote fetch path. Since the rest of the PR is cross-platform-conscious (CRLF normalisation, sep usage, win32 npx tests), this is a gap — though admittedly an unlikely one since --source is an advanced/internal flag.

---

5. fetchManifest trusts the response shape via as SkillsManifest

return (await res.json()) as SkillsManifest;

If someone publishes a malformed skills-manifest.json (or a CDN serves an error page as 200), this silently casts whatever JSON came back. A light runtime check (e.g., if (!result.skills) throw ...) would make the failure mode obvious instead of surfacing as a cryptic "Cannot read properties of undefined" later in diffSkills.

---

6. checkSkillsForUpdate reads config twice on cache miss

export async function checkSkillsForUpdate(force?: boolean): Promise<SkillsUpdateMeta> {
  if (!force && cacheFresh(readConfig().lastSkillsCheck, Date.now())) return getSkillsUpdateMeta();
  // ...
}

When the cache is fresh, readConfig() is called once in cacheFresh, then getSkillsUpdateMeta() calls readConfig() again. Minor — readConfig is cheap (cached) — but worth noting.

---

7. Behavioural change in init: interactive prompt removed

The interactive init path (second defineCommand block in init.ts) previously asked "Install AI coding skills?" via clack.confirm. Now it always installs all skills silently (unless --skip-skills). This is a UX decision, not a bug, but it's a non-trivial behavior change for interactive users who might not want skills installed. The PR body mentions this as intentional, and --skip-skills is the escape hatch — just flagging for visibility.

---

8. Minor: args._?.[0] guard in the parent skills command

async run({ args }) {
  if (!args._?.[0]) await installAllSkills();
},

This relies on citty's _ positional array to detect whether a subcommand was invoked. If citty ever changes how it populates _ for subcommands (or if someone types hyperframes skills --json), the guard could misfire and trigger an install. Low risk with citty's current behavior, but a comment explaining the mechanism would help future readers.

---

CI status

All required checks pass. CodeQL (javascript-typescript) is still pending but the flagged findings are on dead code from a prior push and a false-positive TOCTOU (see point 3 above). No blockers.

Verdict

Solid work. The design is clean, the hashing is deterministic, and the feature fills a real gap. The items above are discussion-tier — none are merge blockers. Ship it after a quick pass on points 1 and 5 (the local-only dead code and the unvalidated fetch response), or document them as known limitations.

— Miga

[miguel-heygen]

Reviewed current HEAD 1cf81c761af40d810908b26fd0f17a51c860d77f after Miga's LGTM at the earlier 8255774 head.

Strengths:

• packages/cli/src/utils/skillsManifest.ts:87 keeps the manifest hash deterministic over the whole skill directory, with CRLF normalization for text files.
• packages/cli/src/utils/skillsManifest.ts:267 adds a clear runtime manifest-shape guard instead of blindly casting remote JSON.
• packages/cli/src/utils/skillsManifest.test.ts:81 now pins that local skills outside the HyperFrames manifest are ignored rather than exposing a dead local-only status.

Blocker:

• packages/cli/src/commands/skills.ts:163 exposes hyperframes skills update as the recovery path after skills check reports stale/missing skills, but it calls installAllSkills(), and installAllSkills() still swallows both missing npx and failed skills add attempts (packages/cli/src/commands/skills.ts:67 and :76-80). That means the advertised agent/CI gate from packages/cli/src/commands/skills.ts:149 (hyperframes skills check || npx hyperframes skills update) can exit 0 even when the update did not install anything. For this new command surface, failed update must be observable/non-zero. Suggested shape: make installAllSkills return success/failure or accept a strict option; keep best-effort behavior only where desired, but have skills update set process.exitCode = 1 or throw when npx is unavailable or skills add fails. Add a unit test that simulates spawn closing non-zero and asserts the update command fails/non-zero.

CI note: required checks were still pending when I reviewed (Build, Typecheck, Test, CLI smoke, Windows, preview parity, CodeQL JS/TS), so the PR is not stampable yet even after the blocker is fixed.

Verdict: REQUEST CHANGES
Reasoning: The freshness checker is useful, but the new update command can currently report success after a failed install, which breaks the agent/CI recovery contract this PR introduces.

— Magi

[jrusso1020]

/code-review — additive to Miga's #4579505434 (head 8255774) and Magi's REQUEST_CHANGES at #4579649137 (head 1cf81c7)

Reviewed at head 1cf81c7. Miga's items 1/2/4/5 are addressed in this revision (verified at skillsManifest.ts:267 validator, skillsManifest.ts:267 asSkillsManifest, skillsManifest.ts:351 isAbsolute, skillsManifest.test.ts:81 updated test). Magi's blocker (silent-success on failed update) stands.

This post focuses on what James asked specifically: "make sure it robustly handles all agents like skills add or gstack." Verdict + 4 additive findings below.

Robustness verdict (James's ask)

Covered: install methods landing in any of {.claude, .agents, .codex, .cursor}/skills × {project cwd, global $HOME} are detected by defaultSkillRoots (skillsManifest.ts:151-164). That includes hyperframes init, hyperframes skills, hyperframes skills update, manual npx skills add, and manual git clone into any of those conventions.

Real gaps for install-method robustness:

1. gstack is not a concept in this repo (grep -rn returns zero hits). If James meant a specific installer / agent tool, point me at it — the verdict for that tool depends on what path it writes to. Best guess: he meant generic "agent install workflows beyond skills add," which is the next bullet.

2. Closed-list of agent conventions drifts from the CLI's own agent detection. packages/cli/src/telemetry/agent_runtime.test.ts:204+ already tests for gemini_managed_agent (detected via /.agents/ mount + gVisor), windsurf (TERM_PROGRAM), and others — but defaultSkillRoots only enumerates 4 conventions. A user installing HF skills via, say, a Gemini-sandbox agent's native skill dir (NOT .agents/skills) lands them somewhere skills check silently returns location: null for. From the user's POV: "I installed it, why does check say none?"

   Mitigations the PR already has: --dir override exists. But it requires the user to know about it.

   Suggested shape: either mine the agent-detection signal the telemetry module already computes, or scan a broader curated list of */skills/<name>/SKILL.md patterns under cwd + $HOME. Could also be config-driven (a hyperframes.json field for additional roots).

Additive findings (not in Miga or Magi)

3. Test gap on the install-detection layer. skillsManifest.test.ts (130 LOC) covers diffSkills and hashSkillBundle thoroughly. But locateInstall, defaultSkillRoots, agentFromDir — the exact code paths that determine install-method robustness — have zero test coverage. For James's robustness ask, this is the surface that needs locking. Recommend: temp-dir fixtures (.claude/skills/<name>/SKILL.md etc.), call locateInstall(['<name>']), assert root.dir + root.agent. One test per convention × scope = 8 cases.

4. Init pays a remote GitHub round-trip on every invocation (per 8e3435f06). ensureSkillsCurrent (init.ts) now calls checkSkills → resolveLatestManifest → git ls-remote + fetch. Bounded by FETCH_TIMEOUT_MS = 4000 so worst case +4-8 s on a network-isolated init. Best-effort fallback handles fetch failure (installs anyway). Worth noting in SKILL.md alongside the --skip-skills mention; not a blocker.

5. findRepoManifest walks up only 8 levels. (skillsManifest.ts:249-259). Hardcoded depth. Deep monorepos / nested-workspace layouts (e.g. some pnpm or Nx setups can be >8 deep before hitting the repo root) silently fall through to the remote fetch. Unlikely to bite in practice for HF's shallow layout; flagging for completeness.

6. Comment in gen-skills-manifest.ts mentioning the local-only status is now stale (post-1cf81c7 cleanup). Quick grep on the diff: the type has been removed from skillsManifest.ts but the manifest-generator script's header comment still talks about a 4-state space. Sub-grep before merge.

CI status

Required checks still pending (Build, Typecheck, Test, CLI smoke, Windows, preview parity, CodeQL JS/TS) per Magi's note. Not stampable until those settle.

Verdict

The core feature is solid; Miga + Magi between them have surfaced every code-blocker I'd flag. James's robustness question lands at finding #2 — the install-detection convention list drifts from the agent-detection list the same package already maintains. Worth either expanding the list to match telemetry or making detection signal-driven.

Not stamping (PR author is external-allowlisted, not a trusted-stamper per our protocol).

— Jerrai

[jrusso1020]

Follow-up on finding #2 (convention-list drift) — sharper after reading the upstream

Reading vercel-labs/skills' README: "Supports OpenCode, Claude Code, Codex, Cursor, and 68 more agents." So the upstream tool we delegate to via npx skills add can install into 72 agent destinations. defaultSkillRoots (skillsManifest.ts:151-164) enumerates 4 of them. The other 68 are silent-misses for skills check.

Concrete repro path: an OpenCode user runs hyperframes init from inside OpenCode → upstream npx skills add auto-detects OpenCode and installs HF skills into OpenCode's native skill dir (not .claude, not .codex, not .cursor) → next hyperframes skills check returns location: null, exit 0, "No HyperFrames skills found in the usual locations." The user installed them but can't get a freshness signal.

gstack (the other reference) isn't actually a concern here — Garry Tan's bundle installs into Claude Code's conventions, which defaultSkillRoots already covers. And post-1cf81c7 cleanup, diffSkills correctly ignores gstack's skills (they aren't in the HF manifest), so the two coexist cleanly in .claude/skills/.

Suggested fix (one of):

1. Query upstream for its agent → install-path mapping (if npx skills exposes it; if not, file a feature request).
2. Auto-discover: scan for any <dir>/skills/<name>/SKILL.md under cwd and $HOME where <name> matches a manifest entry. Future-proofs without a closed list.

Option 2 is simpler and stays current as upstream adds agents. Not a blocker for this PR — could land as a follow-up — but the install-method-agnostic claim needs either this or an explicit "--dir is required for non-claude/codex/cursor installs" footnote.

— Jerrai

[miga-heygen]

Re-review: skills version check, update, and freshness manifest

SHA reviewed: 497faf6454fb0d7e9307f0616705ed245d1bcd1a
Previous review: 825577435 (8 discussion-tier findings, none blocking)

What changed since 825577435 (3 new commits)

Four of my original eight findings were addressed directly in 1cf81c7:

#	Finding	Status
1	local-only status unreachable	Fixed — type + summary field removed; diffSkills only iterates manifest skills now
2	listFilesSorted double-sorts	Fixed — per-directory .sort() dropped; single final sort remains
3	Windows path detection in resolveLatestManifest	Fixed — uses path.isAbsolute()
4	fetchManifest blind as cast	Fixed — asSkillsManifest() runtime shape guard
5	checkSkillsForUpdate reads config twice on hit	Unchanged (informational, not acted on — fine)
6	Interactive init prompt removed	Superseded — ensureSkillsCurrent() checks GitHub, installs only when stale; --skip-skills is the escape hatch
7	args._?.[0] guard fragility	Unchanged (informational — fine)
8	CodeQL TOCTOU	N/A — the direct-from-GitHub update code (staging dir, downloadFile, etc.) was removed entirely; manifest module is now check-only

Magi's blocker (silent-success on failed skills update) is addressed in 497faf6: installAllSkills now accepts strict: true; the update subcommand uses it, and two new tests verify non-zero exit on spawn failure and zero exit on success.

James/Rames's robustness concern (closed 4-agent convention list) is addressed in 497faf6: defaultSkillRoots replaced by discoverSkillRoots, which scans structurally for <host>/skills under cwd + $HOME (+ XDG .config/<host>/skills). No closed list. Test suite covers 11 agent conventions, scope priority, claude-code preference, unknown-host discovery. This is a thorough fix.

New findings on 497faf6

All discussion-tier. None blocking.

---

1. discoverSkillRoots scans only immediate children — nested agent dirs are invisible

discoverSkillRoots calls listSubdirs(base) and listSubdirs(join(base, ".config")), so it finds <base>/.kiro/skills and <base>/.config/opencode/skills. But if an agent convention nests deeper than one level under .config (e.g. a hypothetical .config/some-vendor/agent/skills), it would be missed. This is fine for every known agent today, but since the PR's goal is "no closed list, future-proof," worth a sentence in the JSDoc noting the one-level-deep assumption so a future author knows where to look if a new agent breaks the pattern.

---

2. agentLabel edge case: host dir is just . or ..

function agentLabel(hostDir: string): string {
  const name = hostDir.replace(/^\.+/, "");
  return name === "claude" ? "claude-code" : name || "unknown";
}

If hostDir is "." or "..", name becomes "" and the label is "unknown". Not a bug (those won't appear from listSubdirs), but the regex ^\.+ strips all leading dots — so a host like ..hidden becomes hidden rather than .hidden. Cosmetic only, since listSubdirs filters to actual directory entries.

---

3. Test isolation: skills update tests call import("./skills.js") without resetting module state

The two new skills update tests (skills update exits non-zero when the install fails and skills update exits zero on a successful install) import ./skills.js dynamically. Because vi.resetModules() runs in beforeEach, these get fresh module instances — good. But they manipulate process.exitCode directly and restore it in a finally block. If the test throws before the finally (e.g. in a future refactor that adds an await that rejects), process.exitCode leaks. A safer pattern is afterEach(() => { process.exitCode = 0; }), but this is minor.

---

4. ensureSkillsCurrent catch block silently proceeds to install on any error

try {
  const result = await checkSkills({ cwd: destDir });
  needsInstall = result.updateAvailable;
} catch {
  // Couldn't reach GitHub (offline, rate-limited) — install anyway.
}

The catch block treats any checkSkills failure as "offline, install anyway." If checkSkills throws for a local reason (e.g. malformed manifest on disk, readFileSync throwing on a permissions error), the catch hides a real problem and triggers a potentially unnecessary reinstall. Fine for the "init is best-effort" contract, but logging the error at debug level would help when diagnosing "why does init always reinstall?"

---

5. The direct-from-GitHub update machinery was fully removed — good, but skillsManifest.ts still imports unused fs symbols

The current skillsManifest.ts imports are:

import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";

Clean. But the original commit (visible in the CodeQL comment's diff context) imported cpSync, mkdirSync, rmSync, writeFileSync too, and those were removed. Just confirming the current head is clean — it is.

---

CI status

All required checks pass. Two Windows jobs (Render on windows-latest, Tests on windows-latest) and CodeQL (javascript-typescript) are still pending but non-blocking. The CodeQL findings from the initial push (TOCTOU race in gen-skills-manifest.ts, insecure temp file, network data written to file) are all on dead code from the removed direct-update machinery — the current skillsManifest.ts has none of those patterns.

Verdict

Every previously raised concern (mine, Magi's blocker, James/Rames's robustness gap) has been addressed. The discoverSkillRoots approach is the right call — structural discovery beats a maintained list. The strict-mode update fix closes the agent/CI recovery contract gap cleanly. Test coverage on the install-detection layer is solid (11 conventions, scope priority, unknown-host).

The new findings above are all informational — none affect correctness or the merge decision. Ship it.

— Miga

[jrusso1020]

Re-review at head 497faf645 — all prior findings addressed

Verified at 497faf645. This revision addresses every item from my prior reviews plus Magi's blocker:

Addressed

1. Magi's blocker (update swallows install failures) — installAllSkills now accepts a strict option; updateCommand passes strict: true and sets process.exitCode = 1 on failure (skills.ts:172-185). Two new tests in skills.test.ts simulate the non-zero spawn close + the success path. check || update recovery contract is now honest.

2. My finding #2 (convention drift — 72 agents vs 4) — completely refactored. defaultSkillRoots is replaced by discoverSkillRoots (skillsManifest.ts:182-202) which scans <base>/<host>/skills/ AND <base>/.config/<host>/skills/ (XDG) — auto-discovers any agent host structurally. No closed list. Sorted with claude-code first, alphabetical after. Tested with "auto-discovers an unknown/new agent host (no closed list)".

3. My finding #3 (test gap on install detection) — 128 new test lines covering: project > global scope, claude-code preference, no-install case, --dir override, unknown-host auto-discovery, multi-convention scope ordering. The install-detection layer is now well-pinned.

4. My finding #4 (init does remote round-trip) — documented in SKILL.md.

5. My finding #5 (findRepoManifest 8-level limit) — bumped to 16 levels, with a comment explaining the bound.

6. My finding #6 (stale local-only comment in gen-skills-manifest.ts) — grep returns zero remaining references. Clean.

Verdict on James's robustness ask, post-this-commit

For the install-method-agnostic concern: resolved. Auto-discovery scans the filesystem for the structural pattern (<host>/skills/<manifest-skill>/SKILL.md) rather than enumerating known agents. So:

• gstack-way install (Garry Tan's Claude Code bundle → lands in .claude/skills/) → detected via discoverSkillRoots, agent inferred as claude-code. ✓
• vercel-labs/skills add into any of its 72 agents (e.g., OpenCode user installing into ~/.opencode/skills/ or ~/.config/opencode/skills/) → detected, agent inferred as opencode. ✓
• Future agent ecosystems that follow the same <host>/skills/<name>/SKILL.md convention → detected without code change. ✓
• Non-conforming installs (some hypothetical agent that uses <host>/agent-skills/<name>/SKILL.md or similar) → still need --dir override, but this is an acceptable escape hatch for an unknowable edge.

Small notes (none blocking)

• Cosmetic edge case in discoverSkillRoots: the first loop iterates ALL direct subdirs of base including .config, so if a layout like <base>/.config/skills/<name>/SKILL.md exists (unusual — most things use <base>/.config/<host>/skills/), the dir would be picked up with agent: "config". The XDG branch handles the proper layout. Low-probability, no fix needed.
• Agent-current preference: if a user has BOTH Claude Code and Cursor installs and is currently running on Cursor, the check reports Claude Code's state first (due to the sort priority). Could optionally prefer the detected agent runtime (TERM_PROGRAM / CLAUDECODE / etc., same signals telemetry/agent_runtime.ts already uses), but this is a polish-tier improvement, not a robustness gap.

CI

Still pending on Windows + Tests + CodeQL JS/TS — same as before. Not stampable until those settle.

Verdict

No more blocking concerns from my side. Holding off on stamping per the external-author protocol; routing the stamp decision back to James once CI clears.

— Jerrai

[miguel-heygen]

Re-reviewed current HEAD 497faf6454fb0d7e9307f0616705ed245d1bcd1a.

Strengths:

• packages/cli/src/commands/skills.ts:64-85 resolves my prior blocker cleanly: installAllSkills() now has strict mode, while best-effort callers keep the old behavior.
• packages/cli/src/commands/skills.ts:178-183 makes hyperframes skills update fail non-zero when the recovery install fails, preserving the advertised skills check || skills update contract.
• packages/cli/src/utils/skillsManifest.ts:176-202 is the right robustness shape for the gstack / vercel-skills concern: structural discovery of <host>/skills and XDG <host>/skills, not a closed agent list.

Blocker:

• packages/cli/scripts/gen-skills-manifest.ts:78 — the latest GitHub status is still failing the aggregate CodeQL check with one high alert, js/file-system-race / “Potential file system race condition,” on this write. Per the repo gate I cannot stamp while a required/security check is red. Either adjust the write path to satisfy CodeQL or resolve/dismiss the alert with the repo’s normal security-review path, then rerun/let checks settle. The Windows tests job was also still pending when I pulled checks.

Verdict: REQUEST CHANGES
Reasoning: My previous functional blocker is fixed, but the current head is not stampable while the aggregate CodeQL check is failing with a high-severity alert on changed code.

— Magi

[jrusso1020]

Re-review at head a74315337 — all CI green, ready to merge pending stamp authorization

Verified at a74315337. Both red-CI items from my prior review are resolved cleanly:

CodeQL js/file-system-race → resolved

gen-skills-manifest.ts now uses try { readFileSync } catch instead of existsSync → readFileSync. The check-then-read race is eliminated; semantics are identical (missing/unreadable file → committed = null → write fresh manifest). This is option (a) from my prior workaround list. Inline comment correctly captures the intent.

CI: CodeQL job now passes (was failing).

Windows test flake → resolved

npxCommand.test.ts bumped from 10 s → 30 s per-call timeout and added an explicit 60 s vitest budget on the npx --version smoke test. The comment explains the rationale (real npx cold-start on Windows CI exceeds vitest's 5 s default). Keeps the assertion meaningful (still asserts a real semver string).

CI: Tests on windows-latest, Render on windows-latest, CLI: npx shim (windows-latest) all now pass.

Overall state

• All required CI checks: green.
• All findings from this review chain (Miga, Magi, mine): addressed.
• Feature substance: clean. Structural auto-discovery answers the install-method-robustness question; strict update closes Magi's blocker; CodeQL fixed; Windows tests fixed.

Stamping

Not stamping from my account per the standing external-author protocol — routing the stamp decision back to James in the Slack thread.

— Jerrai

[miga-heygen]

Re-review #3: skills version check, update, and freshness manifest

SHA reviewed: a7431533780c1412ca5726f96e9e4c51205b9154
Previous reviews: 825577435 (LGTM, 8 discussion items), 497faf64 (ship it, 5 informational items)

What changed since 497faf64

Two focused CI-fix commits:

5ac3a6c9eb — resolve CodeQL file-system race + de-flake Windows npx test

1. gen-skills-manifest.ts: replaced existsSync(outPath) ? readFileSync(...) : null with a straight try { readFileSync } catch { null } — eliminates the check-then-write TOCTOU race that CodeQL flagged as js/file-system-race (high). The existsSync import is also cleaned up. Behavior is identical: missing or unreadable file means "no committed manifest." Clean fix, the comment explains the rationale well.

2. npxCommand.test.ts: bumped exec timeout from 10s to 30s and added a 60s vitest timeout on the real npx --version smoke test. This is the right call — Windows CI cold-starts are genuinely slow. Keeping the real assertion (not mocking) preserves the test's value.

a743153378 — repair garbled npx smoke-test timeout comment

The explanatory comment in 5ac3a6c9eb accidentally got interleaved between the it() callback closing brace and the timeout argument, breaking oxfmt and cascading through preflight → preview-parity → regression. This commit moves the comment above the it() call. Straightforward formatting fix.

Verdict

Both commits are narrow, correct CI fixes for issues Magi flagged in the prior review cycle. No functional changes to the feature itself. All CI checks are green across the full matrix (including CodeQL, Windows tests, and the new skills-manifest sync job). The full review chain's findings (my 8 from round 1, 5 from round 2, Magi's CodeQL blocker, Magi's strict-mode blocker) are all addressed.

Ship it. Ready for stamp.

— Miga

[jrusso1020]

Approved.

[miguel-heygen]

Re-reviewed current HEAD a7431533780c1412ca5726f96e9e4c51205b9154 after James's explicit stamp authorization in Slack.

Audited: packages/cli/src/commands/skills.ts, packages/cli/src/commands/skills.test.ts, packages/cli/src/utils/skillsManifest.ts, packages/cli/src/utils/skillsManifest.test.ts, packages/cli/scripts/gen-skills-manifest.ts, packages/cli/src/utils/npxCommand.test.ts, current GitHub checks.
Trusting: broader CI workflow/package metadata wiring based on green required checks and prior reviewer convergence.

Strengths:

• packages/cli/src/commands/skills.ts:64-85 keeps install best-effort for interactive/init callers while adding strict failure behavior for callers that need a real recovery signal.
• packages/cli/src/commands/skills.ts:178-183 preserves the documented skills check || skills update contract by failing non-zero when the install fails.
• packages/cli/src/utils/skillsManifest.ts:176-202 resolves the gstack / vercel-skills robustness concern with structural discovery of <host>/skills and XDG <host>/skills, rather than a closed agent list.
• packages/cli/scripts/gen-skills-manifest.ts:52-60 removes the CodeQL-triggering existsSync precheck while preserving the missing-manifest behavior, and all CodeQL checks are now green.

No remaining blockers from me. Current checks are green, including CodeQL and both Windows jobs.

Verdict: APPROVE
Reasoning: The prior functional blocker and CI/security gate are both resolved at the current head, with tests covering the strict update failure path and install-root discovery.

— Magi