skills readme was getting junk files#967
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the generated Skills README from listing “junk”/overly-verbose bundled asset paths by changing how skill asset paths are discovered and then updating the generated docs/README.skills.md output accordingly.
Changes:
- Restricts skill asset recursion in
parseSkillMetadata()to specific top-level directories. - Updates the generated skills table to show some bundled assets as directory entries (e.g.,
templates,examples,scripts/cache-generator) instead of enumerating every file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| eng/yaml-parser.mjs | Changes skill asset enumeration logic (affects README generation and other consumers of assets). |
| docs/README.skills.md | Regenerated skills README reflecting the new bundled-asset listing behavior. |
Comments suppressed due to low confidence (1)
eng/yaml-parser.mjs:146
- The inline comment says this lists "all files" and "recursing through subdirectories", but the updated logic only recurses into a small allowlist of directory names and may add directories themselves to the
assetslist. Please update the comment to match the new behavior so future readers don’t assume it’s a full recursive file walk.
// List bundled assets (all files except SKILL.md), recursing through subdirectories
const getAllFiles = (dirPath, arrayOfFiles = []) => {
You can also share your feedback on Copilot code review. Take the survey.
| const assetPaths = ['references', 'assets', 'scripts']; | ||
|
|
||
| files.forEach((file) => { | ||
| const filePath = path.join(dirPath, file); | ||
| if (fs.statSync(filePath).isDirectory()) { | ||
| if (fs.statSync(filePath).isDirectory() && assetPaths.includes(file)) { | ||
| arrayOfFiles = getAllFiles(filePath, arrayOfFiles); | ||
| } else { | ||
| const relativePath = path.relative(skillPath, filePath); |
There was a problem hiding this comment.
Changing getAllFiles to only recurse into references/assets/scripts means metadata.assets can now include directory entries (e.g., templates, examples, scripts/cache-generator) instead of listing all files. Downstream consumers treat assets as files (e.g., validate-skills checks each asset path size via statSync), so directories will bypass per-file size validation and also skew asset counts in website metadata. Consider keeping parseSkillMetadata().assets as a full file list (and collapse/group only when rendering the README), or update the validation/consumers to recurse into directories when an asset entry is a folder.
This issue also appears on line 145 of the same file.
See below for a potential fix:
files.forEach((file) => {
const filePath = path.join(dirPath, file);
const stat = fs.statSync(filePath);
if (stat.isDirectory()) {
| // List bundled assets (all files except SKILL.md), recursing through subdirectories | ||
| const getAllFiles = (dirPath, arrayOfFiles = []) => { | ||
| const files = fs.readdirSync(dirPath); | ||
| const assetPaths = ['references', 'assets', 'scripts']; |
There was a problem hiding this comment.
Quote style is inconsistent with the rest of this file (which primarily uses double quotes). To keep formatting consistent and avoid lint/prettier churn, consider switching ['references', 'assets', 'scripts'] to use double-quoted strings.
| const assetPaths = ['references', 'assets', 'scripts']; | |
| const assetPaths = ["references", "assets", "scripts"]; |
Pull Request Checklist
npm startand verified thatREADME.mdis up to date.Description
Type of Contribution
Additional Notes
By submitting this pull request, I confirm that my contribution abides by the Code of Conduct and will be licensed under the MIT License.