fix(skill): pass directory to getAllSkills and fix async test timing#3098
fix(skill): pass directory to getAllSkills and fix async test timing#3098sjawhar wants to merge 1 commit intocode-yeongyu:devfrom
Conversation
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Fixes a TypeScript compilation error by enforcing the required directory parameter and improves test stability with better isolation and timing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 684c4e902e
ℹ️ 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".
| const discovered = await getAllSkills({ | ||
| disabledSkills: options?.disabledSkills, | ||
| browserProvider: options?.browserProvider, | ||
| directory: options.directory, |
There was a problem hiding this comment.
Define directory on SkillLoadOptions
createSkillTool now reads options.directory, but SkillLoadOptions (in src/tools/skill/types.ts) still has no directory field, so this change introduces strict TypeScript errors (TS2339 here and TS2353 at the createSkillTool({ directory: process.cwd() }) call). In environments that run tsc --noEmit during CI/build, this blocks the pipeline until SkillLoadOptions is updated (or the new property access is removed).
Useful? React with 👍 / 👎.
Summary
createSkillTool()to requiredirectoryparameter instead of defaulting to{}, which no longer satisfies theSkillLoadOptionsinterface afterdirectorybecame requireddirectorythrough togetAllSkills()call inside the skill tool'sgetSkills()functionasync-description-refresh.test.tsto use an isolated temp directory and increase poll interval (0ms -> 50ms) to account for real async I/O in skill discoveryProblem
After
directorywas added as a required field onSkillLoadOptions, two issues appeared:createSkillTool(options: SkillLoadOptions = {})— the default{}no longer satisfies the interface sincedirectory: stringis required. TypeScript reportsTS2741: Property 'directory' is missing.getAllSkills()inside the tool was not receivingdirectoryfrom options, so skill discovery couldn't resolve project-level skills correctly.The async description refresh test used
setTimeout(resolve, 0)which was sufficient whengetAllSkillshad no real I/O, but now that it receives a directory and does filesystem discovery, 50ms intervals are needed for the async chain to complete.Changes
src/tools/skill/tools.ts= {}fromcreateSkillToolparameter (callers must pass options)directory: options.directoryto thegetAllSkills()callskillexport usesprocess.cwd()as fallback directorysrc/tools/skill/async-description-refresh.test.tsmkdtempSyncfor isolated test directory (prevents real project skills from interfering)waitForRefreshSummary by cubic
Require a
directoryincreateSkillTool, pass it togetAllSkills, and set the defaultskillexport to useprocess.cwd()to restore correct skill discovery. Stabilize the async refresh test with an isolated temp directory and a 50ms poll interval.Written for commit 684c4e9. Summary will update on new commits.