[OPIK-7791] [QA] Proposed e2e specs from the 2.2.50 → 2.2.51 release exploration - #8161
[OPIK-7791] [QA] Proposed e2e specs from the 2.2.50 → 2.2.51 release exploration#8161CometActions wants to merge 2 commits into
Conversation
…250) Two specs proposed by the 2.2.50 -> 2.2.51 release QA side flow, from flows a human verified on staging first. online-evaluation-json-looking-judge-prompts.spec.ts covers @cap:online-evaluation.list-rules: a project holding a prose control rule plus six judge prompts that open with '[' still lists every rule over the project-scoped API, keeps the workspace-wide (sampler-shared) listing at 200, and renders a row per rule with no 5xx from the evaluators endpoint. online-evaluation-judge-prompt-round-trip.spec.ts covers @cap:online-evaluation.edit-rule: an example-array-then-prose prompt reads back byte-exact over the API and in the edit dialog, and survives a no-op dialog save; a genuine content_array message still reads back structured, with its image_url url and detail intact, after a no-op re-save. The estate cannot reach these shapes through the create-rule dialog, which only emits the canned templates, so both specs seed over REST. Supporting additions: createLlmJudgeRule / findAutomationRuleEvaluatorsPage / getLlmJudgeMessages / resaveAutomationRuleFromReadBack on the backend client, and openEditDialogByName / readPromptMessageText / submitDialog / cancelDialog on OnlineEvaluationPage. Generated by the release QA side flow. Needs human 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
⏭️ 38 skipped (no matching files changed)
|
…ule note - json-looking-judge-prompts.spec.ts: the JsonLookingPrompt.why field was populated and never read, so a missing row failed with "row for <name>" and the reader had to go back to the table to learn which mapper branch that shape reaches. Carry name+why through the UI step and put it in the message. - taxonomy.yaml: the edit-rule note read as though the edit dialog was driven for both prompt shapes. Only the plain-string half is; the content_array half is API-only, because the dialog renders no image part. Scoped the note to say so, and recorded the open question — the content_array test asserts content-shape preservation, which no capability in this area names, so it rides on edit-rule until a human decides whether to add a key or drop the tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 Generated-test reviewPR #8161 — OPIK-7791 [QA] Proposed e2e specs from the 2.2.50 → 2.2.51 release explorationCometActions · draft: yes · 6 files, 2 new specs · head VERDICT: ready with notes — one open decision for a human (the Do not flip the draft flag on the strength of this report. A person makes that Blockers (0)None. Both specs collect, both are placed correctly, the tags that matter are Should fix (1 — a decision, not an edit)1.
Why it is not a blocker: Why I did not fix it: the two honest fixes are (a) add a capability for Fixed in this PR (2)Both pushed as 1. 2. The Notes (3)1. The
Verified, not inferred: I checked I deliberately did not fix it, and removing 2. Pre-existing duplicate in the file this PR extends. 3. Type health of this PR's own code, checked under a workaround. Because What I verified positively
Bot comments
Estate gates: tag_lint PASS · tsc FAIL (pre-existing on main) · playwright --list PASSRe-run after my edits:
What I could not verify
review_generated_tests.yml · |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| }); | ||
| }); | ||
|
|
||
| test('A genuinely multimodal judge message still reads back as structured content', { tag: ['@cap:online-evaluation.edit-rule'] }, async ({ |
There was a problem hiding this comment.
Structured test falsely reports edit coverage
The structured contentArray test is tagged edit-rule, and edit-rule is marked covered in tests_end_to_end/coverage/taxonomy.yaml:675-680, even though this test only verifies API-level preservation rather than UI editing; coverage therefore overstates edit capability — should we add a dedicated capability and retag the test, or remove the tag?
Supporting evidence from every grouped finding:
-
tests_end_to_end/coverage/taxonomy.yaml:675-680: The taxonomy marks
edit-ruleascovered: trueeven though the content-array behavior has no matching capability and remains unresolved, so coverage reports classify it underedit-rule— should we add a dedicated capability and map the test to it, or remove the behavior from this capability’s coverage and leave it uncovered? -
tests_end_to_end/e2e/tests/online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts:94-94: This test is tagged
edit-ruleeven though it only reads back and re-saves multimodalcontentArraydata, so it overstatesonline-evaluation's UI-edit coverage — should we add a dedicated kebab-case capability and use it here, or remove the tag?
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/tests/online-evaluation/online-evaluation-judge-prompt-round-trip.spec.ts`
around lines 94-150, the multimodal judge-message test only performs API read-back and
re-save assertions, but is tagged as `online-evaluation.edit-rule` even though it does
not exercise the edit UI. Add a dedicated kebab-case capability to the project’s
capability taxonomy for multimodal content round-trips, then replace the existing
capability tag on this test with the new one.
| export interface JudgeMessageContentPartRef { | ||
| type: string; | ||
| text: string | null; | ||
| imageUrl: { url: string; detail: string | null } | null; |
There was a problem hiding this comment.
API response types hide wire field names
These API-facing interfaces rename the wire fields to imageUrl and contentArray in JudgeMessageRef, so their declared response shapes don't match the image_url and content_array JSON the implementation reads — should we use the exact backend keys or document these as normalized client representations instead?
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 197-200, update
JudgeMessageContentPartRef and the related JudgeMessageRef definition so their names and
documentation accurately reflect whether they model raw backend JSON or a normalized
client result. Prefer using the wire keys image_url and content_array for API-facing
types, or explicitly rename/document these as normalized representations and keep the
conversion in getLlmJudgeMessages consistent. Update any affected consumers and comments
so the declared types no longer contradict the JSON contract.
| /** 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 }; | ||
| }>; |
There was a problem hiding this comment.
Judge helper forwards invalid message shapes
JudgeMessageWrite leaves both content and contentArray optional, and createLlmJudgeRule serializes each independently at tests_end_to_end/e2e/core/backend/client.ts:219-227, so callers can send both wire fields or neither and receive a remote rejection or ambiguous payload — should the helper enforce exactly one shape at its boundary?
Supporting evidence from every grouped finding:
-
tests_end_to_end/e2e/core/backend/client.ts:220-227:
JudgeMessageWriteaccepts neithercontentnorcontentArray, or both, socreateLlmJudgeRulecan send evaluator-invalid messages and receive a server rejection — should we model it as a union of{ role; content: string; contentArray?: never }and{ role; content?: never; contentArray: ... }? -
tests_end_to_end/e2e/core/backend/client.ts:219-227: Both
JudgeMessageWritecontent fields are optional andcreateLlmJudgeRuleemits them independently, so requests can contain bothcontentandcontent_arrayor neither — should we enforce exactly one at the boundary?
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 219-227, update
`JudgeMessageWrite` and `createLlmJudgeRule` so each judge message must provide exactly
one of `content` or `contentArray`. Model the type as an exclusive/discriminated union
and add a runtime boundary check before `rawFetch` that rejects messages with both
fields or neither, rather than silently emitting an invalid payload.
| const { status, message, location } = await rawFetch( | ||
| 'POST', | ||
| '/v1/private/automations/evaluators/', | ||
| { |
There was a problem hiding this comment.
New specs bypass public evaluator client
createLlmJudgeRule seeds through rawFetch('POST', '/v1/private/automations/evaluators/'), so the E2E setup bypasses public Opik/suite-client APIs and violates .agents/skills/writing-e2e-tests/conventions.md — should we use sdk.api.automationRuleEvaluators.createAutomationRuleEvaluator as in workspace-role-resource-actions.ts, or record an approved isolated exception for the unsupported message-shape seam?
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/core/backend/client.ts around lines 1641-1644, refactor
`createLlmJudgeRule` so E2E setup does not call the private
`/v1/private/automations/evaluators/` endpoint through `rawFetch`. Use the existing
public `sdk.api.automationRuleEvaluators.createAutomationRuleEvaluator` client/bridge
path and adapt its response handling as needed; if it cannot support the judge message
shape, document and obtain an explicitly approved, isolated exception instead of
silently using `rawFetch`.
| // 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: [] }; |
There was a problem hiding this comment.
Evaluator failures lose diagnostics
The non-200 branch drops the message from rawFetch when replacing the response with { status, total: 0, names: [] }, so evaluator failures expose only a generic expectation mismatch — should we retain the message in AutomationRuleEvaluatorPageRef while preserving the non-throwing status-based behavior?
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 1711-1713, update
`findAutomationRuleEvaluatorsPage` so non-200 evaluator-list responses preserve the
diagnostic `message` from `rawFetch` instead of returning only status and empty data.
Extend `AutomationRuleEvaluatorPageRef` with an appropriate message field and
destructure/return it in the failure result, while keeping the existing status-based,
non-throwing behavior and ensuring successful responses still satisfy the type.
| 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, |
There was a problem hiding this comment.
No-op resave destroys evaluator state
resaveAutomationRuleFromReadBack reconstructs an incomplete PATCH at tests_end_to_end/e2e/core/backend/client.ts:1808-1815: it omits persisted filters and sends project_ids: [projectId] instead of the complete association set, so a no-op replay clears filters and detaches a multi-project evaluator from every other project — should the helper derive both values from the complete GET state, or explicitly reject unsupported rule shapes?
Supporting evidence from every grouped finding:
-
tests_end_to_end/e2e/core/backend/client.ts:1804-1815:
resaveAutomationRuleFromReadBackomitsfiltersfrom its PATCH body, so the backend maps it to null and replaying a filtered evaluator clearsautomation_rules.filters— should we include the read-backfiltersin the payload? -
tests_end_to_end/e2e/core/backend/client.ts:1808-1815: The read-back PATCH sends
project_ids: [projectId]instead of the rule’s existing project set, so the backend replaces all junction rows with that single association and replaying a multi-project evaluator detaches it from the other projects — should we preserve the full association set? -
tests_end_to_end/e2e/core/backend/client.ts:1809-1812:
resaveAutomationRuleFromReadBackalways sendsproject_ids: [projectId], so a no-op read/save drops the rule's other project associations and narrows its scope — should we preserveproject_idsfrom the read response or reject multi-project rules? -
tests_end_to_end/e2e/core/backend/client.ts:1808-1815:
resaveAutomationRuleFromReadBackrebuilds an exact PATCH without persistedfiltersand withproject_ids: [projectId], so the backend clears saved filters and detaches other projects from multi-project rules. Should we derive both from the complete GET state, includingprojects, or explicitly limit and test the helper’s supported rule shape?
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 1789-1815, fix
`resaveAutomationRuleFromReadBack` so its PATCH preserves the evaluator’s complete
persisted state instead of reconstructing an incomplete update. Include the `filters`
returned by GET and derive `project_ids` from the evaluator’s full project association
list rather than `[projectId]`; update the response typing and add coverage for filters
and multi-project rules, or explicitly reject unsupported rule shapes.
| // 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) { |
There was a problem hiding this comment.
Broad listener reports unrelated 5xx responses
The UI wire assertion matches the evaluator collection by substring, so it also accepts other origins, /evaluators-backup, query-string occurrences, and detail/log subroutes — should we parse the URL and require the configured API origin plus /v1/private/automations/evaluators with a path boundary and optional trailing slash/query?
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/tests/online-evaluation/online-evaluation-json-looking-judge-prompts.spec.ts`
around lines 153-153, tighten the evaluator response filter in the UI wire assertion.
Parse each response URL and require the configured API origin and the exact
`/v1/private/automations/evaluators` collection pathname, allowing only an optional
trailing slash and query string; do not match other origins, similarly named paths, or
evaluator detail/log subroutes.
| # 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" } |
There was a problem hiding this comment.
Coverage taxonomy falsely reports edit coverage
edit-rule is marked covered: true even though field editing remains untested, so coverage gates and reviewers may treat rename/model/prompt edits as covered based only on hydration and no-op re-save — should we add a field-edit assertion, or keep it uncovered and add a narrower prompt round-trip capability?
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/coverage/taxonomy.yaml` around lines 675-680, correct the `edit-rule`
coverage declaration so it does not claim field editing is covered when the tests only
perform prompt hydration and no-op re-save round trips. Either add an end-to-end
assertion that renames a rule, changes its model, or rewrites its prompt and then keep
`edit-rule` covered, or mark `edit-rule` uncovered and introduce a narrower capability
(such as prompt round-trip preservation) with an accurate note.
1 participant
Where these came from
The 2.2.50 → 2.2.51 release exploration worked the release's manual-verification
list against
staging.dev.comet.com/opik(workspaceopik-testing). Both flowsbelow were verified by hand on staging first — these specs make permanent
what that pass checked once.
Both come from #8150 (OPIK-8250):
a judge message's content is persisted as a plain
Stringwhether the authortyped prose or the UI built a content array, so
AutomationModelEvaluatorMapperinferred the shape from the leading character. A prompt legitimately opening with
[was parsed as JSON, failed, and 500'd the whole evaluators listing — everyrule in the project plus the workspace-wide
findAll()the online-scoringsampler calls per trace batch.
Which ref these were written and run against
Written and run against the release tag
2.2.51(d5ac079), which is theref the exploration verified.
mainis simply where they have to merge, and itwill have moved on — please re-run on
mainbefore promoting this out of draft.The specs
1.
online-evaluation-json-looking-judge-prompts.spec.tsCovers
@cap:online-evaluation.list-rules(@t2-cuj).Seeds a prose control rule plus six judge prompts that each open with
[andreach a different branch of the mapper's fallback —
[Source Text] …(thecustomer prompt, not JSON at all),
[1, 2] …(valid JSON, wrong element type),[null] …,[{}] …(notypediscriminator),[] …, and[{"type": "text", "text": "example"}] Now grade the output above.Then asserts:set — the whole collection, not just containment, so a listing that silently
dropped the unreadable rows would fail rather than pass;
non-empty;
/automations/evaluatorsduring load.The control rule is what makes a green listing mean something: the regression was
collateral, one bad row took down every other rule in the project too.
Verification: PASSED on staging against
2.2.51.2.
online-evaluation-judge-prompt-round-trip.spec.tsCovers
@cap:online-evaluation.edit-rule(@t2-cuj), two tests.Test 1 — an example-array-then-prose prompt survives the edit dialog and a
re-save byte for byte. The old reader stopped at the first complete JSON array,
so
[{"type": "text", "text": "example"}] Now grade the output above.lost theinstruction it exists for, and the next save wrote the truncation down. The spec
seeds over REST, asserts the API returns the string whole and not promoted to
content_array, opens the row's Actions → Edit dialog and asserts thehydrated prompt is byte-identical, submits the dialog unchanged, reopens it
and asserts it again, then re-checks the server. The no-op submit is the gesture
that turned a read bug into permanent data loss.
Test 2 — a genuinely multimodal judge message still reads back as structured
content. The guard against over-correcting: falling back to "it is all just a
string" would make test 1 pass while destroying every real multimodal rule. Seeds
a
content_arraywith a text part and animage_urlpart, asserts the read-backkeeps
urlanddetail, re-saves exactly what was read, and asserts it again.Verification: both PASSED on staging against
2.2.51.How they were run
From
tests_end_to_end/e2e/, against$OPIK_BASE_URL=https://staging.dev.comet.com/opik:Plus the estate's own three checks from
.agents/skills/writing-e2e-tests:tag_lint.py—61 specs checked, 1 exempt, 0 problem(s).npx playwright test tests/online-evaluation/withWORKERS=2leavesonline-evaluation-non-object-sections.spec.tsred, inLogsPage.waitForReadywaiting on the traces table. This is pre-existingstaging flake, not a regression from this PR: the same directory run on a
clean
2.2.51tree with the same settings fails three specs (that one plusboth
online-evaluation-sampling-ratetests), all in the sameLogsPage.waitForReadyon the traces table. That POM is untouched here. Allthree pass when run alone.
npx tsc --noEmit— currently fails at the config level on2.2.51foreveryone, before any file is checked:
tsconfig.json(13,5): error TS5102: Option 'baseUrl' has been removed.(
typescript: "^7.0.2"resolves to 7.0.2, which droppedbaseUrl). Leftalone — it is not this PR's to fix. Typechecked instead with TypeScript 5.9.3
over the same config: the only error is the pre-existing
core/backend/client.ts: Duplicate identifier 'deleteDashboard', presentidentically on a clean tree. This PR adds no new type errors.
I also ran a negative control on the two assertions doing the most work — the
CodeMirror prompt read and the UI row count — by mutating each to the wrong
expected value. Both failed, and the prompt read reported the full string
including the trailing instruction and the blank line. Neither assertion is
passing vacuously.
What is in the diff besides the specs
core/backend/client.ts—createLlmJudgeRule(the existingcreateAutomationRuleonly buildsuser_defined_metric_pythonrules, whichhave no
content/content_arraysplit),findAutomationRuleEvaluatorsPage(returns the HTTP status instead of throwing, because the status is the
assertion),
getLlmJudgeMessages,resaveAutomationRuleFromReadBack.pom/online-evaluation.page.ts—openEditDialogByName,submitDialog,cancelDialog,readPromptMessageText, and aruleRowsgetter thatwaitForReady/ruleRownow share. No selector changes.coverage/taxonomy.yaml— both specs added tospecs:;list-rulesandedit-ruleflipped tocovered: trueatt2-cujwith scoping notes.No FE change was needed — every element already had a
data-testidor a stablerole.
On the
edit-ruleclaim: test 1 opens the dialog through the row's Editaction, asserts what it hydrated, and submits it, so the capability is genuinely
exercised. But editing a field through the dialog — renaming, changing the
model, rewriting the prompt — is still uncovered, and the taxonomy
note:saysso rather than implying more than was tested.
What I deliberately did not write
The exploration produced two candidates and both are here; nothing was
dropped. The list was already filtered upstream — three of its five ranked
items produced no candidate:
000121against clustered ClickHouse) — skipped duringexploration: deployment-level DDL on a shared managed environment.
aiu_nanoon ingested copilot spans) — blocked:cipx_spends/cipx_spend_blockshave DAOs and an ingestion listener but no REST readsurface, so the write is triggerable and the assertion is not without direct
ClickHouse access.
registry, but that is release plumbing, not app behaviour; there is nothing in
the e2e estate for it to assert.
Items 3 and 4 are the whole of #8155
and neither got a hands-on check on staging. If anything in this release still
needs a human, it is those two, and it needs a self-hosted deployment rather than
staging.
One incidental thing the exploration flagged, unrelated to this release and not
asserted on here: the online-evaluation page fires three failing background
requests on load regardless of rule content —
403 GET /v1/private/datasets/export-jobs,404 GET /v1/private/agent-configs/blueprints/history/projects/{id},404 GET /v1/private/agent-insights/jobs/{id}. Spec 1's wire check is scoped to/automations/evaluatorsand to 5xx precisely so it does not become a monitorfor those.