hide dev skills - #157
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new lint rule, prevent-skills-sh-publishing, which ensures that internal skills are marked with metadata: internal: true in their frontmatter to prevent accidental publishing. It also transitions several local skill files to remote dependencies, updates the skills-lock.json lockfile, and adds corresponding unit tests. The review feedback suggests checking for yamlParsingError to avoid duplicate diagnostics, adding a unit test for malformed YAML handling, and aligning the documented diagnostic shapes in RULES.md with the actual error messages in the implementation.
| expect(errors, isNotEmpty); | ||
| expect(errors.first.message, contains('Missing YAML frontmatter')); | ||
| }); | ||
|
|
There was a problem hiding this comment.
To ensure that the rule correctly ignores malformed YAML (which is already handled by the valid-yaml-metadata rule), we should add a unit test verifying that no errors are returned when yamlParsingError is present.
test('returns no errors when there is a YAML parsing error', () async {
final rule = PreventSkillsShPublishingRule(severity: AnalysisSeverity.warning);
final context = SkillContext(
directory: Directory('dummy'),
rawContent: '---\ninvalid: yaml: : mapping\n---\n',
yamlParsingError: 'YAML parsing error details',
);
final List<ValidationError> errors = await rule.validate(context);
expect(errors, isEmpty);
});
|
|
||
| @override | ||
| Future<List<ValidationError>> validate(SkillContext context) async { | ||
| final errors = <ValidationError>[]; |
There was a problem hiding this comment.
If you run this locally will it not trigger on all your installed skills? Is something else skipping the git ignored skills?
There was a problem hiding this comment.
If you look on line 17 the rule is disabled by default. So that begs the question where is it enabled. That happens in tool/dart_skills_lint/dart_skills_lint.yaml where we are listing the 3 skills that are checked in.
There was a problem hiding this comment.
Hmmm ok, similar to my other comment that approach seems less than ideal when adding any new skills (you have to remember to manually add them or get no lints).
An opt-out model would likely be better.
IMO in general the whole skills "standard" just does a terrible job of handling mixed third party vs locally developed skills.
We could develop our own standard, putting custom skills under skills/local or something, and applying the lints to all of those. 🤷♂️
I don't think this has to be addressed in this PR in any case.
There was a problem hiding this comment.
FWIW I added a test in tool/dart_skills_lint/test/prevent_skills_sh_publishing_rule_test.dart to enforce the behavior you are worried about in this repo.
Fixes: #156
Exclusions are done by following the documenation in https://github.com/vercel-labs/skills/blob/main/README.md#optional-fields
I dont actually have a way to test that the scraper that is run by vercel will work or a way to trigger it but this is in their documentation and in the code (https://github.com/vercel-labs/skills/blob/main/src/skills.ts#L81-L86)
Commits: