fix: keep review results when CLI exits non-zero on validation failure

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<string, X>`), 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
+  }
+}

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);

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 {