[OPIK-8102] [QA] Proposed e2e specs for the LLM-judge Gemini thinking level (from #8158 exploration) - #8160
Conversation
… level Two Playwright specs for the behaviour #8158 introduces, written and run against that PR's own environment. The failure mode is silent — a judge rule that carries a thinking block it should not looks identical in the list and scores traces normally, it is just slower and more expensive — so every assertion is made against the persisted evaluator over REST, with the form asserted separately as the thing the user reads. - A Flash Lite judge preselects None, offers None/Minimal/Low/Medium/High, and saves no custom_parameters at all. Reopened it hydrates None; set to Minimal it persists {thinking: {level: minimal}}; reopened and resaved untouched it keeps that level; set back to None the block is REMOVED, not left behind. That last step is the one #8158's head commit fixes. - The control: Gemini 3.5 Flash still preselects Medium, is never offered None, and still saves {thinking: {level: medium}} — so a later "just send nothing for Gemini" simplification cannot pass unnoticed. No completion is made: the model picker only needs a provider key to exist, so a dummy Gemini key makes both models selectable and everything asserted is decided before the judge would run. Supporting changes, all additive: - geminiProviderKey fixture — adopt-or-seed, because a provider key is workspace-global and `gemini` cannot be namespaced per test the way a custom provider can. The spec runs serial for the same reason. - backendClient.getLlmJudgeModel — reads code.model.custom_parameters, which the pinned SDK's evaluator shape does not carry. Nullable on purpose: absent and empty are different answers when the point is removal. - OnlineEvaluationPage — model picker scoped to its provider group (Gemini and Vertex AI publish identical model labels), model-parameters popover, thinking-level read/list/set, and an edit-dialog opener. Taxonomy: online-evaluation.edit-rule flipped to covered (t2-cuj), the spec added to the area's list, and create-llm-judge-rule's note extended. Generated by the release QA side flow; needs review before merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
tests
📋 PR Linter Failed❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the |
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const body = (await response.json()) as { content: ProviderKeyRef[] }; | ||
| return body.content.find((key) => key.provider_name === providerName) ?? null; | ||
| return body.content; |
There was a problem hiding this comment.
Malformed list responses cause opaque lookup crashes
listProviderKeys casts parsed JSON and returns body.content without checking that content is an array, so malformed successful responses reach .find and crash with an opaque TypeError — should we validate the response shape at the REST boundary before returning it?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/core/provider-keys.ts` around lines 55-56, update
`listProviderKeys` so it does not blindly cast the parsed response and return
`body.content`. Validate that the response is an object with a `content` property that
is an array before returning it; otherwise throw a descriptive error indicating the list
response has an invalid shape, so `findProviderKeyByName` and
`findProviderKeyByProvider` never call `.find` on malformed data.
| await this.dialog.getByTestId('add-edit-rule-dialog-submit').click(); | ||
| await this.dialog.waitFor({ state: 'hidden' }); |
There was a problem hiding this comment.
Immediate REST assertions race edit PATCH
The submitRuleDialog spec waits only for the dialog to hide, so the edit callback can close it before updateMutate’s PATCH and onSettled invalidation finish, letting the next getLlmJudgeModel GET read stale custom_parameters and fail nondeterministically — should we await the mutation response or poll the REST value before asserting?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/pom/online-evaluation.page.ts` around lines 264-265, update
`submitRuleDialog` so it does not return merely when the controlled dialog becomes
hidden; the edit handler starts the PATCH asynchronously and the next assertion can
observe stale `custom_parameters`. Coordinate the submit with the mutation's observable
network response, or poll the rule's REST value until the submitted parameters are
persisted before completing the helper.
| private get modelParametersTrigger(): Locator { | ||
| return this.dialog.locator('button:has(svg.lucide-settings2)'); | ||
| } |
There was a problem hiding this comment.
Use stable model-parameters test ID
PromptModelConfigs tests locate the trigger through Lucide's lucide-settings2 class, so a harmless icon change breaks them — should we add data-testid="model-parameters-trigger" and switch the locator to getByTestId, as .agents/skills/writing-e2e-tests/conventions.md recommends?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`tests_end_to_end/e2e/pom/online-evaluation.page.ts` around lines 339-341, update the
`modelParametersTrigger` locator to use `getByTestId('model-parameters-trigger')`
instead of the structural `lucide-settings2` selector. Add the matching kebab-case
`data-testid="model-parameters-trigger"` to the `PromptModelConfigs` gear button in its
frontend component, following the project’s E2E selector conventions, and preserve the
existing uniqueness assertion.
There was a problem hiding this comment.
Commit 9fdebd5 addressed this comment by adding data-testid="model-parameters-trigger" to the gear button and switching the POM locator to getByTestId('model-parameters-trigger') while retaining the uniqueness assertion.
| * and a spec that cannot tell "no block" from "empty block" cannot assert | ||
| * that. | ||
| */ | ||
| customParameters: Record<string, unknown> | null; |
There was a problem hiding this comment.
REST response type hides wire-format field
The handwritten raw REST type exposes backend custom_parameters as customParameters, while getLlmJudgeModel reads model.custom_parameters, so the type no longer matches the payload and callers rely on an undocumented mapping — should we rename the field and returned object to custom_parameters, or introduce a separately mapped camelCase DTO?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
tests_end_to_end/e2e/core/backend/client.ts around lines 203 and 1631-1637, update the
`LlmJudgeModelDetail` interface and `getLlmJudgeModel` method so the raw REST field
remains named `custom_parameters`, matching the backend payload and
`model.custom_parameters` access. Remove the undocumented camelCase transformation; if a
camelCase domain DTO is needed, define it separately and map to it explicitly.
| customParameters: | ||
| custom === null || custom === undefined | ||
| ? null | ||
| : (custom as Record<string, unknown>), |
There was a problem hiding this comment.
Accepts malformed custom parameters
customParameters casts every non-null JSON value to Record<string, unknown>, so scalars and arrays reach object-only consumers and violate the accessor contract — should we require typeof custom === 'object' && !Array.isArray(custom) and throw a clear error for other non-null shapes?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
tests_end_to_end/e2e/core/backend/client.ts around lines 1633-1636, update
`getLlmJudgeModel` so `custom_parameters` is not blindly cast to `Record<string,
unknown>`. Preserve `null` for null or undefined, validate that other values are
non-null objects and not arrays, and throw a clear error identifying the invalid
`custom_parameters` shape before returning it.
Replace two fragile selectors in the new online-evaluation POM with stable test ids, and document a cross-spec hazard on the workspace-global Gemini key. - The model-parameters gear was addressed by `svg.lucide-settings2`, an internal lucide-react class derived from the icon name at build time and renamed without warning by a version bump. Add a `data-testid` to the trigger in PromptModelConfigs and select on that instead. The button is icon-only and its "Model parameters" string lives in a tooltip, so no role/label selector exists. Test-only hook, no behaviour change; same pattern as the add-edit-rule-dialog-submit id added by #6874. - The LLM-judge model combobox was matched by a hardcoded provider-name regex, which stops matching for any provider outside the list. Scope it by the select-a-llm-model test id PromptModelSelect already ships. - Note in the geminiProviderKey fixture that playground-providers.spec.ts is the other writer of that key and wants a real completion from it, so the two must not share a run. Gates: tag_lint PASS, playwright --list PASS (121 tests, unchanged). tsc still fails on the pre-existing TS5102 baseUrl error in tsconfig.json, which is identical on main and untouched here; a scoped re-check with baseUrl removed shows zero type errors in this PR's own code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 Generated-test reviewPR #8160 — OPIK-8102 [QA] Proposed e2e specs for the LLM-judge Gemini thinking level (from #8158 exploration)CometActions · draft: yes · 8 files, 1 new spec (2 tests) VERDICT: ready with notes — do not flip the draft flag yourself; a human must. This is a good spec. Every factual claim it makes about the surface it drives, I Blockers (0)None. Fixed in this review (3)
Should fix (0)Notes (2)
Bot comments
Estate gates
What I verifiedEvery factual claim the spec makes about the frontend, checked against source at
What I could NOT verify
One thing to be aware ofThis PR targets review_generated_tests.yml · |
1 participant
Where this came from
Exploratory testing of #8158 (
[OPIK-8102] [FE] fix: add a "none" thinking level so Flash Lite models stay non-thinking) on that PR's own deployed environment,https://pr-8158.dev.comet.com(2.2.51-8158-merge-3187, OSS install, workspacedefault). A human worked the flows by hand there first; these two specs are the flows that came back verified and strong.This PR targets
AndreiCautisanu/OPIK-8102/flash-lite-thinking-none, notmain— deliberately. Both specs assert behaviour that only exists in #8158: onmain,Gemini 3.5 Flash Litestill defaults tominimaland its level list has noNoneat all, so the first spec fails on its very first assertion. The specs are written against fb5fc93, that PR's head. They should merge with #8158, not ahead of it.What the specs assert
The failure mode this covers is silent. A judge rule that carries a thinking block it should not looks identical in the rules list, scores traces normally, and reads back correctly in the form — the only symptom is that every judge call is slower and costs more. So each assertion is made against the persisted evaluator over REST (
GET /v1/private/automations/evaluators/{id}→code.model.custom_parameters), with the dialog asserted separately as the thing a user actually reads.Both are
@t2-cuj @area:online-evaluation, intests/online-evaluation/online-evaluation-thinking-level.spec.ts.1.
A Flash Lite judge saves no thinking block, keeps a level that was chosen, and clears it again on None@cap:online-evaluation.create-llm-judge-rule·@cap:online-evaluation.edit-ruleGemini 3.5 Flash LiteNone, Minimal, Low, Medium, Highcustom_parametersis absent — not{}, not{thinking: {}}{thinking: {level: minimal}}{thinking: {level: minimal}}— the newnonedefault must not overwrite a level someone chosecustom_parametersis gone againThat last row is the one #8158's head commit (
make "none" clear a persisted thinking block, not just skip adding one) exists for, and it is the row the PR's own vitest suites cannot reach: it needs the full dialog → save → rehydrate → save round trip.custom_parametersis compared as a whole object rather than probed for a key, so a leftover sibling underthinkingfails rather than passing quietly.2.
A thinking-by-default Gemini model is untouched: Flash preselects Medium, is never offered None, and saves the level@cap:online-evaluation.create-llm-judge-ruleThe control.
Gemini 3.5 Flashstill preselects Medium, is not offeredNone, and still saves{thinking: {level: medium}}. Without it, a later "just stop sending thinking for Gemini" simplification would silently switch every Gemini judge to non-thinking and spec 1 would still pass.Verification
Run against
https://pr-8158.dev.comet.com— the same environment the exploration used — fromtests_end_to_end/e2e/:Also run:
python3 tests_end_to_end/coverage/tag_lint.py --taxonomy … --estate tests_end_to_end→61 specs checked, 1 exempt, 0 problem(s).tests/online-evaluation/directory, since this change touches a shared POM: 8 passed, 1 skipped, 1 failed. The skip is the LLM-judge smoke test (ensureModelAvailable— noANTHROPIC_API_KEY/OPENAI_API_KEY/OPENROUTER_API_KEYon this runner). The failure isonline-evaluation-python-metric-errors.spec.ts: the environment answersPython evaluation failed (HTTP '500'): 500 Internal Server Error: Failed to execute codewhere the spec expects the classified400 Bad Request: Execution failed: the metric produced no output. Not from this branch — verified by checking out fb5fc93 clean and running that one spec, which fails identically. It looks like an environment/backend-side condition onpr-8158.dev.comet.comand is worth a separate look, but it is nothing to do with these specs.tsc --noEmiton the estate reports exactly one error,core/backend/client.ts: Duplicate identifier 'deleteDashboard', which reproduces unchanged onmain— it is not from this branch. (Separately,npx tscwith the repo's pinnedtypescript@7.0.2fails ontsconfig.jsonbefore reaching any source:Option 'baseUrl' has been removed. Also pre-existing onmain, and worth a fix in its own PR — the typecheck in the writing-e2e-tests skill's checklist cannot currently run as written.)No completion is ever made and no LLM key is needed: the model picker lists a provider's models as soon as a key for it exists, and everything asserted here is decided before the judge would run. A dummy Gemini key is enough, which is what makes both specs deterministic and free.
Supporting changes (all additive)
fixtures/provider-key.fixture.ts— newgeminiProviderKeyfixture. Adopt-or-seed: a provider key is workspace-global andgeminicannot be namespaced per test the wayproviderKeys.createOauthnamespaces a custom provider, so if the workspace already has a Gemini key the fixture uses it and leaves it alone; otherwise it seeds a dummy one and deletes it afteruse(), honouringshouldLeaveArtifacts. Because the resource is unnamespaceable, the spec declarestest.describe.configure({ mode: 'serial' })— verified:--workers=4still reports "Running 2 tests using 1 worker".core/provider-keys.ts—createProviderKeynow also reports the created id from theLocationheader, and takesapi_key/ optionalprovider_nameso a built-in provider can be seeded. Deliberately returnsstring | nullrather than throwing on a missing header, so the existing token-auth callers (which clean up byprovider_name) are behaviour-identical; the new fixture, which needs the id, is the one that complains.core/backend/client.ts—getLlmJudgeModel(ruleId). Readscode.modelraw, because the pinned SDK's evaluator shape has nocustom_parameters. Nullable on purpose: absent and empty are different answers when the whole point is removal.pom/online-evaluation.page.ts— new methods only; no existing method was changed, so no sibling spec's behaviour moves. Model picker scoped to its provider group, because Gemini and Vertex AI publish identical display labels (Gemini 3.5 Flash Liteis bothgemini-3.5-flash-liteandvertex_ai/gemini-3.5-flash-lite) and an unscoped lookup would match two options — both the group and the option are asserted totoHaveCount(1)rather than taking.first().One selector needs a reviewer's eye. The model-parameters gear (
PromptModelConfigs) is an icon-only button with no accessible name and nodata-testid— its "Model parameters" string lives in a tooltip, which contributes nothing to the accessible name — so the POM selects it by its icon (button:has(svg.lucide-settings2)), asserted to resolve to exactly one element inside the dialog. Perconventions.mdthe right fix is adata-testidon the trigger, in this same change. It is not here on purpose: these specs were verified against a prebuilt deployment of #8158, where a newly added attribute would not exist, so adding it would have meant shipping specs I could not run. Happy to add it (data-testid="model-parameters-trigger") and switch the POM over if you would rather have that than a verified run.Taxonomy
online-evaluation.edit-ruleflipped tocovered: true, tier: t2-cuj— with a note scoping the claim to the dialog's model-settings pane, since the prompt, variable mapping, filters and sampling controls are still unexercised by an edit. The spec is added to the area'sspecs:list, andcreate-llm-judge-rule's note records the new Gemini-model creation path.What was deliberately not written
One candidate of three was dropped: "Playground: switching to a Flash Lite model must not carry a thinking level onto the completion request." The exploration marked it
weak, and it is the one flow that is currently wrong: coming from the playground's own default Gemini model, the Thinking level control stays at High andPOST /v1/private/chat/completionssendscustom_parameters.thinking.level: "high"ongemini-3.5-flash-lite. Two code paths write the config on a model change (PlaygroundPrompt.tsx): a provider change callsgetDefaultConfigByProviderand applies the new"none"default, while a model change within Gemini callsupdateProviderConfig, which only rewrites the level when the current one is not in the new model's list — and"high"is in Flash Lite's list both before and after this PR.That is not a regression from #8158 (
updateProviderConfigis not in the diff, and the pre-PR list also contained"high"), but it does mean an OSS user with only a Gemini key has no way to reach the path where the fix applies, so on the playground Flash Lite still runs with thinking at maximum. A spec written now would fail on merge. It is left unwritten pending the author's call on whether the playground's within-provider carry-over is in scope here —playground.configure-model-settingsstayscovered: false.Please read before merging
Generated by the release QA side flow — written, run and committed by an automated agent. It is a draft on purpose and needs a human to judge whether each assertion is the right one before it is promoted. Base branch is #8158's head, not
main.Related: #8158