From 2274de4de7db6d3178a02be75937de8f979866a0 Mon Sep 17 00:00:00 2001 From: RaymondVerhoef Date: Wed, 3 Jun 2026 22:05:34 +0200 Subject: [PATCH] fix: stop warning about merged themes in curriculum schedule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the KB has more than 26 themes the curriculum AI is required to merge — so theme labels not appearing as week names are expected, not warnings. The previous validation surfaced this as a console.warn that read like an error in the learning station console. - validateSchedule now only flags missing theme labels when themes_kb <= 26 - adds a real coverage check: warns when learning topics are absent from every week (the actual signal we care about) - adds vitest coverage for both behaviours Closes #12 Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/__tests__/curriculumService.test.js | 97 +++++++++++++++++++++ src/lib/curriculumService.js | 49 ++++++++--- 2 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 src/lib/__tests__/curriculumService.test.js diff --git a/src/lib/__tests__/curriculumService.test.js b/src/lib/__tests__/curriculumService.test.js new file mode 100644 index 0000000..fb22d77 --- /dev/null +++ b/src/lib/__tests__/curriculumService.test.js @@ -0,0 +1,97 @@ +import { describe, expect, it } from 'vitest'; +import { validateSchedule } from '../curriculumService'; + +const week = (n, theme, topicIds, duration = 30) => ({ + week_number: n, + theme, + topic_ids: topicIds, + estimated_duration: duration, + week_rationale: 'r', +}); + +const makeTopic = (id, theme) => ({ + id, + theme, + type: 'concept', + learning_relevance: 'include', +}); + +const buildScheduleFromTopics = (topics) => { + const ids = topics.map(t => t.id); + return Array.from({ length: 26 }, (_, i) => { + const chunk = ids.slice(i * 2, i * 2 + 2); + return week(i + 1, topics[i * 2]?.theme || 'General', chunk.length ? chunk : [ids[0]]); + }); +}; + +describe('validateSchedule', () => { + it('does not warn about missing themes when merging was required (themes_kb > 26)', () => { + const topics = []; + for (let i = 0; i < 60; i++) topics.push(makeTopic(`t${i}`, `Theme ${i % 30}`)); + const schedule = buildScheduleFromTopics(topics); + + const result = validateSchedule(schedule, topics); + expect(result.valid).toBe(true); + const themeWarning = result.warnings.find(w => w.includes('not scheduled')); + expect(themeWarning).toBeUndefined(); + }); + + it('warns about missing themes only when merging was not required', () => { + const topics = []; + for (let i = 0; i < 10; i++) topics.push(makeTopic(`t${i}`, `Theme ${i}`)); + const schedule = Array.from({ length: 26 }, (_, i) => + week(i + 1, 'Theme 0', ['t0']) + ); + + const result = validateSchedule(schedule, topics); + const themeWarning = result.warnings.find(w => w.includes('not scheduled')); + expect(themeWarning).toBeDefined(); + expect(themeWarning).toMatch(/9 theme/); + }); + + it('warns when learning topics are absent from every week (real coverage gap)', () => { + const topics = [ + makeTopic('t1', 'A'), + makeTopic('t2', 'A'), + makeTopic('t3', 'B'), + ]; + const schedule = Array.from({ length: 26 }, (_, i) => + week(i + 1, 'A', ['t1']) + ); + + const result = validateSchedule(schedule, topics); + const coverageWarning = result.warnings.find(w => w.includes('not covered')); + expect(coverageWarning).toBeDefined(); + expect(coverageWarning).toMatch(/2 learning topic/); + }); + + it('ignores topics with type=fact or learning_relevance=exclude in coverage', () => { + const topics = [ + makeTopic('t1', 'A'), + { ...makeTopic('t2', 'A'), type: 'fact' }, + { ...makeTopic('t3', 'B'), learning_relevance: 'exclude' }, + ]; + const schedule = Array.from({ length: 26 }, (_, i) => + week(i + 1, 'A', ['t1']) + ); + + const result = validateSchedule(schedule, topics); + expect(result.warnings.find(w => w.includes('not covered'))).toBeUndefined(); + }); + + it('flags hard errors: wrong week count, bad duration, unknown topic_id', () => { + const topics = [makeTopic('t1', 'A')]; + const schedule = [week(1, 'A', ['t1'])]; + const result = validateSchedule(schedule, topics); + expect(result.valid).toBe(false); + expect(result.errors[0]).toMatch(/exactly 26 weeks/); + + const fullSchedule = Array.from({ length: 26 }, (_, i) => week(i + 1, 'A', ['t1'])); + fullSchedule[2].estimated_duration = 5; + fullSchedule[5].topic_ids = ['missing-id']; + const result2 = validateSchedule(fullSchedule, topics); + expect(result2.valid).toBe(false); + expect(result2.errors.some(e => e.includes('out-of-range duration'))).toBe(true); + expect(result2.errors.some(e => e.includes('unknown topic_id'))).toBe(true); + }); +}); diff --git a/src/lib/curriculumService.js b/src/lib/curriculumService.js index 02c1387..990e602 100644 --- a/src/lib/curriculumService.js +++ b/src/lib/curriculumService.js @@ -60,8 +60,16 @@ export function buildThemeTopicMap(topics) { /** * Validates a 26-week schedule against the provided topics. - * Checks for exactly 26 weeks, duration range, theme existence, and topic existence. - * Returns { valid: boolean, errors: string[] } + * + * Warning policy: + * - Theme names not appearing as a week label are NOT a warning when the KB + * has more than 26 themes — the AI is required to merge in that case, and + * topic_ids from the merged themes are carried through under the chosen + * week label. We only warn about missing themes when merging wasn't needed. + * - Real coverage is measured on TOPICS: a topic that exists in the KB but + * is absent from every week's topic_ids is a genuine gap and gets a warning. + * + * Returns { valid: boolean, errors: string[], warnings: string[] } */ export function validateSchedule(schedule, topics) { const errors = []; // Hard errors — schedule is unusable @@ -71,10 +79,13 @@ export function validateSchedule(schedule, topics) { errors.push(`Schedule must contain exactly 26 weeks. Found ${schedule?.length || 0}.`); } - const validThemes = new Set(topics.filter(t => t.type !== 'fact' && t.learning_relevance !== 'exclude').map(t => t.theme || 'General')); + const learningTopics = topics.filter(t => t.type !== 'fact' && t.learning_relevance !== 'exclude'); + const validThemes = new Set(learningTopics.map(t => t.theme || 'General')); const validTopicIds = new Set(topics.map(t => t.id)); + const learningTopicIds = new Set(learningTopics.map(t => t.id)); const scheduledThemes = new Set(); + const scheduledTopicIds = new Set(); for (let i = 0; i < (schedule || []).length; i++) { const week = schedule[i]; @@ -86,7 +97,7 @@ export function validateSchedule(schedule, topics) { } // Allow AI-merged theme names — only flag truly unknown themes as warnings scheduledThemes.add(week.theme); - + if (!week.topic_ids || week.topic_ids.length === 0) { errors.push(`Week ${week.week_number} has no topic_ids.`); } else { @@ -94,20 +105,30 @@ export function validateSchedule(schedule, topics) { if (!validTopicIds.has(tId)) { errors.push(`Week ${week.week_number} references unknown topic_id: ${tId}`); } + scheduledTopicIds.add(tId); } } } - // Theme coverage — soft warnings, not hard errors - // When there are more themes than 26 weeks, the AI must merge some. - const missingThemes = []; - for (const t of validThemes) { - if (!scheduledThemes.has(t)) { - missingThemes.push(t); + // Theme coverage — only warn when merging wasn't required. When themes_kb > 26 + // the AI is *required* to merge, so absent theme labels are expected. + if (validThemes.size <= 26) { + const missingThemes = []; + for (const t of validThemes) { + if (!scheduledThemes.has(t)) { + missingThemes.push(t); + } + } + if (missingThemes.length > 0) { + warnings.push(`${missingThemes.length} theme(s) not scheduled: ${missingThemes.join(', ')}`); } } - if (missingThemes.length > 0) { - warnings.push(`${missingThemes.length} theme(s) not directly scheduled (topics may appear under merged themes): ${missingThemes.join(', ')}`); + + // Topic coverage — the real signal. Topics carried under a merged theme are + // still covered; topics absent from every week are not. + const missingTopicCount = [...learningTopicIds].filter(id => !scheduledTopicIds.has(id)).length; + if (missingTopicCount > 0) { + warnings.push(`${missingTopicCount} learning topic(s) not covered by any week of the schedule.`); } return { valid: errors.length === 0, errors, warnings }; @@ -224,7 +245,9 @@ Rules: throw new Error(`Generated schedule failed validation after retry:\n- ${validationResult.errors.join('\n- ')}`); } - // Log warnings but don't fail + // Log warnings but don't fail. With themes_kb > 26 the AI must merge themes, + // so most "missing theme" noise is filtered upstream in validateSchedule — + // anything that lands here is a genuine coverage gap worth surfacing. if (validationResult.warnings.length > 0) { console.warn('[Curriculum] Schedule generated with warnings:', validationResult.warnings); } -- 2.49.1