Open
Conversation
Closes #114. The CLI subcommand and module were named `skilify` but the natural derivation from `skill` is `skillify`. Hard switch — no `skilify` alias. - Rename src/skilify/ -> src/skillify/ (incl. skilify-worker.ts and spawn-skilify-worker.ts inside) - Rename src/commands/skilify.ts -> skillify.ts (export runSkillifyCommand) - Rename 16 claude-code/tests/skilify-*.test.ts files - Tree-wide case-aware text replace across src/, openclaw/, pi/extension-source/, .github/workflows/, README, RELEASE_CHECKLIST, esbuild.config.mjs, vitest.config.ts, hook tests - Remove stale <agent>/bundle/skilify-worker.js artifacts; rebuild bundles to <agent>/bundle/skillify-worker.js Add src/skillify/legacy-migration.ts: one-shot renameSync of ~/.deeplake/state/skilify/ -> ~/.deeplake/state/skillify/ on first boot of the renamed version. Wired into all four mkdirSync entry points (state.ts, scope-config.ts, manifest.ts, auto-pull.ts). EXDEV/EPERM falls back to leaving the legacy dir in place. OpenClaw can't import the shared skillify TS modules (self-contained bundle), so the migration is inlined into openclaw/src/index.ts and called before the existing fsMkdir on the state dir — once the new dir exists the migration becomes a no-op and any legacy data would be orphaned. Add claude-code/tests/skillify-legacy-migration.test.ts: 5 tests covering legacy-missing, current-already-exists (no clobber), happy-path rename, idempotent re-call (module-level attempted flag), and renameSync EXDEV failure swallowed. CLI dispatcher in src/cli/index.ts already only matches "skillify"; "skilify" falls through to "Unknown command: skilify" — no alias logic to remove.
Resolves conflicts from main's post-#112 follow-ups: - 5dead1f autopull: throttle removed (run every SessionStart, idempotent) - 9b54af0 maybeAutoPull -> autoPullSkills - 26cb2e2 skilify-auto-pull test covers the DeeplakeApi-backed query path - 7c57d6d / 86d140a docs: skilify -> skillify - README condensed; details moved to docs/SKILLIFY.md Resolutions: - src/skillify/auto-pull.ts: take main's no-throttle body, apply skillify path rename. readIntervalMs/readLastRun/writeLastRun and the autopull-last-run.json timestamp file are gone. The legacy-state migration import is no longer needed in this file (no state-dir touchpoints remaining). - src/hooks/{,codex/,cursor/,hermes/}session-start.ts: take main's autoPullSkills callsite + comment, keep src/skillify import path. - src/cli/index.ts USAGE: take main's no-throttle docstring, keep skillify subcommand name. - claude-code/tests/skillify-auto-pull.test.ts: take main's deletion of the throttle test blocks + DeeplakeApi mock setup, apply skillify rename to remaining import path. - claude-code/tests/codex-session-start-hook.test.ts: take main's autoPullSkillsMock binding, keep src/skillify import path. - README.md: take main's condensed Skillify section pointing at docs/SKILLIFY.md. - bundle/* (esbuild outputs): take ours then `npm run build` to regenerate from the resolved sources. Bundles regenerated. Test suite green (2184/2184).
📝 WalkthroughWalkthroughProject-wide rename from “skilify” to “skillify,” adding a one-time legacy state-directory migration and updating CLI, hooks, bundles, build entries, docs, and tests accordingly. ChangesSkillify rename and state migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Claude finished @efenocchi's task in 4m 47s —— View job Claude PR Review
No blocking issues found. The rename is consistent end-to-end and the migration is the right shape:
Two minor non-bug observations (no action needed):
|
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 31 files changed
Generated for commit 9921919. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
openclaw/src/index.ts (1)
346-355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease the worker lock on pre-spawn failures.
After Line 410 acquires
${projectKey}.worker.lock, themkdir, config-write, and spawn-error paths just return. That leaves a stale lock behind, and every lateragent_endfor the same channel gets skipped as if a worker were still running. A single transient filesystem/spawn error can permanently disable OpenClaw skill mining until the user deletes the lock manually.Suggested fix
import { existsSync as fsExists, mkdirSync as fsMkdir, openSync as fsOpen, closeSync as fsClose, writeFileSync as fsWriteFile, constants as fsConstants, - readFileSync as fsReadFile, renameSync as fsRename, + readFileSync as fsReadFile, renameSync as fsRename, rmSync as fsRm, } from "node:fs"; ... -function tryAcquireOpenclawSkillifyLock(projectKey: string): boolean { +function tryAcquireOpenclawSkillifyLock(projectKey: string): string | null { try { migrateOpenclawSkillifyLegacyStateDir(); fsMkdir(OPENCLAW_SKILLIFY_STATE_DIR, { recursive: true }); const lockPath = joinPath(OPENCLAW_SKILLIFY_STATE_DIR, `${projectKey}.worker.lock`); const fd = fsOpen(lockPath, fsConstants.O_CREAT | fsConstants.O_EXCL | fsConstants.O_WRONLY); fsClose(fd); - return true; - } catch { return false; } + return lockPath; + } catch { return null; } } ... - if (!tryAcquireOpenclawSkillifyLock(projectKey)) { + const lockPath = tryAcquireOpenclawSkillifyLock(projectKey); + if (!lockPath) { return; } + const releaseLock = () => { + try { fsRm(lockPath, { force: true }); } catch {} + }; const tmpDir = joinPath(tmpdir(), `deeplake-skillify-openclaw-${projectKey}-${Date.now()}`); try { fsMkdir(tmpDir, { recursive: true, mode: 0o700 }); } - catch (e: any) { a.loggerWarn?.(`skillify spawn: mkdir failed: ${e?.message ?? e}`); return; } + catch (e: any) { releaseLock(); a.loggerWarn?.(`skillify spawn: mkdir failed: ${e?.message ?? e}`); return; } ... try { fsWriteFile(configPath, JSON.stringify(config), { mode: 0o600 }); } - catch (e: any) { a.loggerWarn?.(`skillify spawn: config write failed: ${e?.message ?? e}`); return; } + catch (e: any) { releaseLock(); a.loggerWarn?.(`skillify spawn: config write failed: ${e?.message ?? e}`); return; } ... } catch (e: any) { + releaseLock(); a.loggerWarn?.(`skillify spawn: spawn failed: ${e?.message ?? e}`); }Also applies to: 399-460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openclaw/src/index.ts` around lines 346 - 355, The lock file created by tryAcquireOpenclawSkillifyLock can be left behind when subsequent steps (mkdir, config write, or spawn) fail; update the flow so any path that returns after acquiring the lock ensures the lock is released and closed (remove the `${projectKey}.worker.lock` file and close/unlink any fd) — modify tryAcquireOpenclawSkillifyLock to return both a success flag and a handle or path (or throw on partial failure) and update the caller code (the OpenClaw worker spawn/config write paths that run after migrateOpenclawSkillifyLegacyStateDir and fsMkdir) to call a new release function that closes and unlinks the lock on any early return or on spawn errors; use the existing symbols migrateOpenclawSkillifyLegacyStateDir, OPENCLAW_SKILLIFY_STATE_DIR, fsOpen/fsClose and add a release function like releaseOpenclawSkillifyLock(lockPath|fd) and invoke it in all error/early-return branches so no stale lock remains.codex/bundle/skillify-worker.js (1)
452-461:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the legacy-state migration before the first read.
Line 777 calls
readState()before any write/lock path runs. On the first post-rename worker run, an existing~/.deeplake/state/skilify/<project>.jsonis still invisible here, solastDateandskillsGeneratedlook empty and the worker can re-mine old sessions or recreate skills. MovemigrateLegacyStateDir()intoreadState()(or call it before the initial read inmain()).💡 Minimal fix
function readState(projectKey) { + migrateLegacyStateDir(); const p = statePath(projectKey); if (!existsSync4(p)) return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codex/bundle/skillify-worker.js` around lines 452 - 461, readState currently reads state files before legacy-state migration runs causing missing legacy state on first run; call migrateLegacyStateDir() before attempting to read to ensure old ~/.deeplake/state/skilify/<project>.json is moved/visible. Update the readState(projectKey) function to first invoke migrateLegacyStateDir() (or ensure main() calls migrateLegacyStateDir() before the first readState call), then proceed to compute p via statePath(projectKey) and use existsSync4/readFileSync2/JSON.parse as before; this guarantees lastDate/skillsGenerated are populated from migrated files before any read occurs.claude-code/tests/skillify-cli.test.ts (1)
38-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis suite now mutates the real
~/.deeplake/state/skillifyconfig.Line 38 resolves the live production state path up front, and
beforeEach()deletes that file. After the rename, an interrupted local test run can clobber a developer’s actual Skillify config. Please run the whole file under a tempHOMEand derive the config path from that temp home instead ofhomedir()at import time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude-code/tests/skillify-cli.test.ts` around lines 38 - 69, The test currently computes STATE_DIR and CONFIG_PATH at module import using homedir(), which mutates the real ~/.deeplake/state/skillify; change the setup to compute those paths at runtime inside beforeEach (or derive them from process.env.HOME) so tests run under a temporary HOME. Specifically, remove or stop using the top-level constants STATE_DIR and CONFIG_PATH computed with homedir(), and instead compute STATE_DIR and CONFIG_PATH inside beforeEach (or read process.env.HOME) before any rmSync/readFileSync/writeFileSync calls; update afterEach to use the same runtime-computed variables so temp HOME is consistently used. Ensure references in beforeEach/afterEach and any helper mocks (e.g., loadConfigMock) use the new runtime-computed STATE_DIR/CONFIG_PATH names.hermes/bundle/capture.js (1)
1148-1189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetached spawn failures will leak the worker lock until timeout.
spawn()does not throw synchronously on launch failures (e.g.,nohup,node, or the worker file missing); instead, the error emits asynchronously on the child'serrorevent. This code calls.unref()immediately without attaching an error handler, so the caller'stry/catchblock never observes the failure and never releases the lock. The lock remains stale for up to 10 minutes, suppressing future skillify runs.Attach an error handler before calling
.unref(), or return the ChildProcess and handle the error in the caller alongside the lock release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hermes/bundle/capture.js` around lines 1148 - 1189, In spawnSkillifyWorker: the detached child created by spawn3("nohup", ["node", workerPath, configFile]) is unref()d immediately without any 'error' handler so asynchronous spawn failures can leak the worker lock; fix by capturing the returned ChildProcess (from spawn3), attach a child.on('error', ...) handler that logs the error and triggers lock release/cleanup for the given projectKey/currentSessionId, and only then call .unref() (or alternatively return the ChildProcess to the caller so the caller can attach the error handler and release the lock). Ensure you reference workerPath and configFile when logging, and use the same skillifyLog/error path as existing code to record failures.src/commands/skillify.ts (1)
50-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStatus still misreports migrated state files.
After this rename,
~/.deeplake/state/skillifycan containautopull-last-run.json, but this filter only excludesconfig.jsonandpulled.json, sostate: N project(s)gets inflated. The same block also still printslastDate, while the state writer storesupdatedAt, so active projects showlast=never.Suggested fix
const files = readdirSync(dir).filter( - f => f.endsWith(".json") && f !== "config.json" && f !== "pulled.json", + f => + f.endsWith(".json") && + f !== "config.json" && + f !== "pulled.json" && + f !== "autopull-last-run.json", ); @@ const s = JSON.parse(readFileSync(join(dir, f), "utf-8")) as { - project: string; counter: number; lastDate: string | null; skillsGenerated: string[]; + project: string; + counter: number; + lastDate?: string | null; + updatedAt?: number | string | null; + skillsGenerated?: string[]; }; - const skills = s.skillsGenerated.length === 0 ? "none" : s.skillsGenerated.join(", "); - console.log(` - ${s.project} (counter=${s.counter}, last=${s.lastDate ?? "never"}, skills=${skills})`); + const last = + typeof s.updatedAt === "number" + ? new Date(s.updatedAt).toISOString() + : typeof s.updatedAt === "string" + ? s.updatedAt + : s.lastDate ?? "never"; + const skills = Array.isArray(s.skillsGenerated) && s.skillsGenerated.length > 0 + ? s.skillsGenerated.join(", ") + : "none"; + console.log(` - ${s.project} (counter=${s.counter}, last=${last}, skills=${skills})`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/skillify.ts` around lines 50 - 69, The file-filtering and state-parsing need to be updated: extend the readdirSync filter (the files variable) to also exclude "autopull-last-run.json" so it doesn't inflate the tracked-project count, and update the parsed state shape used in the for loop (the s object) to read updatedAt instead of lastDate and print that (falling back to "never"); also defensively handle missing/empty skillsGenerated when computing skills. Update references around files, the readdirSync filter, the s parsing/type, skillsGenerated, and the console.log that prints last/skills.codex/bundle/stop.js (2)
965-985:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize state before the first
SessionEndworker run.This path only calls
resetCounter()whenreadState(projectKey)already exists, but it still spawns the worker when the state file is missing. In the worker bundles in this PR,recordSkill()andadvanceWatermark()both return early ifreadState(projectKey)isnull, so the first codex run can mine skills without ever persistinglastDate/lastUuid. That makes the same sessions eligible again on the next stop.Suggested fix
- if (readState(projectKey)) { - resetCounter(projectKey); - } + const state = readState(projectKey); + if (state) { + resetCounter(projectKey); + } else { + writeState(projectKey, { + project, + projectKey, + counter: 0, + lastUuid: null, + lastDate: null, + skillsGenerated: [], + updatedAt: Date.now() + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codex/bundle/stop.js` around lines 965 - 985, The code spawns a SessionEnd worker even when no state exists, causing the worker's recordSkill/advanceWatermark to skip persisting lastDate/lastUuid; ensure state is initialized before spawning by checking readState(projectKey) and, if null, creating/initializing the persisted state (or calling resetCounter(projectKey) unconditionally) so that spawnSkillifyWorker(...) always runs with a valid persisted state; update the block around deriveProjectKey/tryAcquireWorkerLock/readState to initialize the project's state prior to calling spawnSkillifyWorker to guarantee subsequent recordSkill/advanceWatermark will persist progress.
731-772:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle async
spawn()errors insidespawnSkillifyWorker.The surrounding
try/catchonly sees synchronous failures. Ifnohup,node, or the worker path cannot be executed,spawn2()reports that via the child'error'event after this helper returns. In that case the code logs a successful spawn and leaves the worker lock held until stale-lock expiry.Suggested fix
- skillifyLog(`${reason}: spawning skillify worker for project=${project} key=${projectKey}`); - const workerPath = join7(bundleDir, "skillify-worker.js"); - spawn2("nohup", ["node", workerPath, configFile], { - detached: true, - stdio: ["ignore", "ignore", "ignore"] - }).unref(); - skillifyLog(`${reason}: spawned skillify worker for ${projectKey}`); + skillifyLog(`${reason}: spawning skillify worker for project=${project} key=${projectKey}`); + const workerPath = join7(bundleDir, "skillify-worker.js"); + const child = spawn2("nohup", ["node", workerPath, configFile], { + detached: true, + stdio: ["ignore", "ignore", "ignore"] + }); + child.once("error", (err) => { + skillifyLog(`${reason} spawn failed: ${err.message}`); + releaseWorkerLock(projectKey); + }); + child.once("spawn", () => { + skillifyLog(`${reason}: spawned skillify worker for ${projectKey}`); + child.unref(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codex/bundle/stop.js` around lines 731 - 772, spawnSkillifyWorker currently ignores asynchronous child-process startup failures; capture the child returned from spawn2 (const child = spawn2(...)), call child.unref(), and attach child.on('error', err => { skillifyLog(`${reason}: failed to spawn skillify worker for ${projectKey}: ${err.message || err}`); /* perform cleanup: release the worker/session lock for currentSessionId/projectKey here */ }); to ensure errors are logged and the worker lock is released — either invoke an existing unlock function (e.g., releaseSessionLock or clearWorkerLock with currentSessionId/projectKey) or accept and call an opts.onSpawnError callback if no unlock helper exists.
🧹 Nitpick comments (1)
vitest.config.ts (1)
254-272: ⚡ Quick winAdd
src/skillify/legacy-migration.tsto the per-file coverage gate.This PR adds a new load-bearing file, but it is not listed in
coverage.thresholds. Since this config is the repo’s mechanism for enforcing touched-file coverage, the migration path can now regress without tripping CI.♻️ Suggested change
"src/skillify/auto-pull.ts": { statements: 90, branches: 70, functions: 90, lines: 90 }, "src/skillify/extractors/index.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, "src/skillify/gate-parser.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, "src/skillify/gate-runner.ts": { statements: 90, branches: 60, functions: 90, lines: 90 }, + "src/skillify/legacy-migration.ts": { statements: 80, branches: 80, functions: 80, lines: 80 }, "src/skillify/pull.ts": { statements: 90, branches: 75, functions: 90, lines: 90 }, "src/skillify/scope-config.ts": { statements: 90, branches: 90, functions: 90, lines: 90 }, "src/skillify/skill-writer.ts": { statements: 90, branches: 80, functions: 90, lines: 90 },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vitest.config.ts` around lines 254 - 272, Add the new file "src/skillify/legacy-migration.ts" to the coverage.thresholds map in vitest.config.ts so CI enforces per-file coverage for this load-bearing migration; add an entry similar to the other skillify files, e.g. "src/skillify/legacy-migration.ts": { statements: 90, branches: 70, functions: 90, lines: 90 }, ensuring the thresholds object includes that key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bundle/cli.js`:
- Around line 4737-4753: The catch in migrateLegacyStateDir currently swallows
all renameSync errors; change it to only suppress the documented fallback cases
(cross-device and permission errors) by checking err.code and handling only
'EXDEV' (cross-device) and the permission codes ('EACCES' and 'EPERM'), logging
the fallback for those; for any other err.code rethrow the error so callers see
unexpected failures (keep references to attempted, legacy, current, renameSync,
and dlog in your edit).
In `@claude-code/bundle/session-start.js`:
- Around line 777-793: The migration currently swallows all errors from
renameSync in migrateLegacyStateDir (symbols: migrateLegacyStateDir, renameSync,
dlog, attempted, legacy, current, existsSync5), which hides real I/O failures;
change the catch to inspect err.code and only treat the documented fallback
codes (e.g., "EXDEV" and "EPERM") as benign (log and leave legacy dir in place),
but for any other error codes (e.g., "EIO", "ENOSPC", etc.) rethrow or log as an
actual error (so the failure is visible) instead of silently continuing.
In `@claude-code/tests/skillify-legacy-migration.test.ts`:
- Around line 28-41: The tests currently only set process.env.HOME (sandboxHome)
in beforeEach but do not stub node:os.homedir(), so on Windows the code under
test can still resolve the real user homedir; update beforeEach to also mock/spy
on os.homedir() to return sandboxHome (store the original os.homedir reference),
and update afterEach to restore the original os.homedir; keep the existing
prevHome logic and cleanup (chmodSync/rmSync) but ensure the os.homedir mock is
removed in afterEach so subsequent tests are unaffected.
In `@RELEASE_CHECKLIST.md`:
- Around line 84-95: Update the contradictory statement that OpenClaw “has none”
for mining-style features so it matches the rest of the doc: change the line
that claims mining doesn’t apply to OpenClaw to state that OpenClaw does capture
sessions and can be mined (referencing the agent_end hook and the
spawnOpenclawSkillifyWorker wiring in openclaw/src/index.ts and the produced
bundle openclaw/dist/skillify-worker.js); ensure the text mentions the agent_end
capture (see openclaw/src/index.ts:agent_end/line ~903) and that the mining
worker is spawned from spawnOpenclawSkillifyWorker and uses
openclaw/dist/skillify-worker.js so the checklist is consistent.
In `@src/cli/index.ts`:
- Around line 231-233: The skillify command branch currently calls
runSkillifyCommand(args.slice(1)) without awaiting it, which can cause unhandled
rejections; change the branch to await the dispatcher (await
runSkillifyCommand(args.slice(1))) so any rejection flows into the surrounding
main().catch(...) path and ensure the enclosing function is async (or already
is) so awaiting is valid; update the branch that checks if (cmd === "skillify")
and replace the non-awaited call with an awaited call to runSkillifyCommand.
---
Outside diff comments:
In `@claude-code/tests/skillify-cli.test.ts`:
- Around line 38-69: The test currently computes STATE_DIR and CONFIG_PATH at
module import using homedir(), which mutates the real
~/.deeplake/state/skillify; change the setup to compute those paths at runtime
inside beforeEach (or derive them from process.env.HOME) so tests run under a
temporary HOME. Specifically, remove or stop using the top-level constants
STATE_DIR and CONFIG_PATH computed with homedir(), and instead compute STATE_DIR
and CONFIG_PATH inside beforeEach (or read process.env.HOME) before any
rmSync/readFileSync/writeFileSync calls; update afterEach to use the same
runtime-computed variables so temp HOME is consistently used. Ensure references
in beforeEach/afterEach and any helper mocks (e.g., loadConfigMock) use the new
runtime-computed STATE_DIR/CONFIG_PATH names.
In `@codex/bundle/skillify-worker.js`:
- Around line 452-461: readState currently reads state files before legacy-state
migration runs causing missing legacy state on first run; call
migrateLegacyStateDir() before attempting to read to ensure old
~/.deeplake/state/skilify/<project>.json is moved/visible. Update the
readState(projectKey) function to first invoke migrateLegacyStateDir() (or
ensure main() calls migrateLegacyStateDir() before the first readState call),
then proceed to compute p via statePath(projectKey) and use
existsSync4/readFileSync2/JSON.parse as before; this guarantees
lastDate/skillsGenerated are populated from migrated files before any read
occurs.
In `@codex/bundle/stop.js`:
- Around line 965-985: The code spawns a SessionEnd worker even when no state
exists, causing the worker's recordSkill/advanceWatermark to skip persisting
lastDate/lastUuid; ensure state is initialized before spawning by checking
readState(projectKey) and, if null, creating/initializing the persisted state
(or calling resetCounter(projectKey) unconditionally) so that
spawnSkillifyWorker(...) always runs with a valid persisted state; update the
block around deriveProjectKey/tryAcquireWorkerLock/readState to initialize the
project's state prior to calling spawnSkillifyWorker to guarantee subsequent
recordSkill/advanceWatermark will persist progress.
- Around line 731-772: spawnSkillifyWorker currently ignores asynchronous
child-process startup failures; capture the child returned from spawn2 (const
child = spawn2(...)), call child.unref(), and attach child.on('error', err => {
skillifyLog(`${reason}: failed to spawn skillify worker for ${projectKey}:
${err.message || err}`); /* perform cleanup: release the worker/session lock for
currentSessionId/projectKey here */ }); to ensure errors are logged and the
worker lock is released — either invoke an existing unlock function (e.g.,
releaseSessionLock or clearWorkerLock with currentSessionId/projectKey) or
accept and call an opts.onSpawnError callback if no unlock helper exists.
In `@hermes/bundle/capture.js`:
- Around line 1148-1189: In spawnSkillifyWorker: the detached child created by
spawn3("nohup", ["node", workerPath, configFile]) is unref()d immediately
without any 'error' handler so asynchronous spawn failures can leak the worker
lock; fix by capturing the returned ChildProcess (from spawn3), attach a
child.on('error', ...) handler that logs the error and triggers lock
release/cleanup for the given projectKey/currentSessionId, and only then call
.unref() (or alternatively return the ChildProcess to the caller so the caller
can attach the error handler and release the lock). Ensure you reference
workerPath and configFile when logging, and use the same skillifyLog/error path
as existing code to record failures.
In `@openclaw/src/index.ts`:
- Around line 346-355: The lock file created by tryAcquireOpenclawSkillifyLock
can be left behind when subsequent steps (mkdir, config write, or spawn) fail;
update the flow so any path that returns after acquiring the lock ensures the
lock is released and closed (remove the `${projectKey}.worker.lock` file and
close/unlink any fd) — modify tryAcquireOpenclawSkillifyLock to return both a
success flag and a handle or path (or throw on partial failure) and update the
caller code (the OpenClaw worker spawn/config write paths that run after
migrateOpenclawSkillifyLegacyStateDir and fsMkdir) to call a new release
function that closes and unlinks the lock on any early return or on spawn
errors; use the existing symbols migrateOpenclawSkillifyLegacyStateDir,
OPENCLAW_SKILLIFY_STATE_DIR, fsOpen/fsClose and add a release function like
releaseOpenclawSkillifyLock(lockPath|fd) and invoke it in all error/early-return
branches so no stale lock remains.
In `@src/commands/skillify.ts`:
- Around line 50-69: The file-filtering and state-parsing need to be updated:
extend the readdirSync filter (the files variable) to also exclude
"autopull-last-run.json" so it doesn't inflate the tracked-project count, and
update the parsed state shape used in the for loop (the s object) to read
updatedAt instead of lastDate and print that (falling back to "never"); also
defensively handle missing/empty skillsGenerated when computing skills. Update
references around files, the readdirSync filter, the s parsing/type,
skillsGenerated, and the console.log that prints last/skills.
---
Nitpick comments:
In `@vitest.config.ts`:
- Around line 254-272: Add the new file "src/skillify/legacy-migration.ts" to
the coverage.thresholds map in vitest.config.ts so CI enforces per-file coverage
for this load-bearing migration; add an entry similar to the other skillify
files, e.g. "src/skillify/legacy-migration.ts": { statements: 90, branches: 70,
functions: 90, lines: 90 }, ensuring the thresholds object includes that key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4da8c25-9cac-4de5-9758-997cb1b0f25b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (80)
.github/workflows/ci.yml.github/workflows/release.ymlRELEASE_CHECKLIST.mdbundle/cli.jsclaude-code/bundle/capture.jsclaude-code/bundle/session-end.jsclaude-code/bundle/session-start.jsclaude-code/bundle/skillify-worker.jsclaude-code/tests/codex-session-start-hook.test.tsclaude-code/tests/cursor-session-start-hook.test.tsclaude-code/tests/hermes-session-start-hook.test.tsclaude-code/tests/session-start-hook.test.tsclaude-code/tests/sessions-table.test.tsclaude-code/tests/skillify-agent-roots.test.tsclaude-code/tests/skillify-auto-pull.test.tsclaude-code/tests/skillify-bundle-scan.test.tsclaude-code/tests/skillify-cli.test.tsclaude-code/tests/skillify-extractor.test.tsclaude-code/tests/skillify-gate-parser.test.tsclaude-code/tests/skillify-gate-runner.test.tsclaude-code/tests/skillify-legacy-migration.test.tsclaude-code/tests/skillify-manifest.test.tsclaude-code/tests/skillify-pull.test.tsclaude-code/tests/skillify-scope-config.test.tsclaude-code/tests/skillify-session-start-injection.test.tsclaude-code/tests/skillify-skill-writer.test.tsclaude-code/tests/skillify-skills-table.test.tsclaude-code/tests/skillify-state.test.tsclaude-code/tests/skillify-triggers.test.tsclaude-code/tests/skillify-unpull.test.tscodex/bundle/session-start.jscodex/bundle/skillify-worker.jscodex/bundle/stop.jscursor/bundle/capture.jscursor/bundle/session-end.jscursor/bundle/session-start.jscursor/bundle/skillify-worker.jsesbuild.config.mjshermes/bundle/capture.jshermes/bundle/session-end.jshermes/bundle/session-start.jshermes/bundle/skillify-worker.jsopenclaw/skills/SKILL.mdopenclaw/src/index.tspi/bundle/autopull-worker.jspi/bundle/skilify-worker.jspi/bundle/skillify-worker.jspi/extension-source/hivemind.tssrc/cli/index.tssrc/cli/install-pi.tssrc/commands/skillify.tssrc/hooks/capture.tssrc/hooks/codex/session-start.tssrc/hooks/codex/stop.tssrc/hooks/cursor/capture.tssrc/hooks/cursor/session-end.tssrc/hooks/cursor/session-start.tssrc/hooks/hermes/capture.tssrc/hooks/hermes/session-end.tssrc/hooks/hermes/session-start.tssrc/hooks/session-end.tssrc/hooks/session-start.tssrc/skillify/agent-roots.tssrc/skillify/auto-pull.tssrc/skillify/autopull-worker.tssrc/skillify/extractors/index.tssrc/skillify/gate-parser.tssrc/skillify/gate-runner.tssrc/skillify/legacy-migration.tssrc/skillify/manifest.tssrc/skillify/pull.tssrc/skillify/scope-config.tssrc/skillify/skill-writer.tssrc/skillify/skillify-worker.tssrc/skillify/skills-table.tssrc/skillify/spawn-skillify-worker.tssrc/skillify/state.tssrc/skillify/triggers.tssrc/skillify/unpull.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- pi/bundle/skilify-worker.js
Comment on lines
+4737
to
+4753
| function migrateLegacyStateDir() { | ||
| if (attempted) | ||
| return; | ||
| attempted = true; | ||
| const root = join15(homedir5(), ".deeplake", "state"); | ||
| const legacy = join15(root, "skilify"); | ||
| const current = join15(root, "skillify"); | ||
| if (!existsSync12(legacy)) | ||
| return; | ||
| if (existsSync12(current)) | ||
| return; | ||
| try { | ||
| renameSync(legacy, current); | ||
| dlog(`migrated ${legacy} -> ${current}`); | ||
| } catch (err) { | ||
| dlog(`migration failed (${err.code ?? "unknown"}); leaving legacy dir in place`); | ||
| } |
There was a problem hiding this comment.
Restrict the migration fallback to the documented fs errors.
This currently suppresses every renameSync failure and silently proceeds with a fresh skillify state dir. Unexpected failures here will look like state loss and be very hard to diagnose. Please only downgrade the cross-device / permission cases called out in the migration plan, and rethrow everything else from the source helper before regenerating this bundle.
Suggested fix
function migrateLegacyStateDir() {
if (attempted)
return;
attempted = true;
const root = join15(homedir5(), ".deeplake", "state");
const legacy = join15(root, "skilify");
const current = join15(root, "skillify");
if (!existsSync12(legacy))
return;
if (existsSync12(current))
return;
try {
renameSync(legacy, current);
dlog(`migrated ${legacy} -> ${current}`);
} catch (err) {
- dlog(`migration failed (${err.code ?? "unknown"}); leaving legacy dir in place`);
+ const code = err?.code;
+ if (code === "EXDEV" || code === "EPERM" || code === "EACCES") {
+ dlog(`migration failed (${code}); leaving legacy dir in place`);
+ return;
+ }
+ throw err;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundle/cli.js` around lines 4737 - 4753, The catch in migrateLegacyStateDir
currently swallows all renameSync errors; change it to only suppress the
documented fallback cases (cross-device and permission errors) by checking
err.code and handling only 'EXDEV' (cross-device) and the permission codes
('EACCES' and 'EPERM'), logging the fallback for those; for any other err.code
rethrow the error so callers see unexpected failures (keep references to
attempted, legacy, current, renameSync, and dlog in your edit).
Comment on lines
+777
to
+793
| function migrateLegacyStateDir() { | ||
| if (attempted) | ||
| return; | ||
| attempted = true; | ||
| const root = join9(homedir5(), ".deeplake", "state"); | ||
| const legacy = join9(root, "skilify"); | ||
| const current = join9(root, "skillify"); | ||
| if (!existsSync5(legacy)) | ||
| return; | ||
| if (existsSync5(current)) | ||
| return; | ||
| try { | ||
| renameSync(legacy, current); | ||
| dlog(`migrated ${legacy} -> ${current}`); | ||
| } catch (err) { | ||
| dlog(`migration failed (${err.code ?? "unknown"}); leaving legacy dir in place`); | ||
| } |
There was a problem hiding this comment.
Only swallow the documented fallback errors.
This helper currently treats every renameSync() failure as benign. That matches EXDEV/EPERM, but it also hides unexpected I/O failures like EIO or ENOSPC, which leaves users on a fresh skillify state dir while their legacy manifest/state is silently ignored.
Suggested fix
try {
renameSync(legacy, current);
dlog(`migrated ${legacy} -> ${current}`);
} catch (err) {
- dlog(`migration failed (${err.code ?? "unknown"}); leaving legacy dir in place`);
+ const code = err && typeof err === "object" && "code" in err ? err.code : "unknown";
+ if (code === "EXDEV" || code === "EPERM") {
+ dlog(`migration failed (${code}); leaving legacy dir in place`);
+ return;
+ }
+ throw err;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude-code/bundle/session-start.js` around lines 777 - 793, The migration
currently swallows all errors from renameSync in migrateLegacyStateDir (symbols:
migrateLegacyStateDir, renameSync, dlog, attempted, legacy, current,
existsSync5), which hides real I/O failures; change the catch to inspect
err.code and only treat the documented fallback codes (e.g., "EXDEV" and
"EPERM") as benign (log and leave legacy dir in place), but for any other error
codes (e.g., "EIO", "ENOSPC", etc.) rethrow or log as an actual error (so the
failure is visible) instead of silently continuing.
Comment on lines
+28
to
+41
| beforeEach(() => { | ||
| sandboxHome = mkdtempSync(join(tmpdir(), "skillify-migration-")); | ||
| prevHome = process.env.HOME; | ||
| process.env.HOME = sandboxHome; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (prevHome === undefined) delete process.env.HOME; | ||
| else process.env.HOME = prevHome; | ||
| // chmodSync the sandbox readable in case a test removed perms; otherwise | ||
| // rmSync hits EACCES on cleanup and leaks the temp dir across runs. | ||
| try { chmodSync(sandboxHome, 0o755); } catch { /* nothing */ } | ||
| rmSync(sandboxHome, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
find . -name "skillify-legacy-migration.test.ts" -type fRepository: activeloopai/hivemind
Length of output: 119
🏁 Script executed:
# Also find the source file being tested
find . -name "legacy-migration.ts" -path "*/skillify/*" -type fRepository: activeloopai/hivemind
Length of output: 100
🏁 Script executed:
# Search for homedir usage in the codebase
rg "homedir" --type ts --type jsRepository: activeloopai/hivemind
Length of output: 50377
🏁 Script executed:
# Read the test file to see the current setup
cat -n ./claude-code/tests/skillify-legacy-migration.test.ts | head -50Repository: activeloopai/hivemind
Length of output: 2296
🏁 Script executed:
# Also check the source file being tested
cat -n ./src/skillify/legacy-migration.tsRepository: activeloopai/hivemind
Length of output: 1621
🏁 Script executed:
# Let me verify Node.js os.homedir() behavior on Windows
web_searchRepository: activeloopai/hivemind
Length of output: 116
🌐 Web query:
Node.js os.homedir() implementation Windows USERPROFILE environment variables
💡 Result:
On Windows, Node.js os.homedir() returns the current user’s home directory by delegating to libuv’s uv_os_homedir(), which (1) checks the USERPROFILE environment variable first and (2) if it’s not set, falls back to querying the Windows profile directory for the current user (via the Windows API used by libuv). [1][2][3] More detail: 1) Node.js implementation - Node’s os.homedir() implementation is a thin wrapper: it calls uv_os_homedir(buf, &len) to fill a buffer and returns the resulting path string. [2] 2) libuv Windows implementation and USERPROFILE - In libuv’s Windows code for uv_os_homedir, it first attempts to read the USERPROFILE environment variable (uv_os_getenv("USERPROFILE", ...)). [4] - If USERPROFILE is not found, it falls back to obtaining the profile/home directory for the current user via uv_os_get_passwd(&pwd), and then copies pwd.homedir into the output buffer. [4] 3) Node.js public documentation summary - Node’s os.homedir() docs state: on Windows, it uses the USERPROFILE environment variable if defined; otherwise it uses the path to the current user’s profile directory. [3] Practical implication - If process.env.USERPROFILE is set in the environment of the Node process, os.homedir() will use it; changing HOME typically won’t affect os.homedir() on Windows because the documented/preferred variable is USERPROFILE on that platform. [3][4]
Citations:
- 1: os.homedir() prefers USERPROFILE to HOME nodejs/node#13818
- 2: https://github.com/nodejs/node/blob/main/src/node_os.cc
- 3: https://nodejs.org/api/os.html
- 4: https://github.com/libuv/libuv/blob/v1.x/src/win/util.c
🏁 Script executed:
# Check how other tests handle homedir() mocking
rg -A 5 "vi.mock.*homedir" claude-code/tests/ --type tsRepository: activeloopai/hivemind
Length of output: 47
🏁 Script executed:
# Also check if there are environment variable mocking patterns for USERPROFILE
rg "USERPROFILE|HOMEDRIVE|HOMEPATH" --type tsRepository: activeloopai/hivemind
Length of output: 47
🏁 Script executed:
# Check how cli-auth.test.ts handles homedir mocking (mentioned in the homedir search results)
cat -n ./claude-code/tests/cli-auth.test.ts | head -50Repository: activeloopai/hivemind
Length of output: 2359
🏁 Script executed:
# Check auth-creds.test.ts which was also mentioned in the search
rg -B 5 -A 10 "homedir" ./claude-code/tests/auth-creds.test.ts | head -40Repository: activeloopai/hivemind
Length of output: 1341
🏁 Script executed:
# Let's verify what platform this project is expected to run on
cat ./package.json | grep -A 5 -B 5 "engines\|os"Repository: activeloopai/hivemind
Length of output: 536
🏁 Script executed:
# Also check if there are CI configs that might indicate Windows testing
find . -name "*.yml" -o -name "*.yaml" | grep -E "(workflow|ci|github)" | head -5Repository: activeloopai/hivemind
Length of output: 236
🏁 Script executed:
# Check the CI workflow to see what OS platforms are tested
cat ./.github/workflows/ci.yml | head -40Repository: activeloopai/hivemind
Length of output: 1279
🏁 Script executed:
# Check if there's any mention of Windows or cross-platform testing
grep -r "windows\|WIN32\|platform" ./.github/workflows/ --include="*.yml" --include="*.yaml"Repository: activeloopai/hivemind
Length of output: 47
🏁 Script executed:
# Check more of the CI workflow
cat ./.github/workflows/ci.yml | grep -A 20 "runs-on"Repository: activeloopai/hivemind
Length of output: 1381
Sandbox os.homedir() on Windows as well.
These tests only override HOME, but the code under test calls node:os.homedir(). On Windows that resolves from USERPROFILE / HOMEDRIVE / HOMEPATH, so this suite can hit the real ~/.deeplake/state and mutate real user data when run on Windows systems.
🛡️ Suggested fix
let sandboxHome: string;
let prevHome: string | undefined;
+let prevUserProfile: string | undefined;
+let prevHomeDrive: string | undefined;
+let prevHomePath: string | undefined;
beforeEach(() => {
sandboxHome = mkdtempSync(join(tmpdir(), "skillify-migration-"));
prevHome = process.env.HOME;
+ prevUserProfile = process.env.USERPROFILE;
+ prevHomeDrive = process.env.HOMEDRIVE;
+ prevHomePath = process.env.HOMEPATH;
process.env.HOME = sandboxHome;
+ process.env.USERPROFILE = sandboxHome;
+ delete process.env.HOMEDRIVE;
+ delete process.env.HOMEPATH;
});
afterEach(() => {
if (prevHome === undefined) delete process.env.HOME;
else process.env.HOME = prevHome;
+ if (prevUserProfile === undefined) delete process.env.USERPROFILE;
+ else process.env.USERPROFILE = prevUserProfile;
+ if (prevHomeDrive === undefined) delete process.env.HOMEDRIVE;
+ else process.env.HOMEDRIVE = prevHomeDrive;
+ if (prevHomePath === undefined) delete process.env.HOMEPATH;
+ else process.env.HOMEPATH = prevHomePath;
// chmodSync the sandbox readable in case a test removed perms; otherwise
// rmSync hits EACCES on cleanup and leaks the temp dir across runs.
try { chmodSync(sandboxHome, 0o755); } catch { /* nothing */ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude-code/tests/skillify-legacy-migration.test.ts` around lines 28 - 41,
The tests currently only set process.env.HOME (sandboxHome) in beforeEach but do
not stub node:os.homedir(), so on Windows the code under test can still resolve
the real user homedir; update beforeEach to also mock/spy on os.homedir() to
return sandboxHome (store the original os.homedir reference), and update
afterEach to restore the original os.homedir; keep the existing prevHome logic
and cleanup (chmodSync/rmSync) but ensure the os.homedir mock is removed in
afterEach so subsequent tests are unaffected.
Comment on lines
+84
to
95
| | **Pi** | `pi/extension-source/hivemind.ts` (raw .ts, no bundle) | full (session_start, input, tool_result, message_end, session_shutdown) | ✅ via `pi/bundle/skillify-worker.js` spawned from session_shutdown | ✅ inline in `CONTEXT_PREAMBLE` | self-contained extension; pi compiles the .ts at load time | | ||
| | **OpenClaw**| `openclaw/src/index.ts`, `openclaw/skills/SKILL.md` | `before_prompt_build`, `before_agent_start`, `agent_end` | ✅ via `openclaw/dist/skillify-worker.js` spawned from `agent_end` | ✅ in `openclaw/skills/SKILL.md` | gateway plugin: captures sessions to the same `sessions` table; bypasses esbuild's `child_process` stub via `createRequire(import.meta.url)` | | ||
|
|
||
| **Mining (worker firing on session end)** applies wherever the agent | ||
| captures sessions — which is all six. Earlier drafts of this section | ||
| incorrectly described OpenClaw mining as "N/A by design"; OpenClaw | ||
| does in fact capture sessions to the same `sessions` Deeplake table | ||
| (see `openclaw/src/index.ts:903` agent_end hook), and the worker can | ||
| mine them just like any other agent. The wiring lives at | ||
| `openclaw/src/index.ts:spawnOpenclawSkilifyWorker` and fires from the | ||
| agent_end hook after each capture. Bundle: `openclaw/dist/skilify-worker.js` | ||
| `openclaw/src/index.ts:spawnOpenclawSkillifyWorker` and fires from the | ||
| agent_end hook after each capture. Bundle: `openclaw/dist/skillify-worker.js` | ||
| (separate esbuild entry, isolated from the gateway's child_process |
There was a problem hiding this comment.
Resolve OpenClaw mining guidance contradiction.
This updated section says OpenClaw mining is wired and active, but Line 113 still states mining-style features don’t apply because OpenClaw “has none.” Please align those statements to avoid misleading future checklist runs.
Suggested doc fix (outside this changed hunk)
-- [ ] **OpenClaw**: if the feature has any agent-facing surface (commands, tools, discoverability text), update `openclaw/skills/SKILL.md` so the host agent learns about it. Mining-style features that need session lifecycle don't apply (openclaw has none) — document the limitation in the SKILL.md instead of pretending parity
+- [ ] **OpenClaw**: if the feature has any agent-facing surface (commands, tools, discoverability text), update `openclaw/skills/SKILL.md` so the host agent learns about it. For mining/session-capture features, validate the `agent_end` capture + worker path in `openclaw/src/index.ts` and confirm bundle wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@RELEASE_CHECKLIST.md` around lines 84 - 95, Update the contradictory
statement that OpenClaw “has none” for mining-style features so it matches the
rest of the doc: change the line that claims mining doesn’t apply to OpenClaw to
state that OpenClaw does capture sessions and can be mined (referencing the
agent_end hook and the spawnOpenclawSkillifyWorker wiring in
openclaw/src/index.ts and the produced bundle openclaw/dist/skillify-worker.js);
ensure the text mentions the agent_end capture (see
openclaw/src/index.ts:agent_end/line ~903) and that the mining worker is spawned
from spawnOpenclawSkillifyWorker and uses openclaw/dist/skillify-worker.js so
the checklist is consistent.
Comment on lines
+231
to
233
| if (cmd === "skillify") { | ||
| runSkillifyCommand(args.slice(1)); | ||
| return; |
There was a problem hiding this comment.
Await the skillify dispatcher.
This is the only command branch that doesn’t await its handler. If runSkillifyCommand() rejects, the failure bypasses main().catch(...) and can surface as an unhandled rejection instead of the normal CLI error path.
🐛 Suggested fix
if (cmd === "skillify") {
- runSkillifyCommand(args.slice(1));
+ await runSkillifyCommand(args.slice(1));
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (cmd === "skillify") { | |
| runSkillifyCommand(args.slice(1)); | |
| return; | |
| if (cmd === "skillify") { | |
| await runSkillifyCommand(args.slice(1)); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 231 - 233, The skillify command branch
currently calls runSkillifyCommand(args.slice(1)) without awaiting it, which can
cause unhandled rejections; change the branch to await the dispatcher (await
runSkillifyCommand(args.slice(1))) so any rejection flows into the surrounding
main().catch(...) path and ensure the enclosing function is async (or already
is) so awaiting is valid; update the branch that checks if (cmd === "skillify")
and replace the non-awaited call with an awaited call to runSkillifyCommand.
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
1 participant
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.
Closes #114.
Summary
Hard switch from
skilifytoskillifyacross the tree. The CLI subcommand and module were namedskilifybut the natural derivation fromskillisskillify. No deprecation alias —hivemind skilify ...now returnsUnknown command: skilify.Scope
src/skilify/->src/skillify/(incl.skilify-worker.ts,spawn-skilify-worker.ts)src/commands/skilify.ts->skillify.ts,runSkilifyCommand->runSkillifyCommandclaude-code/tests/skilify-*.test.tsfiles renamedsrc/,openclaw/,pi/extension-source/,.github/workflows/, README, RELEASE_CHECKLIST,esbuild.config.mjs,vitest.config.ts, hook tests<agent>/bundle/skilify-worker.jsartifacts removed; bundles regenerated to<agent>/bundle/skillify-worker.jsState directory auto-migration
New
src/skillify/legacy-migration.ts: one-shotrenameSyncof~/.deeplake/state/skilify/->~/.deeplake/state/skillify/on first boot of the renamed version. Wired into all fourmkdirSyncentry points (state.ts,scope-config.ts,manifest.ts,auto-pull.ts). EXDEV/EPERM falls back to leaving the legacy dir in place.OpenClaw is a self-contained bundle that can't import from
src/skillify/, so the migration is inlined intoopenclaw/src/index.tsand called before the existingfsMkdiron the state dir — once the new dir exists the migration becomes a no-op and any legacy data would be orphaned.Tests
New
claude-code/tests/skillify-legacy-migration.test.ts(5 tests): legacy-missing, current-already-exists (no clobber), happy-path rename preservingconfig.json/pulled.json/per-project state, idempotent re-call (module-levelattemptedflag), andrenameSyncEXDEV swallowed.Audit
After the rename, the only
skilifyreferences in the tree are 4 intentional string literals (the migration helper's legacy-path target, the inlined openclaw copy, JSDoc, and a test fixture) plus their 18 esbuild compilations inbundle/. Zero filenames, zero camelCase/PascalCase identifiers.Risk
Users with
hivemind skilifyaliases / scripts get a hard error after upgrade. State directory auto-migrates so installed-skill manifests, scope config, and per-project state survive without manual intervention. IfrenameSyncfails (cross-device link, perms), the legacy dir stays put and the new dir starts fresh —pullrepopulatespulled.json, butunpullof pre-rename installs may need manual cleanup.Sequencing
Originally planned to wait for #112 to merge to avoid conflicts; #112 has since landed plus follow-ups (autopull throttle removal,
maybeAutoPull -> autoPullSkillsrename, README condensed intodocs/SKILLIFY.md). Branch was based on the pre-merge tip of #112 and merged withorigin/mainafterward; conflicts inauto-pull.ts, the foursession-start.tsfiles, the CLI USAGE block, two test files, README, and 6 bundle outputs are resolved in148337d.Test plan
npm testgreen (2184/2184 in CI)npm run buildproduces all 12 + 10 + 9 + 9 + 1 + 1 + 1 + 1 bundles~/.deeplake/state/skilify/, the directory has been renamed in place to~/.deeplake/state/skillify/on the first SessionStart and installed pulled skills still resolvehivemind skilify(legacy spelling) returnsUnknown command: skilifyhivemind skillifyworks as documentedSummary by CodeRabbit
Bug Fixes
skilifytoskillifythroughout the system.~/.deeplake/state/skilifyto~/.deeplake/state/skillifyon first run.skillifynaming.Chores