Skip to content

Commit ece35b9

Browse files
spboyerCopilot
andcommitted
fix: address PR review feedback
1. Fix sed regex: replace \s with [[:space:]] for portable whitespace matching in version extraction (review comment on line 333) 2. Expand version-bump check scope to include .github/skills/** in addition to plugin/skills/** (review comment on line 205) 3. Make license and metadata.version checks produce warnings instead of errors in frontmatter CLI. Missing fields are reported as warnings (⚠️) that don't fail the workflow; invalid formats (e.g. bad semver) remain errors (❌) that do fail. Updated tests to verify warning severity. (review comment on cli.ts:236) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0c1207e commit ece35b9

3 files changed

Lines changed: 38 additions & 18 deletions

File tree

.github/workflows/pr.yml

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ jobs:
203203
with:
204204
files: |
205205
plugin/skills/**
206+
.github/skills/**
206207
207208
- name: Setup Node.js
208209
if: steps.changed-skills.outputs.any_changed == 'true'
@@ -307,7 +308,7 @@ jobs:
307308
SKILL_FILES=""
308309
for file in ${{ steps.changed-plugin-skills.outputs.all_changed_files }}; do
309310
dir=$(dirname "$file")
310-
while [ "$dir" != "plugin/skills" ] && [ "$dir" != "plugin" ] && [ "$dir" != "." ]; do
311+
while [ "$dir" != "plugin/skills" ] && [ "$dir" != ".github/skills" ] && [ "$dir" != "plugin" ] && [ "$dir" != ".github" ] && [ "$dir" != "." ]; do
311312
if [ -f "$dir/SKILL.md" ]; then
312313
SKILL_FILES="$SKILL_FILES $dir/SKILL.md"
313314
break
@@ -326,11 +327,11 @@ jobs:
326327
fi
327328
328329
for skill_file in $SKILL_FILES; do
329-
# Get version in base branch
330-
BASE_VERSION=$(git show origin/${{ github.base_ref }}:"$skill_file" 2>/dev/null | sed -n '/^---$/,/^---$/p' | grep -E '^\s+version:' | head -1 | sed 's/.*version:\s*"\{0,1\}\([^"]*\)"\{0,1\}/\1/' || echo "")
330+
# Get version in base branch (use [[:space:]] for portable whitespace matching)
331+
BASE_VERSION=$(git show origin/${{ github.base_ref }}:"$skill_file" 2>/dev/null | sed -n '/^---$/,/^---$/p' | grep -E '^[[:space:]]+version:' | head -1 | sed 's/.*version:[[:space:]]*"\{0,1\}\([^"]*\)"\{0,1\}/\1/' || echo "")
331332
332333
# Get version in PR branch
333-
HEAD_VERSION=$(sed -n '/^---$/,/^---$/p' "$skill_file" | grep -E '^\s+version:' | head -1 | sed 's/.*version:\s*"\{0,1\}\([^"]*\)"\{0,1\}/\1/')
334+
HEAD_VERSION=$(sed -n '/^---$/,/^---$/p' "$skill_file" | grep -E '^[[:space:]]+version:' | head -1 | sed 's/.*version:[[:space:]]*"\{0,1\}\([^"]*\)"\{0,1\}/\1/')
334335
335336
# New skill (no base version) — skip, it starts at 1.0.0
336337
if [ -z "$BASE_VERSION" ]; then

scripts/src/frontmatter/__tests__/frontmatter.test.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -259,20 +259,23 @@ describe("Frontmatter Spec Validator", () => {
259259
expect(validateLicense("MIT")).toEqual([]);
260260
});
261261

262-
it("fails for missing license", () => {
262+
it("warns for missing license", () => {
263263
const issues = validateLicense(undefined);
264264
expect(issues).toHaveLength(1);
265265
expect(issues[0].check).toBe("license");
266+
expect(issues[0].severity).toBe("warning");
266267
});
267268

268-
it("fails for null license", () => {
269+
it("warns for null license", () => {
269270
const issues = validateLicense(null);
270271
expect(issues).toHaveLength(1);
272+
expect(issues[0].severity).toBe("warning");
271273
});
272274

273-
it("fails for empty license", () => {
275+
it("warns for empty license", () => {
274276
const issues = validateLicense("");
275277
expect(issues).toHaveLength(1);
278+
expect(issues[0].severity).toBe("warning");
276279
});
277280

278281
it("fails for non-string license", () => {
@@ -293,16 +296,18 @@ describe("Frontmatter Spec Validator", () => {
293296
expect(validateMetadataVersion({ version: "3.2.1" })).toEqual([]);
294297
});
295298

296-
it("fails for missing metadata", () => {
299+
it("warns for missing metadata", () => {
297300
const issues = validateMetadataVersion(undefined);
298301
expect(issues).toHaveLength(1);
299302
expect(issues[0].check).toBe("metadata-version");
303+
expect(issues[0].severity).toBe("warning");
300304
});
301305

302-
it("fails for missing version in metadata", () => {
306+
it("warns for missing version in metadata", () => {
303307
const issues = validateMetadataVersion({ author: "Microsoft" });
304308
expect(issues).toHaveLength(1);
305309
expect(issues[0].check).toBe("metadata-version");
310+
expect(issues[0].severity).toBe("warning");
306311
});
307312

308313
it("fails for non-semver version", () => {

scripts/src/frontmatter/cli.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const META_SKILLS_DIR = resolve(REPO_ROOT, ".github", "skills");
3838
export interface ValidationIssue {
3939
check: string; // Check identifier (e.g., "name-format", "description-format")
4040
message: string; // Human-readable explanation
41+
severity?: "error" | "warning"; // Defaults to "error" if omitted
4142
}
4243

4344
export interface ValidationResult {
@@ -188,6 +189,7 @@ export function validateLicense(license: unknown): ValidationIssue[] {
188189
issues.push({
189190
check: "license",
190191
message: "Missing 'license' field in frontmatter",
192+
severity: "warning",
191193
});
192194
} else if (typeof license !== "string") {
193195
issues.push({
@@ -209,6 +211,7 @@ export function validateMetadataVersion(metadata: unknown): ValidationIssue[] {
209211
issues.push({
210212
check: "metadata-version",
211213
message: "Missing 'metadata' block with 'version' field in frontmatter",
214+
severity: "warning",
212215
});
213216
return issues;
214217
}
@@ -220,6 +223,7 @@ export function validateMetadataVersion(metadata: unknown): ValidationIssue[] {
220223
issues.push({
221224
check: "metadata-version",
222225
message: "Missing 'version' in metadata block",
226+
severity: "warning",
223227
});
224228
return issues;
225229
}
@@ -332,31 +336,41 @@ function main(): void {
332336
console.log("\n📋 Frontmatter Spec Validator\n");
333337
console.log("────────────────────────────────────────────────────────────");
334338

335-
let totalIssues = 0;
339+
let totalErrors = 0;
340+
let totalWarnings = 0;
336341
let skillsWithIssues = 0;
337342

338343
for (const file of skillFiles) {
339344
const result = validateSkillFile(file);
345+
const errors = result.issues.filter(i => i.severity !== "warning");
346+
const warnings = result.issues.filter(i => i.severity === "warning");
340347

341348
if (result.issues.length === 0) {
342349
console.log(` ✅ ${result.skill}`);
343350
} else {
344-
skillsWithIssues++;
345-
totalIssues += result.issues.length;
346-
347-
console.log(` ❌ ${result.skill}${result.issues.length} issue(s)`);
348-
for (const issue of result.issues) {
349-
console.log(` [${issue.check}] ${issue.message}`);
351+
if (errors.length > 0) skillsWithIssues++;
352+
totalErrors += errors.length;
353+
totalWarnings += warnings.length;
354+
355+
const icon = errors.length > 0 ? "❌" : "⚠️";
356+
console.log(` ${icon} ${result.skill}${errors.length} error(s), ${warnings.length} warning(s)`);
357+
for (const issue of errors) {
358+
console.log(` ❌ [${issue.check}] ${issue.message}`);
359+
}
360+
for (const issue of warnings) {
361+
console.log(` ⚠️ [${issue.check}] ${issue.message}`);
350362
}
351363
}
352364
}
353365

354366
console.log("\n────────────────────────────────────────────────────────────");
355367

356-
if (totalIssues === 0) {
368+
if (totalErrors === 0 && totalWarnings === 0) {
357369
console.log(`\n✅ All ${skillFiles.length} skill(s) passed frontmatter validation.\n`);
370+
} else if (totalErrors === 0) {
371+
console.log(`\n✅ All ${skillFiles.length} skill(s) passed with ${totalWarnings} warning(s).\n`);
358372
} else {
359-
console.log(`\n❌ ${totalIssues} issue(s) found in ${skillsWithIssues} skill(s).\n`);
373+
console.log(`\n❌ ${totalErrors} error(s) and ${totalWarnings} warning(s) found in ${skillsWithIssues} skill(s).\n`);
360374
process.exitCode = 1;
361375
}
362376
}

0 commit comments

Comments
 (0)