feat(experiments-v3): pairwise compare end-to-end with winner-by-id label#5142
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds pairwise compare evaluator support end to end: backend contracts and payload passthrough, native judge execution, phase-2 orchestrator flow, aggregates and exports, and V3 editor/table/header rendering for pairwise verdicts. ChangesPairwise compare feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aryansharma28
force-pushed
the
worktree-pairwise-mvp
branch
from
June 26, 2026 11:06
cd159e7 to
08a0df6
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@langevals/evaluators/langevals/langevals_langevals/pairwise_compare.py`:
- Around line 189-194: The prompt rendering in PairwiseCompareEvaluator
currently uses self.settings.prompt.format with a fixed key set, so unknown
user-defined placeholders can raise KeyError and crash evaluation. Update the
prompt rendering logic in PairwiseCompareSettings/PairwiseCompareEvaluator to
handle missing placeholders safely by either switching to a forgiving
substitution approach that leaves unknown fields intact or wrapping the format
call in a KeyError guard and falling back gracefully. Keep the existing input,
golden, and candidate output substitutions, but ensure custom prompts with extra
tokens do not fail.
In `@langwatch/src/components/checks/DynamicZodForm.tsx`:
- Around line 484-491: The `DynamicZodForm` logic uses an explicit `any` in the
`isLiteralUnion` check, which violates the no-explicit-any rule. Replace the
`any` annotation on the `ZodUnion` options callback with a safer type such as
`unknown`, then narrow it with the existing `instanceof z.ZodLiteral` check in
the `element.options.every(...)` path. Keep the fix localized to the
`fieldSchema_`, `element`, and `isLiteralUnion` block so the `include_metrics`
handling still infers literal option values correctly.
In `@langwatch/src/components/evaluators/EvaluatorEditorShared.tsx`:
- Around line 133-141: `pairwiseContext.targets` in `EvaluatorEditorShared`
expects full `TargetConfig[]`, but `useOpenEvaluatorEditor` is currently
stripping each target down to only `{ id }`, which breaks name resolution in
`useTargetName`. Update the `pairwiseContext` construction in
`useOpenEvaluatorEditor` to pass the original `targets` array through unchanged
instead of mapping it to id-only objects, so pairwise editor forms can display
human-readable target names.
In
`@langwatch/src/experiments-v3/components/EvaluatorPanel/PairwiseConfigForm.tsx`:
- Around line 153-159: The update helper in PairwiseConfigForm should not call
onChange inside the setDraft updater because the updater must stay pure and can
run twice under StrictMode. Move the onChange(next) call out of the setDraft
callback in the update function so PairwiseConfigForm.update only computes and
stores the next draft state, then notifies the parent once with the finalized
next value.
In `@langwatch/src/experiments-v3/components/PairwiseAggregateHeader.tsx`:
- Around line 111-164: The PairwiseAggregateHeader component is dead code and
should be removed since it is not rendered or imported anywhere. Delete the
PairwiseAggregateHeader export and its file, and then check whether
ResolvedAggregateBar or any related aggregate-header component has no remaining
runtime consumers; if so, remove that as well to keep the experiments-v3
components clean.
In `@langwatch/src/experiments-v3/components/PairwiseCompareCell.tsx`:
- Around line 187-229: The verdict rendering in PairwiseCompareCell is
duplicated in both the ResolvedVerdict path and the no-variant fallback,
contributing to the file exceeding the source-line limit. Extract the shared
winner/tie plus Popover markup into a reusable VerdictBody component that takes
winnerSide, winnerName, loserName, and reasoning props, then use that component
in both locations to remove the repeated JSX and bring the file under the
threshold.
In `@langwatch/src/experiments-v3/hooks/useEvaluationsV3Store.ts`:
- Around line 684-694: The pairwise mapping merge in useEvaluationsV3Store is
preserving stale candidate_a/candidate_b/golden/input entries when a pick is
cleared because existingTarget.mappings is spread before the newly derived
mappings. Update the mapping merge so the pairwise-managed keys are removed from
the prior dataset mapping before applying derivePairwiseTargetMappings, using
the existing state.datasets loop and the newDatasetMappings assembly to rebuild
each dataset’s mapping from only the current pairwise selections.
In `@langwatch/src/experiments-v3/utils/__tests__/computeAggregates.test.ts`:
- Around line 700-768: The current pairwise aggregate tests only cover legacy
A/B/tie labels and never exercise candidate-id normalization in
computePairwiseAggregate/normalizePairwiseLabel. Add a test case in
computeAggregates.test.ts where the processed result uses a variant identifier
(the target id or prompt handle) and provide targets with promptId so the label
must be normalized instead of skipped. Assert that the aggregate counts and
perRow entry are recorded correctly for this handle-vs-promptId path.
In `@langwatch/src/experiments-v3/utils/computeAggregates.ts`:
- Around line 339-360: The pairwise aggregate lookup is using promptId as the
“handle,” but orchestrator.ts emits the prompt handle as the label, so labels
never match and rows become null. Update computeAggregates.ts in the
variantAHandle/variantBHandle resolution and the normalizePairwiseLabel call so
the function receives the actual prompt handle (or otherwise align both sides on
the same identifier); if targets only expose id/promptId, thread the real handle
into this function instead of deriving it from promptId.
In `@langwatch/src/server/experiments-v3/execution/orchestrator.ts`:
- Around line 296-309: The verdict identifier emitted by variantIdentifierFor is
the prompt handle when available, but computeAggregates must not assume the
echoed label is promptId. Update the aggregation matching logic to accept the
handle returned by variantIdentifierFor (and only fall back to promptId/id when
needed) so handle-based verdicts are correctly associated; use
variantIdentifierFor and the prompt lookup in loadedPrompts as the reference
points when aligning the contract.
In `@specs/experiments/pairwise-compare-mvp.feature`:
- Around line 24-26: The pairwise-compare MVP spec still asserts the old verdict
labels, which conflicts with the new evaluator contract. Update the scenario
around the row-level verdict result in pairwise-compare-mvp.feature to match the
actual emitted label format from the evaluator, using the winning candidate
identifier or "tie" instead of only "A"/"B"/"tie". Align this wording with the
label handling in TargetCell so the spec and implementation describe the same
result shape.
- Around line 35-39: The scenario text is stale and does not match what
AggregateHeaderBar actually renders. Update the Then steps in the pairwise
compare feature to reflect the shipped scoreboard format from AggregateHeaderBar
and the browser test’s “New scoreboard format” expectation, including the
leader/other wording, ties, and total verdicts. Remove the “Bias-corrected”
indicator assertion since that element is not rendered by the header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: faa6339e-f283-4915-8534-326f2cfe4621
⛔ Files ignored due to path filters (12)
docs/pr5106/01-pairwise-config-form.pngis excluded by!**/*.pngdocs/pr5106/02-aggregate-header-bar.pngis excluded by!**/*.pngdocs/pr5106/03-aggregate-header-losses.pngis excluded by!**/*.pngdocs/pr5106/04-row-verdict-a-wins.pngis excluded by!**/*.pngdocs/pr5106/05-row-verdict-tie.pngis excluded by!**/*.pngdocs/pr5106/06-row-verdict-b-wins.pngis excluded by!**/*.pngdocs/pr5106/07-evaluator-chip-tints.pngis excluded by!**/*.pngdocs/pr5106/08-llm-judge-list-live.pngis excluded by!**/*.pngdocs/pr5106/09-pairwise-settings-form-live.pngis excluded by!**/*.pngdocs/pr5106/10-workbench-pairwise-form-live.pngis excluded by!**/*.pngdocs/pr5106/11-experiments-workbench-live.pngis excluded by!**/*.pngdocs/pr5106/12-mapping-gap-demonstrated.pngis excluded by!**/*.png
📒 Files selected for processing (42)
langevals/evaluators/langevals/langevals_langevals/pairwise_compare.pylangevals/evaluators/langevals/tests/test_pairwise_compare.pylangevals/langevals_core/langevals_core/base_evaluator.pylangevals/scripts/generate_evaluators_ts.pylangevals/ts-integration/evaluators.generated.tslangwatch/src/components/checks/DynamicZodForm.tsxlangwatch/src/components/evaluators/EvaluatorEditorShared.tsxlangwatch/src/components/evaluators/EvaluatorTypeSelectorContent.tsxlangwatch/src/components/targets/TargetTypeSelectorDrawer.tsxlangwatch/src/experiments-v3/components/AggregateHeaderBar.tsxlangwatch/src/experiments-v3/components/EvaluationsV3Table.tsxlangwatch/src/experiments-v3/components/EvaluatorPanel/PairwiseConfigForm.tsxlangwatch/src/experiments-v3/components/PairwiseAggregateHeader.tsxlangwatch/src/experiments-v3/components/PairwiseCompareCell.tsxlangwatch/src/experiments-v3/components/PairwiseVerdictRow.tsxlangwatch/src/experiments-v3/components/RowVerdictStrip.tsxlangwatch/src/experiments-v3/components/TableMetaWrappers.tsxlangwatch/src/experiments-v3/components/TargetSection/EvaluatorChip.tsxlangwatch/src/experiments-v3/components/TargetSection/TargetCell.tsxlangwatch/src/experiments-v3/components/TargetSection/TargetHeader.tsxlangwatch/src/experiments-v3/components/__tests__/pairwise-preview.browser.test.tsxlangwatch/src/experiments-v3/hooks/useEvaluationsV3Store.tslangwatch/src/experiments-v3/hooks/useExecuteEvaluation.tslangwatch/src/experiments-v3/hooks/useOpenEvaluatorEditor.tslangwatch/src/experiments-v3/hooks/useOpenTargetEditor.tslangwatch/src/experiments-v3/types.tslangwatch/src/experiments-v3/utils/__tests__/computeAggregates.test.tslangwatch/src/experiments-v3/utils/__tests__/pairwiseHandoffs.test.tslangwatch/src/experiments-v3/utils/computeAggregates.tslangwatch/src/experiments-v3/utils/evaluatorEditorCallbacks.tslangwatch/src/experiments-v3/utils/mappingInference.tslangwatch/src/experiments-v3/utils/mappingValidation.tslangwatch/src/experiments-v3/utils/pairwiseHandoffs.tslangwatch/src/server/background/workers/evaluationsWorker.tslangwatch/src/server/evaluations/evaluators.generated.tslangwatch/src/server/evaluations/evaluators.tslangwatch/src/server/experiments-v3/execution/__tests__/orchestrator.test.tslangwatch/src/server/experiments-v3/execution/orchestrator.tslangwatch/src/server/experiments-v3/execution/types.tslangwatch/src/server/routes/__tests__/evaluator-input-coercion.unit.test.tslangwatch/src/server/routes/evaluations-legacy.tsspecs/experiments/pairwise-compare-mvp.feature
Contributor
Author
Visual walkthrough — all flows
If reviewers want fresh screenshots before merge, ping me and I'll re-render against the latest HEAD and post them as plain image uploads (no codebase commit). |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
langwatch/src/server/experiments-v3/execution/orchestrator.ts (3)
247-556: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the new exported
generatePairwiseCells.
langwatch/src/server/experiments-v3/execution/orchestrator.tsLine 247 addsgeneratePairwiseCells, and the function spans Lines 247-556. Break it into focused helpers for dependency resolution, pairwise payload assembly, and synthetic evaluator config creation. As per path instructions: “FAIL when a NEW exported function exceeds 60 source lines or has cyclomatic complexity > 10.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@langwatch/src/server/experiments-v3/execution/orchestrator.ts` around lines 247 - 556, The new exported generatePairwiseCells function is too large and complex, so split it into smaller helpers while keeping the same behavior. Extract separate helpers for resolving variant targets, building pairwise candidate payloads, and creating the synthetic evaluator config, then have generatePairwiseCells orchestrate those helpers. Keep the exported entrypoint focused and ensure each helper is referenced from generatePairwiseCells so the logic remains unchanged.Source: Path instructions
285-289: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse object parameters for new multi-argument helpers.
withEvaluatorScores(output, rowIndex, variantId)at Line 285 andsetIfDefined(key, value)at Line 847 violate the TypeScript rule for multi-argument functions. Change the signatures and local call sites to object destructuring. As per coding guidelines: “Use named parameters via object destructuring for functions with multiple arguments:fn({ a, b })notfn(a, b).”Also applies to: 847-849
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@langwatch/src/server/experiments-v3/execution/orchestrator.ts` around lines 285 - 289, The new helper signatures use positional parameters, which conflicts with the TypeScript guideline for multi-argument functions. Update withEvaluatorScores and setIfDefined to accept a single object parameter using named destructuring, then adjust every local call site in orchestrator.ts to pass an object instead of separate arguments. Keep the function names and their internal usage consistent so the refactor is applied everywhere these helpers are called.Source: Coding guidelines
232-235: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRoute pairwise evaluator re-runs through Phase 2.
langwatch/src/server/experiments-v3/execution/orchestrator.tsLine 235 says pairwise evaluators run in Phase 2, butscope.type === "evaluator"still schedules[evaluatorConfig]in Phase 1 with nocell.pairwise. The new input builder only fillscandidate_a_*/candidate_b_*whencell.pairwiseexists;langevals/pairwise_comparerequires those fields, so a single pairwise evaluator re-run fails instead of producing a verdict. Route this scope through dependency expansion +generatePairwiseCells, or reject pairwise evaluator scope. As per coding guidelines: “Do not write comments describing behavior that the code doesn't actually implement.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@langwatch/src/server/experiments-v3/execution/orchestrator.ts` around lines 232 - 235, The Phase 1 evaluator scheduling in orchestrator.ts still sends pairwise evaluators through the normal single-cell path, which does not populate the candidate_a/candidate_b inputs required by pairwise_compare. Update the scope handling in the orchestrator flow that builds evaluator work for scope.type === "evaluator" so pairwise configs are either expanded through the same dependency logic and generatePairwiseCells path used for Phase 2, or explicitly rejected before scheduling. Make sure the evaluatorConfig filtering and pairwise cell creation are aligned so pairwise re-runs cannot enter Phase 1 without cell.pairwise data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@langwatch/src/server/experiments-v3/execution/orchestrator.ts`:
- Around line 247-556: The new exported generatePairwiseCells function is too
large and complex, so split it into smaller helpers while keeping the same
behavior. Extract separate helpers for resolving variant targets, building
pairwise candidate payloads, and creating the synthetic evaluator config, then
have generatePairwiseCells orchestrate those helpers. Keep the exported
entrypoint focused and ensure each helper is referenced from
generatePairwiseCells so the logic remains unchanged.
- Around line 285-289: The new helper signatures use positional parameters,
which conflicts with the TypeScript guideline for multi-argument functions.
Update withEvaluatorScores and setIfDefined to accept a single object parameter
using named destructuring, then adjust every local call site in orchestrator.ts
to pass an object instead of separate arguments. Keep the function names and
their internal usage consistent so the refactor is applied everywhere these
helpers are called.
- Around line 232-235: The Phase 1 evaluator scheduling in orchestrator.ts still
sends pairwise evaluators through the normal single-cell path, which does not
populate the candidate_a/candidate_b inputs required by pairwise_compare. Update
the scope handling in the orchestrator flow that builds evaluator work for
scope.type === "evaluator" so pairwise configs are either expanded through the
same dependency logic and generatePairwiseCells path used for Phase 2, or
explicitly rejected before scheduling. Make sure the evaluatorConfig filtering
and pairwise cell creation are aligned so pairwise re-runs cannot enter Phase 1
without cell.pairwise data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fccbc943-01af-4a9c-b649-6c2032968b84
⛔ Files ignored due to path filters (4)
docs/screenshots/pr5142/01-add-comparison-purple-swords.pngis excluded by!**/*.pngdocs/screenshots/pr5142/02-pairwise-drawer-full-form.pngis excluded by!**/*.pngdocs/screenshots/pr5142/03-verdicts-rendered.pngis excluded by!**/*.pngdocs/screenshots/pr5142/04-reasoning-popover.pngis excluded by!**/*.png
📒 Files selected for processing (1)
langwatch/src/server/experiments-v3/execution/orchestrator.ts
Contributor
Author
✅ Verified end-to-end — code + power-user demoTests
Power-user end-to-end demoAttached an
In the screenshot:
Live data from this run
What this proves
Ready for review + merge. |
Aryansharma28
commented
Jun 26, 2026
Aryansharma28
left a comment
Aryansharma28
left a comment
Contributor
Author
There was a problem hiding this comment.
Goated review — verified pass
Treating this as a deep adversarial review rather than a courtesy stamp. Methodology: read existing reviews (CodeRabbit + bots), bucket the 60-file diff by risk, deep-read the high-risk buckets (orchestrator, evaluator dispatch, langevals contract, new client store paths), adversarially verify each finding, drop on uncertainty, dedupe against existing inline comments. Did not re-raise anything CodeRabbit already flagged.
Existing findings still load-bearing (NOT re-raised)
- @coderabbitai's "handle vs promptId" pair (orchestrator.ts:354 + computeAggregates.ts:360) — confirmed real.
variantIdentifierForemits the prompt handle (e.g.say-hi) ascandidate_a_id/candidate_b_id, which langevals echoes back as the verdict label. ButcomputePairwiseAggregateFromResultslooks upt.promptId(the KSUID, e.g.prompt_6IFkbb…) and passes it as the "handle" tonormalizePairwiseLabel. These will never match → aggregate counts come out0/0/0→ the column-header readsTiedeven when there are real wins. Cell-level rendering inPairwiseCompareCellis unaffected (it usesuseTargetNameto resolve the handle and compares against that). Column-header summary is wrong. This is the single most important fix on the PR. - @coderabbitai's PairwiseAggregateHeader.tsx dead-code call-out — the file is no longer imported (
EvaluationsV3Table.tsxstrips it when the aggregate bar was removed). Recommend deleting the file + the unuseduseEvaluationsV3Store/useShallow/PLACEHOLDER_TARGETplumbing inside it. - @coderabbitai's StrictMode purity warning on
PairwiseConfigForm.tsx:159— callingonChange(next)insidesetDraft's updater is real. Easy fix: computenextoutside, set draft, thenonChange.
New findings (not flagged elsewhere)
- A1 —
[NON-BLOCKER]orchestrator.ts~line 1524: skip-reasonpushEventloop doesn't checkabortedmid-iteration; if the user aborts during emission, the UI still receives synthetic "Waiting on..." errors. One-line fix:if (await abortManager.isAborted(runId)) break;at the top of the loop. - A2 —
[NON-BLOCKER]PairwiseCompareCell.tsxfriendlyError— the "Waiting on an upstream variant" branch sets the headline to a generic phrase AND uses the raw detail (which already begins with "Waiting on…") as the hint. Reads twice. Drop the hint (the raw is already on the popover) or shorten the headline. - A3 —
[NON-BLOCKER]orchestrator.ts:333-338(withEvaluatorScoresJSON.stringify catch): the fallbackString(output ?? "")produces"[object Object]"for non-serializable outputs and feeds it to the judge prompt — silently corrupts the candidate text. Safer: ifJSON.stringifythrows AND the output isn't a string, skip the score block entirely and return the original output.
Verdict
Comment — not requesting changes. The handle/promptId aggregate bug is the only "must fix before this is honestly shipped" item and it's CodeRabbit's, not mine. Once that's addressed the rest is polish.
What I couldn't verify from the diff alone
- Whether any deployed dashboard/runbook references the column-header score format — if monitoring depends on the aggregate counts, the handle/promptId bug has user-visible blast radius that I can't size from here.
- Whether the
seedTargetOutputsround-trip behaves correctly when the client and server disagree on what an output looks like (e.g. legacy non-string outputs from older runs). Sample test pre-merge would buy more confidence.
Aryansharma28
force-pushed
the
worktree-pairwise-mvp
branch
from
June 26, 2026 14:38
e2ae93c to
d12e056
Compare
Contributor
Author
Goated-review findings — resolvedPushed BLOCKER (CodeRabbit)
NON-BLOCKERS
Verification
CI is running on the new commit; will keep watching until green and then hand back for Sergio's re-review. |
Collaborator
Ruthless review — pairwise compare (54 files / +6.4k)Verdict: not mergeable as-is. Two P1 defects (one breaks a shipped surface, one can clobber stored verdicts on any scoped rerun) plus a wire-contract regression that hits every langevals evaluator. Core swap-and-confirm logic and the column-target happy path are sound; the failures are at the edges — scope handling, label format across surfaces, and the shared input path. All findings traced to source. P1 — Evaluator-chip pairwise surface is dead against current langevals output
P1 — Phase 2 is scope-blind: any scoped rerun regenerates the whole pairwise column
P2 — Unbounded extras passthrough widens the wire contract for every langevals evaluator
P2 — Spec deliverables shipped as tested-but-unwired dead code (PR honesty)
P2 — Python judge output parse is unguarded; the headline
|
Contributor
Author
All CodeRabbit findings resolved — ready for re-reviewPushed three follow-up commits since the last review ( Disposition
Verification
10 of 12 fixed in-PR. The 2 skipped are: an unrelated @sergioestebance — ready for your re-review. This is the bulletproof pass. |
Contributor
Author
Self goated-review — one more bug found and fixed (
|
Collaborator
Ruthless review — verdict: 🟡 functionally sound, not clean-merge-readyCI is fully green and mergeable, and the core runtime path is correctly implemented and well-tested: per-row verdicts, winner-by-id Two issues should be cleared before merge; the rest are follow-ups. None are runtime regressions. P2 — Dead code shipped + spec describes unreachable features
Feature files are the requirements — this is green CI on code that doesn't ship + a spec that lies. Fix: either wire P2 — Unscoped contract widening in the dispatch layer
P3 — follow-ups, not blocking
Already addressed (verified — not re-flagging)Handle/promptId mismatch (fixed + tested), Residual testing gapNo test drives the real Recommendation: clear the two P2s before merge (both fast: delete-or-wire the dead modules + trim the spec; gate the 🤖 ruthless-review |
Contributor
Author
|
Both P2s addressed at P2#1 — dead code + spec lies →
P2#2 — unscoped extras passthrough →
Bonus — chip-render dead under new contract →
Exported
Typecheck clean; 762/762 unit tests passing locally across P3 follow-ups ( |
Adds Python entry/settings/result classes for the new langevals/pairwise_compare evaluator plus the BDD spec under specs/experiments/. evaluate() returns Skipped for now — implementation lands in follow-up commits per the steps in the issue. Parent epic: #5099 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implement the Python evaluator body for pairwise compare:
- _judge() runs one litellm tool-call with a deterministic verdict
schema ({reasoning, winner}) and translates the displayed-slot
winner back to the original candidate label so swap-and-confirm
works correctly.
- evaluate() runs one judge call by default; when swap_and_confirm
is True (the default) it issues a second call with positions
swapped and returns "tie" on disagreement (PandaLM-style position
bias mitigation).
- include_metrics is rendered into the prompt only when non-empty;
metrics follow the displayed slot, not the original candidate.
- Returns PairwiseCompareResult with score (0.0/0.5/1.0), label,
reasoning text, and summed judge cost.
Add `allow_extra=True` opt-in to EvaluatorEntry.__init_subclass__
so the multi-candidate shape (candidate_a_*, candidate_b_*, golden)
can declare typed fields without smuggling structured data through
the `contexts: list[str]` slot. Default behavior unchanged for the
existing single-output evaluator family; the error message now
points new authors at the opt-in.
Refs #5100, epic #5099.
Mock litellm.completion at the seam so the deterministic logic (swap-and-confirm, order translation, metrics injection, score mapping) is exercised without API keys. Coverage: - skipped when a candidate output is missing - single judge call when swap_and_confirm=False - exactly two judge calls when swap_and_confirm=True - agreement returns the agreed winner - disagreement returns "tie" - order translation when running the swapped half directly - metrics injected into the prompt only when include_metrics non-empty - score map: A=0.0, tie=0.5, B=1.0 Real-API integration test pattern (a la test_llm_boolean) can be added later; the logic checks above are the high-value ones.
Add the langevals/pairwise_compare entry to evaluators.generated.ts (both the langevals/ts-integration source and its langwatch copy), matching the shape and indentation style of the surrounding generated entries. Surgical patch instead of a full codegen rerun: the existing file has accumulated formatting drift since its last regen (commit d583fbe), so a fresh `uv run python scripts/generate_evaluators_ts.py` produces ~5000 lines of unrelated ordering/whitespace churn. Keeping this PR's diff focused on the feature; codegen-drift cleanup is a separate concern. Also: switch include_metrics from `default_factory=list` to `default=[]` so the codegen (which only reads `field.default`) doesn't crash on PydanticUndefined. Pydantic v2 deep-copies the default at instantiation, so the usual mutable-default footgun does not apply. Refs #5100.
Add an optional `pairwise` block to evaluatorConfigSchema, with its own exported sub-schema (`pairwiseEvaluatorConfigSchema`) so the UI form and the server execution path can share the type. Fields: - variantA, variantB: TargetConfig ids whose per-row outputs are the two candidates being judged. - goldenField: dataset field name whose value is the reference answer. - includeMetrics: per-candidate metrics injected into the judge prompt (cost / duration). Empty by default. The server-side `executionRequestSchema` consumes the same shared `evaluatorConfigSchema`, so no mirror is needed in `src/server/experiments-v3/execution/types.ts`. Adding the field is backward compatible: existing evaluator configs validate unchanged because `pairwise` is optional. Refs #5100.
Two-pass orchestration so pairwise evaluators can run against both variants' outputs without inventing a cross-cell dependency graph: Phase 1 (existing flow, unchanged for non-pairwise users): - generateCells emits per-(row, target) cells with all NON-pairwise evaluators attached. - Pairwise evaluators are filtered out here — they would crash because candidate_b's output is not available within a single per-target cell. Phase 1 -> Phase 2 bridge: - runOrchestrator now tracks successful target outputs by (rowIndex, targetId) in a new `completedTargetOutputs` map as target_result events flow through processEventForStorage. Phase 2 (new): - After Phase 1's await Promise.all(activeCells), generatePairwiseCells emits one synthetic cell per (pairwise evaluator, rowIndex) where BOTH variantA and variantB outputs exist. The cell sets targetId=variantA and skipTarget=true so the existing workflow builder + skip-target fast path execute without modification. - buildEvaluatorInputs grows a pairwise branch that bypasses the per-target mapping system: golden from cell.datasetEntry[goldenField], candidate_a_*/candidate_b_* from cell.pairwise, and `input` from variantA's existing dataset mapping (with a fallback to cell.datasetEntry.input). - Cells run through the same semaphore + executeCell loop, so abort, progress, and CH dispatch all keep working. Rows where one variant failed are silently skipped — there is no honest pairwise verdict without both candidates. Out of scope (separate issues): - N-way (#5101): generatePairwiseCells can be generalized later, the Phase-2 hook stays the same. - Cross-experiment comparison (#5102). Refs #5100.
Three new UI components for the pairwise compare evaluator, plus a
winner/loser visual state on the existing EvaluatorChip:
PairwiseConfigForm (EvaluatorPanel/PairwiseConfigForm.tsx)
- Variant A / Variant B target selects; variant B excludes variant A
so users can't pick the same target twice.
- Golden dataset-field select.
- Include cost / Include latency checkboxes.
- "Bias-corrected (2x judge calls)" indicator.
RowVerdictStrip
- Per-row strip showing winner (Variant A name / B name / "Tie")
with a popover for judge reasoning.
AggregateHeaderBar
- Tally "<A> wins X · <B> wins Y · Ties Z"
- Bias-corrected indicator + total judge cost.
- Filter chips (All / A / B / Losses).
- Export + Promote-A + Promote-B buttons (handoffs wired in step F).
EvaluatorChip
- New `pairwiseState` prop: "winner" tints green, "loser" tints red,
"tie" stays neutral. Callers compute this by mapping the verdict
label ("A"/"B"/"tie") to the chip's target id via the evaluator's
pairwise config.
Out of scope this commit:
- Routing langevals/pairwise_compare to PairwiseConfigForm inside
ConfigPanel, and rendering RowVerdictStrip + AggregateHeaderBar
inside EvaluationsV3Table. Those wirings touch heavier state-
plumbing and are tracked separately; the components are
self-contained and ready to import.
Refs #5100.
Found during self-review. When `loadedPrompts.get(promptId)?.handle` came
back undefined (prompt deleted, or worker cache miss), the orchestrator
fell back to emitting the raw `promptId` KSUID ("prompt_6IFkbb…") as
the verdict identifier. But the aggregator's normalizer only matches
against (a) legacy A/B/tie, (b) the variant's internal target id, or
(c) the supplied handle hint — a promptId KSUID matches none of those,
so the verdict was silently dropped and the column-header summary
collapsed to 0/0/0.
Fix: skip the promptId fallback. Priority is now handle → target.id.
Both sides know how to normalize either.
Two new orchestrator tests:
- handle present in loadedPrompts → cell's candidate.id is the handle
- handle missing → cell's candidate.id is the internal target id, NOT
the promptId KSUID
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
These modules and three of their spec scenarios described UI behavior the PR explicitly removed (top aggregate bar, "Copy as bug report", "Losses" filter chip). With no production importers they were dead weight and a 430-line "spec lies" surface. Cut the source, the dedicated tests, the preview-only AggregateHeaderBar test cases, and the three unreachable scenarios; replace them with one scenario that matches what the live column scoreboard (TargetHeader) actually renders. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…iseLabel The PR description claimed the chip-tint and per-row verdict surfaces accept both legacy "A"/"B"/"tie" and the new winner-by-id label format. They did not — TargetCell.resolvePairwiseState and renderPairwiseVerdicts both did literal `label === "A" | "B" | "tie"` checks, and RowVerdictStrip's "A" / "B" ternary picked the wrong name when the stored label was the winning candidate's prompt handle (e.g. "say-hi"). Under the new orchestrator contract the chip-tint surface silently stopped rendering and per-row strips silently stopped appearing for any prompt-typed pairwise run. Export `normalizePairwiseLabel` from `computeAggregates.ts` so both surfaces share the same matcher as `computePairwiseTargetAggregate`, then resolve the slot inside hook-friendly seams: - `PairwiseAwareEvaluatorChip` wraps `EvaluatorChip` for pairwise evaluators. It calls `useTargetName` on both variants at the top of its render (which `evaluators.map(...)` cannot do) and computes the chip's winner/loser/tie tint from the normalized slot. - `PairwiseVerdictRow` now accepts a raw `label: string` and does the same normalization before delegating to `RowVerdictStrip`; if the label matches neither variant's id nor handle, the strip returns null instead of falling through to a wrong-name render. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The pairwise PR added a non-canonical-fields spread in both the worker (evaluationsWorker.ts) and the legacy route (evaluations-legacy.ts) so pairwise's `candidate_a_*` / `candidate_b_*` reached the langevals Python evaluator. The spread was unconditional, which silently widened the wire contract for every other langevals evaluator — any stray mapping output on a strict pydantic model would now 422 instead of being stripped. Blast radius was all evaluators; only pairwise was exercised. Look up the evaluator definition via AVAILABLE_EVALUATORS and bound the extras to `requiredFields ∪ optionalFields`. Pairwise's `candidate_*` fields are declared in its catalog, so they still pass through; any other evaluator's mappings remain canonical-6-only. Also replace a `// @ts-ignore` introduced earlier in this PR's diff with `// @ts-expect-error <reason>` (per project rule). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collapse the multi-line Omit type and useMemo deps array onto one line to match Biome's print width — caught by reviewdog-action-biome on the chip-render fix commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Aryansharma28
force-pushed
the
worktree-pairwise-mvp
branch
from
June 29, 2026 09:11
01ac545 to
a7439c7
Compare
Contributor
|
Automated low-risk assessment This PR was evaluated against the repository's Low-Risk Pull Requests procedure and does not qualify as low risk.
This PR requires a manual review before merging. |
sergioestebance
approved these changes
Jun 29, 2026
This was referenced Jun 26, 2026
0xdeafcafe
added a commit
that referenced
this pull request
Jul 3, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
4 tasks
6 tasks
Aryansharma28
added a commit
that referenced
this pull request
Jul 6, 2026
…ing (#5367) * fix(experiments-v3): resolve pairwise comparator UX bugs from dogfooding Fixes a batch of issues surfaced while dogfooding the pairwise compare feature (#5142, #5195): mislabeled buttons, dark-mode contrast failures, inconsistent drawer styling, a raw 400 error leaking to users, and two state-management bugs where the config drawer silently dropped user input (Variant A/B/Golden field, and Include cost/duration) on re-render. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(experiments-v3): forward topLevel through ZodDefault unwrap in DynamicZodForm The prior fix for duplicated Swap And Confirm / Allow Tie labels only suppressed the inline label when topLevel=true, but the ZodDefault unwrap branch recursed without forwarding it — every settings field with .default() (which includes both of these) silently fell back to topLevel=false, so the duplicate label bug was still live for the one evaluator it was meant to fix. Verified live: reopening the Pairwise Compare drawer now shows each label once, and Include cost/duration toggles persist correctly across save + reopen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(experiments-v3): add regression test for missing-mapping alert click Closes the one verification gap noted in the bug-fix batch: the alert icon's onClick previously fell through to the chip's own uncontrolled Menu.Trigger before stopPropagation could run. Asserts the click calls onEdit exactly once and never renders the chip's own dropdown items. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(lint): satisfy biome formatting in pairwise bugfix files CI's langwatch-app-ci lint job failed on line-wrapping in ModelSelector.tsx, iconsMap.tsx, and evaluations-legacy.ts. Applied via `biome check --write`, no logic changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(experiments-v3): apply CodeRabbit review suggestions - Rename EvaluatorChip's menuOpen/setMenuOpen to isMenuOpen/setIsMenuOpen to satisfy the repo's boolean-naming convention (is/has/should prefix). - Drop the redundant createEvaluator() call in the new alert-click test, reusing the already-constructed evaluator instance instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(ui): reuse providerKey instead of re-deriving in ModelSelector CodeRabbit: selectedItem.value.split("/")[0] duplicated the already- computed providerKey (selectedItem.value === model by construction of the find() above), so just reuse it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(experiments-v3): apply remaining CodeRabbit outside-diff findings - Rename DynamicZodForm's topLevel param to isTopLevel (boolean naming convention), same as the earlier menuOpen -> isMenuOpen rename. - Fix a real, pre-existing bug in totalColumnPercentage: the dataset/ target summation loops reconstructed column IDs as `dataset_${id}` and bare `target.id`, but resize actually stores under the header's real ID (`dataset.${id}` / `target.${targetId}`, per the columnHelper.accessor calls above). The mismatch meant a resized dataset/target column's width never factored into the table's total, silently falling back to the default percentage. Same root cause as the pairwise-column width bug this PR already fixes, just for the other two column types — verified by tracing the actual header IDs written by the resize handler and columnHelper definitions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
…ate_a_id/output) Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward. This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 7, 2026
* refactor: remove Elasticsearch and the BullMQ legacy stack
The TraceService / EvaluationService / ExperimentRunService /
FilterService facades all had two backends — a ClickHouse one and an
Elasticsearch one — but every call was already going through ClickHouse.
The ES backends were constructed and never invoked. The legacy BullMQ
stack under src/server/background/ had been replaced piecewise by the
event-sourcing collector (recordSpan) but the old workers, queues, and
the tasks that wrote to ES were still wired up at boot.
This commit deletes both stacks from the application layer:
ES removal (live read/write paths):
- Drop the ElasticsearchTraceService / ElasticsearchEvaluationService /
ElasticsearchExperimentRunService / ElasticsearchFilterService /
ElasticsearchAnalyticsService backends (constructed but never called).
- Drop the evaluationEsSync + experimentRunEsSync reactors and the
annotationEsSync hook — no more dual-write.
- Drop the ES analytics timeseries + scenario-analytics query builders.
- Drop the ES experiments batch-evaluation repository + the ES
scenario-event service/repository (the SimulationFacade was already
routing reads to ClickHouse).
- Strip Elasticsearch URL/API-key fields from the org settings input
schema, the settings UI, and getApp().organizations.update.
- Drop the per-org ES-driven cron handlers
(scenario_analytics, trace_analytics msearch, retention_period_cleanup,
schedule_topic_clustering) and the ES task scripts they invoked
(elasticMigrate, cold/*, deleteTracesRetentionPolicy, syncAnnotations,
syncTraceCosts, backfillTraceAnalytics, backfillScenarioAnalytics,
fixCustomMetadataOutsideCustomField, clearTopics, rerunChecks).
- Collapse env.IS_QUICKWIT branches.
- Replace ES query-builder type imports
(@elastic/elasticsearch/lib/api/types) with neutral structural types
in the filter / analytics / common-router files that never actually
ship queries to ES anymore.
- Move the Protections type out of src/server/elasticsearch/protections
to src/server/traces/protections.
BullMQ legacy stack removal:
- Delete src/server/background/ entirely (worker entry, the six queue
files, collectorWorker, evaluationsWorker, topicClusteringWorker,
usageStatsWorker, anomalyDetectionWorker).
- Move the pure helpers under background/workers/collector/ (cost,
common, evaluations, evaluationNameAutoslug, metrics, piiCheck, rag)
into src/server/tracer/collector/ — these are consumed app-wide and
don't depend on BullMQ.
- Extract runEvaluation / runEvaluationForTrace / customEvaluation /
DataForEvaluation from the deleted evaluationsWorker into
src/server/evaluations/runEvaluation.ts, switching from ES
getTraceById / getTracesGroupedByThreadId to TraceService.getById /
getTracesByThreadId.
- Rewrite src/workers.ts: drop the 15-minute self-restart timer
(helm/docker already restart-on-exit), drop the BullMQ Worker
startups, initialize the worker-role app, and start the EE
governance ingestion-puller worker via a new
ee/governance/services/pullers/pullerQueue.ts.
- Migrate src/server/api/routers/httpProxyTracing.ts from
scheduleTraceCollectionWithFallback to getApp().traces.recordSpan via
CollectorSpanUtils — the only remaining caller of the old collector
queue.
- Strip the legacy collector dedup (fetchExistingMD5s + 256-version
cap) from src/server/routes/collector.ts; event sourcing handles
dedup downstream.
- Drop /api/start_workers, /api/rerun_checks,
/api/cron/schedule_topic_clustering,
/api/cron/scenario_analytics,
/api/cron/traces_retention_period_cleanup — the routes were
exclusively legacy stack triggers.
- Delete src/server/topicClustering/ (BullMQ-only scheduler) and the
/settings/topic-clustering page + command-bar entry + route.
- Rewrite TraceUsageService CH-only (drop the EsClientFactory
constructor parameter and the splitProjectsByFlag fallback).
Core ES dir + package:
- Delete src/server/elasticsearch.ts + src/server/elasticsearch/
(traces, transformers, patchOpensearch, patchQuickwit).
- Delete elastic/ (helpers, mappings, schema, all migrations).
- Remove @elastic/elasticsearch from package.json + lockfile.
- src/components/checks/TryItOut.tsx: drop the
transformElasticSearchSpanToSpan call; CH spans already arrive in the
domain Span shape.
Facade collapse:
- Collapse EvaluationService into a single CH-backed class
(delete clickhouse-evaluation.service.ts, rename methods to throw on
missing CH client instead of returning null, keep getEvaluationInputs
nullable for the legitimate empty case).
- Collapse ExperimentRunService into a single CH-backed class
(delete clickhouse-experiment-run.service.ts, fold
listRunsForExperimentSlugPaginated into the renamed class).
- Collapse FilterServiceFacade → FilterService into a single CH-backed
class (delete clickhouse-filter.service.ts).
- Delete SimulationFacade entirely — every consumer (suite router,
scenario events router, simulation-runs REST, cancellation router,
onboarding-checks service) now calls getApp().simulations.runs
directly.
193 files changed, -29.8K LOC. The only remaining BullMQ consumer in
the codebase is the EE governance ingestion puller, which keeps
BullMQ's repeatable-jobs cron primitive until it migrates to event
sourcing (follow-up).
* refactor(context): isolate the BullMQ adapter to the EE governance puller
After the legacy stack removal the only BullMQ-importing file left in
src/server/ was the context adapter at adapters/bullmq.ts, and the only
caller of its withJobContext helper was the EE governance ingestion
puller worker. The other three exports
(createContextFromJobData / getJobContextMetadata / JobDataWithContext)
have no BullMQ dependency — they're pure ALS plumbing.
- Move the pure helpers into src/server/context/core.ts so the
framework-neutral request-context module no longer touches BullMQ.
- Move withJobContext (the one piece that imports BullMQ's Job type)
into ee/governance/services/pullers/withJobContext.ts, co-located
with its only consumer.
- Move the existing withJobContext test cases (new __context shape +
legacy __payload migration paths) into
ee/governance/services/pullers/__tests__/withJobContext.unit.test.ts.
- Delete src/server/context/adapters/bullmq.ts and update
asyncContext.ts to re-export the pure helpers from core only.
Result: BullMQ is now contained entirely in
ee/governance/services/pullers/. The next time the puller migrates to
event sourcing, bullmq and bullmq-otel come out of package.json as a
one-liner with no other callers to migrate.
* fix: restore topic clustering (deleted in error)
Topic clustering was incorrectly bundled into the ES/BullMQ removal in
the parent commit. It is a live feature that:
- Reads from ClickHouse via TraceService (not ES).
- Uses BullMQ only for the worker queue, which is fine — the legacy
stack removal targeted the per-call collector / evaluations workers,
not every BullMQ consumer.
Co-locate the queue + worker under src/server/topicClustering/ (rather
than the deleted src/server/background/queues/ + workers/), and re-home
the shared BullMQ helpers (QueueWithFallback + withJobContext) under
src/server/queues/ since they are now used by both the EE governance
puller and topic clustering.
Restored:
- src/server/topicClustering/{topicClustering,types,topicClusteringQueue,topicClusteringWorker}.ts
- src/server/topicClustering/__tests__/* (3 files)
- src/tasks/runTopicClustering.ts
- /api/cron/schedule_topic_clustering handler
- project.triggerTopicClustering tRPC mutation
- /settings/topic-clustering page route + command-bar entry
Wired the topicClusteringWorker into src/workers.ts alongside the EE
ingestion puller.
Test-mock fixes for stale ~/server/background/... imports flagged in
review (3 integration tests + 2 unit tests, plus a now-defunct queues-dir
scan in no-retention-gc.unit.test.ts):
- httpProxyTracing.integration.test: rewrite to mock
getApp().traces.recordSpan and decode the OTLP span into the legacy
CollectorJob shape via a small facade — assertion bodies unchanged.
- evaluations.azure-byok.integration.test: drop mocks of long-gone
runEvaluationJob / startEvaluationsWorker symbols; mock just
runEvaluationForTrace at its new home.
- span-pii-redaction.service.test + topicClustering tests: update mock
paths from ~/server/background/... to the new locations.
* fix(lockfile): drop stale @elastic/elasticsearch importer + pin vite
CI was failing on `pnpm install --frozen-lockfile` because the lockfile
still listed @elastic/elasticsearch ~8.15.3 as a direct dep of langwatch/
even though the package.json no longer declares it. The previous rebase
resolution preserved main's lockfile but kept my branch's package.json,
so they drifted.
Surgical fix: remove the @elastic/elasticsearch entry from the importer
block. Orphan package + peer-dep references downstream are inert and
clear out next time someone regenerates the lockfile.
Also pin `vite` to 8.0.10 via overrides. vite 8.0.16 (published
2026-06-01) hasn't aged past the workspace's 7-day minimumReleaseAge
gate yet, so any `--no-frozen-lockfile` run hits the maturity check
trying to resolve vitest's peer. The pin doesn't help today's gate trip
(overrides apply after maturity checks), but it stops future regens from
churning between 8.0.10 and the latest unaged patch.
* fix: restore node:crypto import in httpProxyTracing.ts
The legacy-stack removal commit dropped `import crypto from "node:crypto"`
along with the other imports it was rewriting, but two callers of
`crypto.randomBytes` (line 96/97 — trace_id and span_id generators)
still reference it. Typecheck on the global Web Crypto sees no
`randomBytes` and fails.
* fix: chase test fallout from the ES/BullMQ removal
CI flushed out a pile of stale references that survived the previous
commits because nothing on the happy path touched them.
Production:
- evaluation.service.ts: getEvaluationInputs returns null when no
ClickHouse client is available (commit message claimed this, code
threw — restore the documented contract; the eval-inputs panel is
load-bearing on the trace drawer and a 500 cascade poisons polling).
- httpProxyTracing.ts: don't throw when the project lookup misses —
fall back to DEFAULT_PII_REDACTION_LEVEL. The tenantId is the
authoritative project id from the caller; the project row is only
consulted for the redaction setting and is fine to default.
Test fixes:
- internal-routes-auth.integration.test: repoint at
/api/cron/old_lambdas_cleanup (the deleted retention-cleanup,
start_workers, rerun_checks endpoints no longer exist; the kept
destructive cron exercises the same cronPolicy() wiring).
- metrics.test + metrics.unit.test: vi.mock paths were one level deep
too far after the collector move from src/server/background/workers/
to src/server/tracer/. Real getLLMModelCosts then hit Prisma in unit
tests with no DB.
- experiment-run.service.getRun-null.unit.test: the test was patching
service.clickHouseService (gone after the facade collapse). Rewrite
to mock getClickHouseClientForProject so the "client unavailable"
and "row not yet materialized" cases stay covered.
- experiment-run-dedup-safety + trace-dedup-oom-safety: the structural
guard read clickhouse-experiment-run.service.ts directly. Point at
the merged experiment-run.service.ts (clickhouse-trace.service.ts
still exists — only the experiment-run side collapsed).
- cancellation-event-sourcing.integration.test: vi.mock("~/server/redis")
was a partial factory that dropped makeQueueName. After the TC
restoration the test's transitive imports need it; spread
importOriginal() so the rest of the module passes through.
- httpProxyTracing.integration.test: also mock the relative-path
spelling of ~/server/app-layer/app so the route's own import is
intercepted regardless of how vitest canonicalises specifiers.
* fix(build): isolate makeQueueName so the client bundle stops pulling ioredis
The boot-smoke test was failing the entire PR because the simulations
route chunk (`__...path__-XXXXX.js`) tried to evaluate ioredis at
top-level in the browser and tripped `process is not defined`
(via `process.nextTick`).
Root cause: when the legacy stack was removed, `makeQueueName` moved
from `~/server/background/queues/makeQueueName.ts` (a tiny standalone
file) into `~/server/redis.ts`. `~/server/scenarios/scenario.constants.ts`
imports `makeQueueName`, and the SimulationsPage route transitively
pulls in scenario.constants for its queue identifiers. So every
SimulationsPage chunk now dragged the whole `~/server/redis` module
— and ioredis — into the client bundle.
Fix: pull `makeQueueName` back into its own file
(`~/server/queues/makeQueueName.ts`), re-export it from `~/server/redis`
for compatibility, and switch scenario.constants to the dedicated path.
Local `vite build` + `node scripts/smoke-boot.mjs` now reports
"BOOT SMOKE PASSED" with no externalised-node-builtin warnings.
* fix: hoist vi.mock factory + mark orphaned scenarios @unimplemented
httpProxyTracing.integration.test was failing every test ("There was
an error when mocking a module") because the vi.mock factories
referenced a top-level `fakeApp` variable. Vitest hoists vi.mock
above all top-level code, so closures over outer variables fail.
Move the shared mock fn into vi.hoisted() and inline the factories.
feature-parity: 19 orphaned scenarios survived the legacy-stack
removal because the tests that bound them lived under
src/server/background/ (deleted) or referenced ES storage layers
that no longer exist. Tag the orphans @unimplemented so the parity
check stops failing — re-binding them to event-sourcing-backed
tests is follow-up work, not in scope here:
- Redis Cluster compat: 4 scenarios tested BullMQ queue hash-tag
behaviour. After the legacy-stack removal the only BullMQ users
are the EE puller + topic clustering; the tests can be rewritten
against those queues but it's a fresh integration-test surface.
- Extensible scenario metadata: 11 ES-storage-round-trip and
metadata-namespace scenarios. ES is gone; equivalent ClickHouse
projection coverage is a follow-up.
- Monitor execution backend: 3 evaluator-settings scenarios that
were bound to evaluationsWorker.integration.test.ts (gone).
- API endpoint authorization: drop "Worker and ops trigger endpoints
reject callers..." (the endpoints don't exist) and repoint the
destructive-cron scenario at the surviving old-lambdas-cleanup
route.
* fix(topic-clustering): break circular import causing worker boot TDZ
Restoring topic-clustering reintroduced a runtime regression: workers
crashed on boot with `ReferenceError: Cannot access 'TOPIC_CLUSTERING_QUEUE'
before initialization`. The cycle is
topicClusteringWorker.ts
-> topicClustering.ts (line 15: clusterTopicsForProject)
-> topicClusteringQueue.ts (line 14: scheduleTopicClusteringNextPage)
-> topicClusteringWorker.ts
When workers.ts imports the queue first, queue.ts re-enters worker.ts via
topicClustering.ts. queue.ts line 18 then reads `TOPIC_CLUSTERING_QUEUE.NAME`
while worker.ts is still on its imports — the const is in TDZ.
Extract the queue name + job-type tuple to a leaf
`topicClusteringQueue.constants.ts` with no cyclic edges. Both worker.ts
and queue.ts now import the constant from there; the cycle on
`runTopicClusteringJob` survives but that's a hoisted function declaration,
so its binding is available regardless of which side enters first.
Caught by `make quickstart all-local`. With the fix the worker logs
"topic clustering checks worker registered" and "topic clustering worker
active, waiting for jobs!" cleanly.
* fix(prompts): stop polling traces.getById for failed assistant messages
When a prompt submission errors (e.g. NLP unreachable, provider 500,
upstream rate-limited), the chat renders an ErrorMessage instead of an
AssistantMessage — but TraceMessage was still mounted unconditionally
on every non-streaming assistant turn. That meant every failed
submission also mounted a trace polling component looking up a trace
that was never emitted, producing a chain of 404s on
`api.traces.getById` (retry: 10, exp-backoff to 60s) and a "Maximum
update depth exceeded" React warning in posthog session-replay.
Gate the TraceMessage mount on `!isError`. Errored assistant turns have
no backing trace; the only thing left to render is the ErrorMessage,
which is already shown above.
Verified live against `make quickstart all-local` with no NLP container
(repro path for "LangWatch NLP is unreachable"). Before: 15 console
errors per submit; after: 1 unrelated pre-existing SVG warning.
* chore(evaluations): drop dead imports and string throw in runEvaluation
- remove unused imports left over from the evaluations-worker extraction
(CostReferenceType, CostType, nanoid, getProtectionsForProject,
captureException, withScope, createLogger + the unused logger instance)
- throw new Error("trace not found") instead of a bare string
* fix(tests): chase moved modules in tests added on main
- Protections import: ~/server/elasticsearch/protections -> ~/server/traces/protections
- getRun integration test: clickhouse-experiment-run.service was folded
into experiment-run.service; use ExperimentRunService
* chore: purge remaining Elasticsearch remnants from the audit
- usage metering: drop the dead MeterBackend concept entirely —
resolveUsageMeter no longer takes clickhouseAvailable or returns a
backend (nothing ever consumed it; the only non-ClickHouse value was
the removed Elasticsearch fallback); UsageService and presets wiring
updated, backend-selection tests removed
- delete stale comments claiming ES is a live backend (org settings
migration note, experiment-run facade fallback, filter fallback,
license-enforcement counting source, mapper/type docs, stall
detection, synthetic error span, llmParameterMap, ee licensing)
- reword legacy snake_case mapper docs that described ES as current
Intentionally kept: history notes, regression guards asserting ES
absence, legacy ESBatchEvaluation/ElasticSearch* shape names, and the
unused Prisma elasticsearch* columns (follow-up migration).
* fix: chase moved cost module in model-cost-span-preview and drop stale UsageService ctor arg
* chore(lint): satisfy biome on files touched by the main merge
Biome's organizeImports ordering shifted after merging main's import
changes; re-sort the affected files. Also drop the dead `internalAuth`
const and collapse two `!x || !x.method()` guards to optional chaining
in misc.ts (biome noUnusedVariables + useOptionalChain).
* fix(rebase): re-integrate main features that landed in rebased-over code
Rebasing onto origin/main dropped three main-side changes that lived in
files this branch deletes/relocates/restored; the merges that carried
them are gone in a linear history, so re-apply them explicitly:
- runEvaluation: port #4729 (unified scoped data privacy) — native
evaluator short-circuit (isNativeEvaluatorType/executeNativeEvaluation)
+ augmentEvaluationResult(droppedCategories) on both return paths. The
branch extracted runEvaluation from the pre-#4729 worker, so without
this the data-privacy augmenter silently vanished from evals.
- topicClustering: take origin/main's topicClustering.ts (+ co-located
./topicClusteringQueue import) — the restored copy was a pre-#4653/
#4828/#4571 snapshot still routing to the removed python langwatch_nlp
service. Restores nlpgo-only routing (#4653), the single argMax count
pass (#4828), and the page-fetch read-stream cap (#4571). Align the two
topicClustering tests with main (langevals mock) + co-located queue path.
collectorWorker's #4729 data-privacy is already applied downstream in the
event-sourcing recordSpanCommand path, so collector.ts (recordSpan) needs
no change.
* fix(rebase): chase main-renamed modules + narrowed APIs after rebase
The dropped main-merges also carried module renames and a signature
narrowing that the branch's deleted/relocated files and main's newer
files now straddle. Re-apply across the affected files so typecheck +
biome pass clean (0 type errors):
- Protections moved out of src/server/elasticsearch/ -> src/server/traces/;
retarget 10 main-added files (projection, tracesV2, redactAttributes,
data-privacy tests) that still imported the old path.
- main consolidated the generated zod/type modules: evaluators.zod.generated
-> evaluators, {tracer,experiments}/types.generated -> {tracer,experiments}/
types. Retarget the legacy-route files (collector, misc, evaluations-legacy).
- piiCheck relocation background/workers/collector -> tracer/collector:
retarget the two stray test imports.
- captureException narrowed to Error|string (#2155): wrap the unknown
catch args via toError() in evaluations-legacy + misc.
- Project.piiRedactionLevel removed by #4729: collector + misc track-event
spans now pass DEFAULT_PII_REDACTION_LEVEL (the policy is resolved
downstream in the recordSpan pipeline).
biome --write applied (import sort + optional-chain); one pre-existing
noUnusedVariables warning remains (also present on origin/main).
* fix(evaluations): enrich trace evaluations in runEvaluationForTrace (P1)
The worker→runEvaluation extraction swapped the legacy
getTraceById({ includeEvaluations: true }) for TraceService.getById,
which routes through getTracesWithSpans and does NOT enrich evaluations
(that only happens in the paginated getAllTracesForProject path). The
field mapper reads `trace.evaluations ?? []`, so any evaluator whose
input maps from the `evaluations` source (a prior evaluator's result)
was silently fed [] — a quiet correctness regression with no crash.
Fetch evaluations via TraceService.getEvaluationsMultiple (already mapped
to the legacy Evaluation[] shape the mapper expects) and attach them to
the trace before buildDataForEvaluation, restoring parity with the worker.
Note: this path still has no executing test (its spec is @unimplemented);
a regression test that asserts the `evaluations` source maps real data is
the right follow-up.
* fix(ci): repair rebase regressions in unit + integration tests
Four CI failures, all from the rebase taking older snapshots of branch-only
files or losing main's schema/query evolution:
- evaluations-legacy.ts: restored the branch's working getEvaluatorDataForParams
(coercedString/coerceEvaluatorScalar preprocessing + the CODE_EVALUATOR_CHECK_PREFIX
code-evaluator branch were dropped to an older snapshot). Re-applied the
main-driven module renames + captureException(toError) narrowing on top.
Fixes evaluator-input-coercion.unit.test.ts (10).
- misc.ts: restored the RoleBinding-aware /mcp/authorize handler (the old
legacy-TeamUser check had been re-introduced, returning 500/false-403).
Re-applied renames + toError + DEFAULT_PII_REDACTION_LEVEL. Fixes
mcp-authorize.rolebinding.unit.test.ts (4).
- experiment-run.service.ts: getRun now uses main's scalar
UpdatedAt = (SELECT max(UpdatedAt) ...) single-run read instead of the
IN-tuple. Fixes experiment-run-dedup-safety.unit.test.ts (2).
- httpProxyTracing.ts: drop the now-invalid select { piiRedactionLevel }
(#4729 removed Project.piiRedactionLevel) that threw PrismaClientValidationError;
use DEFAULT_PII_REDACTION_LEVEL. Fixes httpProxyTracing.integration.test.ts.
biome --write applied across the rebased diff to clear reviewdog lint.
* fix(p2)+test: stale ES string, document worker boot-ordering, P1 regression test
- settings.tsx: drop the stale "or Elasticsearch" from the org-update error
toast (the ES config fields were removed).
- workers.ts: document why the puller/topic-clustering worker + queue modules
are imported via await import() — they construct Redis-connecting
QueueWithFallback instances at module load, so they must load only after
verifyRedisReady(). (Not converted to top-level: that would connect to Redis
before it's verified.)
- runEvaluation.evaluations-enrichment.unit.test.ts: regression test locking the
P1 fix — an evaluator mapping from the `evaluations` source receives the
trace's real evaluations (via getEvaluationsMultiple), not a silent []. Verified
it fails when the enrichment is removed.
* fix(remediation): restore behavior dropped in the ES/BullMQ removal
Verified P0 + behavioral P1s from the PR #4547 audit (the blocker set). Each path regressed because logic was dropped while extracting code out of the deleted BullMQ worker.
- workers.ts: re-wire worker-role boot — scenario execution pool (P0: simulations were silently skipped on workers), worker /metrics listener, anomaly detector, ClickHouse storage-stats collection; add graceful SIGINT/SIGTERM shutdown with handle cleanup.
- runEvaluation.ts: restore code-evaluator routing (code/<id> -> runCodeEvaluator) and the embeddings_model env setup + openai/moderation guard the extracted module lost vs the canonical service.
- evaluation-execution.service.ts: enrich trace.evaluations in executeForTrace before building mapped data (parity with runEvaluationForTrace; the monitor path fed an empty evaluations source).
- routes/collector.ts: route REST ingestion through ingestNormalizedSpan so it shares the (tenant,trace,span) dedup gate + ADR-022 spool with OTLP, instead of calling recordSpan directly.
- tracer/collector: restore unit coverage for the moved span-tree/text-extraction helpers (deleted common.test.ts had no committed replacement).
typecheck clean; 235 evaluation unit tests green.
* refactor(analytics,experiments-v3): remove dead Elasticsearch query and mapper builders
Audit §B6 follow-up to PR #4547. No live callers remain after the ES backends were dropped: generateTracesPivotQueryConditions/generateFilterConditions (analytics/common.ts) and mapEsRunToExperimentRun/mapEsBatchEvaluationToRunWithItems + ESRunAggregationBucket (experiments-v3/mappers.ts). Keeps the still-live mapEsTargetsToTargets (used by routes/evaluations-legacy.ts). Drops the now-orphaned tests and a stale filter-conditions comment.
* docs(specs): drop stale Elasticsearch/BullMQ references from scenario specs
Audit §C follow-up. extensible-scenario-metadata: neutralize/remove the ES-storage scenarios (scenario events persist to ClickHouse via event sourcing). monitor-execution-backend: note evaluationsWorker + its test were removed with the BullMQ stack (monitor evals now run via executeEvaluation.command.ts) and correct results-stored-in-Elasticsearch -> ClickHouse. redis-cluster-compatibility: minor comment wording.
* fix(ui): restore canonical ModelOption import in LlmConfigField
Audit P2 follow-up. Replace the hand-rolled ModelOption type with the import from ~/server/topicClustering/types; topic clustering was restored in this PR, so the canonical type exists.
* fix(lint): satisfy biome organize-imports on PR-changed files
* fix(test): stub traceService.getEvaluationsMultiple in EvaluationExecutionService integration mocks
The executeForTrace evaluations-enrichment (getEvaluationsMultiple, added in 689be85) broke three integration tests whose hand-rolled traceService mocks didn't stub the new method (TypeError: getEvaluationsMultiple is not a function across shards 1/2/5). Stub it to resolve {} (no-op enrichment), mirroring the unit-test mock. typecheck clean.
* fix: address /review-pr re-audit findings (P1 collector + P2 worker hardening)
- collector.ts (P1): REST /api/collector counted ingestion failures via Promise.allSettled 'rejected', but ingestNormalizedSpan catches its own errors and RESOLVES with {status:'failed'} (never rejects) — so rejectedSpans was always 0 and failed spans were reported to the SDK as success with no error log. Now inspects the resolved SpanIngestionResult.status (still treats an unexpected rejection as a failure).
- workers.ts /metrics (P2 security): the worker metrics listener served the full prom-client registry unauthenticated on 0.0.0.0 while the web path is bearer-gated. Extract isMetricsAuthorized into server/metrics.ts and apply it to the worker listener too (fail-closed in prod).
- workers.ts shutdown (P2): gracefulShutdown discarded the topic-clustering + ingestion-puller worker handles and never closed the App; now registers both close handles and closes getApp() (CH/Redis/Prisma) last.
- evaluation-execution.service.unit.test.ts (P2 test): the A4 executeForTrace evaluations-enrichment was untested (every mock stubbed getEvaluationsMultiple to {}); add a test asserting prior evaluations reach the mapped data.
typecheck + biome clean; 208 evaluation unit tests pass.
* chore: drop unused prisma import in httpProxyTracing (codeql/code-quality)
* docs(experiment-run.service): document getRun's deliberate null-on-no-CH (#4547 review)
Per Aryansharma28's review (thread PRRT_kwDOKRXhvM6L6N-6): getRun's null-on-no-CH was a deliberate UX call (PR #3483 dogfood — fold-window polling race) but the class-level JSDoc still claimed all methods return null, and getRun's own JSDoc didn't explain the divergence from listRuns / getRunAggregates / listRunsForExperimentPaginated (which throw). Update the class JSDoc to call out the exception, expand getRun's JSDoc with the rationale + the route caller's intentional 404 mapping, and add an inline comment at the no-CH guard.
* fix(tests): retarget imports from deleted background/collector and elasticsearch/protections (rebase fixup)
PR #4547 moves src/server/background/workers/collector/cost -> src/server/tracer/collector/cost and src/server/elasticsearch/protections -> src/server/traces/protections. Upstream commits landed 3 files that import from the old paths; the rebase onto current main surfaced the dangling imports. Retarget to the new locations.
* fix(evaluations): preserve evaluator-specific extras (pairwise candidate_a_id/output)
Upstream #5142 (pairwise_compare) added a 'preserve evaluator-specific fields' block to background/workers/evaluationsWorker.ts so evaluators that declare extras via required/optional fields (e.g. langevals/pairwise_compare's candidate_a_id, candidate_a_output, candidate_b_id, candidate_b_output) have those fields flow through to langevals via ...extras instead of being stripped by the canonical-6 forward.
This PR extracts runEvaluation out of the deleted worker; without this port, pairwise-compare evaluations silently strip the candidate fields and 422 from langevals. Port the block into runEvaluation.ts (kept path) at the same relative location — just before the stagedLangevalsFetch call — bounded to the evaluator's declared requiredFields ∪ optionalFields so a stray mapping output on a non-pairwise evaluator can't ride through.
* docs(specs): reconcile feature specs with the ES/BullMQ removal (/review-pr audit)
Rewrites scenarios that mandated merging BullMQ jobs with ES scenario
events, restores tags where behaviour ships again with test coverage
(redis-cluster hash-tags, extensible scenario metadata), and drops
implementation-detail phrasing flagged by the audit.
* test: restore coverage lost with the deleted ES/BullMQ test files (/review-pr audit)
Re-homes the deleted suites' assertions against the surviving subjects:
TraceUsageService (incl. the CH-unavailable ?? 0 branch), makeQueueName
hash-tags, metrics auth fail-closed + getWorkerMetricsPort, REST
collector result handling + 429 span cap, googleDLP redaction, stall
derivation, RUN_STARTED metadata pass-through, extensible-metadata
schemas, evaluator-extras allowlist, and monitor settings resolution.
* fix: address /review-pr full-audit findings (P0 spend-spike + P1s)
Restores behaviours silently dropped with the BullMQ stack: the
5-minute spend-spike anomaly evaluation tick, daily self-hosted usage
telemetry, and OTel instrumentation in the worker process. Forwards the
bearer token on the /workers/metrics proxy, returns 500 when every span
in an ingest request fails, fixes googleDLPClearPII discarding its own
redactions, and gates decrypted s3SecretAccessKey behind manage
permission. Deduplicates the track_event route against the shared
service, extracts member-classification helpers, honours traceIds in CH
timeseries, and deletes the dead ES-era helpers, builders, and modules
the audit flagged.
* test(specs): fix @Scenario bindings broken by the ES/BullMQ spec reconciliation
- Retarget five scenario-run-utils annotations to the renamed scenarios
in unified-run-table.feature (ES/BullMQ wording was rewritten)
- Drop the vacuous 'does not import Elasticsearch' source check; the
module no longer exists and the spec scenario was removed
- Restore the missing binding for 'Uses cached count within TTL' on the
trace-usage cache test
- Mark the event_details aggregation-keys scenario @unimplemented: its
ES binding died with the ES backend and events.event_details is not
yet supported by the ClickHouse metric translator
* fix(dev): stop build-mcp-server.sh timestamps breaking under FORCE_COLOR
concurrently (pnpm dev) sets FORCE_COLOR, which makes console.log wrap
numbers in ANSI colour codes even when piped — the coloured start
timestamp then got interpolated into the elapsed-time eval and killed
the whole prepare chain with a SyntaxError. Write raw strings instead.
* feat(nlpgo): route xai, groq, cerebras, and deepseek providers
The playground proxy's provider inference only knew the six original
providers, so a default model like xai/grok-4.3 hard-400'd with
missing_provider on /go/proxy/v1/chat/completions. Map the remaining
TS-registry providers on both nlpgo paths (x-litellm headers for the
playground proxy, inline credentials for llmexecutor/Studio):
- xai / groq / cerebras are Bifrost-native and ride the ProviderID
passthrough with a plain API key
- deepseek is absent from Bifrost's enum; route it through the vLLM
(openai-compat) adapter defaulting to https://api.deepseek.com
- plain api-key providers share one Generic inline-credentials slot
instead of four copy-pasted per-provider slots; the duplicated
provider switches in gatewayproxy are merged into providerForPrefix
Spec: specs/nlp-go/proxy.feature (provider-prefix routing scenarios)
* feat(scenarios): surface typed generation errors from the Go gateway
nlpgo's handled-error envelope (herr) was flattened to a 500 with a
bare string by the scenario-generate route, leaving the browser with
"bad_request" and no way to react. Now:
- nlpgoHandledErrorFrom() parses the envelope off the AI SDK's
APICallError (unwrapping RetryError) into a DomainError keyed by
meta.reason, forwarded as domainError on the response
- the browser service throws ScenarioGenerationError carrying
kind + meta; classifyGenerationError matches typed kinds before
string sniffing (missing_provider → config tier + configure CTA)
- unknown kinds expand kind/message/meta into the raw-message surface
instead of showing an opaque status word
* feat(scenarios): show which model Generate uses, with a settings link
Both the scenario editor panel and the AI create modal now render
"Uses <model> · Change" next to their Generate buttons (resolved via
the scenarios.generator feature key), linking to model provider
settings. Config-tier generation failures get a "Model settings"
toast action. AICreateModal grows an optional footerHint slot.
* fix(ui): dark-mode tokens and layout stability on scenario surfaces
- CurrentDrawer: the lazy-drawer Suspense fallback rendered a 200px
block in page flow, shoving the whole page down while the chunk
loaded; it is now a fixed overlay where the drawer appears
- SimulationCard: reserve clearance for the floating status pill so it
no longer covers the last preview lines
- SaveAndRunMenu, Stop buttons (RunRow, ScenarioGridCard), and the
variable-mapping editor swapped hard-coded light-palette literals
(blue.50, gray.100, orange.50, bg white, raw gray-500 vars) for
semantic tokens so dark mode is readable; missing-mapping error uses
fg.error
* chore: regenerate pnpm lockfile after rebase onto main
This was referenced Jul 8, 2026
fix(experiments): disambiguate same-name pairwise variants with numbering + click-to-highlight
#5523
Merged
sergioestebance
added a commit
that referenced
this pull request
Jul 17, 2026
Adds the check that would have stopped the 42 PNGs this PR removes. Fails a PR that adds an image outside the directories where images legitimately live (docs/images, docs/media, langwatch/public, assets, specs, python-sdk/examples), and points the author at the pr-screenshots repo instead. A path allowlist rather than a reference scan on purpose: "is this image used anywhere?" reads like the better rule but flags plenty of legitimate docs images, which are referenced only via the docs site's own path conventions. Verified against the history it is meant to catch — it fails #5142, #5367 and #5523, and passes a real docs-image commit (#4796) and this PR's own deletions.
This was referenced Jul 17, 2026
sergioestebance
added a commit
that referenced
this pull request
Jul 17, 2026
…#5889) * chore: remove committed PR screenshots, close the gitignore gap Three directories of browser-QA screenshots (42 PNGs, ~5.8MB) reached main across PRs #5142, #5452 and #5523: langwatch/.pr-screenshots/ 20 PNGs + .gitkeep langwatch/docs/pairwise-bugfixes/ 10 PNGs docs/pr5106/ 12 PNGs None are referenced by any code, spec, doc or test — they were PR-body evidence, which by convention lives in the langwatch/pr-screenshots repo and is linked by raw URL, never committed to the product tree. Deleting them loses nothing: all three PR bodies link the images by *branch* name, and those branches were deleted on merge, so every one of those URLs already 404s today. langwatch/docs/ held nothing but the artifact dump, so it goes with it. The .gitignore rule that should have caught this was path-anchored to `langwatch/pr-screenshots/` and missed the dot-prefixed spelling actually used. Replaced with `**/pr-screenshots/` + `**/.pr-screenshots/`. * ci: fail PRs that commit screenshots outside image homes Adds the check that would have stopped the 42 PNGs this PR removes. Fails a PR that adds an image outside the directories where images legitimately live (docs/images, docs/media, langwatch/public, assets, specs, python-sdk/examples), and points the author at the pr-screenshots repo instead. A path allowlist rather than a reference scan on purpose: "is this image used anywhere?" reads like the better rule but flags plenty of legitimate docs images, which are referenced only via the docs site's own path conventions. Verified against the history it is meant to catch — it fails #5142, #5367 and #5523, and passes a real docs-image commit (#4796) and this PR's own deletions. * ci: fail the image guard loudly on git-diff errors CodeRabbit: with set -o pipefail, a git diff failure (e.g. a bad BASE_REF) was swallowed by the || true that exists only to absorb grep's exit-1 on no match, so the guard silently exited 0 — the one thing a guard must never do. Split git diff onto its own line so its failure trips set -e; grep keeps its guarded no-match. Verified: bad ref now exits 128, valid ref still passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5100
Closes #5130
Supersedes #5106
Summary
pairwise_compareevaluator returning the winning candidate's identifier (or"tie") inresult.labelso SDK / REST / MCP consumers read the winner directly — no slot-letter dereference.loadedPrompts.get(promptId).handle) before dispatch so what lands in ClickHouse (Label) is e.g."say-hi"instead of"target_…".candidate_a_*extras (orchestrator → route → worker → client), and adds per-variant evaluator-score injection into the judge prompt.What this changes
Backend / data contract
langevals/pairwise_compare.py:labelis the winner's candidate id (entry.candidate_a_id/entry.candidate_b_id) or"tie". Regeneratedts-integration/evaluators.generated.tsand synced viapnpm copy:langevals-types.orchestrator.ts: newvariantIdentifierFor()resolves prompt → handle → promptId → target id (chip-style + column-target paths).completedTargetEvaluatorScoresmap captures Phase 1 evaluator results per(row, target);withEvaluatorScores()appends them to the candidate output text fed to the judge.evaluations-legacy.ts: stops stripping non-canonical fields fromdata; surfaces real error messages instead of"Internal error".evaluationsWorker.ts: forwardscandidate_*extras to the langevals proxy call.useExecuteEvaluation.ts: client sendstarget.pairwisein the execute payload.UI
Swordsicon on the Pairwise Compare column header (matches the picker card).langy-test-pr… wins 2 · 1 tie/say-hi wins 3/Tied/Tied · 1 tie. Names truncated to 18 chars; tooltip shows full breakdown.=icon for ties. Judge "Call 1 (A in slot A, B in slot B):" preamble stripped from reasoning preview.friendlyError()maps auth / 404 / rate-limit / missing-candidate errors to actionable headlines + hints; raw stack behind "show details" popover.Programmatic contract
GET /api/trace/{traceId} { "evaluations": [{ "evaluatorType": "langevals/pairwise_compare", "status": "processed", "score": 1.0, "label": "langy-test-prompt-1779807355405", "details": "Candidate A does not answer the question about hours …" }] }Both legacy
"A"/"B"/"tie"and the new candidate-id label are accepted by the UI matchers — historical rows keep rendering.Verified
/langevals/pairwise_compare/evaluatereturnslabel: <winner candidate id>✅Test plan
<winner> wins N · M tieswith Swords iconSELECT Label FROM evaluation_runs WHERE EvaluatorType='langevals/pairwise_compare'— values are prompt handles or"tie", notA/BRefs: #5131 #5100
🤖 Generated with Claude Code
Summary by CodeRabbit