diff --git a/src/fixtures/review-validation-failed.json b/src/fixtures/review-validation-failed.json new file mode 100644 index 0000000..22c3211 --- /dev/null +++ b/src/fixtures/review-validation-failed.json @@ -0,0 +1,149 @@ +{ + "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": "compatibility_field", + "status": "passed", + "message": "'compatibility' field not present (optional)" + }, + { + "name": "allowed_tools_field", + "status": "passed", + "message": "'allowed-tools' field not present (optional)" + }, + { + "name": "metadata_version", + "status": "passed", + "message": "'metadata' field not present (optional)" + }, + { + "name": "metadata_field", + "status": "passed", + "message": "'metadata' field not present (optional)" + }, + { + "name": "license_field", + "status": "passed", + "message": "'license' field not present (optional)" + }, + { + "name": "frontmatter_unknown_keys", + "status": "passed", + "message": "No unknown frontmatter keys found" + }, + { + "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 from what's in front of them. Two complementary tests: each artifact should be understandable in isolation (cold-reader), and the codebase should look designed-from-scratch (from-scratch test). Code that fails either is structurally wrong; the fix is renaming, restructuring, narrowing types, inlining, extracting, or removing – not adding comments.\nTRIGGER when: reviewing a diff (your own pre-PR pass before opening, or someone else's PR); assessing whether a name, return shape, type (e.g. an opaque `Record`), or abstraction in the diff carries its meaning to a cold reader; deciding whether new code landed in the right place.\nDO NOT TRIGGER when: trivial bug fixes (off-by-one, null guard, typo); editing test fixtures; one-line edits to existing structures; pure formatting or rename refactors that don't change shape.\n" + }, + "descriptionJudge": { + "success": true, + "evaluation": { + "scores": { + "specificity": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "trigger_term_quality": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "completeness": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "distinctiveness_conflict_risk": { + "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 + }, + "contentJudge": { + "success": true, + "evaluation": { + "scores": { + "conciseness": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "actionability": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "workflow_clarity": { + "score": 0, + "reasoning": "Deterministic validation failed; LLM judge was not run." + }, + "progressive_disclosure": { + "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..22ab531 100644 --- a/src/main.test.ts +++ b/src/main.test.ts @@ -283,6 +283,26 @@ describe('runSkillReview', () => { expect(result.error).toBe('Command not found'); }); + test('exit code 1 with valid JSON (validation failure) is not an error', async () => { + // The CLI prints the full review JSON to stdout and then exits 1 when + // validation.overallPassed is false. We must preserve that review. + 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, 'Skill validation failed', 1); + + const result = await runSkillReview('skills/test/SKILL.md', 50); + expect(result.error).toBeUndefined(); + expect(result.score).not.toBe(-1); + // contentJudge.normalizedScore is 0 in the fixture → score 0. + expect(result.score).toBe(0); + // Review content from the parsed JSON should be rendered, not empty. + expect(result.output.length).toBeGreaterThan(0); + expect(result.output).toContain('Review Details'); + }); + test('CLI failure with threshold 0 still passes', async () => { // @ts-expect-error mock assignment Bun.spawn = makeMockSpawn('', 'some error', 1); diff --git a/src/skill-review.ts b/src/skill-review.ts index b4fe675..9fb5873 100644 --- a/src/skill-review.ts +++ b/src/skill-review.ts @@ -128,7 +128,15 @@ export async function runSkillReview( ]); const exitCode = await proc.exited; - if (exitCode !== 0) { + + // The CLI prints the full review JSON to stdout and THEN exits 1 when + // validation.overallPassed is false ("Skill validation failed"). A non-zero + // exit therefore does NOT mean the review produced no output — the JSON on + // stdout is the most valuable feedback. Only treat a non-zero exit as a hard + // error when stdout has no usable JSON (genuine crash / not-found / network). + const jsonStr = extractJson(stdout); + + if (exitCode !== 0 && !jsonStr) { console.warn( `tessl skill review failed for ${skillFilePath} (exit code ${exitCode}): ${stderr}`, ); @@ -141,7 +149,6 @@ export async function runSkillReview( }; } - const jsonStr = extractJson(stdout); if (!jsonStr) { console.warn(`No JSON found in skill review output for ${skillFilePath}`); return {