feat(skills): add sync all CLIs action

[Myoontyee]

Summary

• Add a Tauri command that syncs every installed skill to all supported CLI skill directories.
• Add a Skills panel button to trigger the bulk sync action with localized status messages.
• Add a regression test covering the all-CLI sync behavior.

Test plan

• cargo test --manifest-path src-tauri/Cargo.toml sync_all_to_supported_apps_enables_and_copies_every_installed_skill (blocked locally: disk space exhausted during compilation)
• pnpm typecheck (blocked locally: node_modules are not installed in this checkout)

Notes

• This PR is intentionally split from the proxy usage bug fix to keep the feature review separate.

[farion1231]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2022d9cab9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Count skipped apps only when sync is actually skipped

sync_all_to_supported_apps increments skipped_count for already-enabled apps, but it still executes sync_to_app_dir for those same app/skill pairs. This lets one operation be counted as both skipped and synced (or failed), so result totals become internally inconsistent and callers cannot reliably interpret progress metrics from SkillSyncAllResult.

Useful? React with 👍 / 👎.

[farion1231]

@claude review

[claude]

Claude finished @farion1231's task in 2m 43s —— View job

---

Code Review

• Gather context from PR metadata
• Read changed files in Rust backend
• Read changed files in React frontend
• Analyze and post review findings

---

Overall this is a clean, well-scoped feature. The layer separation (service → command → hook → component), i18n coverage across all three locales, and the included regression test are all good practices. A few issues worth addressing before merge, one of which is a logic bug that makes the result metrics unreliable.

---

P2 — Bug: skipped_count is misleading and creates double-counted metrics

src-tauri/src/services/skill.rs (~line 1418)

for app in &supported_apps {
    if skill.apps.is_enabled_for(app) {
        skipped_count += 1;   // counted as skipped…
    }

    match Self::sync_to_app_dir(&skill.directory, app) {
        Ok(()) => {
            updated_apps.set_enabled_for(app, true);
            synced_count += 1;  // …but also counted as synced
        }
        ...
    }
}

sync_to_app_dir is not idempotent — it unconditionally removes the destination before recreating it (symlink or copy). Nothing is actually skipped. A skill already enabled for all 5 apps would produce skipped_count = 5 and synced_count = 5 — 10 counter increments for 5 operations — making both fields unreliable.

The fix depends on intent:

Option A — Actually skip already-enabled apps (no-unnecessary-resync):

if skill.apps.is_enabled_for(app) {
    skipped_count += 1;
    continue;  // don't call sync_to_app_dir
}

Option B — Always force-resync (idempotent "sync all"), drop the skip concept:

// Remove skipped_count and the is_enabled_for check entirely;
// synced_count tracks every successful operation

Fix this →

---

P2 — Test doesn't catch the double-count bug

src-tauri/tests/skill_sync.rs (~line 165)

The test asserts skipped_count == 1 (correct: Claude was pre-enabled) and failures.len() == 0, but never asserts synced_count. With the current implementation synced_count would be 5 (all apps succeed), which would reveal that a skipped+synced overlap occurred. Adding this assertion would have caught the bug:

// If skipping: 1 skipped (Claude) + 4 newly synced = 5 total ops
assert_eq!(result.synced_count, 4);
// or, if force-resync intent: everything synced, nothing skipped
assert_eq!(result.synced_count, 5);
assert_eq!(result.skipped_count, 0);

Fix this →

---

P3 — Success toast uses total counts, not actual synced counts

src/components/skills/UnifiedSkillsPanel.tsx (~line 295)

toast.success(
  t("skills.syncAll.success", {
    skillCount: result.skillCount,  // total skills
    appCount: result.appCount,       // total supported apps (always 5)
  }),

result.syncedCount is returned from the backend and typed in SkillSyncAllResult, but never used in the UI. The message always shows "Synced N skills to 5 CLIs" regardless of how many were actually newly synced. After the skip-count fix above, using syncedCount (or a combination of syncedCount/skippedCount) would give users more accurate feedback.

---

P3 — supported_skill_apps() silently diverges when new AppType variants are added

src-tauri/src/services/skill.rs (~line 470)

The exhaustive match in SkillApps::is_enabled_for / set_enabled_for will cause a compile error if a new AppType is added without updating it. supported_skill_apps() has no such guard — it hardcodes an array that could be silently incomplete. A brief comment noting the exclusion reason for OpenClaw (and flagging the need to update when new types are added) would prevent a future silent omission:

fn supported_skill_apps() -> [AppType; 5] {
    // OpenClaw excluded: Skills not supported. Update this list when new AppType variants are added.
    [AppType::Claude, AppType::Codex, AppType::Gemini, AppType::OpenCode, AppType::Hermes]
}

---

P3 — DB update failure mid-loop leaves filesystem/DB partially inconsistent

src-tauri/src/services/skill.rs (~line 1442)

db.update_skill_apps(&skill.id, &updated_apps)?;

A ? here will abort the loop on the first DB write failure. Filesystem changes (symlinks created) for the current skill will have happened, but subsequent skills in the map won't be processed. On re-run the missed skills would be picked up, but the partial state isn't reflected in the returned error. This is probably acceptable for an MVP, but worth a comment or a consideration to accumulate errors rather than fail-fast if you want reliable bulk operations.

---

Positives

• Clean layer separation: service logic, Tauri command, React hook, and UI component are all properly isolated.
• All three i18n locales (en, ja, zh) are updated consistently with matching keys.
• The get_home_dir() refactor incidentally cleans up a verbose error call — good catch.
• Button is correctly disabled when no skills are installed (skills.length === 0).

[farion1231]

@claude review

(Re-running with the updated review prompt for comparison.)

[claude]

Claude finished @farion1231's task in 1m 37s —— View job

---

Code Review (re-run with updated prompt)

• Read changed files in Rust backend
• Read changed files in React frontend
• Analyze and post review findings

---

Important — skipped_count double-counts already-enabled apps (confidence: 95)

src-tauri/src/services/skill.rs:1414-1429

for app in &supported_apps {
    if skill.apps.is_enabled_for(app) {
        skipped_count += 1;          // counted as skipped…
    }

    match Self::sync_to_app_dir(&skill.directory, app) {
        Ok(()) => {
            updated_apps.set_enabled_for(app, true);
            synced_count += 1;        // …and also as synced
        }
        ...
    }
}

sync_to_app_dir (skill.rs:1668) is non-idempotent: it unconditionally removes the destination and recreates the symlink/copy. Nothing is actually skipped, but the loop credits one operation to both counters. A skill enabled for all 5 apps emits skipped_count = 5 and synced_count = 5 — 10 increments for 5 operations. The fields are returned across the Tauri boundary in SkillSyncAllResult, and the test at skill_sync.rs:165 already asserts skipped_count == 1 for one pre-enabled app, locking in the wrong semantics.

Two valid fixes depending on intent:

// A: actually skip already-enabled apps
if skill.apps.is_enabled_for(app) {
    skipped_count += 1;
    continue;
}

// B: drop the skip concept; sync is always idempotent re-sync
// remove the is_enabled_for branch and skipped_count entirely

Fix this →

---

One blocking issue. Other items raised in the prior review (test assertion gap, toast wording, future-proofing comment, DB-mid-loop partial state) are below the bar for this stricter pass.
• Branch: feat/sync-all-skills-to-clis