[OPIK-8262] [QA] Proposed e2e spec from the #8162 exploration: thread-scoped rule fan-out - #8167
Conversation
Proposed by the QA test-radar side flow from a human-verified exploration of #8162, and written against that PR's head. A manual evaluation naming N threads against one thread-scoped rule must become N independent scoring cycles -- the behaviour enqueueThreadMessage now produces by emitting one stream entry per thread id rather than one entry carrying the whole list. The spec asserts entities_queued, exactly one evaluator call per thread (not 1, not 9), the score on every thread server-side, and the score rendered in every thread's panel. The evaluator-call count is the load-bearing assertion: all three outcomes -- correct, collapsed, duplicated -- end with every thread carrying a score, because the duplicate case overwrites its own value. Supporting: a thread-cohort fixture that seeds three conversations and resolves their thread model ids (throwing if any never aggregates), a thread-scoped variant of the constant-score python metric, and backend client support for trace_thread_user_defined_metric_python rules and POST /v1/private/manual-evaluation/threads. Taxonomy: online-evaluation.rule-scope-thread-span flipped to covered for THREAD scope only; span scope stays untested and the note says so. automation-logs deliberately stays false -- this spec reads the log stream over the API and never opens the page that key names. 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 timingNo linted files changed — nothing to run. ⏭️ 44 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| arguments?: Record<string, string>; | ||
| /** | ||
| * Which evaluator the rule is. Defaults to the trace-scoped python | ||
| * metric; `trace_thread_user_defined_metric_python` is the thread-scoped | ||
| * one, which fires per conversation rather than per trace. | ||
| */ | ||
| type?: 'user_defined_metric_python' | 'trace_thread_user_defined_metric_python'; | ||
| triggerScope?: 'production' | 'experiment' | 'both'; | ||
| enabled?: boolean; | ||
| }): Promise<string> { | ||
| const type = args.type ?? 'user_defined_metric_python'; | ||
| const isThreadScoped = type === 'trace_thread_user_defined_metric_python'; |
There was a problem hiding this comment.
Empty arguments bypass local validation
The trace-scoped guard accepts an explicitly empty arguments object because it checks only truthiness, so a valid TypeScript call reaches rawFetch with code.arguments: {} and PythonEvaluatorService.evaluate rejects the empty data map before the metric runs — should we reject empty maps with Object.keys(args.arguments ?? {}).length === 0 and add a regression test that rawFetch is not called?
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 1532-1543, update
`createAutomationRule` so trace-scoped evaluators reject `arguments` when it is missing
or an empty object, not just when it is falsy. Validate the map by checking its key
count before calling `rawFetch`, while preserving the thread-scoped restriction. Add a
regression test asserting that an empty argument map throws and `rawFetch` is not
called.
| threads: (page.content ?? []).map((t) => ({ | ||
| id: String(t.id ?? ''), | ||
| threadModelId: t.threadModelId ?? null, |
There was a problem hiding this comment.
Prefilter test misses wrong thread UUID
listThreads exposes threadModelId, but comparable() in thread-id-prefilter.spec.ts omits it, so EQUAL, CONTAINS, and windowed differential checks can pass with incorrect model IDs — should we add threadModelId: row.threadModelId to the projection?
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 1728-1730, ensure the new
`listThreads` `threadModelId` field is covered by the shared `comparable()` projection
used by `thread-id-prefilter.spec.ts`. Add `row.threadModelId` alongside the other
projected thread fields so EQUAL, CONTAINS, and windowed differential assertions detect
incorrect or missing operational UUIDs.
| // The first place the split is observable. A request that collapsed to a | ||
| // single entry still answers 202. | ||
| expect(queued.entitiesQueued, 'one entry per thread named in the request').toBe( | ||
| threads.length, |
There was a problem hiding this comment.
Queued count falsely proves fan-out
entitiesQueued counts submitted entities because evaluateThreads returns threadModelIds.size() while discarding enqueueThreadMessage's Mono<Void>, so packing all three IDs into one Redis entry still returns 3 and passes the assertion. Could we keep it as confirmation that three entities were accepted and update the comment/assertion message to use the evaluator-call/per-thread log assertions as fan-out evidence 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/tests/online-evaluation/online-evaluation-thread-fanout.spec.ts`
around lines 81-84, update the `entitiesQueued` assertion in the manual evaluation step
because it confirms only that the requested entities were accepted, not that one Redis
stream entry was created per thread. Keep the assertion as an accepted-entity-count
check, but revise the comment and assertion message so they do not call it evidence of
splitting; rely on the evaluator-call and per-thread log assertions later in the test
for fan-out verification.
… assertions Review of the thread fan-out spec. Three contained readability fixes; no behaviour change: - state the 300s timeout budget, as both neighbouring online-evaluation specs do, and why it sits just above the inner 180s poll - give the cohort set-comparison and the per-thread panel score assertion failure messages, so a 3am failure names which thread rendered wrong rather than only "expected 1, received 0" inside a loop Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 Generated-test reviewPR #8167 — OPIK-8262 [QA] Proposed e2e spec from the #8162 exploration: thread-scoped rule fan-outCometActions · draft: yes · 7 files, 1 new spec VERDICT: ready with notes — one taxonomy decision needs a human before merge. The spec is genuinely good. It asserts the thing its tag claims, its teardown is in Blockers (0)None. Should fix (1) — needs a human decision, not fixable by me1. The capability key names two scopes. The spec covers thread scope only; the PR This is exactly the case the brief forbids me to fix myself: splitting the key into
I lean (a), because a note is prose that no tool reads, and span scope is a real Notes (2)1. The
I re-ran the typecheck against a repaired config (
Both fixes are estate-wide and belong in their own PR, not stapled to a test PR — 2. The PR is stacked on an unmerged backend branch. Base is What I verified (and how)
Fixes applied and pushedCommit
Bot comments
Estate gatesRe-run after my edits, all three: tag_lint PASS · tsc FAIL (pre-existing, not this PR) · playwright --list PASS
What I could not verify
Do not merge — and I have not marked it readyPer the standing rule I have not flipped the draft flag and have not merged, review_generated_tests.yml · |
| // Budget for the longest chain: the cohort fixture's 60s thread-aggregation | ||
| // poll, the 180s wait for the rule to store scores, then three thread panels | ||
| // opened in sequence. Kept just above the 180s inner poll so that one fires | ||
| // first — it fails naming the threads that never got scored, which beats an | ||
| // opaque "test timeout exceeded". | ||
| test.setTimeout(300_000); |
There was a problem hiding this comment.
300-second timeout cannot cover UI chain
The outer timeout leaves only 60s after fixture and scoring, while the three serial waitForFullyLoaded calls can consume 180s before navigation and assertions, so valid runs can terminate before the final panel assertion — should we increase the budget to cover the full chain and overhead?
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/tests/online-evaluation/online-evaluation-thread-fanout.spec.ts`
around lines 42-47, update the outer test timeout budget for the thread fanout scenario.
It must exceed the fixture and scoring phases (up to 240 seconds), all three serial
`waitForFullyLoaded` calls (up to 60 seconds each), and navigation/assertion overhead,
so a valid run cannot time out before the final panel assertion. Update the accompanying
comment to document the revised worst-case budget.
1 participant
What this is
One new e2e spec proposed by the QA test-radar side flow, written from a human-verified
exploration of #8162 (OPIK-8262 classify provider errors by status and stop collapsing
trace-thread fan-out).
This PR targets
thiagohora/OPIK-8262-retry-classification-and-fanout, notmain. The spec waswritten and run against that branch's head,
c20c9e018a23b9fc45927d0224c80959cc7208ae, which isalso the commit deployed to the PR environment the exploration used. It is a regression guard on
the fan-out this PR rewrote, so it belongs on top of the change it tests; retargeting it to
mainonce #8162 merges is a rebase, not a rewrite.
Generated by an automated QA flow. It needs human review before merge and is deliberately a
draft. No reviewers requested — a human promotes it.
Where it came from
The exploration ran on the PR's own deployment (
pr-8162.dev.comet.com, 2.2.53-8162-merge-3189,OSS install, workspace
default) and reached both halves of the change by hand before any specexisted. It produced two strong candidates; this PR contains the one of them that can be written
as a deterministic, self-contained spec. The other is dropped, with reasons, below.
The spec
tests/online-evaluation/online-evaluation-thread-fanout.spec.ts— ✅ passes@t2-cuj·@area:online-evaluation·@cap:online-evaluation.rule-scope-thread-spanWhat it asserts. A manual evaluation naming N threads against one thread-scoped rule becomes
N independent scoring cycles — the behaviour
OnlineScorePublisher.enqueueThreadMessagenowproduces by emitting one stream entry per thread id instead of one entry carrying the whole list.
Concretely, over a 3-thread cohort:
entities_queued: 3,rules_applied: 1;the split) and not 9 (each entry still carrying all three) — and each thread id appears in
exactly one of them;
1.0,source: online_scoring, comparedas the whole score set per thread rather than by looking this rule's score up among others;
The evaluator-call count is the load-bearing assertion. All three failure modes end with every
thread carrying a score — the duplicate case overwrites its own value — so the scores alone cannot
tell a correct fan-out from a collapsed one. Only the call count can.
Why it is deterministic. A constant-1.0 python thread metric: no provider key, no model
verdict, no wall-clock dependence. The rule sits at
sampling_rate: 0, so the production samplercan never fire it and the manual request is the only possible trigger — and the spec asserts every
thread is unscored before the request, so the later assertions are attributable to the fan-out
rather than to ordinary sampling.
What it does not prove. On the happy path the pre-#8162 code also scored all three threads: it
iterated the id list inside a single stream entry. So this is a regression guard on the split
(nothing lost, nothing duplicated), not a before/after discriminator. The behaviour only the
new code has — a per-thread retry budget, so one thread's retryable failure replays only that
thread — needs a retryable failure in the thread scorer, which the exploration could not provoke on
this estate (the python evaluator's failures are terminal 400s, and the thread LLM path needs a
provider key). It is deliberately out of scope here rather than asserted weakly.
Supporting changes
fixtures/thread-cohort.fixture.ts(new) — seeds three independent threads (2 turns each)through the public SDK bridge and resolves each to its
thread_model_id, which is whatmanual-evaluation/threadsaddresses. It polls until all three threads have been aggregated andthrows naming the missing ones if they never are: a spec handed a short cohort would assert
over whatever arrived and pass having tested a smaller fan-out than it claims. No teardown — the
traces, threads and scores all live in the
projectfixture's project, and the rule is cleanedup by the existing
automationRulesCleanupfixture. The suite's own run-prefix sweep reportednothing left behind after the verification run.
core/backend/client.ts—createAutomationRulegains atypeso it can build atrace_thread_user_defined_metric_pythonrule (thread code carries ametricand no argumentmap; passing one is now a hard error rather than a field sent and ignored); new
evaluateThreadsManually, which the pinned SDK cannot express at all;ThreadRowRefgainsthreadModelId.core/metrics/python-metric-source.ts—buildConstantThreadScoreMetric. A separate builderfrom
buildConstantScoreMetricbecause the python backend dispatches the two differently:metric.score(data)positionally fortrace_thread_*rules,metric.score(**data)fortrace-scoped ones.
coverage/taxonomy.yaml— spec added to the area'sspecs:list;online-evaluation.rule-scope-thread-spanflipped tocovered: true, tier: t2-cujwith a notescoping it to thread scope only — no spec creates a span-scoped rule, so that half stays
untested and the note says so rather than letting the key read as fully covered.
online-evaluation.automation-logsis deliberately leftcovered: false: this spec reads therule log stream over
GET /automations/evaluators/{id}/logsand never opens the/$workspaceName/automation-logspage that key names, which is exactly the distinction theexisting comment on that entry asks callers to respect.
Verification
Run against the PR's own deployment — the same environment and build the exploration used:
OPIK_API_KEYis set only because this target's hostname ends incomet.com, which makes thepinned TS SDK demand a key even though the install has no auth. It is not needed for a local OSS
run and the spec does not depend on it.
Also run:
python3 tests_end_to_end/coverage/tag_lint.py --taxonomy tests_end_to_end/coverage/taxonomy.yaml --estate tests_end_to_end→60 specs checked, 1 exempt, 0 problem(s).A deliberate mutation of the evaluator-call assertion (
3→4) was run and failed asintended, so that assertion is known to discriminate rather than merely to pass.
tests/online-evaluation/in full, becausecore/backend/client.tsandfixtures/index.tsare shared: 7 passed, 1 skipped, 1 failed (1.7m).
The one failure is
online-evaluation-python-metric-errors.spec.ts, and it is not caused bythis change — it fails identically on the unmodified branch, verified by re-running it with
these changes stashed. On this deployment a python metric that exits 0 without a result line is
still reported as
Python evaluation failed (HTTP '500'): 500 Internal Server Error: Failed to execute code, where that spec expects the classified400 Bad Request: Execution failed: the metric produced no output. Worth someone's attention — either the classification fix is not inthis build's python backend, or it has regressed — but it is out of scope here and nothing in
this PR touches it.
Typecheck, with a caveat you should know about.
npx tsc --noEmitdoes not run on this branchtoday, and did not before this change:
tsconfig.jsonstill setsbaseUrl, which the pinnedtypescript@^7.0.2removed (error TS5102). It was typechecked with TypeScript 5.9 instead, whichreports exactly one error — a pre-existing duplicate
deleteDashboardincore/backend/client.tspresent on the unmodified branch too. Zero new errors from this change. Both are worth fixing, but
not in a QA spec PR.
What was deliberately not written
The exploration's other strong candidate — "a permanent provider failure is attempted once; a
transient one is retried", the retry-classification half of this PR — is dropped. It is the
more valuable of the two and it was verified by hand (401 → 1 delivery held over 30 minutes;
429 → 3 deliveries at exact 10-minute intervals), but it cannot currently be written as a spec this
suite should own:
such facility is the mock gateway in
services/mock-token-auth/, which answers 401 only and,per
core/mock-auth.ts'smockAuthSkipReason(), is reachable only from a local backend —never from a deployment, including the one where this behaviour can be observed. The
exploration's substitute was
https://httpbingo.org/status/<code>?x=; a permanent spec whoseverdict depends on a third-party service is a flake source and is ruled out by the estate's own
determinism rule.
onlineScoring.pendingMessageDuration— 10 minutes, deployment configuration, not settablefrom a test. Both halves of the pair need it: without waiting out a redelivery window, "attempted
once" and "attempted once so far" are the same observation.
Asserting only the permanent half would be worse than nothing: it would pass equally well if every
provider error had been made non-retryable, which is the direction that silently loses evaluations.
The prerequisite is a small one — teach
services/mock-token-auth/mock_token_auth_service.pytoanswer a caller-chosen status, then write the spec against
mockGatewayUrlForBackendon a localOSS run where
REDIS_SCORING_PENDING_MESSAGE_DURATIONcan be turned down. That is a change worthmaking, and it needs a local OSS backend to verify on, which this flow did not have. Shipping the
spec unverified against that unbuilt facility would have been a guess.
Also not covered, and named here so the list reads as filtered rather than exhausted: the automatic
thread-close fan-out path (
TraceThreadOnlineScorerPublisher— same split, same scorer, differentcaller; gated behind a 15-minute inactivity timeout), the VertexAI/GAX branch, and the legacy
multi-id migrate branch. The exploration could not reach any of them either.
Review notes
.agents/skills/writing-e2e-tests/SKILL.mdandconventions.mdonmain. One stepof that workflow was not run: step 3, live-UI discovery via the Playwright MCP, which is not
available to this flow. No new page object was written to compensate — the spec drives the
existing
LogsPageandThreadPanelPageonly, and adds no new selector.column-picker interaction no POM in the estate models, and the thread panel's Feedback scores tab
is where a user actually reads a thread-level score (the table hides those columns by default) —
so the spec asserts there, for all three threads rather than a sample.
Related: #8162