Merge pull request 'fix: stop warning about merged themes in curriculum schedule' (#13) from fix/issue-12-curriculum-merge-warning into main

Reviewed-on: #13

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

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
+  // Theme coverage — only warn when merging wasn't required. When themes_kb > 26
-  // When there are more themes than 26 weeks, the AI must merge some.
+  // the AI is *required* to merge, so absent theme labels are expected.
-  const missingThemes = [];
+  if (validThemes.size <= 26) {
-  for (const t of validThemes) {
+    const missingThemes = [];
-    if (!scheduledThemes.has(t)) {
+    for (const t of validThemes) {
-      missingThemes.push(t);
+      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);
   }