feat(skills): default CodeWhale skill pack (bundled v5)

[Hmbown]

Summary

Ships the v0.9.1 default end-user skill pack required by #4691, based on post-#4675 main (Skills Manager ancestry included).

Catalog (bundled version 5)

End-user (model + user): interview, plan, implement, debug, test, review, security-review, simplify, verify, research, frontend-design, webapp-testing, document, dataviz, docx, pdf, pptx, xlsx

Compatibility aliases: documents → docx, presentations → pptx, spreadsheets → xlsx

Power (explicit-only): batch, dependency-update, release

System & extension (unchanged meta): skill-creator, skill-installer, plugin-creator, mcp-builder, delegate, fleet-manager

Optional integration: Feishu is not auto-installed for every user.

Upgrade / safety

Case	Behavior
Fresh install	Installs full v5 pack + marker 5
Upgrade from marker 4	Adds new skills; refreshes only exact shipped bodies
Unchanged v4-best-practices	Retired (exact body match only)
Modified v4-best-practices	Preserved
User-edited bundled skill	Never overwritten by name alone
Feishu	Optional; not universally installed

Base

origin/main containing #4675 / #4679 Skills Manager contracts.

Tests

CARGO_NET_OFFLINE=true cargo test -p codewhale-tui --bin codewhale-tui --locked skills::system
# 14 passed

Deliberately partial vs full #4691

This PR ships the asset catalog + install/migration machinery and provider-free install tests. Still follow-up-able (not blocking install):

• Full routing-eval fixture matrix + locale metadata tables for all shipped locales
• Deeper model-invocable metadata schema unification across prompt/tool surfaces
• Opt-in live Kimi K3 + second-model smoke command docs

Those can land as fast follow-ups without blocking the pack assets.

Closes #4691

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 3m 37s —— View job

---

Review: PR #4695 — default CodeWhale skill pack (bundled v5)

• Gather context (diff against origin/main, read changed files)
• Review crates/tui/src/skills/system.rs install/migration logic
• Review new/modified SKILL.md assets
• Check provider/model safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
• Check for reuse/duplication/simplification opportunities
• Check test coverage
• Post final review

Note: cargo invocations (cargo test, even cargo --version) required approval in this sandbox and none was granted, so I could not independently execute skills::system tests. Please update --allowedTools if you want me to run the build/test suite directly next time. I did trace the logic by hand against the existing test suite instead.

Correctness (crates/tui/src/skills/system.rs)

The install_one state machine (lines 270–307) is the core risk surface — I traced all four match arms against every scenario in the upgrade table (fresh install, version bump, newly-introduced skill, user-deleted dir, user-modified body) and didn't find a hole:

• Fresh install (None, _, false) → install. ✅
• Newly-bundled skill for an older marker (Some(_), Some(v), _ where v < introduced_in) fires regardless of dir_exists, which looks surprising at first, but the body-preservation guard at lines 296–302 (existing != skill.body → skip) is a second line of defense that catches the one real-world case this could hit: a user who happened to create a same-named custom skill before it became bundled. Good defense in depth.
• Version bump for an existing dir (Some(v), _, true where v != BUNDLED_SKILL_VERSION) → refresh only if body still matches shipped content.
• User-deleted directory at current version → correctly falls through to _ => false and is never recreated (covered by user_deleted_dir_is_not_recreated / user_deleted_all_dirs_are_not_recreated).

retire_unchanged_v4_best_practices (lines 351–363) is byte-exact-only, independent of the marker check, and is safe to call unconditionally every launch (worst case: a harmless no-op re-write of the same marker value).

One thing worth double-checking with the author rather than a bug per se: when should_install is true but the body is preserved (user-modified), install_one returns Ok(false), so changed never flips to true for that skill. If every bundled skill happens to be user-modified, the version marker never advances past the old value, meaning the full match logic re-runs (and no-ops) on every subsequent launch instead of settling at v5. Not a correctness bug (behavior is still correct/idempotent), just a minor perpetual-recompute case worth a comment if intentional.

