From 40f355c6365a0bbb09941b2ea4e6bfbf38a48eab Mon Sep 17 00:00:00 2001 From: Alan Pope Date: Sun, 21 Jun 2026 23:37:35 +0100 Subject: [PATCH] fix: render validation.checks instead of nonexistent validation.output The result-assembly in src/skill-review.ts read `parsed.validation?.output`, but `tessl skill review --json` never emits a `validation.output` field. The real contract exposes `validation.checks` (an array of { name, status, message }) alongside `overallPassed`, `errorCount`, and `warningCount`. Because the guarded field never existed, the "Validation Checks" section was silently dropped from every PR comment. Model the real `validation` shape and render the checks as a markdown table with status markers plus a one-line pass/fail summary. Existing tests encoded the old buggy `{ output: ... }` shape, so the success fixture is updated to the real shape and a golden-derived fixture (src/fixtures/review-validation-failed.json) drives a regression test asserting a failing "error" check renders. --- src/fixtures/review-validation-failed.json | 74 ++++++++++++++++++++++ src/main.test.ts | 45 ++++++++++++- src/skill-review.ts | 53 ++++++++++++++-- 3 files changed, 163 insertions(+), 9 deletions(-) create mode 100644 src/fixtures/review-validation-failed.json diff --git a/src/fixtures/review-validation-failed.json b/src/fixtures/review-validation-failed.json new file mode 100644 index 0000000..8357620 --- /dev/null +++ b/src/fixtures/review-validation-failed.json @@ -0,0 +1,74 @@ +{ + "review": { + "reviewScore": 18 + }, + "validation": { + "checks": [ + { + "name": "skill_md_line_count", + "status": "passed", + "message": "SKILL.md line count is 171 (<= 500)", + "details": { + "line_count": 171, + "max_lines": 500 + } + }, + { + "name": "frontmatter_valid", + "status": "passed", + "message": "YAML frontmatter is valid", + "details": { + "keys": ["name", "description"] + } + }, + { + "name": "name_field", + "status": "passed", + "message": "'name' field is valid: 'legibility-review'", + "details": { + "value": "legibility-review", + "length": 17 + } + }, + { + "name": "description_field", + "status": "error", + "message": "'description' must not contain XML tags", + "details": { + "length": 915 + } + }, + { + "name": "body_present", + "status": "passed", + "message": "SKILL.md body is present", + "details": { + "body_chars": 17335 + } + } + ], + "overallPassed": false, + "errorCount": 1, + "warningCount": 0, + "skillName": "legibility-review", + "skillDescription": "How to review code so a reader without your context can understand each piece." + }, + "contentJudge": { + "success": true, + "evaluation": { + "scores": { + "conciseness": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + } + }, + "overall_assessment": "Skill failed deterministic validation. LLM judge evaluation was skipped.", + "suggestions": [ + "Fix the deterministic validation errors before running LLM judge." + ], + "validation_skipped": true + }, + "weightedScore": 0, + "normalizedScore": 0 + } +} diff --git a/src/main.test.ts b/src/main.test.ts index cf4243b..a22a294 100644 --- a/src/main.test.ts +++ b/src/main.test.ts @@ -258,7 +258,23 @@ describe('runSkillReview', () => { normalizedScore: 0.85, evaluation: 'Good skill definition.', }, - validation: { output: 'All checks passed.' }, + validation: { + checks: [ + { + name: 'frontmatter_valid', + status: 'passed', + message: 'YAML frontmatter is valid', + }, + { + name: 'body_present', + status: 'passed', + message: 'SKILL.md body is present', + }, + ], + overallPassed: true, + errorCount: 0, + warningCount: 0, + }, }); // @ts-expect-error mock assignment @@ -269,10 +285,35 @@ describe('runSkillReview', () => { expect(result.score).toBe(85); expect(result.passed).toBe(true); expect(result.error).toBeUndefined(); - expect(result.output).toContain('All checks passed.'); + expect(result.output).toContain('### Validation Checks'); + expect(result.output).toContain('Validation passed'); + expect(result.output).toContain('YAML frontmatter is valid'); + expect(result.output).toContain('SKILL.md body is present'); expect(result.output).toContain('Good skill definition.'); }); + test('renders failing validation checks from golden fixture', async () => { + const fixture = await Bun.file( + new URL('./fixtures/review-validation-failed.json', import.meta.url), + ).text(); + + // @ts-expect-error mock assignment + Bun.spawn = makeMockSpawn(fixture, '', 0); + + const result = await runSkillReview('skills/legibility-review/SKILL.md', 0); + expect(result.output).toContain('### Validation Checks'); + // Summary reflects the failing run. + expect(result.output).toContain('Validation failed'); + expect(result.output).toContain('1 error(s)'); + // The failing "error" check is rendered with its message and marker. + expect(result.output).toContain('description field'); + expect(result.output).toContain("'description' must not contain XML tags"); + expect(result.output).toContain('❌'); + // A passing check is also rendered. + expect(result.output).toContain('YAML frontmatter is valid'); + expect(result.output).toContain('✅'); + }); + test('CLI failure (non-zero exit)', async () => { // @ts-expect-error mock assignment Bun.spawn = makeMockSpawn('', 'Command not found', 1); diff --git a/src/skill-review.ts b/src/skill-review.ts index b4fe675..6724811 100644 --- a/src/skill-review.ts +++ b/src/skill-review.ts @@ -53,10 +53,49 @@ function formatEvaluation(value: unknown): string { return parts.length > 0 ? parts.join('\n') : JSON.stringify(value, null, 2); } -/** Safely convert an unknown value to a readable string */ -function stringify(value: unknown): string { - if (typeof value === 'string') return value; - return JSON.stringify(value, null, 2); +interface ValidationCheck { + name: string; + status: 'passed' | 'warning' | 'error'; + message: string; +} + +interface Validation { + checks?: ValidationCheck[]; + overallPassed?: boolean; + errorCount?: number; + warningCount?: number; +} + +/** Marker emoji for a validation check status */ +function statusMarker(status: ValidationCheck['status']): string { + if (status === 'passed') return '✅'; + if (status === 'warning') return '⚠️'; + return '❌'; +} + +/** Render validation checks into a readable markdown table with a summary line. */ +function formatValidation(validation: Validation): string { + const parts: string[] = []; + + const passed = validation.overallPassed ? 'passed' : 'failed'; + const errors = validation.errorCount ?? 0; + const warnings = validation.warningCount ?? 0; + parts.push( + `**Validation ${passed}** — ${errors} error(s), ${warnings} warning(s)`, + ); + + const checks = validation.checks ?? []; + if (checks.length > 0) { + parts.push('', '| | Check | Message |', '|--|-------|---------|'); + for (const check of checks) { + const label = check.name.replace(/_/g, ' '); + parts.push( + `| ${statusMarker(check.status)} | **${label}** | ${check.message} |`, + ); + } + } + + return parts.join('\n'); } /** @@ -154,7 +193,7 @@ export async function runSkillReview( } let parsed: { contentJudge?: { normalizedScore?: number; evaluation?: unknown }; - validation?: { output?: unknown }; + validation?: Validation; }; try { @@ -174,9 +213,9 @@ export async function runSkillReview( const score = Math.round(normalizedScore * 100); const outputParts: string[] = []; - if (parsed.validation?.output) { + if (parsed.validation?.checks && parsed.validation.checks.length > 0) { outputParts.push( - '### Validation Checks\n\n' + stringify(parsed.validation.output), + '### Validation Checks\n\n' + formatValidation(parsed.validation), ); } if (parsed.contentJudge?.evaluation) {