From 55abf982edc41461700252893d64b0c53963176f Mon Sep 17 00:00:00 2001 From: Rotimi Best Date: Mon, 29 Jun 2026 01:07:02 +0100 Subject: [PATCH] fix(question-types): realign registry ids with the question_type table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry hardcoded numeric ids that drifted from the database `question_type` table: MATCHING and HOTSPOT were inserted mid-list (ids 9 and 11) but never seeded, shifting ORDERING/LINK/WORD_BANK/ STAR/VIDEO_RECORDING one or two ids past their real rows. Every id emitter inherited the wrong values — the exercise editor, the AI question generator, and the grading/scoring gate — so e.g. an ORDERING question was stored as id 10 (LINK in the DB), came back as a manual type, and never auto-graded. - Move MATCHING/HOTSPOT to the end (ids 14/15) and flag them `disabled`, restoring the append-only id convention and realigning ORDERING=9, LINK=10, WORD_BANK=11, STAR=12, VIDEO_RECORDING=13 with the DB. - Add a registry test asserting every enabled type's id matches the canonical `question_type` rows and disabled types stay past the live range, so ids can't silently drift again. - Constrain the agent tool schema to enabled, DB-backed ids so the model can't emit a disabled id with no row. - Drop disabled types from the teacher prompt's type list and guidance. - Centralize the dashboard question-type picker on the `disabled` flag. This also fixes video-recording detection, which compared against the stale id 15 instead of the real 13. --- .../src/lib/features/ui/question/constants.ts | 19 ++-- packages/ai-assistant/src/prompt/teacher.ts | 14 +-- .../src/services/agent/agent-tool-schemas.ts | 26 +++--- .../src/question-type-registry.ts | 55 +++++++----- .../question-types/src/question-type-types.ts | 2 + .../tests/question-type-registry.test.ts | 87 ++++++++++++++----- 6 files changed, 131 insertions(+), 72 deletions(-) diff --git a/apps/dashboard/src/lib/features/ui/question/constants.ts b/apps/dashboard/src/lib/features/ui/question/constants.ts index 340eaaf2..94b68da3 100644 --- a/apps/dashboard/src/lib/features/ui/question/constants.ts +++ b/apps/dashboard/src/lib/features/ui/question/constants.ts @@ -23,18 +23,13 @@ const QUESTION_TYPE_LABEL_KEYS: Partial> = { 'course.navItem.lessons.exercises.all_exercises.edit_mode.question_types.video_recording' }; -// Temp hidden: MATCHING, HOTSPOT -const HIDDEN_QUESTION_TYPE_KEYS = new Set([QUESTION_TYPE_KEY.MATCHING, QUESTION_TYPE_KEY.HOTSPOT]); - -export const QUESTION_TYPES = QUESTION_TYPE_REGISTRY.filter((qt) => !HIDDEN_QUESTION_TYPE_KEYS.has(qt.key)).map( - (questionType) => ({ - id: questionType.id, - key: questionType.key, - label: questionType.label, - labelKey: QUESTION_TYPE_LABEL_KEYS[questionType.key], - autoGradable: questionType.autoGradable - }) -); +export const QUESTION_TYPES = QUESTION_TYPE_REGISTRY.filter((qt) => !qt.disabled).map((questionType) => ({ + id: questionType.id, + key: questionType.key, + label: questionType.label, + labelKey: QUESTION_TYPE_LABEL_KEYS[questionType.key], + autoGradable: questionType.autoGradable +})); export const DEFAULT_QUESTION_TYPE = QUESTION_TYPES.find( (questionType) => questionType.key === QUESTION_TYPE_KEY.RADIO diff --git a/packages/ai-assistant/src/prompt/teacher.ts b/packages/ai-assistant/src/prompt/teacher.ts index 9223b665..93276a5d 100644 --- a/packages/ai-assistant/src/prompt/teacher.ts +++ b/packages/ai-assistant/src/prompt/teacher.ts @@ -13,14 +13,18 @@ export type BuildTeacherSystemPromptOptions = { }; function buildQuestionTypeListBlock(isOrgOnPaidPlan: boolean): string { - const allowed = QUESTION_TYPE_REGISTRY.filter((t) => isOrgOnPaidPlan || !PREMIUM_QUESTION_TYPE_KEYS.has(t.key)); + // `disabled` types are built but have no `question_type` DB row, so they must + // never be advertised to the model (the tool schema rejects their ids). + const offered = QUESTION_TYPE_REGISTRY.filter((t) => !t.disabled); + const allowed = offered.filter((t) => isOrgOnPaidPlan || !PREMIUM_QUESTION_TYPE_KEYS.has(t.key)); const listing = allowed.map((t) => `- ${t.id} = ${t.typename} — ${t.label}`).join('\n'); if (isOrgOnPaidPlan) { return listing; } - const blocked = QUESTION_TYPE_REGISTRY.filter((t) => PREMIUM_QUESTION_TYPE_KEYS.has(t.key)) + const blocked = offered + .filter((t) => PREMIUM_QUESTION_TYPE_KEYS.has(t.key)) .map((t) => t.typename) .join(', '); @@ -59,15 +63,13 @@ Every question you pass to \`create_exercise\` or \`add_questions\` MUST include - **SHORT_ANSWER** — recalling a specific term, command, value, or short phrase. Use when there's a small set of right answers you can list. - **NUMERIC** — anything quantitative: math, calculations, counts, sizes, rates. Default for any "how many / what value / calculate X" question. - **FILL_BLANK** — testing syntax, sequence completion, or code/sentence patterns where position matters ("\`SELECT \_\_ FROM users\`"). -- **MATCHING** — pairing related concepts (term ↔ definition, problem ↔ solution, tool ↔ purpose). Use whenever you'd otherwise write multiple RADIO questions all asking "what is the definition of X?". - **ORDERING** — sequencing steps, ranking by criterion, timeline reconstruction. Use for any "what's the right order" question instead of RADIO with shuffled options. - **WORD_BANK** — vocab application, classification, filling multiple blanks from a shared pool. Stronger than RADIO when the same concept set applies to many slots. - **TEXTAREA** — open-ended explanation, reflection, written analysis. Use sparingly (manual grading). -- **HOTSPOT** — identifying a region of an image (UI element, anatomy, map). Use when there's a visual the lesson already shows. - **CHECKBOX (multi-select)** — when the question genuinely has multiple correct answers a learner should identify together. Don't use CHECKBOX as a "harder RADIO" — only when "select all that apply" is the natural verb. - **RADIO (single answer)** — concept-level discrimination between mutually exclusive options. Useful but the most overused — when in doubt, ask if one of the types above fits better. -**Anti-pattern to avoid:** an exercise of 8 questions that's 7 RADIO + 1 TRUE_FALSE. That's a "different types" technicality, not real variety. The mix must reflect the *content*: numeric questions get NUMERIC, ordering questions get ORDERING, definition-matching gets MATCHING, etc. +**Anti-pattern to avoid:** an exercise of 8 questions that's 7 RADIO + 1 TRUE_FALSE. That's a "different types" technicality, not real variety. The mix must reflect the *content*: numeric questions get NUMERIC, ordering questions get ORDERING, fill-in patterns get FILL_BLANK, etc. ## Plan Mode vs Agent Mode @@ -339,7 +341,7 @@ When you create an exercise (especially during plan implementation), it must act - For NUMERIC / STAR / WORD_BANK: still ensure the answer is unambiguously derivable from the lesson. ### Per-exercise structure -- Vary question types in the same exercise — minimum 2 distinct \`questionTypeId\` values for any 3–5 question exercise, minimum 4 for 6+ questions, with no single type exceeding half the exercise. Pick the type that matches the cognitive skill (see "Question Types" above): numeric questions get NUMERIC, ordering questions get ORDERING, definition-matching gets MATCHING — do not paper over a missing fit by defaulting to RADIO. +- Vary question types in the same exercise — minimum 2 distinct \`questionTypeId\` values for any 3–5 question exercise, minimum 4 for 6+ questions, with no single type exceeding half the exercise. Pick the type that matches the cognitive skill (see "Question Types" above): numeric questions get NUMERIC, ordering questions get ORDERING, fill-in patterns get FILL_BLANK — do not paper over a missing fit by defaulting to RADIO. - Set non-zero \`points\` per question (default 1; harder questions can be 2). ### Comprehensive final examination (when implementing a full course plan) diff --git a/packages/core/src/services/agent/agent-tool-schemas.ts b/packages/core/src/services/agent/agent-tool-schemas.ts index f5923999..a297f8a2 100644 --- a/packages/core/src/services/agent/agent-tool-schemas.ts +++ b/packages/core/src/services/agent/agent-tool-schemas.ts @@ -5,11 +5,22 @@ import { LANDING_PAGE_METADATA_DESCRIPTION_SECTION_HINT, LANDING_PAGE_SECTION_HTML_AGENT_HINT } from '@cio/ai-assistant/tools'; -import { QUESTION_TYPE_REGISTRY } from '@cio/question-types'; +import { ENABLED_QUESTION_TYPE_REGISTRY } from '@cio/question-types'; import { ZExerciseSectionAfterBehavior } from '@cio/utils/validation/exercise'; import { ZCourseLandingPageUpdate, ZCourseLandingPageMetadataUpdateFields } from '@cio/utils/validation/course'; // courseId is NOT a parameter — it's injected from the authenticated request context. + +// Only question types backed by a `question_type` DB row may be generated. +// Disabled types (e.g. MATCHING/HOTSPOT) have no row and would fail the FK, so +// the model is constrained to the enabled ids rather than the whole registry. +const ENABLED_QUESTION_TYPE_IDS = ENABLED_QUESTION_TYPE_REGISTRY.map((type) => type.id); +const zEnabledQuestionTypeId = z + .number() + .int() + .refine((id) => ENABLED_QUESTION_TYPE_IDS.includes(id), { + message: `questionTypeId must be one of: ${ENABLED_QUESTION_TYPE_IDS.join(', ')}` + }); // This prevents prompt injection from tricking the LLM into targeting another course. export const emptyParam = z.object({}); @@ -63,14 +74,9 @@ export const updateContentParam = z.object({ export const questionSchema = z.object({ question: z.string().min(1), - questionTypeId: z - .number() - .int() - .min(1) - .max(QUESTION_TYPE_REGISTRY.length) - .describe( - 'Required. Use the numeric question type IDs from the teacher system prompt (Question Types). Omitting this field is invalid — set an explicit type on every question and vary types within each exercise.' - ), + questionTypeId: zEnabledQuestionTypeId.describe( + 'Required. Use the numeric question type IDs from the teacher system prompt (Question Types). Omitting this field is invalid — set an explicit type on every question and vary types within each exercise.' + ), points: z.number().min(0).default(1), order: z.number().int().min(0), options: z.array(z.object({ label: z.string().min(1), isCorrect: z.boolean() })) @@ -193,7 +199,7 @@ export const updateQuestionPatchSchema = z .object({ id: z.number().int(), question: z.string().min(1).optional(), - questionTypeId: z.number().int().min(1).max(QUESTION_TYPE_REGISTRY.length).optional(), + questionTypeId: zEnabledQuestionTypeId.optional(), points: z.number().min(0).optional(), order: z.number().int().min(0).optional(), exerciseSectionId: z diff --git a/packages/question-types/src/question-type-registry.ts b/packages/question-types/src/question-type-registry.ts index e82b0fae..dee9b167 100644 --- a/packages/question-types/src/question-type-registry.ts +++ b/packages/question-types/src/question-type-registry.ts @@ -74,29 +74,11 @@ export const QUESTION_TYPE_REGISTRY: readonly QuestionTypeMetadata[] = [ supportsPartialCredit: false, manualGradingRequired: true }, - { - key: QUESTION_TYPE_KEY.MATCHING, - typename: 'MATCHING', - label: 'Matching', - id: 9, - autoGradable: true, - supportsPartialCredit: true, - manualGradingRequired: false - }, { key: QUESTION_TYPE_KEY.ORDERING, typename: 'ORDERING', label: 'Ordering', - id: 10, - autoGradable: true, - supportsPartialCredit: true, - manualGradingRequired: false - }, - { - key: QUESTION_TYPE_KEY.HOTSPOT, - typename: 'HOTSPOT', - label: 'Hotspot', - id: 11, + id: 9, autoGradable: true, supportsPartialCredit: true, manualGradingRequired: false @@ -105,7 +87,7 @@ export const QUESTION_TYPE_REGISTRY: readonly QuestionTypeMetadata[] = [ key: QUESTION_TYPE_KEY.LINK, typename: 'LINK', label: 'Links', - id: 12, + id: 10, autoGradable: false, supportsPartialCredit: false, manualGradingRequired: true @@ -114,7 +96,7 @@ export const QUESTION_TYPE_REGISTRY: readonly QuestionTypeMetadata[] = [ key: QUESTION_TYPE_KEY.WORD_BANK, typename: 'WORD_BANK', label: 'Word bank', - id: 13, + id: 11, autoGradable: true, supportsPartialCredit: true, manualGradingRequired: false @@ -123,7 +105,7 @@ export const QUESTION_TYPE_REGISTRY: readonly QuestionTypeMetadata[] = [ key: QUESTION_TYPE_KEY.STAR, typename: 'STAR', label: 'Star rating', - id: 14, + id: 12, autoGradable: true, supportsPartialCredit: false, manualGradingRequired: false @@ -132,13 +114,40 @@ export const QUESTION_TYPE_REGISTRY: readonly QuestionTypeMetadata[] = [ key: QUESTION_TYPE_KEY.VIDEO_RECORDING, typename: 'VIDEO_RECORDING', label: 'Video recording', - id: 15, + id: 13, autoGradable: false, supportsPartialCredit: false, manualGradingRequired: true + }, + // Built but not offered in the product. Kept at the END of the id sequence + // (append-only) so they never shift the ids of types that exist in the DB. + // They have no `question_type` row, so `disabled` keeps them out of pickers, + // the AI tool enum, and validation. + { + key: QUESTION_TYPE_KEY.MATCHING, + typename: 'MATCHING', + label: 'Matching', + id: 14, + autoGradable: true, + supportsPartialCredit: true, + manualGradingRequired: false, + disabled: true + }, + { + key: QUESTION_TYPE_KEY.HOTSPOT, + typename: 'HOTSPOT', + label: 'Hotspot', + id: 15, + autoGradable: true, + supportsPartialCredit: true, + manualGradingRequired: false, + disabled: true } ] as const; +/** Types actually offered in the product (excludes `disabled` ones with no DB row). */ +export const ENABLED_QUESTION_TYPE_REGISTRY = QUESTION_TYPE_REGISTRY.filter((type) => !type.disabled); + export const QUESTION_TYPE_BY_KEY = Object.freeze( QUESTION_TYPE_REGISTRY.reduce( (acc, type) => { diff --git a/packages/question-types/src/question-type-types.ts b/packages/question-types/src/question-type-types.ts index 75b02f9a..90cd3196 100644 --- a/packages/question-types/src/question-type-types.ts +++ b/packages/question-types/src/question-type-types.ts @@ -8,4 +8,6 @@ export interface QuestionTypeMetadata { autoGradable: boolean; supportsPartialCredit: boolean; manualGradingRequired: boolean; + /** Built in code but not offered in the product (no `question_type` row). */ + disabled?: boolean; } diff --git a/packages/question-types/tests/question-type-registry.test.ts b/packages/question-types/tests/question-type-registry.test.ts index 1b50c6a7..f4e4c707 100644 --- a/packages/question-types/tests/question-type-registry.test.ts +++ b/packages/question-types/tests/question-type-registry.test.ts @@ -82,26 +82,10 @@ const EXPECTED_REGISTRY = [ supportsPartialCredit: false, manualGradingRequired: true }, - { - key: QUESTION_TYPE_KEY.MATCHING, - typename: 'MATCHING', - id: 9, - autoGradable: true, - supportsPartialCredit: true, - manualGradingRequired: false - }, { key: QUESTION_TYPE_KEY.ORDERING, typename: 'ORDERING', - id: 10, - autoGradable: true, - supportsPartialCredit: true, - manualGradingRequired: false - }, - { - key: QUESTION_TYPE_KEY.HOTSPOT, - typename: 'HOTSPOT', - id: 11, + id: 9, autoGradable: true, supportsPartialCredit: true, manualGradingRequired: false @@ -109,7 +93,7 @@ const EXPECTED_REGISTRY = [ { key: QUESTION_TYPE_KEY.LINK, typename: 'LINK', - id: 12, + id: 10, autoGradable: false, supportsPartialCredit: false, manualGradingRequired: true @@ -117,7 +101,7 @@ const EXPECTED_REGISTRY = [ { key: QUESTION_TYPE_KEY.WORD_BANK, typename: 'WORD_BANK', - id: 13, + id: 11, autoGradable: true, supportsPartialCredit: true, manualGradingRequired: false @@ -125,7 +109,7 @@ const EXPECTED_REGISTRY = [ { key: QUESTION_TYPE_KEY.STAR, typename: 'STAR', - id: 14, + id: 12, autoGradable: true, supportsPartialCredit: false, manualGradingRequired: false @@ -133,13 +117,51 @@ const EXPECTED_REGISTRY = [ { key: QUESTION_TYPE_KEY.VIDEO_RECORDING, typename: 'VIDEO_RECORDING', - id: 15, + id: 13, autoGradable: false, supportsPartialCredit: false, manualGradingRequired: true + }, + { + key: QUESTION_TYPE_KEY.MATCHING, + typename: 'MATCHING', + id: 14, + autoGradable: true, + supportsPartialCredit: true, + manualGradingRequired: false, + disabled: true + }, + { + key: QUESTION_TYPE_KEY.HOTSPOT, + typename: 'HOTSPOT', + id: 15, + autoGradable: true, + supportsPartialCredit: true, + manualGradingRequired: false, + disabled: true } ] as const; +// Canonical `question_type` rows in the database (the source of truth ids are +// resolved against). Ids 1-13 are the live rows in insertion order; MATCHING and +// HOTSPOT are intentionally absent (built-but-disabled). This list guards against +// the registry id ever drifting from the DB again. +const CANONICAL_DB_QUESTION_TYPES: ReadonlyArray<{ id: number; typename: string }> = [ + { id: 1, typename: 'RADIO' }, + { id: 2, typename: 'CHECKBOX' }, + { id: 3, typename: 'TEXTAREA' }, + { id: 4, typename: 'TRUE_FALSE' }, + { id: 5, typename: 'SHORT_ANSWER' }, + { id: 6, typename: 'NUMERIC' }, + { id: 7, typename: 'FILL_BLANK' }, + { id: 8, typename: 'FILE_UPLOAD' }, + { id: 9, typename: 'ORDERING' }, + { id: 10, typename: 'LINK' }, + { id: 11, typename: 'WORD_BANK' }, + { id: 12, typename: 'STAR' }, + { id: 13, typename: 'VIDEO_RECORDING' } +]; + describe('question type registry', () => { it('contains exactly the supported question type keys', () => { expect(new Set(QUESTION_TYPE_REGISTRY.map((entry) => entry.key))).toEqual( @@ -176,6 +198,29 @@ describe('question type registry', () => { } }); + it('registry ids match the canonical question_type DB rows (no drift)', () => { + for (const row of CANONICAL_DB_QUESTION_TYPES) { + const key = QUESTION_TYPE_TYPENAME_TO_KEY[row.typename]; + expect(key, `typename ${row.typename} missing from registry`).toBeDefined(); + expect(QUESTION_TYPE_IDS[key], `id mismatch for ${row.typename}`).toBe(row.id); + } + }); + + it('keeps disabled types appended after every DB-backed type', () => { + const dbIds = new Set(CANONICAL_DB_QUESTION_TYPES.map((row) => row.id)); + const maxDbId = Math.max(...CANONICAL_DB_QUESTION_TYPES.map((row) => row.id)); + + for (const entry of QUESTION_TYPE_REGISTRY) { + if (entry.disabled) { + // Disabled types have no DB row and must sit past the live id range. + expect(dbIds.has(entry.id), `disabled ${entry.typename} must not reuse a DB id`).toBe(false); + expect(entry.id).toBeGreaterThan(maxDbId); + } else { + expect(dbIds.has(entry.id), `enabled ${entry.typename} must map to a DB row`).toBe(true); + } + } + }); + it('returns undefined for unknown typename and id lookups', () => { expect(getQuestionTypeByTypename('UNKNOWN')).toBeUndefined(); expect(getQuestionTypeById(999)).toBeUndefined();