fix: retry curated skill installs on every startup#582
Merged
alchemistklk merged 1 commit intomainfrom Mar 26, 2026
Merged
Conversation
…stalls Curated skills that failed ClawHub download during first launch were never retried because initialize() only ran when the ledger didn't exist. Now initialize() runs on every non-CI startup — both copyStaticSkills and getCuratedSlugsToEnqueue are idempotent via ledger checks. Also fixes dev mode skills dir to use NEXU_HOME-scoped path instead of Electron userData, so dev and packaged app behavior are consistent.
📝 WalkthroughWalkthroughThe pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/Runtime
participant Service as SkillhubService
participant DB as SkillDB
participant CuratedSkills as copyStaticSkills
participant Queue as Install Queue
Note over App,Queue: New Flow: Initialize on Every Startup
App->>Service: start()
activate Service
Service->>Service: initialize()
activate Service
Service->>DB: Check existing ledger
DB-->>Service: Ledger data
Service->>CuratedSkills: copyStaticSkills()
activate CuratedSkills
CuratedSkills->>DB: Fetch all known slugs
DB-->>CuratedSkills: Slug set
CuratedSkills->>CuratedSkills: For each STATIC_SKILL_SLUG:<br/>Check if in ledger
note right of CuratedSkills: If in ledger → Skip<br/>If not → Copy
CuratedSkills-->>Service: Skipped/Copied results
deactivate CuratedSkills
Service->>Service: getCuratedSlugsToEnqueue()
Service->>Queue: enqueue(slug, "managed")
deactivate Service
Queue-->>Service: Queued
deactivate Service
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/main/runtime/manifests.ts (1)
237-244: Consolidate skills-dir resolution into one local constant.The same packaged/dev path decision is duplicated in two places. Computing it once reduces drift risk.
♻️ Suggested refactor
- ensureDir( - isPackaged - ? getOpenclawSkillsDir(userDataPath) - : path.resolve( - runtimeConfig.paths.nexuHome, - "runtime/openclaw/state/skills", - ), - ); + const openclawSkillsDir = ensureDir( + isPackaged + ? getOpenclawSkillsDir(userDataPath) + : path.resolve( + runtimeConfig.paths.nexuHome, + "runtime/openclaw/state/skills", + ), + ); @@ - OPENCLAW_SKILLS_DIR: isPackaged - ? getOpenclawSkillsDir(userDataPath) - : ensureDir( - path.resolve( - runtimeConfig.paths.nexuHome, - "runtime/openclaw/state/skills", - ), - ), + OPENCLAW_SKILLS_DIR: openclawSkillsDir,Also applies to: 332-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/runtime/manifests.ts` around lines 237 - 244, Duplicate logic deciding the skills directory path is used in multiple places; extract it into a single local constant and reuse it where ensureDir(...) is called. Introduce a const (e.g. skillsDir) computed once using the existing condition with isPackaged ? getOpenclawSkillsDir(userDataPath) : path.resolve(runtimeConfig.paths.nexuHome, "runtime/openclaw/state/skills"), then replace both ensureDir(...) calls (and any other occurrences around the ensureDir calls) with ensureDir(skillsDir) so the packaged/dev resolution is centralized and cannot drift.apps/controller/tests/skillhub-service.test.ts (1)
467-467: Optional: avoid implying a specific ledger file format in this test.Since this test only needs the file to exist, writing an empty file is clearer than writing JSON into a
.dbpath.♻️ Suggested cleanup
- writeFileSync(env.skillDbPath, JSON.stringify({ skills: [] })); + writeFileSync(env.skillDbPath, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/tests/skillhub-service.test.ts` at line 467, The test currently writes JSON to env.skillDbPath using writeFileSync(JSON.stringify(...)) which implies a specific ledger file format; change the test to simply create an empty file at env.skillDbPath (e.g., use writeFileSync(env.skillDbPath, '') or equivalent) so the test only ensures the file exists without asserting a JSON/database format; update the call site where writeFileSync is used in the test to write an empty string instead of JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/controller/tests/skillhub-service.test.ts`:
- Line 467: The test currently writes JSON to env.skillDbPath using
writeFileSync(JSON.stringify(...)) which implies a specific ledger file format;
change the test to simply create an empty file at env.skillDbPath (e.g., use
writeFileSync(env.skillDbPath, '') or equivalent) so the test only ensures the
file exists without asserting a JSON/database format; update the call site where
writeFileSync is used in the test to write an empty string instead of JSON.
In `@apps/desktop/main/runtime/manifests.ts`:
- Around line 237-244: Duplicate logic deciding the skills directory path is
used in multiple places; extract it into a single local constant and reuse it
where ensureDir(...) is called. Introduce a const (e.g. skillsDir) computed once
using the existing condition with isPackaged ?
getOpenclawSkillsDir(userDataPath) : path.resolve(runtimeConfig.paths.nexuHome,
"runtime/openclaw/state/skills"), then replace both ensureDir(...) calls (and
any other occurrences around the ensureDir calls) with ensureDir(skillsDir) so
the packaged/dev resolution is centralized and cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1de28cdd-b261-4d7f-a7df-80ad7b0cece1
📒 Files selected for processing (4)
apps/controller/src/services/skillhub-service.tsapps/controller/src/services/skillhub/curated-skills.tsapps/controller/tests/skillhub-service.test.tsapps/desktop/main/runtime/manifests.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64452b212c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
Author
|
/cr |
|
✅ CR topic created in Feishu topic group Refly CR. |
nettee
approved these changes
Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #612
What
Curated skills that failed ClawHub download during first launch were never retried on subsequent boots.
Why
initialize()(which enqueues curated skills) only ran whenisFirstLaunchwas true — i.e., when the skill ledger file didn't exist. Failed installs wrote no ledger record, so they'd be eligible for retry, but the code never re-checked after the first launch.How
isFirstLaunchgate —initialize()now runs on every non-CI startupcopyStaticSkillsso user-uninstalled static skills are not re-copiedcopyStaticSkillsskips when SKILL.md exists on disk OR slug is known in ledger;getCuratedSlugsToEnqueuefilters against all known slugsOPENCLAW_SKILLS_DIRto useNEXU_HOME-scoped path for consistent dev/packaged behaviorAffected areas
apps/controller/src/services/skillhub-service.tsapps/controller/src/services/skillhub/curated-skills.tsapps/desktop/main/runtime/manifests.tsChecklist
pnpm typecheckpassespnpm testpasses (199 tests, including new retry test)pnpm lintpassesSummary by CodeRabbit