diff --git a/tests_end_to_end/coverage/taxonomy.yaml b/tests_end_to_end/coverage/taxonomy.yaml index 9b384fb824a..30d1494cbd0 100644 --- a/tests_end_to_end/coverage/taxonomy.yaml +++ b/tests_end_to_end/coverage/taxonomy.yaml @@ -648,18 +648,36 @@ areas: - online-evaluation/online-evaluation-sampling-rate.spec.ts - online-evaluation/online-evaluation-python-metric-errors.spec.ts - online-evaluation/online-evaluation-non-object-sections.spec.ts + - online-evaluation/online-evaluation-json-looking-judge-prompts.spec.ts + - online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts capabilities: create-llm-judge-rule: { covered: true, tier: t1-smoke } llm-judge-scores: { covered: true, tier: t1-smoke, note: "bimodal safe/unsafe" } create-python-rule: { covered: true, tier: t1-smoke } python-rule-scores: { covered: true, tier: t1-smoke, note: "deterministic 3x1.0 / 2x0.0; t2 also covers sub-path mappings over non-object sections and the 400-class classification of a metric that exits 0 without a result line" } scores-in-trace-panel: { covered: true, tier: t1-smoke } - list-rules: { covered: false } + list-rules: { covered: true, tier: t2-cuj, note: "project-scoped and workspace-wide listing over a project holding six bracket-opening judge prompts, plus the rules page rendering a row for each" } rule-scope-thread-span: { covered: false, note: "span/thread scope flags" } rule-filters: { covered: false } sampling-rate: { covered: true, tier: t2-cuj, note: "50% rule vs 100% control over one 30-trace batch, binomial band 15-85%; plus a 0%-rate rule at trigger_scope=both, which must skip every SDK trace and still score experiment/playground/optimization ones" } clone-rule: { covered: false } - edit-rule: { covered: false } + # Scoped, and the two halves are not equally scoped: + # - plain-string prompt: the edit dialog IS opened, its hydrated prompt + # asserted byte-exact, and submitted unchanged, so the UI save round + # trip is real. + # - content_array prompt: API-level only. The dialog renders no image + # part, so the url/detail that a regression would drop are not + # observable in the UI; that half is a read-then-write over REST and + # no dialog gesture is made. + # Editing a FIELD through the dialog (renaming, changing the model, + # rewriting the prompt) is still uncovered either way. + # + # OPEN, needs a human call: the content_array test asserts judge-message + # content-SHAPE preservation, which no capability in this area names. It + # rides on edit-rule for want of a better key. Either add a capability for + # it or drop that tag and leave the behaviour honestly uncovered — do not + # let it sit here as a silent widening of edit-rule. + edit-rule: { covered: true, tier: t2-cuj, note: "plain-string prompt: edit-dialog hydration + no-op re-save, byte-for-byte; content_array prompt: API-only read-then-write re-save, no dialog gesture; editing a field through the dialog is not covered" } enable-disable-rule: { covered: true, tier: t2-cuj, note: "edit-dialog switch; control rule proves scoring stopped, then resumed" } delete-rule: { covered: true, tier: t2-cuj, note: "row kebab delete; control rule proves scoring stopped" } # Deliberately still false. online-evaluation-python-metric-errors.spec.ts diff --git a/tests_end_to_end/e2e/core/backend/client.ts b/tests_end_to_end/e2e/core/backend/client.ts index f5fd6663e11..7d23198442d 100644 --- a/tests_end_to_end/e2e/core/backend/client.ts +++ b/tests_end_to_end/e2e/core/backend/client.ts @@ -187,6 +187,61 @@ export interface AutomationRuleLogRef { message: string; } +/** + * One content part of a structured (multimodal) judge message. + * + * Only the fields the specs assert on are modelled; `video_url` / `audio_url` + * exist on the wire too and are left off deliberately rather than typed and + * ignored. + */ +export interface JudgeMessageContentPartRef { + type: string; + text: string | null; + imageUrl: { url: string; detail: string | null } | null; +} + +/** + * One judge message exactly as `GET /automations/evaluators/{id}` returns it. + * + * `content` and `contentArray` are the API's own two mutually-exclusive shapes + * (`LlmAsJudgeMessage`), and telling them apart is the whole point of the specs + * that use this: prose must come back as `content`, a genuine multimodal array + * must come back as `contentArray`. They are therefore kept as the nullable + * union the server sends rather than collapsed into one "text" field, which + * would erase the distinction under test. + */ +export interface JudgeMessageRef { + role: string; + content: string | null; + contentArray: JudgeMessageContentPartRef[] | null; +} + +/** One judge message as the create endpoint accepts it — set exactly one of the two. */ +export interface JudgeMessageWrite { + role: 'SYSTEM' | 'USER'; + content?: string; + contentArray?: Array<{ + type: string; + text?: string; + image_url?: { url: string; detail?: string }; + }>; +} + +/** + * One page of `GET /automations/evaluators/`, status included rather than thrown. + * + * The listing answering 200 at all is the assertion in + * `online-evaluation-json-looking-judge-prompts.spec.ts` — the regression it + * guards turned the whole page into a 500 — so the status has to be a value the + * spec can compare, not an exception the client raises on its way out. + */ +export interface AutomationRuleEvaluatorPageRef { + status: number; + /** Server-reported total for the query, not the length of this page. */ + total: number; + names: string[]; +} + /** * A trace `input`/`output`/`metadata` payload as the REST API accepts it. * @@ -1546,6 +1601,228 @@ export function makeBackendClient(apiKey: string | null = null, workspaceName: s return id; }, + /** + * Create an LLM-as-judge rule and return its id. + * + * Separate from `createAutomationRule` (which only builds + * `user_defined_metric_python` rules) because the judge shape is what these + * specs are about: the message `content` / `content_array` split, which the + * python shape has no equivalent of. + * + * `rawFetch` again, for the same two reasons as the python creator: the + * pinned SDK has no `trigger_scope`, and creation answers 201 with an empty + * body so the id only exists in the `Location` header. + * + * No provider key is required — the backend validates neither the model name + * nor its availability at create time, so a rule can be seeded on a + * workspace with no LLM provider configured. It simply never scores, which + * is what the listing / read-back specs want. + */ + async createLlmJudgeRule(args: { + projectId: string; + name: string; + messages: JudgeMessageWrite[]; + /** Fraction in [0, 1], the backend's own units — not the dialog's percentage. */ + samplingRate?: number; + /** Provider model identifier, e.g. `gpt-4o`. */ + model?: string; + /** `score()` variable name -> extraction path (e.g. `output.output`). */ + variables?: Record; + /** + * Name of the single output-schema entry. Deliberately NOT defaulted to + * the rule name: rule names carry the run namespace and can approach the + * 150-char column bound, while a score name is a short human label, and + * the edit dialog renders it as one. + */ + scoreName?: string; + enabled?: boolean; + }): Promise { + const scoreName = args.scoreName ?? 'Accuracy'; + const { status, message, location } = await rawFetch( + 'POST', + '/v1/private/automations/evaluators/', + { + body: { + type: 'llm_as_judge', + action: 'evaluator', + name: args.name, + project_ids: [args.projectId], + sampling_rate: args.samplingRate ?? 1, + enabled: args.enabled ?? true, + code: { + model: { name: args.model ?? 'gpt-4o', temperature: 0 }, + messages: args.messages.map((m) => ({ + role: m.role, + ...(m.content === undefined ? {} : { content: m.content }), + ...(m.contentArray === undefined ? {} : { content_array: m.contentArray }), + })), + variables: args.variables ?? { output: 'output.output' }, + schema: [ + { + name: scoreName, + type: 'INTEGER', + description: 'Score assigned by the judge.', + }, + ], + }, + }, + }, + ); + if (status !== 201) { + throw new Error( + `createLlmJudgeRule: expected 201 for '${args.name}', got ${status}: ${message}`, + ); + } + const id = location?.split('/').filter(Boolean).pop(); + if (!id) { + throw new Error( + `createLlmJudgeRule: 201 for '${args.name}' carried no usable Location header ` + + `(got '${location}') — cannot address the rule.`, + ); + } + return id; + }, + + /** + * One page of the evaluators listing, reporting the HTTP status rather than + * throwing on it. + * + * `projectId` omitted issues the workspace-wide listing — the same read the + * online-scoring sampler's `findAll()` performs, and the one that a single + * unreadable rule used to take down for every project at once. + * + * `size` defaults to 100 rather than the endpoint's own 10: a spec that + * seeds n rules and then reads a silently-truncated first page would assert + * against a subset without noticing. + */ + async findAutomationRuleEvaluatorsPage( + opts: { projectId?: string; page?: number; size?: number } = {}, + ): Promise { + const query = new URLSearchParams(); + if (opts.projectId) query.set('project_id', opts.projectId); + query.set('page', String(opts.page ?? 1)); + query.set('size', String(opts.size ?? 100)); + + const { status, json } = await rawFetch( + 'GET', + '/v1/private/automations/evaluators/', + { query }, + ); + // A non-200 is a legitimate result here, not an error to translate: the + // caller asserts on it. Only the shape of a 200 is trusted. + if (status !== 200) return { status, total: 0, names: [] }; + + const page = json as { total?: number; content?: Array<{ name?: string }> }; + const content = page.content ?? []; + if (typeof page.total !== 'number') { + throw new Error( + `findAutomationRuleEvaluatorsPage: 200 response carried no 'total' — ` + + `cannot assert the listing is complete.`, + ); + } + return { + status, + total: page.total, + names: content.map((r) => String(r.name ?? '')), + }; + }, + + /** + * The judge messages of one rule, as the read-back mapper produces them. + * + * This is the surface OPIK-8250 broke: the mapper infers the stored shape + * from the content string, so a prose prompt that happens to open with `[` + * has to come back as `content`, and a genuine multimodal array has to come + * back as `contentArray`. Both fields are surfaced verbatim so a spec can + * assert which one the server chose. + */ + async getLlmJudgeMessages(ruleId: string): Promise { + const { status, message, json } = await rawFetch( + 'GET', + `/v1/private/automations/evaluators/${ruleId}`, + ); + if (status !== 200) { + throw new Error(`getLlmJudgeMessages: ${ruleId} answered ${status}: ${message}`); + } + const rule = json as { code?: { messages?: unknown } }; + const messages = rule.code?.messages; + if (!Array.isArray(messages)) { + throw new Error( + `getLlmJudgeMessages: ${ruleId} returned no code.messages — not a judge rule?`, + ); + } + return messages.map((raw) => { + const m = raw as { + role?: string; + content?: string | null; + content_array?: Array<{ + type?: string; + text?: string | null; + image_url?: { url?: string; detail?: string | null } | null; + }> | null; + }; + return { + role: String(m.role ?? ''), + content: m.content ?? null, + contentArray: + m.content_array?.map((part) => ({ + type: String(part.type ?? ''), + text: part.text ?? null, + imageUrl: part.image_url + ? { url: String(part.image_url.url ?? ''), detail: part.image_url.detail ?? null } + : null, + })) ?? null, + }; + }); + }, + + /** + * Read a rule and write back exactly what was read — the edit dialog's own + * save shape, with nothing edited. + * + * This is the round trip that turned OPIK-8250's read bug into permanent + * data loss: a truncated read fed straight back into a save persisted the + * truncation, so the prompt the user typed was gone even after the read was + * fixed. Echoing the server's own `code` verbatim is the point — building a + * fresh payload here would test this client's serializer instead. + */ + async resaveAutomationRuleFromReadBack(ruleId: string, projectId: string): Promise { + const read = await rawFetch('GET', `/v1/private/automations/evaluators/${ruleId}`); + if (read.status !== 200) { + throw new Error( + `resaveAutomationRuleFromReadBack: GET ${ruleId} answered ${read.status}: ${read.message}`, + ); + } + const rule = read.json as { + type?: string; + name?: string; + sampling_rate?: number; + enabled?: boolean; + trigger_scope?: string; + code?: unknown; + }; + const { status, message } = await rawFetch( + 'PATCH', + `/v1/private/automations/evaluators/${ruleId}`, + { + body: { + type: rule.type, + name: rule.name, + project_ids: [projectId], + sampling_rate: rule.sampling_rate, + enabled: rule.enabled, + trigger_scope: rule.trigger_scope, + code: rule.code, + }, + }, + ); + if (status !== 204) { + throw new Error( + `resaveAutomationRuleFromReadBack: PATCH ${ruleId} expected 204, got ${status}: ${message}`, + ); + } + }, + /** One rule by id, including the `triggerScope` the pinned SDK cannot see. */ async getAutomationRule(ruleId: string): Promise { const { status, message, json } = await rawFetch( diff --git a/tests_end_to_end/e2e/core/backend/index.ts b/tests_end_to_end/e2e/core/backend/index.ts index e2142664fab..3809cc775cd 100644 --- a/tests_end_to_end/e2e/core/backend/index.ts +++ b/tests_end_to_end/e2e/core/backend/index.ts @@ -26,6 +26,10 @@ export { type AutomationRuleRef, type AutomationRuleDetail, type AutomationRuleLogRef, + type AutomationRuleEvaluatorPageRef, + type JudgeMessageRef, + type JudgeMessageContentPartRef, + type JudgeMessageWrite, type TraceJsonSection, type AnnotationQueueDetail, type AnnotationQueueReviewerRef, diff --git a/tests_end_to_end/e2e/pom/online-evaluation.page.ts b/tests_end_to_end/e2e/pom/online-evaluation.page.ts index e16cfbaf0ce..5018a193ace 100644 --- a/tests_end_to_end/e2e/pom/online-evaluation.page.ts +++ b/tests_end_to_end/e2e/pom/online-evaluation.page.ts @@ -68,7 +68,7 @@ export class OnlineEvaluationPage { * whether the project has any rules.) */ async waitForReady(): Promise { - const realRow = this.page.locator('tbody tr[data-row-id]').first(); + const realRow = this.ruleRows.first(); const emptyState = this.page.getByText('No online evaluations yet'); await Promise.race([ realRow.waitFor({ state: 'visible' }), @@ -76,11 +76,19 @@ export class OnlineEvaluationPage { ]); } + /** + * Every rule row the list is currently rendering. + * + * `data-row-id` is the shared DataTable's per-entity stamp, so this counts + * real rows and never the header or an empty-state placeholder. + */ + get ruleRows(): Locator { + return this.page.locator('tbody tr[data-row-id]'); + } + /** Locator for a rule row by name. Uses `data-row-id` row scope + cell-name filter. */ ruleRow(name: string): Locator { - return this.page - .locator('tbody tr[data-row-id]') - .filter({ has: this.page.getByRole('cell', { name, exact: true }) }); + return this.ruleRows.filter({ has: this.page.getByRole('cell', { name, exact: true }) }); } /** @@ -241,6 +249,78 @@ export class OnlineEvaluationPage { }); } + /** + * Open a rule's edit dialog through the row's kebab → Edit, and leave it open. + * + * `setRuleEnabledByName` inlines the same gesture because it owns the whole + * toggle-and-submit flow; this exists for the specs that only want to *read* + * what the dialog hydrated, without changing anything. + */ + async openEditDialogByName(name: string): Promise { + return test.step(`open the edit dialog for rule "${name}"`, async () => { + const row = this.ruleRow(name); + await row.waitFor({ state: 'visible' }); + await row.getByRole('button', { name: 'Actions menu' }).click(); + await this.page.getByRole('menuitem', { name: 'Edit' }).click(); + await this.dialog.waitFor({ state: 'visible' }); + }); + } + + /** + * Submit the add/edit dialog and wait for it to close. + * + * Used on its own by the specs that save a rule without changing anything; + * the `fillAndSubmit…` helpers above do their own submit because they own the + * whole create gesture. + */ + async submitDialog(): Promise { + return test.step('submit the rule dialog', async () => { + await this.dialog.getByTestId('add-edit-rule-dialog-submit').click(); + await this.dialog.waitFor({ state: 'hidden' }); + }); + } + + /** Close the add/edit dialog via its Cancel button, discarding anything typed. */ + async cancelDialog(): Promise { + return test.step('close the rule dialog without saving', async () => { + await this.dialog.getByRole('button', { name: 'Cancel', exact: true }).click(); + await this.dialog.waitFor({ state: 'hidden' }); + }); + } + + /** + * The exact text the dialog hydrated into the judge prompt message of the + * given role. + * + * Read from the rendered CodeMirror lines rather than an input value, because + * that is the only place the text exists — and reading it is the assertion: + * a prompt the read-back mapper truncated arrives here truncated. + * + * `.cm-line` divs are joined with `\n` because that is CodeMirror's document + * model — one line element per document line, with soft wrapping handled + * inside a line rather than by splitting it. This is sound for the short + * judge prompts these specs seed; a document long enough for CodeMirror to + * virtualise its viewport would render only the visible lines, so do not + * reach for this to read a multi-screen prompt. + */ + async readPromptMessageText(role: 'system' | 'user'): Promise { + return test.step(`read the ${role} judge message out of the dialog`, async () => { + // `data-role` sits on the same element as the testid, not inside it, so + // this is an `and()` of two attributes rather than a `filter({ has })`. + const row = this.dialog + .getByTestId('playground-message-row') + .and(this.page.locator(`[data-role="${role}"]`)); + await expect(row, `the dialog must hydrate exactly one ${role} message`).toHaveCount(1); + const content = row.getByTestId('playground-message-editor').locator('.cm-content'); + await content.waitFor({ state: 'visible' }); + return content.evaluate((el) => + Array.from(el.querySelectorAll('.cm-line')) + .map((line) => line.textContent ?? '') + .join('\n'), + ); + }); + } + /** The destructive confirm dialog raised by the row's Delete action. */ get deleteRuleConfirmDialog(): Locator { return this.page.getByRole('dialog').filter({ diff --git a/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-json-looking-judge-prompts.spec.ts b/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-json-looking-judge-prompts.spec.ts new file mode 100644 index 00000000000..2bbdea770cf --- /dev/null +++ b/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-json-looking-judge-prompts.spec.ts @@ -0,0 +1,172 @@ +import { test, expect } from '@e2e/fixtures'; +import { OnlineEvaluationPage } from '@e2e/pom/online-evaluation.page'; + +/** + * One judge prompt per shape that makes the read-back mapper try, and fail, to + * read a prose prompt as multimodal content parts. + * + * `AutomationModelEvaluatorMapper` infers the stored shape from the content + * string, because a message's content is persisted as a plain String whether + * the author typed prose or the UI built a content array. A leading `[` is the + * only hint it has. Every row below is prose a human would plausibly type that + * happens to start with one — and each reaches a different branch of the + * fallback, which is why they are separate rules rather than one representative + * string. + * + * The estate structurally cannot produce these through the UI: the create-rule + * dialog only emits the canned templates and whatever a test types into them, + * and no template opens with a bracket. That is why this spec seeds through the + * REST boundary rather than driving the dialog. + */ +interface JsonLookingPrompt { + /** Used in the rule name and in every failure message. */ + label: string; + /** Which branch of the mapper's fallback this shape lands in. */ + why: string; + content: string; +} + +const OUTPUT_TAIL = '\n\nOUTPUT:\n{{output}}'; + +const JSON_LOOKING_PROMPTS: JsonLookingPrompt[] = [ + { + label: 'source-text', + why: 'not JSON at all — the customer prompt in OPIK-8250', + content: `[Source Text] Grade the answer against the source above.${OUTPUT_TAIL}`, + }, + { + label: 'number-array', + why: 'valid JSON, but the elements are numbers rather than content parts', + content: `[1, 2] are the only scores you may return.${OUTPUT_TAIL}`, + }, + { + label: 'null-element', + why: 'valid JSON array whose element is null, so no element can declare a type', + content: `[null] means no reference answer was supplied.${OUTPUT_TAIL}`, + }, + { + label: 'empty-object', + why: 'valid JSON object element carrying no `type` discriminator', + content: `[{}] is the empty rubric — grade on correctness alone.${OUTPUT_TAIL}`, + }, + { + label: 'empty-array', + why: 'parses, but an empty content list is prose, not a renderable message', + content: `[] is the empty rubric — grade on correctness alone.${OUTPUT_TAIL}`, + }, + { + label: 'array-then-prose', + why: 'opens with a well-formed content part and then continues in prose', + content: `[{"type": "text", "text": "example"}] Now grade the output above.${OUTPUT_TAIL}`, + }, +]; + +/** An ordinary prose prompt. Nothing about it is unusual — that is its job. */ +const CONTROL_PROMPT = `You are a strict grader. Score the answer below out of 1.${OUTPUT_TAIL}`; + +test.describe('Online Evaluation — judge prompts that look like JSON', { tag: ['@t2-cuj', '@area:online-evaluation'] }, () => { + test('A project holding bracket-opening judge prompts still lists every rule, over the API and in the UI', { tag: ['@cap:online-evaluation.list-rules'] }, async ({ + project, + backendClient, + testNamespace, + page, + automationRulesCleanup, + }) => { + const controlName = `${testNamespace}-control`; + // Carried as name+why rather than name alone so a missing row names the + // mapper branch it came from: "row for …-empty-object missing" is a bug + // report, "row missing" is a starting point for one. + const seededRules: Array<{ name: string; why: string }> = [ + { name: controlName, why: 'ordinary prose — the collateral-damage control' }, + ...JSON_LOOKING_PROMPTS.map((p) => ({ + name: `${testNamespace}-${p.label}`, + why: p.why, + })), + ]; + const seededNames = seededRules.map((r) => r.name); + + await test.step( + `Seed a prose control rule plus ${JSON_LOOKING_PROMPTS.length} bracket-opening ones`, + async () => { + // The control is what makes a green listing meaningful. The failure this + // guards was collateral: one unreadable rule took down the listing for + // every OTHER rule in the project too, so a spec that seeded only the + // suspect shapes could not tell "the listing survived" from "the listing + // returned nothing". + await backendClient.createLlmJudgeRule({ + projectId: project.id, + name: controlName, + messages: [{ role: 'USER', content: CONTROL_PROMPT }], + }); + for (const prompt of JSON_LOOKING_PROMPTS) { + await backendClient.createLlmJudgeRule({ + projectId: project.id, + name: `${testNamespace}-${prompt.label}`, + messages: [{ role: 'USER', content: prompt.content }], + }); + } + }, + ); + + await test.step('The project listing answers 200 and carries every seeded rule', async () => { + const listing = await backendClient.findAutomationRuleEvaluatorsPage({ + projectId: project.id, + }); + expect( + listing.status, + 'a single unreadable rule used to turn this whole page into a 500', + ).toBe(200); + // The project is created by the fixture, so its rules are exactly the ones + // seeded above: assert the whole collection, not that ours are somewhere + // in it. A listing that dropped the unreadable rows rather than 500-ing + // would pass a containment check and fail this one. + expect(listing.names.slice().sort(), 'every seeded rule is listed').toEqual( + seededNames.slice().sort(), + ); + expect(listing.total, 'and the server agrees on the count').toBe(seededNames.length); + }); + + await test.step('The workspace-wide listing answers 200 as well', async () => { + // The unscoped read is the one the online-scoring sampler issues per trace + // batch, and it is shared by every project in the workspace — so an + // unreadable rule in one project used to stop sampling for all of them. + // Only the status is asserted: the workspace holds thousands of rules from + // other runs, so its collection is not this test's to pin down. + const workspaceWide = await backendClient.findAutomationRuleEvaluatorsPage(); + expect( + workspaceWide.status, + 'the sampler-shared listing must not 500 because one rule is unreadable', + ).toBe(200); + expect( + workspaceWide.total, + 'and it must actually return rules, not an empty 200', + ).toBeGreaterThanOrEqual(seededNames.length); + }); + + await test.step('The rules page renders a row for every rule, with no 5xx on the wire', async () => { + // Scoped to the evaluators endpoint on purpose. The page also fires + // unrelated background requests that answer 403/404 regardless of rule + // content (datasets/export-jobs, agent-configs, agent-insights); failing + // on those would make this spec a monitor for someone else's endpoint. + const evaluatorServerErrors: string[] = []; + page.on('response', (res) => { + if (res.url().includes('/automations/evaluators') && res.status() >= 500) { + evaluatorServerErrors.push(`${res.status()} ${res.request().method()} ${res.url()}`); + } + }); + + const onlineEval = new OnlineEvaluationPage(page); + await onlineEval.goto(project.id); + await onlineEval.waitForReady(); + + for (const { name, why } of seededRules) { + await expect(onlineEval.ruleRow(name), `row for ${name} — ${why}`).toHaveCount(1); + } + await expect( + onlineEval.ruleRows, + 'the list shows every seeded rule and nothing else', + ).toHaveCount(seededNames.length); + expect(evaluatorServerErrors, 'no 5xx from the evaluators endpoint').toEqual([]); + }); + }); +}); diff --git a/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts b/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts new file mode 100644 index 00000000000..caaa23f0c95 --- /dev/null +++ b/tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts @@ -0,0 +1,152 @@ +import { test, expect } from '@e2e/fixtures'; +import { OnlineEvaluationPage } from '@e2e/pom/online-evaluation.page'; + +/** + * A prose judge prompt that opens with a well-formed content part and then + * keeps going. + * + * This is the shape that lost data rather than merely erroring: the old reader + * stopped at the first complete JSON array, so everything after + * `[{"type": "text", …}]` — the instruction the prompt actually exists for — + * never came back, and the next save wrote the truncation down. The trailing + * `Now grade the output above.` is therefore not decoration; it is the payload + * of the assertion. + */ +const ARRAY_THEN_PROSE_PROMPT = + '[{"type": "text", "text": "example"}] Now grade the output above.\n\nOUTPUT:\n{{output}}'; + +/** A genuinely multimodal message, for the opposite regression. */ +const STRUCTURED_TEXT = 'Grade the answer against the screenshot.\n\nOUTPUT:\n{{output}}'; +const STRUCTURED_IMAGE_URL = 'https://example.com/reference-screenshot.png'; +const STRUCTURED_IMAGE_DETAIL = 'low'; + +test.describe('Online Evaluation — judge prompt round trip', { tag: ['@t2-cuj', '@area:online-evaluation'] }, () => { + test('An example-array-then-prose prompt survives the edit dialog and a re-save byte for byte', { tag: ['@cap:online-evaluation.edit-rule'] }, async ({ + project, + backendClient, + testNamespace, + page, + automationRulesCleanup, + }) => { + const ruleName = `${testNamespace}-array-then-prose`; + + const ruleId = await test.step('Seed the judge rule through the API', async () => { + // Seeded over REST because the create dialog cannot produce this string: + // it only emits the canned templates, none of which opens with a bracket. + return backendClient.createLlmJudgeRule({ + projectId: project.id, + name: ruleName, + messages: [{ role: 'USER', content: ARRAY_THEN_PROSE_PROMPT }], + }); + }); + + await test.step('The API reads the prompt back as the exact string that was written', async () => { + const messages = await backendClient.getLlmJudgeMessages(ruleId); + expect(messages, 'the rule has exactly the one seeded message').toHaveLength(1); + const [message] = messages; + expect( + message.content, + 'prose that merely opens with a content part is prose, and must come back whole', + ).toBe(ARRAY_THEN_PROSE_PROMPT); + expect( + message.contentArray, + 'and it must not be promoted to structured content', + ).toBeNull(); + }); + + const onlineEval = new OnlineEvaluationPage(page); + + await test.step('The edit dialog hydrates the prompt with the trailing instruction intact', async () => { + await onlineEval.goto(project.id); + await onlineEval.waitForReady(); + await onlineEval.openEditDialogByName(ruleName); + expect( + await onlineEval.readPromptMessageText('user'), + 'the dialog shows what the author typed, not a prefix of it', + ).toBe(ARRAY_THEN_PROSE_PROMPT); + }); + + await test.step('Saving the dialog unchanged persists the prompt unchanged', async () => { + // The read bug only became data loss here: the dialog writes back whatever + // it was given, so a truncated read submitted once was a truncated prompt + // for good. Submitting without touching anything is exactly the gesture a + // user makes when they open a rule to change its sampling rate. + await onlineEval.submitDialog(); + + await onlineEval.openEditDialogByName(ruleName); + expect( + await onlineEval.readPromptMessageText('user'), + 'a no-op save must not shorten the prompt', + ).toBe(ARRAY_THEN_PROSE_PROMPT); + await onlineEval.cancelDialog(); + }); + + await test.step('And the server still holds it as a plain string', async () => { + const messages = await backendClient.getLlmJudgeMessages(ruleId); + expect(messages, 'the save must not have added or dropped a message').toHaveLength(1); + expect(messages[0].content, 'byte-identical after the round trip').toBe( + ARRAY_THEN_PROSE_PROMPT, + ); + expect(messages[0].contentArray, 'still not structured content').toBeNull(); + }); + }); + + test('A genuinely multimodal judge message still reads back as structured content', { tag: ['@cap:online-evaluation.edit-rule'] }, async ({ + project, + backendClient, + testNamespace, + automationRulesCleanup, + }) => { + // The guard against over-correcting the fix above. Falling back to "it is + // all just a string" for anything hard to parse would make the truncation + // test pass while quietly destroying every real multimodal rule — the + // image part would come back as JSON text in the prompt box and the judge + // would stop seeing the image. + // + // API-level throughout: the dialog renders only the text part of a + // multimodal message, so the image URL and its `detail` — the fields that + // would be lost — are not observable in the UI at all. + const ruleName = `${testNamespace}-structured-content`; + + const ruleId = await test.step('Seed a judge rule with a text part and an image part', async () => + backendClient.createLlmJudgeRule({ + projectId: project.id, + name: ruleName, + messages: [ + { + role: 'USER', + contentArray: [ + { type: 'text', text: STRUCTURED_TEXT }, + { + type: 'image_url', + image_url: { url: STRUCTURED_IMAGE_URL, detail: STRUCTURED_IMAGE_DETAIL }, + }, + ], + }, + ], + })); + + const assertStructured = async (when: string): Promise => { + const messages = await backendClient.getLlmJudgeMessages(ruleId); + expect(messages, `${when}: exactly the one seeded message`).toHaveLength(1); + const [message] = messages; + expect(message.content, `${when}: must not be flattened into a string`).toBeNull(); + expect(message.contentArray, `${when}: content parts survive`).toEqual([ + { type: 'text', text: STRUCTURED_TEXT, imageUrl: null }, + { + type: 'image_url', + text: null, + imageUrl: { url: STRUCTURED_IMAGE_URL, detail: STRUCTURED_IMAGE_DETAIL }, + }, + ]); + }; + + await test.step('The API reads it back as content parts, url and detail intact', async () => + assertStructured('on first read')); + + await test.step('A re-save of exactly what was read leaves it structured', async () => { + await backendClient.resaveAutomationRuleFromReadBack(ruleId, project.id); + await assertStructured('after a no-op re-save'); + }); + }); +});