Skills: stop using chokidar globs for refresh#54927
Skills: stop using chokidar globs for refresh#5492708820048 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a chokidar v4 compatibility regression where skill file watchers used glob patterns ( Confidence Score: 4/5Safe to merge with one targeted fix recommended: the undefined stats fallback in shouldIgnoreSkillsWatchPath could silently prevent newly-created skill directories from being watched. The core bug fix is correct and well-tested for the primary user path. One P1 edge case exists — stats === undefined for a depth-1 directory causes it to be ignored — but it only affects newly-created skill directories while the watcher is running, not the editing case the PR is focused on. src/agents/skills/refresh.ts — specifically the stats?.isDirectory?.() !== true branch in shouldIgnoreSkillsWatchPath (line 134)
|
| Filename | Overview |
|---|---|
| src/agents/skills/refresh.ts | Replaces glob-based chokidar watch targets with direct directory roots and a function-based ignored callback; one edge case with undefined stats for skill subdirectories. |
| src/agents/skills/refresh.test.ts | Test infrastructure upgraded to capture watcher instances and simulate events; missing add/unlink coverage in the snapshot-bump test. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/skills/refresh.ts
Line: 130-134
Comment:
**Skill subdirectory silently ignored when `stats` is `undefined`**
When `shouldIgnoreSkillsWatchPath` is called by chokidar for a newly-created skill subdirectory (e.g. `skills/new-skill/`) and `stats` is `undefined` (chokidar can call `ignored` before completing a stat in some code paths, or stat can fail on a transient race), the expression `stats?.isDirectory?.() !== true` evaluates to `undefined !== true` → `true`, so the directory is **ignored**. Chokidar will then never traverse into it and will never surface `skills/new-skill/SKILL.md` add/change events.
For the watched-root (`relative === ""`) and known-file cases this is fine, but for the directory case a safe fallback should not treat "unknown stats" as "not a directory":
```suggestion
return stats !== undefined && stats.isDirectory?.() !== true;
```
With this change, when `stats` is `undefined` (unknown), the function returns `false` (don't ignore), letting chokidar proceed to stat the entry and call `ignored` again with actual stats — which is the intended behaviour.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/skills/refresh.test.ts
Line: 113-150
Comment:
**`add`/`unlink` event paths not exercised in the snapshot test**
The new `"bumps the snapshot only for supported skill file changes"` test only fires a `change` event. The `add` and `unlink` handlers are registered separately (`watcher.on("add", …)`, `watcher.on("unlink", …)`) and both go through the same `schedule` guard, but they are never tested. A new skill being created (add) or deleted (unlink) are real user-facing triggers. Consider extending the test to call `handlers.get("add")` and `handlers.get("unlink")` for a valid SKILL.md path and assert the snapshot bumps, and also for an invalid path to confirm they are ignored — mirroring the `change` cases already written.
This is a minor coverage gap; the core logic is shared, so the risk is low, but the regression surface (newly created skill files not triggering a reload) is the kind of thing users would notice.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Skills: stop using chokidar globs for re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (segments.length === 1) { | ||
| if (segments[0] === "SKILL.md") { | ||
| return false; | ||
| } | ||
| return stats?.isDirectory?.() !== true; |
There was a problem hiding this comment.
stats is undefined
When shouldIgnoreSkillsWatchPath is called by chokidar for a newly-created skill subdirectory (e.g. skills/new-skill/) and stats is undefined (chokidar can call ignored before completing a stat in some code paths, or stat can fail on a transient race), the expression stats?.isDirectory?.() !== true evaluates to undefined !== true → true, so the directory is ignored. Chokidar will then never traverse into it and will never surface skills/new-skill/SKILL.md add/change events.
For the watched-root (relative === "") and known-file cases this is fine, but for the directory case a safe fallback should not treat "unknown stats" as "not a directory":
| if (segments.length === 1) { | |
| if (segments[0] === "SKILL.md") { | |
| return false; | |
| } | |
| return stats?.isDirectory?.() !== true; | |
| return stats !== undefined && stats.isDirectory?.() !== true; |
With this change, when stats is undefined (unknown), the function returns false (don't ignore), letting chokidar proceed to stat the entry and call ignored again with actual stats — which is the intended behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/skills/refresh.ts
Line: 130-134
Comment:
**Skill subdirectory silently ignored when `stats` is `undefined`**
When `shouldIgnoreSkillsWatchPath` is called by chokidar for a newly-created skill subdirectory (e.g. `skills/new-skill/`) and `stats` is `undefined` (chokidar can call `ignored` before completing a stat in some code paths, or stat can fail on a transient race), the expression `stats?.isDirectory?.() !== true` evaluates to `undefined !== true` → `true`, so the directory is **ignored**. Chokidar will then never traverse into it and will never surface `skills/new-skill/SKILL.md` add/change events.
For the watched-root (`relative === ""`) and known-file cases this is fine, but for the directory case a safe fallback should not treat "unknown stats" as "not a directory":
```suggestion
return stats !== undefined && stats.isDirectory?.() !== true;
```
With this change, when `stats` is `undefined` (unknown), the function returns `false` (don't ignore), letting chokidar proceed to stat the entry and call `ignored` again with actual stats — which is the intended behaviour.
How can I resolve this? If you propose a fix, please make it concise.| it("bumps the snapshot only for supported skill file changes", async () => { | ||
| vi.useFakeTimers(); | ||
| const mod = await import("./refresh.js"); | ||
| const events: Array<{ workspaceDir?: string; changedPath?: string }> = []; | ||
| const unregister = mod.registerSkillsChangeListener((event) => { | ||
| events.push(event); | ||
| }); | ||
|
|
||
| mod.ensureSkillsWatcher({ | ||
| workspaceDir: "/tmp/workspace", | ||
| config: { | ||
| skills: { | ||
| load: { | ||
| watchDebounceMs: 25, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const watcher = watcherInstances.at(-1); | ||
| expect(watcher).toBeDefined(); | ||
|
|
||
| watcher?.handlers.get("change")?.("/tmp/workspace/skills/my-skill/SKILL.md"); | ||
| await vi.advanceTimersByTimeAsync(25); | ||
| expect(events).toEqual([ | ||
| { | ||
| workspaceDir: "/tmp/workspace", | ||
| reason: "watch", | ||
| changedPath: "/tmp/workspace/skills/my-skill/SKILL.md", | ||
| }, | ||
| ]); | ||
|
|
||
| watcher?.handlers.get("change")?.("/tmp/workspace/skills/my-skill/notes.md"); | ||
| await vi.advanceTimersByTimeAsync(25); | ||
| expect(events).toHaveLength(1); | ||
|
|
||
| // Should NOT ignore normal skill files | ||
| expect(ignored.some((re) => re.test("/tmp/.hidden/skills/index.md"))).toBe(false); | ||
| expect(ignored.some((re) => re.test("/tmp/workspace/skills/my-skill/SKILL.md"))).toBe(false); | ||
| unregister(); | ||
| }); |
There was a problem hiding this comment.
add/unlink event paths not exercised in the snapshot test
The new "bumps the snapshot only for supported skill file changes" test only fires a change event. The add and unlink handlers are registered separately (watcher.on("add", …), watcher.on("unlink", …)) and both go through the same schedule guard, but they are never tested. A new skill being created (add) or deleted (unlink) are real user-facing triggers. Consider extending the test to call handlers.get("add") and handlers.get("unlink") for a valid SKILL.md path and assert the snapshot bumps, and also for an invalid path to confirm they are ignored — mirroring the change cases already written.
This is a minor coverage gap; the core logic is shared, so the risk is low, but the regression surface (newly created skill files not triggering a reload) is the kind of thing users would notice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/skills/refresh.test.ts
Line: 113-150
Comment:
**`add`/`unlink` event paths not exercised in the snapshot test**
The new `"bumps the snapshot only for supported skill file changes"` test only fires a `change` event. The `add` and `unlink` handlers are registered separately (`watcher.on("add", …)`, `watcher.on("unlink", …)`) and both go through the same `schedule` guard, but they are never tested. A new skill being created (add) or deleted (unlink) are real user-facing triggers. Consider extending the test to call `handlers.get("add")` and `handlers.get("unlink")` for a valid SKILL.md path and assert the snapshot bumps, and also for an invalid path to confirm they are ignored — mirroring the `change` cases already written.
This is a minor coverage gap; the core logic is shared, so the risk is low, but the regression surface (newly created skill files not triggering a reload) is the kind of thing users would notice.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b647fb3ea
ℹ️ 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".
| if (segments[0] === "SKILL.md") { | ||
| return false; | ||
| } | ||
| return stats?.isDirectory?.() !== true; |
There was a problem hiding this comment.
The ignored predicate currently returns true for first-level entries unless stats.isDirectory() is explicitly true, but chokidar evaluates ignored paths before stat-ing them in its traversal path (it calls _isIgnored(path) without stats in _addToNodeFs). For a directory like .../skills/my-skill, this line treats it as ignored on the pre-stat check, so chokidar never descends into that folder and nested SKILL.md changes still do not trigger refreshes.
Useful? React with 👍 / 👎.
1 participant
Summary
*/SKILL.mdglob targets thatchokidarv4 no longer expands, so nested skill folders stopped triggering reloads.workspace/skills/<name>/SKILL.mdleaves the runtime on stale skill definitions until a manual refresh or restart.1, and filter events so onlySKILL.mdor<skill>/SKILL.mdpaths can trigger a refresh.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.<skillsRoot>/*/SKILL.mdintochokidar.watch, butchokidarv4 removed glob expansion support.SKILL.mdchange event through the v4-compatible watch path.git blame, prior PR, issue, or refactor if known): Unknown.chokidarversion without glob support, nested skill directories stopped being watched even though root-levelSKILL.mdstill worked.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write
N/A.src/agents/skills/refresh.test.tsskills/<name>/SKILL.mdchanges should trigger a refresh, while sibling files likenotes.mdshould not.User-visible / Behavior Changes
Editing
SKILL.mdinside a nested skill directory now refreshes workspace skills again without a restart whenskills.load.watchis enabled.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
~/.openclaw/workspace/skills/<skill>/SKILL.md.SKILL.md.Expected
Actual
skills/SKILL.mdwas watched; nested skill edits were ignored.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test -- src/agents/skills/refresh.test.tsandpnpm tsgo; the updated unit test simulates a nestedmy-skill/SKILL.mdchange and confirms it bumps the snapshot, whilenotes.mddoes not.SKILL.md, nested<skill>/SKILL.md, ignored cache/build directories, and unrelated files under a skill directory.chokidarinstance outside the unit test harness.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
skills.load.watch.src/agents/skills/refresh.tsskills/begin triggering reload churn.Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.SKILL.mdand<skill>/SKILL.md) and tests both positive and negative cases.