feat(evalboard): render multi-variant runs, one row per (task, arm) - #145
Conversation
An experiment declaring `variants:` writes each arm to its own subtree (<run>/<variant>/<task>/<NN>/) and stamps variant_id on every run.json row. Evalboard hardcoded the `default` segment in every path it built, dropped variant_id on read, and keyed rows on task_id alone, so a two-arm run rendered duplicate ids whose task pages resolved to one arbitrary arm. Variant semantics now live in lib/variants.ts (constant, path-segment guard, row key, arm enumeration) — dependency-free so the server readers and the client renderers share one definition. Path helpers and the blob fetch take a variantId defaulting to "default"; rows key on (task, arm, replicate); ?v= addresses an arm on the task route and the zip download. Replicates still collapse in the grid, arms never do. Every affordance is gated on a run having more than one arm, and each new parameter defaults to what the old hardcoded path produced, so single-arm runs render byte-identically. Covered by a legacy-run fixture alongside the two-arm one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…blended one A multi-variant run rendered its pass rate, cost and time three times over: once pooled across the arms in the headline tiles, then again per arm in a "By variant" strip below. The pooled rate is the one number on that page nobody wants, since it averages configurations that were deliberately made to differ and moves when the arms are merely reordered. The Pass rate tile now reports one row per arm and no pooled rate at all, and the strip is gone. Cost and Time keep their pooled totals, which stay operationally true however many arms produced them, and carry the per-arm split on their sub-line in place of p50/p90. Runs without variants are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…acent Two problems visible only on a real multi-variant run. The per-arm Pass rate tile stacked its arms, which made it the tallest thing in the row and stretched the three single-value tiles beside it into mostly empty boxes. The tile is already double width, so the arms now sit side by side and the spread rides on the label row; the tile keeps the height of the single-arm version. The grid's default ordering ranked each row on its own status, which split exactly the pairs worth reading: a task whose arms agreed kept its rows adjacent, while a task whose arms disagreed had one row sent to the top of the grid and the other to the bottom. Ordering now ranks a task by its worst arm, so failures still sort first and a task's arms stay together. Runs without variants order exactly as before: one row per task means that row is the task's worst arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elper Two pieces of the variant work that carried more machinery than they earned. The variant chip cycled a four-colour palette indexed by the arm's position in the run's sorted arm list. The arm's id is already on the chip, the run page labels its arms the same way, and the colour had to stay stable across the table and the mobile cards to mean anything at all. One neutral chip says the same thing, and it leaves colour in that column meaning pass or fail — the replicate badge beside it — rather than competing for it. The spread label was an exported, separately tested function wrapping two lines of arithmetic over at most a handful of arms. It is computed where it is rendered now; the render test already pins the string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment volume across the variant work was roughly twice what the code needed: several blocks restated the rationale already carried by the commit messages and the README. Cut to the parts that record a decision a reader cannot recover from the code, notably why the row key is length-prefixed and why the default grid ordering ranks a task rather than a row. Also dropped five tests that assert something a neighbouring test already covers: a second "arm not present" case, a replicate lookup subsumed by the arm-scoped enumeration test, the all-arms-missing branch of the sub-line helper, an all-arms-fail ordering case covered by the no-variant equivalent, and a slash-rejection case already asserted in the escape test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @bai-uipath's task in 1m 23s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: feat(evalboard): render multi-variant runs, one row per (task, arm)
PR #145 by @bai-uipath · bai/evalboard-variant-support → main · OPEN · reviewed against cab20ab · all 8 axes · 2026-08-28T02:08Z
Change class: complex — reshapes the evalboard run-row data model from one row per task to one row per (task, arm), changing grid/aggregation/status control flow, blob path resolution, and the download route's identity semantics; correctness requires reasoning about variant fan-out and back-compat with single-variant runs
Architecture, type safety, error handling and harness quality are strong (9.8-10 on four axes) and the variant feature is well factored, but the real risk sits in the new evalboard arm-addressing surface, where a stricter-than-producer id rule silently serves the wrong arm's data with HTTP 200, bare /runs/<id>/<task> links now hard-404 on multi-arm runs, and the whole non-grid path (task detail page, download route, readTaskReview) ships at 0% executed coverage with a four-consecutive-string signature no compiler can protect — all fixable in a focused follow-up, so the bottom line is: merge-worthy code with a small set of must-fix correctness-of-attribution gaps.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.2 / 10 | 0 | 0 | 1 | 3 | Variant-id normalization (?? DEFAULT_VARIANT_ID) is open-coded ~10-11 times across runs.ts/variants.ts instead of being owned by the module (and `variantId: string |
| 2. Type Safety | 9.9 / 10 | 0 | 0 | 0 | 1 | A validated variant id is indistinguishable from an unvalidated one at the type level — isValidVariantId(): id is string narrows to string, and the path-joining helper takes a bare unvalidated string |
| 3. Test Health | 8.4 / 10 | 0 | 1 | 1 | 1 | The new variant plumbing ships with zero executed coverage across every non-grid surface: task detail page, /api/download ?v= normalization + __ zip root, resolveSafePath's widened dispatch, readConversationLog's variantId, the blob prefix/dedupe key, the Variant sort column, and the multi-arm Tasks-header count |
| 4. Security | 9.5 / 10 | 0 | 0 | 1 | 0 | Blob names from the remote listing are joined onto the local cache root with no traversal guard, so a ../ segment in a blob name writes outside the per-source cache dir |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 9.8 / 10 | 0 | 0 | 0 | 2 | Unreachable ?? 0 default in the grid's variant-aware sort would silently rank a row as failed |
| 7. API Surface & Maintainability | 8.3 / 10 | 0 | 1 | 1 | 2 | Variant-less task deep links hard-404 on any run whose arms are not named default, and in-repo link builders still emit the bare form |
| 8. Evaluation Harness Quality | 9.9 / 10 | 0 | 0 | 0 | 1 | taskContentBase discards variantId on the activation branch while rowMatches honours it, so an activation read can pair one arm's run.json row with another arm's content directory |
Overall Score: 9.4 / 10 · Weakest Axis: API Surface & Maintainability at 8.3 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 4 · 🔵 10 across 8 axes.
Blockers
-
[Axis 3] The new variant plumbing ships with zero executed coverage across every non-grid surface: task detail page, /api/download
?v=normalization +__zip root, resolveSafePath's widened dispatch, readConversationLog's variantId, the blob prefix/dedupe key, the Variant sort column, and the multi-arm Tasks-header count (evalboard/app/runs/[id]/[...task]/page.tsx:70) — A fresh v8 report confirms this file is 0/375 statements — no test anywhere imports it (grep -rn 'api/download|\[\.\.\.task\]/page' --include='*.test.ts*'returns nothing). The PR adds +57 lines here and every one of them is the arm-selection contract: line 61const variantId = isValidVariantId(v) ? v : DEFAULT_VARIANT_ID;, line 100const variantQuery = variantId === DEFAULT_VARIANT_ID ? "" :&v=${encodeURIComponent(variantId)};, and its propagation into the replicate selector (line 151`/runs/${id}/${taskId}?r=${ri}${variantQuery}`) and the download button (line 186)}&task=${encodeURIComponent(taskId)}${variantQuery}`). The sharpest hazard is lines 70-75:const review = await readTaskReview(
id,
variantId,
taskId,
replicateDirName(replicate),
source,
);
readTaskReview(runId: string, variantId: string, taskId: string, replicate: string, source) (lib/reviews.ts:46-51) takes four consecutive string parameters, so swapping variantId and taskId — exactly the edit this PR makes, replacing the previous hardcoded literal "default" — type-checks silently under tsc --noEmit and would just render no review. Add app/runs/[id]/[...task]/__tests__/page.test.tsx that vi.mocks @/lib/runs and @/lib/reviews, awaits the async server component, and asserts (a) readTaskReview was called with (id, 'preview-v2', taskId, '00', source) in that order, (b) with ?v=preview-v2 the replicate links and the /api/download href both carry &v=preview-v2, and (c) with no ?v= every emitted href is byte-identical to the pre-variant form (no v= anywhere). Sibling components in app/runs/[id]/[...task]/__tests__/ already render with @testing-library/react, so the harness exists.
2. [Axis 7] Variant-less task deep links hard-404 on any run whose arms are not named default, and in-repo link builders still emit the bare form (evalboard/app/runs/[id]/[...task]/page.tsx:61) — page.tsx:61-63 reads const variantId = isValidVariantId(v) ? v : DEFAULT_VARIANT_ID; … if (!task) notFound();, and lib/runs.ts:2037-2045 makes the row match strict: t.task_id === taskId && (t.variant_id ?? DEFAULT_VARIANT_ID) === variantId. An experiment declares its own arm names (experiments/model-comparison.yaml → sonnet/opus; experiments/prompt-mutations-example.yaml → baseline/step-by-step/…), so such a run has NO default row and no default/ subtree. A URL without ?v= therefore matches zero rows → matches is empty → rawTask undefined → readTaskDetail returns null → hard 404. Before this PR the same URL matched the first row and rendered. This is a breaking change to the route README.md:51 documents (- /runs// — per-task detail: …) with no alias/fallback, and the two pages that build exactly that bare form are not in this diff and were not updated: app/trends/trends-view.tsx:275 (href={/runs/${e.runId}/${taskId}}) and app/watchlist/watchlist-view.tsx:27 (return /runs/${runId}/${taskId};). Fix: when ?v= is absent AND no row matches default, fall back to the task's first row (or redirect to ?v=<that row's variant_id>) instead of 404ing — the PR's own test lib/__tests__/variant-reads.test.ts:164 ("an unknown arm yields no detail") pins the strict behaviour only for an explicitly named unknown arm, which is the case that should stay a 404. Then document the resolved behaviour under README.md's Variants section and update the two link builders.
Non-blocking, but please consider before merge
- [Axis 1] Variant-id normalization (
?? DEFAULT_VARIANT_ID) is open-coded ~10-11 times across runs.ts/variants.ts instead of being owned by the module (andvariantId: string | nullnever uses its null) (evalboard/lib/variants.ts:40) —lib/variants.tsis the new dependency-free module whose stated job is "Variant (experiment arm) semantics, in one dependency-free module" — yet the single rule it encodes (an absent/null arm reads asdefault) is written out by hand 11 times, including twice inside variants.ts itself:
lib/variants.ts:40—const v = row.variantId ?? DEFAULT_VARIANT_ID;lib/variants.ts:50—for (const r of rows) s.add(r.variantId ?? DEFAULT_VARIANT_ID);app/runs/[id]/task-grid.tsx:229—const variant = t.variantId ?? DEFAULT_VARIANT_ID;app/runs/[id]/task-grid.tsx:317-318(insidebyTaskThenVariant)app/runs/[id]/task-grid.tsx:332-333(insidecompare,case "variant")app/runs/[id]/task-grid.tsx:774and:925-926(the twoVariantChipcall sites)app/runs/[id]/run-view.tsx:132—(t) => (t.variantId ?? DEFAULT_VARIANT_ID) === variantId,lib/runs.ts:2043—t.task_id === taskId && (t.variant_id ?? DEFAULT_VARIANT_ID) === variantId
Two of these are not merely similar, they are the same expression 15 lines apart in the same file. compare's variant case:
case "variant":
return (a.variantId ?? DEFAULT_VARIANT_ID).localeCompare(
b.variantId ?? DEFAULT_VARIANT_ID,
);is character-for-character the second leg of byTaskThenVariant at lines 317-320.
Fix: export one accessor from lib/variants.ts and route every site through it —
export function variantOf(row: { variantId?: string | null }): string {
return row.variantId ?? DEFAULT_VARIANT_ID;
}
export function compareVariant(a, b) { return variantOf(a).localeCompare(variantOf(b)); }then taskVariantKey/variantsOf call variantOf, compare's case "variant" returns compareVariant(a, b), and byTaskThenVariant becomes a.taskId.localeCompare(b.taskId) || compareVariant(a, b). lib/runs.ts:2043 needs its own snake_case flavour (t.variant_id) — keep it, but derive the constant from the same module as it already does. This is mechanically checkable in TS: a vitest test asserting variantsOf/taskVariantKey/compare all agree on a row with variantId: null would fail the moment one site drifts.
2. [Axis 3] Vacuous variant assertions: tests pass against the implementation they exist to rule out (taskVariantKey length prefix never exercises the separator; arm-ordering assertion matches the "A" in "Alpha", not the chip) (evalboard/app/runs/[id]/__tests__/task-grid.test.tsx:576) — Lines 575-576 read:
// Within the task, arms are in variant order.
expect(order[0].indexOf("A")).toBeGreaterThanOrEqual(0);
I executed the same fixture and dumped the rendered row text: the four rows are "AlphaAFailed1.001.0s—$0.100—", "AlphaBPassed…", "BetaAPassed…", "BetaBPassed…". humanizeTaskId title-cases the id, so order[0].indexOf("A") is 0 because of the "A" in "Alpha", not the variant chip. The assertion passes if the arms are emitted in reverse order, and it passes if VariantChip is deleted outright — it verifies nothing. (CodeQL's test-tautology query did not flag it.) Replace it with an assertion that reads the Variant cell positionally, e.g. const armOf = (tr: HTMLElement) => within(tr).getByText(/^(A|B)$/).textContent; then expect(rows.slice(0,2).map(armOf)).toEqual(["A", "B"]) — which is what byTaskThenVariant (task-grid.tsx:313-321) actually promises.
3. [Axis 4] Blob names from the remote listing are joined onto the local cache root with no traversal guard, so a ../ segment in a blob name writes outside the per-source cache dir (evalboard/lib/blob.ts:172) — downloadBlob treats the blob name as a trusted relative path:
172: const localPath = path.join(destRoot, blobName);
173: if (await exists(localPath)) return;
174: await fs.mkdir(path.dirname(localPath), { recursive: true });Both enumeration sites feed it names straight off the service — ensureRunDir (316: for await (const blob of c.listBlobsFlat({ prefix })) / 318: ops.push(downloadBlob(container, blob.name, destRoot));) and ensureTaskDir, whose prefix line this PR rewrote (360: : \${runId}/${variantId}/${taskId}/`;then361/367list-and-download).path.joinNORMALIZES.., so a blob literally named ///../../../../tmp/xstill matches the prefix filter and resolves to/tmp/x, outside destRoot; fs.mkdir(..., {recursive:true})then creates the intervening dirs anddownloadToFile writes the content. Every other id that reaches a path in this module is regex-gated (assertValidId/assertValidTaskId/ the newassertValidVariantIdat 336-338), but the blob name — the only segment that is fully attacker-chosen once the container is writable — is not. This is pre-existing (line 172 is unchanged) and requires container-write privilege (PR:H) plus a storage account without a hierarchical namespace, which rejects..segments (AC:H); I file it because it is the sink for the path construction this PR extends, and because the coverage report putslib/blob.tslines 312-322 and 340-371 — exactly these two download loops — at 0% and no test inlib/tests/referencesdownloadBlob/ensureTaskDir/listBlobsFlat. Fix: reject the name before any side effect, e.g. const localPath = path.resolve(destRoot, blobName); if (localPath !== destRoot && !localPath.startsWith(destRoot + path.sep)) throw new Error(...), mirroring the containment check resolveSafePathalready performs inlib/runs.ts:2477-2482. Mechanical gate (TS-appropriate, not a Python CEnnn): add a vitest case in lib/tests/blob.test.tsthat stubs the container client to yield a..-bearing blob name and asserts nothing is written outside the temp destRoot— the same harnesslib/tests/variant-reads.test.tsalready uses; optionally add an ESLintno-restricted-syntaxrule forbiddingpath.join(, )inlib/blob.ts. CVSS:3.1/AV:N/AC:H/PR:H/UI:N/S:U/C:N/I:H/A:N 4. **[Axis 7] isValidVariantId's charset rule is stricter than the producer's contract (and its "Mirrors _is_safe_component" comment is inaccurate); a rejected ?v=is silently coerced todefault, so the wrong arm's grid row, transcript and zip render and detail links 404** (evalboard/lib/variants.ts:17) — variants.ts:17definesconst VARIANT_ID_RE = /^[\w.-]+$/;under the comment atvariants.ts:15"Mirrors coder_eval's reports_junit._is_safe_component." That parity claim is wrong:src/coder_eval/reports_junit.py:106isreturn bool(value) and value not in {".", ".."} and "/" not in value and "\" not in value— a pure containment check with NO charset restriction. On the writer sideExperimentVariant.variant_id (src/coder_eval/models/experiment.py:25) is a bare strwith no validator (contrastexperiment_id, which has a kebab-case field_validatoratexperiment.py:172), and path_utils.build_task_run_dir (src/coder_eval/path_utils.py:122) writes run_dir / variant_id / …unsanitized. Sovariant_id: "gpt 4o"is accepted by the harness and written to disk, but rejected here. Both consumers then fall back silently rather than erroring:app/api/download/route.ts:23 const variantId = isValidVariantId(v) ? v : DEFAULT_VARIANT_ID;andapp/runs/[id]/[...task]/page.tsx:61do the same. On a run that also has an arm literally nameddefault, /api/download?run=…&task=…&v=gpt%204oreturns HTTP 200 with the DEFAULT arm's zip, named.zip(route.ts:39-43 keeps the plain task id whenvariantId === DEFAULT_VARIANT_ID) — indistinguishable from the arm that was asked for. Fix: either widen VARIANT_ID_REto the containment rule the Python side actually enforces (reject/, `, ., .., empty), or add a matching variant_id validator to ExperimentVariant so the two agree; and in the meantime return 400 from /api/download for a present-but-invalid v (the route already 400s on a missing run at route.ts:25-27) instead of coercing. Also correct the false parity comment at variants.ts:15.
Nits
- [Axis 1] The anonymous
{ variantId: string; metrics: RunMetrics }[]shape is spelled out three times instead of being named once (evalboard/app/runs/[id]/run-view.tsx:125) — The same structural type is repeated at three declarations in one file:
run-view.tsx:125—): { variantId: string; metrics: RunMetrics }[] {(return ofcomputeVariantMetrics)run-view.tsx:195—rows: { variantId: string; metrics: RunMetrics }[];(props ofPassRateByVariant)run-view.tsx:234—rows: { variantId: string; metrics: RunMetrics }[],(param ofvariantSub)
Adding a field (an arm's task count, a label, a colour) means editing three places, and tsc will only complain at the consumer that reads the new field — the producer and the other consumer stay silently narrow. The file already names its sibling shape (export interface RunMetrics at line 36), so follow it: add export interface VariantMetrics { variantId: string; metrics: RunMetrics } and use VariantMetrics[] at all three sites.
2. [Axis 1] TaskGrid is now a 516-line component with duplicated desktop/mobile render trees; the PR adds the variant affordance to both and recomputes taskVariantKey(t) three times per row (evalboard/app/runs/[id]/task-grid.tsx:506) — export function TaskGrid({ starts at task-grid.tsx:506 and the file ends at 1022 — one 516-line component, in a file this PR grew from 877 to 1022 lines (run-view.tsx likewise 584 → 741). It is still readable, hence Low rather than the CC 10-20 / god-class bands — but the PR pays the structural tax twice, because the desktop <table> and the mobile card list are parallel render trees over the same rows, so every new cell must be added in both:
- desktop:
key={${taskVariantKey(t)}#${t.replicateIndex ?? 0}}at :742,replicateCounts.get(taskVariantKey(t)) ?? 1at :753,replicatePassCounts.get(taskVariantKey(t)) ?? 0at :756,<VariantChip variantId={t.variantId ?? DEFAULT_VARIANT_ID} />at :771-775 - mobile: the same four at :904, :914, :917, :922-928
That is taskVariantKey(t) called three times per rendered row in each tree, on a route the automated results already flag as the app's largest (/runs/[id], 52.9 kB route / 168 kB first-load JS, vs 11.9 kB for the next largest). It is also computed twice in one expression at :557-558:
for (const t of tasks)
m.set(taskVariantKey(t), (m.get(taskVariantKey(t)) ?? 0) + 1);Cheapest fix inside this PR's blast radius: hoist const key = taskVariantKey(t); once per row (and once per loop iteration at :557) and read key thereafter. Structurally: extract the per-row cell set shared by the two trees into a TaskRowCells component so the next column does not have to be added twice — a make evalboard-verify-gated render test asserting the desktop and mobile trees expose the same set of per-row values would keep them honest.
3. [Axis 1] Per-task readers (readLogTail / readConversationLog / collectTaskFiles) grew to a six-slot positional tail with undefined placeholders at call sites (evalboard/lib/runs.ts:2322) — Appending variantId after source pushes these public lib functions to six positional params, three of them optional:
// lib/runs.ts:2322-2329
export async function readLogTail(
runId: string,
taskId: string,
replicate = 0,
maxBytes = 200_000,
source: Source = DEFAULT_SOURCE,
variantId: string = DEFAULT_VARIANT_ID,
): Promise<string> {so the only caller must skip a middle default with a literal hole (app/runs/[id]/[...task]/page.tsx:87-94):
await readConversationLog(
id,
taskId,
replicate,
undefined,
source,
variantId,
),The undefined hole predates this PR, but the PR widens the signature past the point where positional args stay readable, and the new argument's position is now inconsistent across the sibling readers it travels with — 4th in readTaskReplicates(runId, taskId, source, variantId) (:2052-2056) and collectTaskFiles(runId, taskId, source, variantId) (:2413-2419), 5th in readTaskDetail(runId, taskId, replicate, source, variantId) (:2066-2072) and ensureTaskDir(container, runId, taskId, destRoot, variantId) (lib/blob.ts:328-334), 6th here. A caller that mis-orders two string arguments gets no type error. Collapse the optional tail into one options object — readLogTail(runId, taskId, { replicate, maxBytes, source, variantId }) — which removes the placeholder and makes the argument's position irrelevant across all five readers.
4. [Axis 2] A validated variant id is indistinguishable from an unvalidated one at the type level — isValidVariantId(): id is string narrows to string, and the path-joining helper takes a bare unvalidated string (evalboard/lib/runs.ts:737) — taskContentBase is the function that turns a variant id into a filesystem path, and its parameter carries no evidence of validation:
runs.ts:734-746 function taskContentBase(runId, taskId,
variantId: string = DEFAULT_VARIANT_ID,
source: Source = DEFAULT_SOURCE): string {
...
: path.join(dir, runId, variantId, taskId);
The validator returns nothing stronger than what an unvalidated ?v= param already is — lib/variants.ts:19 export function isValidVariantId(id: unknown): id is string — so the type system cannot tell the two apart, and the invariant ends up enforced three different ways for the same input:
- throw:
lib/blob.ts:48-52assertValidVariantId(called atblob.ts:353, before theif (LOCAL_RUNS_DIR) return;early-out) - return null:
lib/runs.ts:2422 if (!isValidVariantId(variantId)) return null;incollectTaskFiles - nothing:
readTaskDetail(runs.ts:2066-2096),readLogTail(:2322-2337),readConversationLog(:2355-2370) each acceptvariantId: string = DEFAULT_VARIANT_IDand reachtaskContentBaseat:2096/:2337/:2370relying onensureTaskDir's throw, andlib/reviews.ts:46-60 readTaskReview(runId, variantId: string, taskId, replicate, source)path-joinsvariantIdwith no check at all.
Same bad input therefore yields a 404 down one reader and an unhandled throw (500) down three others. The positional shape compounds it:readLogTail(runId, taskId, replicate, maxBytes, source, variantId)is six positional params with four defaulted, which forces the new test to writereadLogTail(AB_RUN, TASK, 0, undefined, undefined, "live-v1")(lib/__tests__/variant-reads.test.ts:158-167), andreadTaskReviewputsvariantIdBEFOREtaskId— two adjacent barestrings whose swap tsc cannot catch (app/runs/[id]/[...task]/page.tsx:70-76passes them in that order, correctly, one line afterpage.tsx:62passes them in the opposite order toreadTaskDetail).
Fix: give the validator a nominal result — export type VariantId = string & { readonly __variantId: unique symbol } with isValidVariantId(id: unknown): id is VariantId — and type taskContentBase, readTaskReview, and the variantId parameter of the four public readers as VariantId. Then an unvalidated ?v= string cannot be path-joined and the three enforcement styles collapse into one that the compiler checks. Alternatively (smaller change) convert the readers' trailing optionals to a single options object, which removes the positional-slot hazard even without branding.
5. [Axis 3] variantSub test fabricates RunMetrics through an as never as double cast (evalboard/app/runs/[id]/__tests__/run-view.test.ts:180) — Line 180 reads metrics: { cost } as never as import("../run-view").RunMetrics,. The as never as T double cast is an unsound escape hatch that defeats the one check that would keep this fixture honest: if variantSub's pick callback later reads a second RunMetrics field, the fixture keeps compiling and the test keeps passing against an object that has only cost. Build the fixture from the real shape instead — e.g. a metrics(partial: Partial<RunMetrics>): RunMetrics helper that spreads over a zero-valued base, mirroring the row() helper already used at the top of this same file (lines 8-40) — so a future required field surfaces as a compile error rather than as undefined at runtime.
6. [Axis 6] Unreachable ?? 0 default in the grid's variant-aware sort would silently rank a row as failed (evalboard/app/runs/[id]/task-grid.tsx:623) — The new worst-arm ranking seeds worstByTask from every row in arr immediately above, so the ?? 0 can never fire:
615 const worstByTask = new Map<string, number>();
616 for (const t of arr) {
617 const r = statusSortRank(t.status);
618 const cur = worstByTask.get(t.taskId);
619 if (cur === undefined || r < cur) worstByTask.set(t.taskId, r);
620 }
621 arr.sort(
622 (a, b) =>
623 (worstByTask.get(a.taskId) ?? 0) -
624 (worstByTask.get(b.taskId) ?? 0) ||
625 byTaskThenVariant(a, b),
626 );If it ever did fire, 0 is not a neutral default — statusSortRank returns 0 for failed/error (lib/status.ts:47: if (c === "failed" || c === "error") return 0;), so an unmapped row would be silently sorted to the top of the grid as though it had failed. Replace ?? 0 with a non-null assertion-free lookup that fails loud in dev (or hoist the map build so the lookup is total), rather than a default that mislabels.
7. [Axis 6] variantSub silently omits an arm that has no value, so the per-arm sub-line cannot be reconciled with the pooled headline (evalboard/app/runs/[id]/run-view.tsx:240) — ```ts
237 const parts: string[] = [];
238 for (const { variantId, metrics } of rows) {
239 const v = pick(metrics);
240 if (v != null) parts.push(${variantId} ${v});
241 }
242 return parts.length ? parts.join(" · ") : undefined;
The Total-cost and Time tiles keep a pooled headline (`run-view.tsx:602` `metrics.cost != null ? ...` and `:617` `fmtDuration(metrics.duration)`) but the sub-line built here drops any arm whose value is null. On a two-arm run where arm B recorded no cost, the tile reads `$0.20` with sub-line `A $0.20` — a reader cannot tell whether arm B cost nothing, has no cost data, or does not exist, and the parts no longer account for the headline. The comment above the function ("Arms missing the value are dropped rather than rendered as a dash") documents the choice but not its consequence. Render the arm with an explicit `—` so a missing measurement is visible as missing.
8. **[Axis 7] README claim "no link carries ?v=" is false for a single-arm run whose arm is not named `default`** (`evalboard/README.md:93`) — README.md:92-95 states "Every one of those is inert on a run without variants: the column is dropped, the tiles keep their existing pooled numbers and percentiles, and no link carries `?v=`." The column/tile half is gated on arm COUNT (`task-grid.tsx:572` `const hasVariants = variantIds.length > 1;`), but the link half is gated on arm NAME (`task-grid.tsx:232` `if (variant !== DEFAULT_VARIANT_ID) params.set("v", variant);`). `experiments/permissions-smoke.yaml` declares exactly one variant, `with-deny`, so on that run the Variant column is dropped and the tiles stay pooled, yet every grid link carries `?v=with-deny`. Reword to "no link carries `?v=` on a run whose only arm is `default`", or gate the link emission on the run's arm count so the doc and the code describe the same condition.
9. **[Axis 7] Comment rot around the variant rename: a stale `default/<task_id>/…` path comment and an orphaned block comment attached to the wrong declaration** (`evalboard/lib/runs.ts:2122`) — The PR rewrote the analogous comments at `lib/runs.ts:727-733` (`<id>/<variantId>/<taskId>`), `lib/runs.ts:2409` (``<variantId>/<taskId>/``) and `lib/blob.ts:351-352`, but `lib/runs.ts:2122-2123` still reads "New layout yields `default/<task_id>/00/artifacts/...`; flat layout yields `default/<task_id>/artifacts/...`" — while the code two lines below now produces `<variantId>/<task_id>/00/artifacts/...` for a variant run (that is precisely why `resolveSafePath` at lib/runs.ts:2467 was widened from `parts[0] === "default"` to `isValidVariantId(parts[0])`). Replace both `default` occurrences with `<variant_id>`.
10. **[Axis 8] taskContentBase discards variantId on the activation branch while rowMatches honours it, so an activation read can pair one arm's run.json row with another arm's content directory** (`evalboard/lib/runs.ts:743`) — `evalboard/lib/runs.ts:743-745` is:
return isActivationTaskId(taskId)
? path.join(dir, runId, "activation", DEFAULT_VARIANT_ID, taskId)
: path.join(dir, runId, variantId, taskId);
and `lib/blob.ts:358-360` does the same for the blob prefix. But `rowMatches` (`lib/runs.ts:2037-2045`) applies `(t.variant_id ?? DEFAULT_VARIANT_ID) === variantId` to *both* branches, including the activation `run.json` read at `lib/runs.ts:2057` / `2079`. So for an activation task the row is selected by the caller's arm while the content path is pinned to `activation/default`. Today this only fails closed (a hand-crafted `?v=live-v1` on an activation task selects the row and then reads a non-existent `activation/default/...` dir, rendering the row's status/score beside empty transcript/log/artifact sections), because the activation sub-run is single-arm and `activation/page.tsx:241` links without `?v=`.
The assumption is stated in the comments but not enforced. Either drop `variantId` from `rowMatches` for the activation branch so both halves agree, or honour it in `taskContentBase` / the blob prefix and let the assumption break loudly if the eval-runner ever runs the activation suite under `variants:` — note that runner lives in the separate `coder-eval-uipath` repo, so nothing in this repo would fail if it changed.
## What's Missing
**Parallel paths:**
- 🟡 Review data was NOT made arm-aware alongside the row identity: the PR makes `taskVariantKey` the grid row key, yet all four review lookups still key on the task id alone — `task-grid.tsx:720` and `:890` (desktop + mobile card), and the review-tag filter at `run-view.tsx:387`/`:402`. The loss happens upstream in `lib/reviews.ts:90 indexByTask`, which does `if (!out.has(e.task_id)) out.set(e.task_id, e)` and throws away the `variant_id` the entry already carries — and the producer contract is explicit that the index has one entry per `(task_id, variant_id, replicate)` triple (`.claude/commands/coder-eval-review.md:85`). Net effect on a 2-arm run: both arms display the first arm's review tags/summary, and clicking a review-tag chip selects both arms' rows. Fix: key `EntriesByTask` on `taskVariantKey` (plus replicate) and pass the row's arm at all four call sites. _(trigger: evalboard/app/runs/[id]/task-grid.tsx)_
- 🟡 Only ONE of the four in-repo task-detail link builders learned `?v=`. `task-grid.tsx:235` appends it; `task-grid.tsx:159` (the in-diff mature carry-forward "open that execution" link), `app/trends/trends-view.tsx:275` and `app/watchlist/watchlist-view.tsx:27` still emit the bare `/runs/<id>/<task>` form, which now `notFound()`s on any run whose arms are not named `default`. (`app/runs/[id]/activation/page.tsx:241` is safe — activation is pinned to the `default` segment by construction.) _(trigger: evalboard/app/runs/[id]/task-grid.tsx)_ _(restates: Axis 7: Variant-less task deep links hard-404 on any run whose arms are not named `default`)_
- 🔵 The task page gained arm addressing but no arm switcher. It renders the arm in the breadcrumb (`page.tsx:117-124`) and keeps the replicate selector, which now carries `variantQuery` (`page.tsx:151`), yet there is no sibling-arm control next to it — the sole way to compare a task's two arms is to go back to the grid and click the other row. The replicate selector is the obvious precedent for the affordance. _(trigger: evalboard/app/runs/[id]/[...task]/page.tsx)_
**Downstream consumers:**
- 🟡 The cross-run aggregators still treat `taskId` as the task identity, so a multi-arm run silently mis-samples. `lib/trends.ts:133` (`if (seenThisRun.has(t.taskId)) continue;`) keeps only the FIRST arm's status per run — the comment justifies first-occurrence sampling for REPLICATES (repeats of one config), which is not what arms are; `lib/watchlist.ts:276 taskSequences` merges both arms into one per-task sequence, so `seq.some(isPass)` in `neverPassed` lets a passing arm mask an arm that never passes; `lib/overview.ts:861-871` rolls repo tags up the same way. `perTaskPassCounts` was correctly re-keyed in `lib/status.ts` for the run page — these three were not, and their row type makes `variantId` optional so nothing fails to compile. _(trigger: evalboard/lib/status.ts)_
- 🟡 The PR argues (README:66-71, and enforces via `PassRateByVariant`) that a pooled pass rate "averages configurations that were deliberately made to differ" and must not appear — but every other surface still publishes exactly that number for the same run, unlabelled: the runs index and window summary (`app/_overview/window-summary.tsx:86`, `lib/overview.ts:915 passRate`), the trends table, and `/path-to-ga`. A 2-arm run also contributes 2× rows to those denominators. Either state the scope ("run page only") in the README or carry the arm split outward. _(trigger: evalboard/app/runs/[id]/run-view.tsx)_
**Tests:**
- 🟠 The new tests cover `lib/variants.ts`, three of the six readers, the grid and the pass-rate tile — but not the task page (`[...task]/page.tsx`, 0/375 statements, incl. the 4-consecutive-string `readTaskReview` argument order), `/api/download`'s `?v=` normalization and `__`-suffixed zip root, `readConversationLog`'s variant arg, `resolveSafePath`'s widened `parts[0]` dispatch, or `ensureTaskDir`'s new `assertValidVariantId` throw + the changed in-flight dedupe key (`task:<c>:<run>/<variant>/<task>` — the pre-PR key would have collapsed two arms' concurrent downloads into one). _(trigger: evalboard/lib/__tests__/variant-reads.test.ts)_ _(restates: Axis 3: The new variant plumbing ships with zero executed coverage across every non-grid surface)_
- 🟡 No mechanical gate would have caught the untested-new-module case: `evalboard/vitest.config.ts` declares no `coverage` block at all and `make evalboard-verify` runs only `tsc --noEmit` + vitest + `next build`, so a brand-new file at 0% coverage is green. TS-appropriate fix (not a Python CEnnn rule): add `test.coverage.thresholds` (e.g. lines/statements 80, `coverage.include` scoped to `lib/**` + `app/**`) to `vitest.config.ts` and wire `--coverage` into the `verify` script so `make evalboard-verify` fails on it. _(trigger: evalboard/lib/variants.ts)_
**Display & mapping dicts:**
- 🟡 The Failed tile was left pooled while every tile around it became arm-aware: Pass rate switched to `PassRateByVariant`, Total cost and Time gained `variantSub` sub-lines, but `run-view.tsx:611-622` still prints one blended `metrics.taskFailed` / `metrics.failedTotal` and its `N fail · N error` sub-line with no arm split. On the PR's own A/B render fixture the tile reads "2" with nothing saying both failures are arm B's — the exact ambiguity the pass-rate change exists to remove. The README's Variants bullet list (README:63-84) enumerates pass rate, cost, time, grid and detail, and likewise omits Failed. _(trigger: evalboard/app/runs/[id]/run-view.tsx)_
**Daily/nightly:**
- 🟡 The nightly blast radius is unstated. Evalboard is the read surface for the blob-backed nightly runs, and this PR changes the content dir EVERY task-detail read resolves to (`taskContentBase` now takes `variantId`), the blob prefix and in-flight dedupe key in `ensureTaskDir`, and turns "no matching row" from a degraded row-summary page into a hard 404. Today's nightly runs are all single-arm `default`, so the intended radius is zero — but nothing in the PR, the README, or the tests says so: the only backward-compatibility evidence is a synthetic `LEGACY_RUN` fixture in `variant-reads.test.ts:104-118`, not a real nightly `run.json` shape. _(trigger: evalboard/lib/runs.ts)_
- 🟡 The cross-repo contract this PR now depends on is unpinned on both sides. Evalboard requires `run.json`'s `variant_id` to equal the on-disk directory segment; Python does honour that (`reports_experiment.eval_result_to_task_dict` always stamps it, `path_utils.build_task_run_dir:122` writes the same string), but `ExperimentVariant.variant_id` (`models/experiment.py:25`) is an unvalidated `str` while the reader enforces `/^[\w.-]+$/` and silently coerces a reject to `default`. There is no parity test in either direction — the TS-appropriate gate is a vitest case asserting `isValidVariantId` and `reports_junit._is_safe_component` agree over a fixed id corpus, run under `make evalboard-verify` (the same pattern `lib/__tests__/pricing-parity.test.ts` already uses for the pricing mirror). _(trigger: evalboard/lib/variants.ts)_ _(restates: Axis 7: isValidVariantId's charset rule is stricter than the producer's contract)_
## Harness & Lint Improvements
**Static checks (lint / type):**
- [ce-lint] New **CE046 `VariantSeamSingleOwner`** — a whole-tree text rule wired as a dedicated `@pytest.mark.lint` class (like CE026/CE033/CE035, not a `BaseRule`, since it scans `.ts`/`.tsx` rather than a Python AST; add `tests/lint/variant_seam.py` + a class in `tests/test_custom_lint.py`). Two clauses over `evalboard/app/**` and `evalboard/lib/**`: (1) the token sequence `?? DEFAULT_VARIANT_ID` may appear only in `evalboard/lib/variants.ts`, which must export `variantOf(row)` / `compareVariant(a, b)` accessors every other read site calls; (2) the literal `default` may not appear as a path segment in a path expression or in a path-shape comment (`default/<task_id>/…`) outside `variants.ts` — use `DEFAULT_VARIANT_ID` / `<variant_id>`. Runs in `make lint` (pure Python, no Node toolchain), which is where a TS seam rule is enforceable for free. _Prevents:_ The 10 open-coded `?? DEFAULT_VARIANT_ID` normalizations (task-grid.tsx:229/317/318/332/333/774/925, run-view.tsx:132, runs.ts:2043, plus two inside variants.ts itself) that the module header claims to own, including the token-identical `byTaskThenVariant` / `compare case "variant"` duplication 15 lines apart; and the stale `default/<task_id>/…` comment at lib/runs.ts:2122-2123 the variant rename left behind.
- [ce-lint] New **CE047 `IdContractParity`** — cross-language parity rule in the spirit of `evalboard/lib/__tests__/pricing-parity.test.ts`, but Python-side so it lands in `make lint`. Extract the reader predicates' regex literals from the TS source (`VARIANT_ID_RE` at evalboard/lib/variants.ts:17, `ID_RE`/`TASK_ID_RE` at evalboard/lib/blob.ts:17/21), translate to `re` (identical ASCII semantics), and assert every id the *writer* can emit is accepted by the corresponding *reader*: construct `ExperimentVariant(variant_id=x)` / `TaskDefinition(task_id=x)` over a fixed corpus (`gpt 4o`, `arm#1`, `sonnet`, `.`, `..`, `a/b`) and fail when the model accepts an id the reader rejects. Passing it requires adding a `field_validator("variant_id")` to `ExperimentVariant` (src/coder_eval/models/experiment.py:25), mirroring the `experiment_id` validator at :171. The rule must also kill the false parity comment at variants.ts:15 ("Mirrors coder_eval's reports_junit._is_safe_component") — reports_junit.py:106 is containment-only (`not in {'.','..'}`, no `/`, no `\`) with no charset restriction at all. _Prevents:_ The reader-stricter-than-writer divergence: an arm named `gpt 4o` is accepted by the harness and written to `<run>/gpt 4o/<task>/` by `path_utils.build_task_run_dir` (path_utils.py:122) but rejected by `isValidVariantId`, so `/api/download?...&v=gpt%204o` silently serves the *default* arm's zip under the plain `<taskId>.zip` name. Also closes the same pre-existing gap for `task_id` (models/tasks.py:435, bare `str`).
- [ce-lint] New **CE048 `NoSilentIdCoercion`** — forbid `isValid<Id>(x) ? x : DEFAULT_<ID>` when `x` is a *present* request input (a `searchParams` value / route query param) anywhere under `evalboard/app/**`: a present-but-invalid id must reject (400 from a route handler, `notFound()` from a page), never fall back. Absent ⇒ default stays legal, so the check keys on the source expression's provenance (raw param vs. already `??`-defaulted) within the same function. _Prevents:_ The two new silent-coercion sites — `app/api/download/route.ts:23`, which returns HTTP 200 with the wrong arm's zip, indistinguishable from the requested arm (the route already 400s on a missing `run` at :25-27), and `app/runs/[id]/[...task]/page.tsx:61`, which renders the wrong arm's row and transcript.
- [ce-lint] New **CE049 `TaskHrefBuilderSeam`** — the task deep-link template `` `/runs/${…}/${…}` `` may appear only inside one builder (`evalboard/app/_lib/task-href.ts`, sibling to the existing `withSource` seam at app/_lib/source-param.ts:22) that appends `?v=` / `?r=` / `?source=` from a typed options object; every other module calls `taskHref(...)`. Purely textual, so it slots in beside CE046. _Prevents:_ The bare-form deep links that hard-404 on any run whose arms are not named `default` — app/trends/trends-view.tsx:275, app/watchlist/watchlist-view.tsx:27, the mature carry-forward link at app/runs/[id]/task-grid.tsx:159 (inside the diff), app/runs/[id]/activation/page.tsx:241 — while only task-grid.tsx:235 appends `?v=`. It also turns the README claim at evalboard/README.md:93 ("no link carries `?v=`") into a testable property of one function instead of a hand-audited invariant over five call sites.
- [ce-lint] New **CE050 `NoWideOptionalTail`** — an exported function in `evalboard/lib/*.ts` may not exceed 4 positional parameters, and may not have two or more *adjacent* same-primitive-typed parameters where at least one is optional; collapse the tail into an options object. Detectable from signature text alone. Pair with the type-level half that `tsc --noEmit` then enforces for free: brand the validated id (`export type VariantId = string & { readonly __variantId: unique symbol }`, `isValidVariantId(id: unknown): id is VariantId`) and type `taskContentBase`, `readTaskReview`, and the four public readers' `variantId` params as `VariantId`, so an unvalidated `?v=` cannot reach a `path.join`. _Prevents:_ The six-positional-param readers with a literal `undefined` hole (`readLogTail`/`readConversationLog`, lib/runs.ts:2322/2355, called as `(id, taskId, replicate, undefined, source, variantId)` at page.tsx:87-94), the inconsistent argument position across sibling readers (4th/5th/6th), and the four-consecutive-`string` swap hazard in `readTaskReview(runId, variantId, taskId, replicate, source)` (lib/reviews.ts:46) — page.tsx:70-76 passes those two ids one line after page.tsx:62 passes them in the opposite order. It also collapses the three enforcement styles for one input (throw at blob.ts:48-52, `return null` at runs.ts:2422, nothing in three other readers) into one the compiler checks.
- [ce-lint] New **CE051 `NoDoubleCastInTests`** — forbid `as never as` and `as unknown as` under `evalboard/**/__tests__/**`; fixtures must be built from the real exported type (e.g. a `metrics(partial: Partial<RunMetrics>): RunMetrics` helper spreading over a zero-valued base, mirroring the `row()` helper at the top of the same file). One-line textual scan. _Prevents:_ `evalboard/app/runs/[id]/__tests__/run-view.test.ts:180` (`metrics: { cost } as never as RunMetrics`), which defeats the one check that keeps the fixture honest — if `variantSub`'s `pick` callback later reads a second `RunMetrics` field, the fixture still compiles and the test passes against `undefined`.
- [bandit-codeql] Add `javascript-typescript` to the CodeQL matrix in `.github/workflows/codeql.yml` (today `languages: python`, single-language — so **no JS/TS file in the repo is analyzed at all**), keeping `queries: security-and-quality` and running it as a matrix job so Python analysis timing is unchanged. This turns on `js/path-injection` and `js/zipslip` over `evalboard/lib/**` plus the `?v=` → `path.join` flows this PR introduces. _Prevents:_ `evalboard/lib/blob.ts:172` — `const localPath = path.join(destRoot, blobName)` with no containment check, fed by the two listing loops (`ensureRunDir` :316/:318 and the PR-rewritten `ensureTaskDir` :360/:361/:367). `path.join` normalizes `..`, so a blob named `<run>/<variant>/<task>/../../../../tmp/x` still matches the prefix filter and writes outside `destRoot`, while every *constructed* id in that module is regex-gated and lib/runs.ts:2477-2482 already implements the exact containment check that is missing here.
**Harness improvements (not statically reachable):**
- Add a `coverage` block to `evalboard/vitest.config.ts` (it has none today, though `@vitest/coverage-v8` is already a devDependency): `provider: 'v8'`, `all: true`, `include: ['app/**','lib/**']`, plus `thresholds` with a per-file floor (e.g. `perFile: true, statements: 40`) or at minimum an entry covering `app/runs/**/page.tsx`; then run `vitest run --coverage` in `package.json`'s `verify` script so `make evalboard-verify` and the CI `evalboard` job fail on a 0%-covered route. Add the route test itself: `app/runs/[id]/[...task]/__tests__/page.test.tsx` that `vi.mock`s `@/lib/runs` + `@/lib/reviews`, awaits the async server component, and asserts `readTaskReview` was called as `(id, 'preview-v2', taskId, '00', source)` in that order, that `?v=preview-v2` propagates into the replicate links and the `/api/download` href, and that with no `?v=` every emitted href is byte-identical to the pre-variant form. _Why not static:_ Coverage is a property of an executed suite — `tsc` and any grep rule see a fully-typed, ordinary file whether or not a single test ever imports it. The 0/375 statements on `app/runs/[id]/[...task]/page.tsx` was only visible by running v8 coverage. _Prevents:_ The zero-coverage variant plumbing on every non-grid surface (task detail page, `/api/download` `?v=` normalization + `__` zip root, `resolveSafePath`'s widened dispatch, `readConversationLog`'s variantId, the blob prefix/dedupe key), and specifically the `readTaskReview` argument swap that type-checks silently and degrades to "no review rendered".
- Add a multi-arm run fixture under `evalboard/lib/__tests__/` — a temp runs dir whose run.json rows are stamped `variant_id: sonnet`/`opus` with content under `<run>/sonnet/…` (the real shape of `experiments/model-comparison.yaml`; even `prompt-mutations-example.yaml`'s control arm is named `baseline`, never `default`) — and assert the no-`?v=` deep link resolves (redirect to the first arm, so the resolved arm stays visible in the URL) instead of returning null, while keeping the existing `variant-reads.test.ts:164` "explicitly named unknown arm 404s" case. _Why not static:_ The defect is a data-dependent join: `rowMatches` (lib/runs.ts:2037-2045) requires `(variant_id ?? 'default') === variantId` while the URL supplies no arm. Nothing in the source text is wrong — it needs a run.json tree with non-`default` arm names to fail. _Prevents:_ The high finding: variant-less task deep links hard-404 on every multi-arm run (a regression from the pre-PR degraded-but-rendering page), dead-ending every trends and watchlist row.
- Add a writer→reader round-trip test that materializes a real multi-arm run directory via `path_utils.build_task_run_dir` (or a checked-in golden of one) and asserts the evalboard readers resolve every arm's run.json row *and* its content dir together — one assertion that the row-selection key and the content-path key are the same key. _Why not static:_ It is a cross-module semantic agreement between two internally-consistent functions: `rowMatches` honours `variantId` on both branches while `taskContentBase` (lib/runs.ts:743-745) and the blob prefix (lib/blob.ts:358-360) pin the activation branch to `activation/<DEFAULT>`. No AST or grep rule can see that two different keys were meant to be one; it surfaces only when a real tree is read. _Prevents:_ The activation-branch mismatch that pairs one arm's run.json row with another arm's content directory (status/score rendered beside an empty transcript, log and artifact list), which today only fails closed because the activation sub-run happens to be single-arm.
- Replace the tautological ordering assertion at `app/runs/[id]/__tests__/task-grid.test.tsx:576` with a positional read of the Variant cell (`within(tr).getByText(/^(A|B)$/)`, assert `["A","B"]`), export `compare` / `byTaskThenVariant` from task-grid.tsx so the ordering contract is unit-testable at all, and adopt a mutation spot-check for the grid's sort/aggregation helpers (a scripted stryker run over task-grid.tsx + lib/variants.ts, or a hand-written reversed-comparator guard) in the nightly job. _Why not static:_ A tautology is well-typed and syntactically ordinary — `tsc` passes it and CodeQL's test-tautology query did not flag it. Only executing the test against a deliberately wrong implementation proves the assertion binds: reversing `byTaskThenVariant` and deleting `VariantChip` outright both leave the current assertion green, because `humanizeTaskId` title-cases "alpha" so `indexOf("A")` matches the task name, not the chip. _Prevents:_ The vacuous variant-ordering assertion, and the same class at `lib/__tests__/variants.test.ts:59` — assertions that hold against the naive implementation they exist to rule out.
- Add a `lib/__tests__/blob.test.ts` case that stubs the container client's `listBlobsFlat` to yield a `..`-bearing blob name against a temp `destRoot` and asserts nothing is written outside it, alongside the source fix (`path.resolve` + `startsWith(destRoot + path.sep)` containment in `downloadBlob`, mirroring `resolveSafePath` at lib/runs.ts:2477-2482). _Why not static:_ The taint source is an Azure SDK async iterator behind an optional dependency; even with CodeQL JS enabled there is no guarantee it models `listBlobsFlat` as a remote source, and the two download loops (blob.ts:312-322 and 340-371) are at 0% executed coverage with no test in the repo referencing `downloadBlob`/`ensureRunDir`/`ensureTaskDir`/`listBlobsFlat`. _Prevents:_ The unguarded `path.join(destRoot, blobName)` write-outside-cache sink, as a backstop to the CodeQL JS enablement.
- Add a desktop/mobile render-parity test for `TaskGrid`: render both trees over the same rows and assert they expose the same set of per-row values (status, variant chip, replicate counts), so a new column cannot be added to one tree only. Pair it with the cheap in-PR cleanup — hoist `const key = taskVariantKey(t)` once per row instead of three calls per row per tree (task-grid.tsx:742/753/756 and :904/914/917) and the double call in one expression at :557-558 — and name the thrice-repeated `{ variantId: string; metrics: RunMetrics }` shape as an exported `VariantMetrics` interface (run-view.tsx:125/195/234). _Why not static:_ "These two JSX subtrees render the same fields" is a semantic equivalence between two independently-written trees; a lint rule can see neither the omission nor the drift without rendering both. (The `VariantMetrics` half *is* statically enforced once the interface exists — by `tsc` itself — which is why it belongs in this cleanup rather than in a new rule.) _Prevents:_ The 516-line `TaskGrid` with parallel desktop/mobile render trees this PR had to edit twice, the redundant `taskVariantKey` recomputation on the app's largest route (52.9 kB route / 168 kB first-load JS), and the three-times-spelled anonymous variant-metrics shape.
## Top 5 Priority Actions
1. Stop silently coercing an invalid `?v=` to the default arm — `evalboard/lib/variants.ts:17` (`/^[\w.-]+$/`) is stricter than the producer's actual contract (`src/coder_eval/reports_junit.py:106` is containment-only and `ExperimentVariant.variant_id` at `src/coder_eval/models/experiment.py:25` has no validator), so `/api/download?...&v=gpt%204o` returns 200 with the DEFAULT arm's zip under the plain task-id name (`evalboard/app/api/download/route.ts:23,39-43`); return 400 on a present-but-invalid `v`, align the two charsets (widen the regex or add a `variant_id` validator), and delete the false "Mirrors _is_safe_component" comment at `evalboard/lib/variants.ts:15`.
2. Restore reachability of variant-less deep links — `evalboard/app/runs/[id]/[...task]/page.tsx:61` falls back to `default` and `rowMatches` (`evalboard/lib/runs.ts:2037-2045`) matches strictly, so on any run whose arms are named (e.g. `sonnet`/`opus`, `baseline`) a bare `/runs/<id>/<task>` 404s; redirect to `?v=<first arm>` when no `default` row exists, keep the explicit-unknown-arm 404 pinned by `evalboard/lib/__tests__/variant-reads.test.ts:164`, and fix the three bare link builders at `evalboard/app/trends/trends-view.tsx:275`, `evalboard/app/watchlist/watchlist-view.tsx:27` and `evalboard/app/runs/[id]/task-grid.tsx:159`.
3. Add executed coverage for the 0/375-statement task detail page (`evalboard/app/runs/[id]/[...task]/page.tsx:70-76`) asserting `readTaskReview(id, variantId, taskId, replicate, source)` argument ORDER — four consecutive `string` params (`evalboard/lib/reviews.ts:46`) mean a `variantId`/`taskId` swap type-checks clean and merely renders no review — plus cases that a `?v=` propagates into the replicate and `/api/download` hrefs and that a variant-less URL emits byte-identical pre-variant links, and wire a `coverage.thresholds` block into `evalboard/vitest.config.ts` so the next 0%-covered file fails `make evalboard-verify`.
4. Replace the vacuous arm-ordering assertion at `evalboard/app/runs/[id]/__tests__/task-grid.test.tsx:576` — `order[0].indexOf("A")` matches the "A" in the humanized task id "Alpha", and the test still passes when `byTaskThenVariant` (`evalboard/app/runs/[id]/task-grid.tsx:313-321`) is reversed or `VariantChip` is deleted — with a positional read of the Variant cell asserting `["A", "B"]`.
5. Add the missing traversal containment check in `downloadBlob` at `evalboard/lib/blob.ts:172` (`path.join(destRoot, blobName)` normalizes a `..`-bearing remote blob name straight out of the cache root), mirroring `resolveSafePath` at `evalboard/lib/runs.ts:2477-2482`, and cover the two untested listing loops (`evalboard/lib/blob.ts:316-318`, `:360-367`) with a vitest case that stubs a `..` blob name.
---
**Stats:** 0 🔴 · 2 🟠 · 4 🟡 · 10 🔵 across 8 axes reviewed.
uipreliga
left a comment
There was a problem hiding this comment.
Fix what you agree with and 🚢
|
where is the a/b getting defined? |
Only experiments/default.yaml names its arm `default`. Every other experiment
names its own — sonnet/opus, baseline, e2e/smoke, with-deny — so an experiment
run has no `default` row at all, and a /runs/<id>/<task> link carrying no ?v=
matched zero rows and 404'd. Trends, the watchlist and every pre-variant
bookmark still emit exactly that form.
Resolution happens once, before any reader runs, so the arm travels to the row
match, the content path, the review, the log and the links on the page. A link
naming an arm the run does not have still 404s: the point of addressing an arm
is that you get that arm or nothing.
Also drops the arm-ordering assertion in task-grid.test.tsx, which read
indexOf("A") on a row whose humanized task id is "Alpha" — it matched the task
name, not the variant chip, and held with the comparator reversed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@tmatup here's the documentation for how coder eval variants work: https://coder-eval.com/docs/ab-experiments This PR only adds the display UI for this feature which already existed. |
|
Test failure is unrelated - merging |

What
Evalboard can now display a coder_eval run that declares
variants:— both arms visible, each task drillable per arm.The framework has been variant-complete for a while: every
run.jsonrow carriesvariant_idandreports_experiment.pyalready emits win rates and a paired comparison. Evalboard was the only layer that could not read any of it. A two-arm run rendered duplicate task ids whose detail pages both resolved to whichever arm happened to be first in the file.Screenshot
Why this shape
experiment.md. The dashboard reports the arms and their spread and stops.Known limitations
Trends, overview and watchlist are unchanged. They already collapse to one representative row per (run, task), so a multi-variant run contributes one arbitrary arm there rather than double-counting.
analysis.mdlikewise still describes both arms as one population. Neither blocks reading an A/B run on its own page; both are follow-ups.🤖 Generated with Claude Code