Provider/model safety (EPIC #2608)

No provider/model routing code is touched by this diff — system.rs only writes/reads SKILL.md bodies and a plain-text version marker. No prefix-inference concerns here.

Design note: "explicit-only" skills are still auto-installed

batch, dependency-update, and release are documented as "Power (explicit-only)" in the PR body, but they're unconditionally included in BUNDLED_SKILLS (system.rs:200-214) and get written to every user's skills dir on install/upgrade like every other bundled skill. "Explicit-only" here is enforced purely through prompt text (the description/body says "Explicit-only" / "Do not start from ambient wording alone") — there's no code-level gate keeping the model from surfacing/using them unprompted, since render_skills_block (skills/mod.rs) lists every bundled skill's name+description the same way regardless of the invocation: frontmatter field (which isn't parsed/used at all — only name, description, and description_<tag> keys are read in parse_skill). If that's the intended design (install for everyone, rely on the model to respect the "explicit-only" wording), this is fine — just flagging so it's a deliberate choice rather than an assumption that these three are gated at install time.

Similarly, the new aliases-for: frontmatter key on documents/presentations/spreadsheets (compat aliases) isn't read by the parser anywhere — it's inert documentation-only metadata today. Harmless, but if there's a plan to have the runtime actually redirect alias skills to their target automatically, that plumbing doesn't exist yet.

Reuse / simplification

No duplication concerns. The bundled first-party installer (system.rs) and the registry-based external skill installer (install.rs) are appropriately separate — different semantics (byte-exact shipped-body preservation vs. registry-tracked provenance) justify not sharing code.

Tests

Coverage is thorough and matches the upgrade/safety table in the PR description precisely: fresh install, idempotence, user-deletion (single + all), version-bump reinstall, partial-previous-install (v4→v5 add), v4-best-practices retirement (unchanged vs. modified), user-modified bundled-skill preservation, and discovery/parse validation for every shipped skill. I didn't find missing edge cases beyond the minor "marker never advances if everything is user-modified" note above, which isn't really testable as a regression since it's not incorrect behavior.

Security

No secret handling, shell/exec, or input-validation surface in this diff — it's static asset bundling plus filesystem read/write behind the existing non-fatal error path at the main.rs call site.

Overall this is solid, carefully-tested work that matches its own stated upgrade contract. No blocking issues found.

[claude]

Claude finished @Hmbown's task in 1m 52s —— View job

---

Review: PR #4695 — default CodeWhale skill pack (bundled v5)

• Gather context (diff against origin/main, read changed files)
• Review crates/tui/src/skills/system.rs install/migration logic
• Review new/modified SKILL.md assets
• Check provider/model safety invariant (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608)
• Check for reuse/duplication/simplification opportunities
• Check test coverage
• Post final review

Note: As before, cargo invocations (cargo test, cargo check) require approval in this sandbox and none was granted, so I traced install_one/install_system_skills by hand against the test suite rather than executing it.

There's no new commit on this branch since the first pass (still 1 commit, 9243e8d), so this is an independent re-verification of the same diff against origin/main. Findings below.

Correctness (crates/tui/src/skills/system.rs)

The install_one state machine (lines 270–307) and the retire_unchanged_v4_best_practices helper hold up against every scenario in the PR's upgrade table (fresh install, version bump, newly-introduced skill, user-deleted dir, user-modified body, unchanged/modified v4-best-practices). All of it is exercised by the added tests and matches by hand-tracing.

One gap not covered by any test: a corrupted or non-numeric version marker permanently disables install/upgrade. In install_one (line 278), installed_number = installed_version.and_then(|v| v.parse::<u32>().ok()). If the marker file exists but its content isn't a clean u32 (e.g. truncated by a crash mid-fs::write at line 343, or hand-edited), the match at lines 280–291 becomes (Some(garbage), None, dir_exists) — none of the three specific arms match (arm 1 needs None, arms 2–3 need Some(version) i.e. a parsed number), so it silently falls through to _ => false for every skill, forever. Since fs::write isn't atomic, an interrupted write (crash/kill mid-launch) is the realistic trigger, and after that a user is permanently stuck on whatever skills happen to already be on disk with no self-healing path — install_system_skills would need to fall back to treating an unparsable marker like None (fresh/repair) rather than a silent no-op. Low severity (requires a rare interrupted-write to hit), but worth a defensive fallback since the failure mode is silent.

Everything else previously flagged (marker-never-advances-if-all-skills-are-user-modified — harmless perpetual no-op, not a bug) still stands and isn't a blocker.

Provider/model safety (EPIC #2608)

Confirmed no provider/model routing code is touched — system.rs only reads/writes SKILL.md bodies and a plain-text version marker. No prefix-inference concerns.

Design note: "explicit-only" skills are still unconditionally installed

batch, dependency-update, and release (system.rs:200–214) are documented "Power (explicit-only)" but are installed for every user exactly like the rest of the bundle — "explicit-only" is enforced only through the skill body's own ## Invocation wording (e.g. batch/SKILL.md:4,10: invocation: explicit-only, "Do not start from ambient wording alone"), not by any code-level gate. render_skills_block (skills/mod.rs) surfaces every bundled skill's name+description identically regardless of the invocation: frontmatter key, which isn't parsed anywhere (only name, description, and description_<tag> are read in parse_skill). Similarly, aliases-for: on documents/presentations/spreadsheets is inert — no runtime redirect exists yet. Both are likely intentional (rely on model instruction-following) rather than bugs, but worth confirming with the author since the frontmatter implies there's supposed to be enforcement.

Reuse / simplification

No duplication concerns — the first-party bundled installer (system.rs) is correctly kept separate from the registry-based external installer (install.rs); the byte-exact preservation semantics differ enough to justify not sharing code.

Tests

Coverage matches the stated upgrade/safety table precisely (fresh install, idempotence, single/all user-deletion, version-bump reinstall, partial-previous-install v4→v5, v4-best-practices retirement unchanged vs. modified, user-modified bundled-skill preservation, discovery/parse validation for every shipped skill). Only gap is the marker-corruption case noted above, which isn't exercised anywhere.

Security

No secret handling, shell/exec, or input-validation surface — static asset bundling plus filesystem read/write behind the existing non-fatal error path (main.rs:8947-8949, confirmed install_system_skills errors are logged as warnings and never abort startup).

---

Overall: solid, well-tested change consistent with its own upgrade contract. One low-severity, untested edge case (corrupted marker → permanent no-op) worth a defensive fix or at least a follow-up issue; the "explicit-only" enforcement gap is worth a quick confirmation from the author that it's intentional. No blocking issues.

Fix marker-parse fallback → on branch codex/v091-default-skill-pack-4691

[Hmbown]

The deliberately non-blocking follow-ups from this PR are now tracked explicitly in #4698 for v0.9.2: routing-eval fixtures, locale routing metadata, runtime treatment of invocation: / aliases-for:, opt-in Kimi K3 plus second-provider smoke documentation, and corrupt marker recovery. This keeps the shipped v5 pack in v0.9.1 while preserving the remaining contract work.