From 23a0c1f410e451bf2c5891df4091b59cae2ae4db Mon Sep 17 00:00:00 2001 From: Alan Pope Date: Sun, 21 Jun 2026 23:37:37 +0100 Subject: [PATCH] fix: score from review.reviewScore (canonical) instead of contentJudge.normalizedScore The displayed score and fail-threshold gate were derived from contentJudge.normalizedScore (* 100). That is the wrong field: the CLI's canonical score is review.reviewScore (already 0-100, blending the content and description judges) and is what `tessl skill review --threshold` gates on. Using contentJudge.normalizedScore makes the action disagree with the CLI. The golden sample shows the divergence: review.reviewScore is 18 while contentJudge.normalizedScore is 0 (the judge was skipped after deterministic validation failed), so the action would display 0% and spuriously fail the PR. Also handle reviewScore null/missing: treat as unavailable (score -1) rather than a hard 0. With threshold 0 it still passes; with threshold > 0 it cannot satisfy the threshold and is not passed, consistent with existing passed logic. Detailed review body still renders from contentJudge.evaluation (unchanged). --- src/main.test.ts | 64 ++++++++++++++++++++++++++++++++++++++------- src/skill-review.ts | 14 ++++++++-- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/main.test.ts b/src/main.test.ts index cf4243b..5b1ef41 100644 --- a/src/main.test.ts +++ b/src/main.test.ts @@ -254,6 +254,7 @@ describe('runSkillReview', () => { test('successful review with JSON output', async () => { const jsonOutput = JSON.stringify({ + review: { reviewScore: 85 }, contentJudge: { normalizedScore: 0.85, evaluation: 'Good skill definition.', @@ -324,33 +325,34 @@ describe('runSkillReview', () => { test('threshold pass/fail logic', async () => { const makeJson = (score: number) => JSON.stringify({ - contentJudge: { normalizedScore: score, evaluation: 'test' }, + review: { reviewScore: score }, + contentJudge: { evaluation: 'test' }, }); - // Score 60% with threshold 50 → passed + // Score 60 with threshold 50 → passed // @ts-expect-error mock assignment - Bun.spawn = makeMockSpawn(makeJson(0.6), '', 0); + Bun.spawn = makeMockSpawn(makeJson(60), '', 0); const passing = await runSkillReview('a/SKILL.md', 50); expect(passing.score).toBe(60); expect(passing.passed).toBe(true); - // Score 40% with threshold 50 → failed + // Score 40 with threshold 50 → failed // @ts-expect-error mock assignment - Bun.spawn = makeMockSpawn(makeJson(0.4), '', 0); + Bun.spawn = makeMockSpawn(makeJson(40), '', 0); const failing = await runSkillReview('b/SKILL.md', 50); expect(failing.score).toBe(40); expect(failing.passed).toBe(false); - // Score 50% with threshold 50 → passed (>= threshold) + // Score 50 with threshold 50 → passed (>= threshold) // @ts-expect-error mock assignment - Bun.spawn = makeMockSpawn(makeJson(0.5), '', 0); + Bun.spawn = makeMockSpawn(makeJson(50), '', 0); const boundary = await runSkillReview('c/SKILL.md', 50); expect(boundary.score).toBe(50); expect(boundary.passed).toBe(true); // Any score with threshold 0 → always passed // @ts-expect-error mock assignment - Bun.spawn = makeMockSpawn(makeJson(0.1), '', 0); + Bun.spawn = makeMockSpawn(makeJson(10), '', 0); const noThreshold = await runSkillReview('d/SKILL.md', 0); expect(noThreshold.score).toBe(10); expect(noThreshold.passed).toBe(true); @@ -388,7 +390,8 @@ describe('runSkillReview', () => { test('JSON with prefix and suffix text', async () => { const json = JSON.stringify({ - contentJudge: { normalizedScore: 0.72, evaluation: 'decent' }, + review: { reviewScore: 72 }, + contentJudge: { evaluation: 'decent' }, }); const stdout = `Running review...\n${json}\nDone.`; @@ -399,6 +402,49 @@ describe('runSkillReview', () => { expect(result.score).toBe(72); expect(result.passed).toBe(true); }); + + test('uses review.reviewScore, not contentJudge.normalizedScore', async () => { + // Golden sample shape: the CLI's canonical reviewScore is 18 while the + // contentJudge judge was skipped (normalizedScore 0). The action must + // report 18, not 0. + const jsonOutput = JSON.stringify({ + review: { reviewScore: 18 }, + validation: { overallPassed: false, errorCount: 1, output: 'failed' }, + contentJudge: { + success: true, + normalizedScore: 0, + evaluation: 'validation skipped', + }, + }); + + // @ts-expect-error mock assignment + Bun.spawn = makeMockSpawn(jsonOutput, '', 0); + + const result = await runSkillReview('a/SKILL.md', 0); + expect(result.score).toBe(18); + expect(result.passed).toBe(true); + }); + + test('reviewScore null → score unavailable', async () => { + const jsonOutput = JSON.stringify({ + review: { reviewScore: null }, + contentJudge: { evaluation: 'no score' }, + }); + + // threshold 0 → still passes despite unavailable score + // @ts-expect-error mock assignment + Bun.spawn = makeMockSpawn(jsonOutput, '', 0); + const passes = await runSkillReview('a/SKILL.md', 0); + expect(passes.score).toBe(-1); + expect(passes.passed).toBe(true); + + // threshold > 0 → cannot satisfy threshold, not passed + // @ts-expect-error mock assignment + Bun.spawn = makeMockSpawn(jsonOutput, '', 0); + const fails = await runSkillReview('b/SKILL.md', 50); + expect(fails.score).toBe(-1); + expect(fails.passed).toBe(false); + }); }); // --------------------------------------------------------------------------- diff --git a/src/skill-review.ts b/src/skill-review.ts index b4fe675..138c149 100644 --- a/src/skill-review.ts +++ b/src/skill-review.ts @@ -153,6 +153,7 @@ export async function runSkillReview( }; } let parsed: { + review?: { reviewScore?: number | null }; contentJudge?: { normalizedScore?: number; evaluation?: unknown }; validation?: { output?: unknown }; }; @@ -170,8 +171,15 @@ export async function runSkillReview( }; } - const normalizedScore = parsed.contentJudge?.normalizedScore ?? 0; - const score = Math.round(normalizedScore * 100); + // The CLI's canonical score is review.reviewScore (already 0-100, blending + // content + description judges). This is the value tessl uses for its own + // --threshold gate, so the action must derive from the same field rather + // than contentJudge.normalizedScore (which can disagree, e.g. golden sample: + // reviewScore 18 vs contentJudge.normalizedScore 0). + const reviewScore = parsed.review?.reviewScore; + const hasScore = typeof reviewScore === 'number'; + // -1 represents an unavailable/unscored result (see SkillReviewResult.score). + const score = hasScore ? Math.round(reviewScore) : -1; const outputParts: string[] = []; if (parsed.validation?.output) { @@ -185,6 +193,8 @@ export async function runSkillReview( ); } + // When the score is unavailable (-1, reviewScore null/missing), threshold 0 + // still passes; any positive threshold cannot be satisfied (-1 < threshold). return { skillPath: skillFilePath, passed: threshold === 0 || score >= threshold,