From 94758acc80dc7e09003da55b461ca7b431be0ad3 Mon Sep 17 00:00:00 2001 From: CarlesUIPath Date: Mon, 27 Jul 2026 18:23:23 +0100 Subject: [PATCH 1/5] fix(evalboard): average per-task metrics across repeats in the run grid The run page task-grid collapsed a task's replicates to a single representative row and displayed that one run's metrics verbatim, so the Cost column (and score/duration/turns/tokens) showed the representative replicate's value instead of the average over the repeats. Add collapseReplicates() to lib/status.ts: it keeps the representative only for categorical fields (status pill, ?r=NN detail link, tags/skill/ model) and averages the quantitative columns across all replicates. With repeats disabled it's a no-op. Round the now-fractional turns display to at most 2 decimals (dropping trailing zeros) in fmtTurnsCount. Co-Authored-By: Claude Opus 4.8 (1M context) --- evalboard/app/runs/[id]/task-grid.tsx | 36 ++++------- evalboard/lib/__tests__/status.test.ts | 82 ++++++++++++++++++++++++++ evalboard/lib/__tests__/turns.test.ts | 6 ++ evalboard/lib/status.ts | 73 +++++++++++++++++++++++ evalboard/lib/turns.ts | 5 +- 5 files changed, 176 insertions(+), 26 deletions(-) create mode 100644 evalboard/lib/__tests__/status.test.ts diff --git a/evalboard/app/runs/[id]/task-grid.tsx b/evalboard/app/runs/[id]/task-grid.tsx index 20fc07b5..eb73d061 100644 --- a/evalboard/app/runs/[id]/task-grid.tsx +++ b/evalboard/app/runs/[id]/task-grid.tsx @@ -14,7 +14,11 @@ import { MaturePill, StatusPill, } from "@/lib/pills"; -import { isPassStatus, perTaskPassCounts, statusSortRank } from "@/lib/status"; +import { + collapseReplicates, + perTaskPassCounts, + statusSortRank, +} from "@/lib/status"; import { displayedTurns, fmtTurnsCount, @@ -506,30 +510,12 @@ export function TaskGrid({ // Collapse replicates to one row per task: repeated runs share a taskId, so // the grid shows a single entry with a k/N ✓ badge; the per-run detail is - // selectable on the task page. The representative is chosen so its status, - // score, cost, duration AND detail link all describe the SAME run: prefer a - // passing replicate when any passed (else the lowest-index one), breaking - // ties by lowest replicateIndex for stability. Pick BEFORE sorting so a - // metric-sorted view still shows one row per task. - const collapsed = useMemo(() => { - const byTask = new Map(); - for (const t of tasks) { - const cur = byTask.get(t.taskId); - if (!cur) { - byTask.set(t.taskId, t); - continue; - } - const curPass = isPassStatus(cur.status); - const tPass = isPassStatus(t.status); - if (curPass !== tPass) { - // A passing replicate always wins over a non-passing one. - if (tPass) byTask.set(t.taskId, t); - } else if ((t.replicateIndex ?? 0) < (cur.replicateIndex ?? 0)) { - byTask.set(t.taskId, t); - } - } - return [...byTask.values()]; - }, [tasks]); + // selectable on the task page. The status pill and detail link come from a + // representative replicate (a passing one when any passed, else the + // lowest-index one), while the quantitative columns (score, duration, cost, + // turns, tokens) are averaged across all repeats — see collapseReplicates. + // Collapse BEFORE sorting so a metric-sorted view still shows one row per task. + const collapsed = useMemo(() => collapseReplicates(tasks), [tasks]); const sorted = useMemo(() => { const arr = [...collapsed]; diff --git a/evalboard/lib/__tests__/status.test.ts b/evalboard/lib/__tests__/status.test.ts new file mode 100644 index 00000000..c0534f33 --- /dev/null +++ b/evalboard/lib/__tests__/status.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, test } from "vitest"; +import { collapseReplicates } from "../status"; +import type { TaskResultSummary } from "../runs"; + +function row( + taskId: string, + extra: Partial = {}, +): TaskResultSummary { + return { + taskId, + replicateIndex: 0, + status: "SUCCESS", + weightedScore: 1.0, + durationSeconds: 1.0, + totalCostUsd: 0.1, + actualCommands: 1, + totalTurns: null, + expectedTurns: null, + hasFinalReply: false, + inputTokens: null, + outputTokens: null, + cacheCreationTokens: null, + cacheReadTokens: null, + model: null, + tags: [], + skill: null, + matureSkipped: false, + ...extra, + }; +} + +describe("collapseReplicates", () => { + test("averages the numeric columns across a task's replicates", () => { + const [collapsed] = collapseReplicates([ + row("t", { replicateIndex: 0, totalCostUsd: 0.1, durationSeconds: 10, weightedScore: 0.9, actualCommands: 4 }), + row("t", { replicateIndex: 1, totalCostUsd: 0.2, durationSeconds: 20, weightedScore: 0.6, actualCommands: 8 }), + row("t", { replicateIndex: 2, totalCostUsd: 0.3, durationSeconds: 30, weightedScore: 0.3, actualCommands: 6 }), + ]); + // Cost is the MEAN of the three repeats — not the first run's 0.1. + expect(collapsed.totalCostUsd).toBeCloseTo(0.2, 10); + expect(collapsed.durationSeconds).toBeCloseTo(20, 10); + expect(collapsed.weightedScore).toBeCloseTo(0.6, 10); + expect(collapsed.actualCommands).toBeCloseTo(6, 10); + }); + + test("keeps categorical fields from the representative (passing replicate wins, then lowest index)", () => { + const [collapsed] = collapseReplicates([ + row("t", { replicateIndex: 0, status: "FAILURE" }), + row("t", { replicateIndex: 1, status: "SUCCESS" }), + ]); + // Representative is the passing replicate (index 1): status + detail link. + expect(collapsed.status).toBe("SUCCESS"); + expect(collapsed.replicateIndex).toBe(1); + }); + + test("single replicate passes through unchanged (repeats disabled)", () => { + const only = row("t", { replicateIndex: 0, totalCostUsd: 0.42, actualCommands: 3 }); + const [collapsed] = collapseReplicates([only]); + expect(collapsed.totalCostUsd).toBe(0.42); + expect(collapsed.actualCommands).toBe(3); + expect(collapsed.status).toBe("SUCCESS"); + }); + + test("averages over non-null values; all-null stays null", () => { + const [collapsed] = collapseReplicates([ + row("t", { replicateIndex: 0, totalCostUsd: 0.1, outputTokens: null }), + row("t", { replicateIndex: 1, totalCostUsd: null, outputTokens: null }), + ]); + // 0.1 averaged over the single non-null value; all-null column → null. + expect(collapsed.totalCostUsd).toBeCloseTo(0.1, 10); + expect(collapsed.outputTokens).toBeNull(); + }); + + test("one row per task, preserving first-seen order", () => { + const out = collapseReplicates([ + row("b", { replicateIndex: 0 }), + row("a", { replicateIndex: 0 }), + row("b", { replicateIndex: 1 }), + ]); + expect(out.map((r) => r.taskId)).toEqual(["b", "a"]); + }); +}); diff --git a/evalboard/lib/__tests__/turns.test.ts b/evalboard/lib/__tests__/turns.test.ts index 017e4f28..5397d554 100644 --- a/evalboard/lib/__tests__/turns.test.ts +++ b/evalboard/lib/__tests__/turns.test.ts @@ -133,4 +133,10 @@ describe("fmtTurnsCount", () => { test("renders zero as 0 (not em dash)", () => { expect(fmtTurnsCount(0)).toBe("0"); }); + + test("caps a fractional average at 2 decimals, dropping trailing zeros", () => { + expect(fmtTurnsCount(15.3333333333)).toBe("15.33"); + expect(fmtTurnsCount(6.5)).toBe("6.5"); + expect(fmtTurnsCount(6.999)).toBe("7"); // rounds to a whole count + }); }); diff --git a/evalboard/lib/status.ts b/evalboard/lib/status.ts index efff8aff..89ae28b1 100644 --- a/evalboard/lib/status.ts +++ b/evalboard/lib/status.ts @@ -8,6 +8,8 @@ // (e.g. StatusPill) also handles flow execution statuses like "Completed" // and "Faulted" and uses its own logic. +import type { TaskResultSummary } from "./runs"; + export type StatusCategory = "passed" | "failed" | "error" | "unknown"; export function statusCategory(status: string | null): StatusCategory { @@ -37,6 +39,77 @@ export function perTaskPassCounts< return m; } +// Mean of the non-null values, or null when every value is null (so the cell +// renders "—" instead of a misleading 0). The averaging primitive behind the +// replicate collapse below. +function meanOrNull(values: readonly (number | null)[]): number | null { + let sum = 0; + let n = 0; + for (const v of values) { + if (v != null) { + sum += v; + n += 1; + } + } + return n ? sum / n : null; +} + +// Collapse per-replicate rows to one row per task for the run grid. Repeated +// runs of a task share a taskId, so this returns a single row per task whose: +// - categorical fields (status, replicateIndex/detail link, tags, skill, +// model, expected_turns, mature flag) come from a REPRESENTATIVE replicate — +// a passing one when any passed, else the lowest-index one — so the status +// pill and the "open detail" link both describe one real run; and +// - quantitative columns (score, duration, cost, turns via actualCommands, +// tokens) are the MEAN across ALL replicates, so the grid reflects the whole +// repeat set rather than just the representative run. (Previously every +// column was the representative's own value, so e.g. cost showed a single +// run's price instead of the average over the repeats.) +// First-seen task order is preserved. With repeats disabled (one replicate per +// task) each mean is that single value, so the output is byte-identical. +export function collapseReplicates( + rows: readonly TaskResultSummary[], +): TaskResultSummary[] { + const groups = new Map(); + for (const t of rows) { + const g = groups.get(t.taskId); + if (g) g.push(t); + else groups.set(t.taskId, [t]); + } + const out: TaskResultSummary[] = []; + for (const group of groups.values()) { + // Representative for the categorical fields: a passing replicate wins + // over a non-passing one; ties break to the lowest replicateIndex. + let rep = group[0]; + for (const t of group) { + const repPass = isPassStatus(rep.status); + const tPass = isPassStatus(t.status); + if (repPass !== tPass) { + if (tPass) rep = t; + } else if ((t.replicateIndex ?? 0) < (rep.replicateIndex ?? 0)) { + rep = t; + } + } + out.push({ + ...rep, + weightedScore: meanOrNull(group.map((t) => t.weightedScore)), + durationSeconds: meanOrNull(group.map((t) => t.durationSeconds)), + totalCostUsd: meanOrNull(group.map((t) => t.totalCostUsd)), + // Turns render from displayedTurns(actualCommands, hasFinalReply); + // averaging the command count carries the average into that column. + actualCommands: meanOrNull(group.map((t) => t.actualCommands)), + totalTurns: meanOrNull(group.map((t) => t.totalTurns)), + inputTokens: meanOrNull(group.map((t) => t.inputTokens)), + outputTokens: meanOrNull(group.map((t) => t.outputTokens)), + cacheCreationTokens: meanOrNull( + group.map((t) => t.cacheCreationTokens), + ), + cacheReadTokens: meanOrNull(group.map((t) => t.cacheReadTokens)), + }); + } + return out; +} + // Default table sort: failures and errors first, unknowns next, passes last. export function statusSortRank(status: string | null): number { const c = statusCategory(status); diff --git a/evalboard/lib/turns.ts b/evalboard/lib/turns.ts index 1eabd030..c4e831f7 100644 --- a/evalboard/lib/turns.ts +++ b/evalboard/lib/turns.ts @@ -73,7 +73,10 @@ export function turnsCellClasses(tint: TurnTint): string { } export function fmtTurnsCount(n: number | null): string { - return n == null ? "—" : `${n}`; + // Collapsed replicate rows carry an averaged (fractional) turn count, e.g. + // 15.333…; cap at 2 decimals and drop trailing zeros so whole counts still + // render as "7" (not "7.00") and a half stays "6.5". + return n == null ? "—" : `${Number(n.toFixed(2))}`; } // Fail a task's turn-budget check once its visible turns exceed the budget by From 54163033760d269e7c8a5b52586000d4ac0d6838 Mon Sep 17 00:00:00 2001 From: CarlesUIPath Date: Thu, 30 Jul 2026 14:27:38 +0100 Subject: [PATCH 2/5] fix(evalboard): surface per-model rows and the model name for multi-model runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard had no concept of an experiment *variant*, so A/B (multi-model) runs were mis-rendered on two surfaces: - Grid: each model produces a task_result sharing the task_id, so the replicate collapse (keyed on task_id alone) folded every model into one row with metrics averaged ACROSS models — the distinct models weren't shown at all. - Detail: the per-task content path was hardcoded to a `default/` subdir, which doesn't exist for a named variant (`kimi-k3/…`), so opening any multi-model task 404'd. Make the variant a first-class dimension end to end: - Data model: capture run.json `variant_id` as TaskResultSummary.variant (null on legacy rows → treated as "default"). - Paths: thread a sanitized `variant` (default "default") through taskContentBase, ensureTaskDir, readTaskDetail (now also matches the row on variant), readTaskReplicates, readLogTail, readConversationLog, collectTaskFiles, resolveSafePath, and the download API. - Grid: collapse + pass-counts key on (taskId, variant) via a new taskGroupKey, so each model keeps its own row and its own metrics. Add a Model column shown only when a run has >1 distinct model, and carry the variant on detail links (?v=). - Detail page: parse ?v=, thread it to every reader, preserve it on the replicate selector + download link, and show a model chip in the header so a task always names the LLM it ran on (single-model runs included). Single-config runs carry variant "default", so their grid, links and paths are byte-for-behavior unchanged (verified: no Model column, no ?v=, model chip still shown). Tests: variant grouping / taskGroupKey collision-safety / perTaskPassCounts, grid Model-column + ?v= link rendering, toTaskRow mapping. Co-Authored-By: Claude Opus 4.8 (1M context) --- evalboard/app/api/download/route.ts | 10 +- evalboard/app/runs/[id]/[...task]/page.tsx | 54 +++++++--- .../[id]/__tests__/run-view.render.test.tsx | 1 + .../app/runs/[id]/__tests__/run-view.test.ts | 1 + .../runs/[id]/__tests__/task-grid.test.tsx | 68 ++++++++++++ evalboard/app/runs/[id]/task-grid.tsx | 79 +++++++++++--- evalboard/lib/__tests__/runs.test.ts | 15 +++ evalboard/lib/__tests__/status.test.ts | 69 +++++++++++- evalboard/lib/blob.ts | 15 ++- evalboard/lib/runs.ts | 101 +++++++++++++----- evalboard/lib/status.ts | 52 ++++++--- 11 files changed, 389 insertions(+), 76 deletions(-) diff --git a/evalboard/app/api/download/route.ts b/evalboard/app/api/download/route.ts index 87b13e79..083631ee 100644 --- a/evalboard/app/api/download/route.ts +++ b/evalboard/app/api/download/route.ts @@ -6,20 +6,24 @@ import { createZip, type ZipEntry } from "@/lib/zip"; export const dynamic = "force-dynamic"; // Bundle a task folder, or a whole run, into a zip download. -// ?run=&task= → just that task's folder (default//) -// ?run= → the entire run folder (run.json + every task dir) +// ?run=&task=[&v=] → just that task's folder +// (//, variant default "default") +// ?run= → the entire run folder (run.json + every task dir) // minus the usual scaffolding noise. In blob mode the collect* helpers fetch // the needed blobs first, so this mirrors what the page would load. export async function GET(req: Request) { const url = new URL(req.url); const runId = url.searchParams.get("run"); const taskId = url.searchParams.get("task"); + // Which A/B variant's copy of the task to zip. Absent → "default" (the + // single-config subdir), so single-model download links are unchanged. + const variant = url.searchParams.get("v") ?? undefined; if (!runId) { return new NextResponse("missing run", { status: 400 }); } const files = taskId - ? await collectTaskFiles(runId, taskId) + ? await collectTaskFiles(runId, taskId, variant) : await collectRunFiles(runId); if (!files) { return new NextResponse("not found", { status: 404 }); diff --git a/evalboard/app/runs/[id]/[...task]/page.tsx b/evalboard/app/runs/[id]/[...task]/page.tsx index 4676d04d..c0db59b9 100644 --- a/evalboard/app/runs/[id]/[...task]/page.tsx +++ b/evalboard/app/runs/[id]/[...task]/page.tsx @@ -33,10 +33,10 @@ export default async function TaskPage({ searchParams, }: { params: Promise<{ id: string; task: string[] }>; - searchParams: Promise<{ r?: string }>; + searchParams: Promise<{ r?: string; v?: string }>; }) { const { id, task: taskSegments } = await params; - const { r } = await searchParams; + const { r, v } = await searchParams; const taskId = taskSegments.join("/"); // Replicate index from ?r=NN — repeated runs of one task share this task // path, so the query param is what selects which replicate's / dir to @@ -45,28 +45,41 @@ export default async function TaskPage({ const parsedR = Number(r); const replicate = r != null && Number.isInteger(parsedR) && parsedR >= 0 ? parsedR : 0; - const task = await readTaskDetail(id, taskId, replicate); + // Variant (model) from ?v=NAME — in a multi-model (A/B) run several variants + // share this task path, so ?v selects which model's / subdir to + // open. Absent → "default" (the single-config subdir); the readers sanitize + // and fall back to "default" for anything unsafe or unknown. + const variant = v ?? "default"; + const task = await readTaskDetail(id, taskId, replicate, variant); if (!task) notFound(); - // Replicate indices available for this task — drives the run selector below. - // [0] (or fewer) for a non-repeated task, so the selector self-hides. - const replicates = await readTaskReplicates(id, taskId); + // Replicate indices available for this task/variant — drives the run + // selector below. [0] (or fewer) for a non-repeated task, so it self-hides. + const replicates = await readTaskReplicates(id, taskId, variant); - // variant is always "default" here; the replicate selects the / dir. + // The replicate selects the / dir within the variant's subtree. // readTaskReview returns null for older runs that predate the review feature. const review = await readTaskReview( id, - "default", + variant, taskId, replicateDirName(replicate), ); - const log = await readLogTail(id, taskId, replicate); + const log = await readLogTail(id, taskId, replicate, variant); const conversation = parseConversation( - await readConversationLog(id, taskId, replicate), + await readConversationLog(id, taskId, replicate, variant), ); const { flowDebug } = task; + // Preserve ?v= on in-page links (replicate selector, download) so switching + // replicates or downloading stays on the SAME model. "default" is the + // implicit fallback, so it's omitted to keep single-config URLs clean. + const variantParam = + variant && variant !== "default" + ? `&v=${encodeURIComponent(variant)}` + : ""; + return (
+ {showModel && ( + + {t.model ?? "—"} + + )} {t.matureSkipped ? ( @@ -819,7 +854,7 @@ export function TaskGrid({ ); return (
@@ -829,11 +864,13 @@ export function TaskGrid({ className="min-w-0 break-words font-semibold text-gray-900 hover:text-studio-blue" matureSourceRuns={matureSourceRuns} replicateCount={ - replicateCounts.get(t.taskId) ?? 1 + replicateCounts.get(taskGroupKey(t)) ?? 1 + } + replicatePassCount={ + replicatePassCounts.get( + taskGroupKey(t), + ) ?? 0 } - replicatePassCount={ - replicatePassCounts.get(t.taskId) ?? 0 - } /> {t.matureSkipped ? ( @@ -843,6 +880,14 @@ export function TaskGrid({ )}
+ {showModel && t.model && ( +
+ {t.model} +
+ )} { const row = toTaskRow({ task_id: "x", expected_turns: null }); expect(row.expectedTurns).toBeNull(); }); + + test("maps variant_id and model_used (multi-model row)", () => { + const row = toTaskRow({ + task_id: "x", + variant_id: "kimi-k3", + model_used: "moonshotai/kimi-k3", + }); + expect(row.variant).toBe("kimi-k3"); + expect(row.model).toBe("moonshotai/kimi-k3"); + }); + + test("legacy row without variant_id yields null variant", () => { + const row = toTaskRow({ task_id: "x" }); + expect(row.variant).toBeNull(); + }); }); describe("aggregateSubAgentUsage", () => { diff --git a/evalboard/lib/__tests__/status.test.ts b/evalboard/lib/__tests__/status.test.ts index c0534f33..9239a24c 100644 --- a/evalboard/lib/__tests__/status.test.ts +++ b/evalboard/lib/__tests__/status.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "vitest"; -import { collapseReplicates } from "../status"; +import { collapseReplicates, perTaskPassCounts, taskGroupKey } from "../status"; import type { TaskResultSummary } from "../runs"; function row( @@ -22,6 +22,7 @@ function row( cacheCreationTokens: null, cacheReadTokens: null, model: null, + variant: null, tags: [], skill: null, matureSkipped: false, @@ -79,4 +80,70 @@ describe("collapseReplicates", () => { ]); expect(out.map((r) => r.taskId)).toEqual(["b", "a"]); }); + + test("keeps a row PER MODEL — variants sharing a taskId are not merged", () => { + // A multi-model (A/B) run: three variants ran the SAME task. They share + // the taskId but are distinct models, so they must stay three rows with + // their OWN metrics — never averaged into one. + const out = collapseReplicates([ + row("t", { variant: "kimi-k3", model: "moonshotai/kimi-k3", totalCostUsd: 0.1 }), + row("t", { variant: "glm-5-2", model: "z-ai/glm-5.2", totalCostUsd: 0.2 }), + row("t", { variant: "deepseek-v4-pro", model: "deepseek/deepseek-v4-pro", totalCostUsd: 0.3 }), + ]); + expect(out).toHaveLength(3); + expect(out.map((r) => r.variant)).toEqual([ + "kimi-k3", + "glm-5-2", + "deepseek-v4-pro", + ]); + // Each row keeps its own cost — no cross-model averaging. + expect(out.map((r) => r.totalCostUsd)).toEqual([0.1, 0.2, 0.3]); + }); + + test("still averages replicates WITHIN one variant of a multi-model run", () => { + const out = collapseReplicates([ + row("t", { variant: "a", replicateIndex: 0, totalCostUsd: 0.1 }), + row("t", { variant: "a", replicateIndex: 1, totalCostUsd: 0.3 }), + row("t", { variant: "b", replicateIndex: 0, totalCostUsd: 1.0 }), + ]); + expect(out).toHaveLength(2); + const a = out.find((r) => r.variant === "a")!; + const b = out.find((r) => r.variant === "b")!; + expect(a.totalCostUsd).toBeCloseTo(0.2, 10); // mean of a's two replicates + expect(b.totalCostUsd).toBeCloseTo(1.0, 10); + }); +}); + +describe("taskGroupKey", () => { + test("a null variant collapses to the same key as an explicit 'default'", () => { + expect(taskGroupKey({ taskId: "t", variant: null })).toBe( + taskGroupKey({ taskId: "t", variant: "default" }), + ); + }); + + test("different variants of the same task yield different keys", () => { + expect(taskGroupKey({ taskId: "t", variant: "kimi-k3" })).not.toBe( + taskGroupKey({ taskId: "t", variant: "glm-5-2" }), + ); + }); + + test("the separator cannot be forged from task-id/variant collisions", () => { + // "ab"+"c" must not equal "a"+"bc" — the control-char separator can't + // appear in an id, so no two distinct (task, variant) pairs collide. + expect(taskGroupKey({ taskId: "ab", variant: "c" })).not.toBe( + taskGroupKey({ taskId: "a", variant: "bc" }), + ); + }); +}); + +describe("perTaskPassCounts", () => { + test("counts each variant of a shared task separately", () => { + const m = perTaskPassCounts([ + row("t", { variant: "a", status: "SUCCESS" }), + row("t", { variant: "b", status: "FAILURE" }), + ]); + expect(m.size).toBe(2); + expect(m.get(taskGroupKey({ taskId: "t", variant: "a" }))).toBe(1); + expect(m.get(taskGroupKey({ taskId: "t", variant: "b" }))).toBe(0); + }); }); diff --git a/evalboard/lib/blob.ts b/evalboard/lib/blob.ts index da5a1e5e..e4c656eb 100644 --- a/evalboard/lib/blob.ts +++ b/evalboard/lib/blob.ts @@ -270,11 +270,16 @@ export async function ensureTaskDir( runId: string, taskId: string, destRoot: string, + // Experiment variant subdir. "default" for single-config runs; a model name + // (e.g. "kimi-k3") in A/B runs. Validated as a path segment so it can't + // escape the run prefix. + variant = "default", ): Promise { assertValidId(runId, "runId"); assertValidTaskId(taskId, "taskId"); + assertValidId(variant, "variant"); if (LOCAL_RUNS_DIR) return; - return dedupe(`task:${runId}/${taskId}`, async () => { + return dedupe(`task:${runId}/${variant}/${taskId}`, async () => { // Activation cases live in the nested sub-run (/activation/...), // so their row + per-case dir come from there; skills tasks from the // top-level run. Fetch the matching run.json for the row lookup. @@ -284,13 +289,13 @@ export async function ensureTaskDir( const c = await getContainer(); const ops: Promise[] = []; // `listBlobsFlat` recurses, so both the flat legacy layout - // (`default//task.json`) and the nested replicate layout - // (`default//00/task.json`) download unchanged — the prefix + // (`//task.json`) and the nested replicate layout + // (`//00/task.json`) download unchanged — the prefix // scope is the task subtree either way. `resolveTaskContentDir` in // runs.ts then picks the right shape at render time. const prefix = activation - ? `${runId}/activation/default/${taskId}/` - : `${runId}/default/${taskId}/`; + ? `${runId}/activation/${variant}/${taskId}/` + : `${runId}/${variant}/${taskId}/`; for await (const blob of c.listBlobsFlat({ prefix })) { // Agent sandboxes that run Python leave a `.venv/` tree (hundreds // of files, tens of MB) under the task dir. No UI page reads it, diff --git a/evalboard/lib/runs.ts b/evalboard/lib/runs.ts index c1d3f824..b5cd76e2 100644 --- a/evalboard/lib/runs.ts +++ b/evalboard/lib/runs.ts @@ -87,6 +87,13 @@ export interface TaskResultSummary { // Model the task ran on (run.json `model_used`). Used to price token // buckets as USD for the Tokens↔USD column toggle. Null on legacy runs. model: string | null; + // Experiment variant this row belongs to (run.json `variant_id`, e.g. + // "default" for a single-config run, or a model name like "kimi-k3" in an + // A/B run). Variant rows share a taskId but live in distinct on-disk subdirs + // (///), so this is what tells sibling models apart in the + // grid and selects the right content dir on the detail page. Null on legacy + // runs that predate the field — treated as "default" when building paths. + variant: string | null; tags: string[]; // Derived primary group. See deriveSkill below for the resolution chain // (new runs use task_path; older runs fall back to a tag heuristic). @@ -352,6 +359,11 @@ interface RawTaskResult { // Model the task ran on (e.g. "claude-sonnet-4-6"). Used to price token // buckets as USD. Absent on legacy runs. model_used?: string | null; + // Experiment variant that produced this row. In an A/B (multi-model) run + // each variant contributes a row sharing the task_id but differing here and + // in model_used, and its artifacts live under ///. + // Absent → single-config run whose content dir is the literal "default". + variant_id?: string | null; // Per-task agent config; `type` is the harness (coder-eval AgentKind, e.g. // "claude-code" | "codex" | "antigravity"). Used to derive the run's harness // when the run-level RunConfig stamp is absent (direct coder-eval / legacy runs). @@ -585,13 +597,31 @@ function isActivationTaskId(taskId: string): boolean { return taskId.startsWith("skill-activation/"); } +// The on-disk subdir a single-config run writes its tasks under. Multi-model +// (A/B) runs replace this with the variant id (e.g. "kimi-k3"); the fallback +// keeps legacy / single-config runs — whose rows carry no variant — resolving. +const DEFAULT_VARIANT = "default"; + +// Sanitize a variant into a safe single path segment. A variant id is reflected +// into a filesystem path and a blob prefix, so anything that isn't a plain id +// (null on legacy rows, or a would-be traversal) collapses to "default". +function variantSegment(variant: string | null | undefined): string { + return variant && isValidId(variant) ? variant : DEFAULT_VARIANT; +} + // Filesystem base for a task's content (before the optional `00` replicate dir): -// activation cases under /activation/default/, skills tasks under -// /default/. -function taskContentBase(runId: string, taskId: string): string { +// activation cases under /activation//, skills tasks under +// //. `variant` defaults to "default" — the subdir a +// single-config run uses and the safe fallback for legacy rows. +function taskContentBase( + runId: string, + taskId: string, + variant: string | null = DEFAULT_VARIANT, +): string { + const v = variantSegment(variant); return isActivationTaskId(taskId) - ? path.join(RUNS_DIR, runId, "activation", "default", taskId) - : path.join(RUNS_DIR, runId, "default", taskId); + ? path.join(RUNS_DIR, runId, "activation", v, taskId) + : path.join(RUNS_DIR, runId, v, taskId); } // Resolve the skill (primary grouping axis) for a task. Two-stage fallback: @@ -637,6 +667,7 @@ export function toTaskRow(t: RawTaskResult): TaskResultSummary { cacheCreationTokens: t.cache_creation_input_tokens ?? null, cacheReadTokens: t.cache_read_input_tokens ?? null, model: t.model_used ?? null, + variant: t.variant_id ?? null, tags, skill: deriveSkill(t.task_path, tags), matureSkipped: t.mature_skipped ?? false, @@ -1776,12 +1807,18 @@ async function resolveTaskContentDir( export async function readTaskReplicates( runId: string, taskId: string, + variant: string | null = DEFAULT_VARIANT, ): Promise { + const v = variantSegment(variant); const data = isActivationTaskId(taskId) ? await readActivationRunJson(runId) : await readRunJson(runId); + // Scope to the selected variant so the run selector on a multi-model task + // lists only THIS model's replicates, not every variant's. const indices = (data?.task_results ?? []) - .filter((t) => t.task_id === taskId) + .filter( + (t) => t.task_id === taskId && variantSegment(t.variant_id) === v, + ) .map((t) => t.replicate_index ?? 0); return [...new Set(indices)].sort((a, b) => a - b); } @@ -1790,8 +1827,10 @@ export async function readTaskDetail( runId: string, taskId: string, replicate = 0, + variant: string | null = DEFAULT_VARIANT, ): Promise { - await ensureTaskDir(runId, taskId, RUNS_DIR); + const v = variantSegment(variant); + await ensureTaskDir(runId, taskId, RUNS_DIR, v); // Activation cases live in the nested activation sub-run; skills tasks in the // top-level run. Read the row from whichever run.json owns this task so the @@ -1799,11 +1838,16 @@ export async function readTaskDetail( const data = isActivationTaskId(taskId) ? await readActivationRunJson(runId) : await readRunJson(runId); - // Repeated runs share a task_id, so match on (task_id, replicate_index). - // Legacy rows carry no replicate_index (null) → treated as replicate 0, so - // an old single-result run still resolves at replicate 0. + // Repeated runs share a task_id, so match on (task_id, replicate_index). In + // a multi-model run several variants ALSO share the task_id at replicate 0, + // so filter on the variant too (rows carrying a variant_id) — otherwise + // every model would resolve to the first variant's row. Legacy rows carry + // neither field (null variant / null replicate_index) → treated as + // ("default", 0), so an old single-result run still resolves. const matches = (data?.task_results ?? []).filter( - (t) => t.task_id === taskId, + (t) => + t.task_id === taskId && + variantSegment(t.variant_id) === v, ); const rawTask = matches.find((t) => (t.replicate_index ?? 0) === replicate) ?? @@ -1811,7 +1855,7 @@ export async function readTaskDetail( if (!rawTask) return null; const row = toTaskRow(rawTask); - const taskDir = taskContentBase(runId, taskId); + const taskDir = taskContentBase(runId, taskId, v); const contentDir = await resolveTaskContentDir(taskDir, replicate); const task = await readJson<{ final_status?: string; @@ -2046,10 +2090,12 @@ export async function readLogTail( runId: string, taskId: string, replicate = 0, + variant: string | null = DEFAULT_VARIANT, maxBytes = 200_000, ): Promise { - await ensureTaskDir(runId, taskId, RUNS_DIR); - const taskDir = taskContentBase(runId, taskId); + const v = variantSegment(variant); + await ensureTaskDir(runId, taskId, RUNS_DIR, v); + const taskDir = taskContentBase(runId, taskId, v); const contentDir = await resolveTaskContentDir(taskDir, replicate); const logPath = path.join(contentDir, "task.log"); const raw = await fs.readFile(logPath, "utf-8").catch(() => ""); @@ -2071,10 +2117,12 @@ export async function readConversationLog( runId: string, taskId: string, replicate = 0, + variant: string | null = DEFAULT_VARIANT, maxBytes = 200_000, ): Promise { - await ensureTaskDir(runId, taskId, RUNS_DIR); - const taskDir = taskContentBase(runId, taskId); + const v = variantSegment(variant); + await ensureTaskDir(runId, taskId, RUNS_DIR, v); + const taskDir = taskContentBase(runId, taskId, v); const contentDir = await resolveTaskContentDir(taskDir, replicate); const logPath = path.join(contentDir, "conversation.log"); const raw = await fs.readFile(logPath, "utf-8").catch(() => ""); @@ -2122,10 +2170,12 @@ export function parseConversation(raw: string): ConversationTurn[] { export async function collectTaskFiles( runId: string, taskId: string, + variant: string | null = DEFAULT_VARIANT, ): Promise<{ relPath: string; abs: string }[] | null> { if (!isValidId(runId) || !isValidTaskId(taskId)) return null; - await ensureTaskDir(runId, taskId, RUNS_DIR); - const taskDir = taskContentBase(runId, taskId); + const v = variantSegment(variant); + await ensureTaskDir(runId, taskId, RUNS_DIR, v); + const taskDir = taskContentBase(runId, taskId, v); const refs = await walkArtifacts(taskDir); if (refs.length === 0) return null; return refs.map((r) => ({ relPath: r.relPath, abs: path.join(taskDir, r.relPath) })); @@ -2151,13 +2201,16 @@ export async function resolveSafePath( relPath: string, ): Promise { if (!isValidId(runId)) return null; - // Artifact URLs embed the task subdir in relPath - // (`default//artifacts/...`) — extract it so the narrow fetch - // hits the right blobs without pulling the whole run. + // Artifact URLs embed the task subdir in relPath as + // `//artifacts/...` — the variant is "default" for a + // single-config run and a model name (e.g. "kimi-k3") in an A/B run. + // Extract both so the narrow fetch hits the right blobs without pulling the + // whole run. Anything that isn't that two-segment task shape (run-level + // files, or the nested activation layout) falls back to the run summary. const parts = relPath.split("/"); - if (parts[0] === "default" && parts[1]) { - if (!isValidId(parts[1])) return null; - await ensureTaskDir(runId, parts[1], RUNS_DIR); + if (parts.length >= 2 && parts[0] !== "activation" && parts[1]) { + if (!isValidId(parts[0]) || !isValidId(parts[1])) return null; + await ensureTaskDir(runId, parts[1], RUNS_DIR, parts[0]); } else { await ensureRunSummary(runId, RUNS_DIR); } diff --git a/evalboard/lib/status.ts b/evalboard/lib/status.ts index 89ae28b1..e984410e 100644 --- a/evalboard/lib/status.ts +++ b/evalboard/lib/status.ts @@ -25,16 +25,36 @@ export function isPassStatus(status: string | null): boolean { return statusCategory(status) === "passed"; } -// Roll per-replicate rows up per task: taskId -> number of replicates that -// passed. Repeated runs share a taskId, so this is the one place the "any +// Separator used to join a task id and its variant into one map key. A control +// character (unit separator) that can never appear in a task id or variant id, +// so the two segments can never collide. +const KEY_SEP = "\u001f"; + +// Grouping key for the "one row per task" collapse. A task's repeated runs +// (replicates) share it, so they fold together; but in a multi-model (A/B) run +// several variants ALSO share a taskId — they must stay DISTINCT rows, one per +// model — so the variant is part of the key. Single-config runs carry variant +// "default" (or null on legacy rows), so the key is effectively the taskId and +// behavior is unchanged. +export function taskGroupKey(t: { + taskId: string; + variant?: string | null; +}): string { + return `${t.taskId}${KEY_SEP}${t.variant ?? "default"}`; +} + +// Roll per-replicate rows up per (task, variant): key -> number of replicates +// that passed. Repeated runs share a key, so this is the one place the "any // replicate passed" aggregation lives — consumed by the run-page pass-rate -// tile AND the grid badge / collapse so they can never disagree. +// tile AND the grid badge / collapse so they can never disagree. Keyed by +// taskGroupKey so a multi-model run counts each model's attempt separately. export function perTaskPassCounts< - T extends { taskId: string; status: string | null }, + T extends { taskId: string; status: string | null; variant?: string | null }, >(rows: readonly T[]): Map { const m = new Map(); for (const r of rows) { - m.set(r.taskId, (m.get(r.taskId) ?? 0) + (isPassStatus(r.status) ? 1 : 0)); + const k = taskGroupKey(r); + m.set(k, (m.get(k) ?? 0) + (isPassStatus(r.status) ? 1 : 0)); } return m; } @@ -54,27 +74,31 @@ function meanOrNull(values: readonly (number | null)[]): number | null { return n ? sum / n : null; } -// Collapse per-replicate rows to one row per task for the run grid. Repeated -// runs of a task share a taskId, so this returns a single row per task whose: +// Collapse per-replicate rows to one row per (task, variant) for the run grid. +// Repeated runs of a task share a taskId, so they fold together; a multi-model +// run keeps one row PER MODEL (the variant is part of the group key), so each +// model's metrics stay separate instead of being averaged across models. Each +// collapsed row's: // - categorical fields (status, replicateIndex/detail link, tags, skill, -// model, expected_turns, mature flag) come from a REPRESENTATIVE replicate — -// a passing one when any passed, else the lowest-index one — so the status -// pill and the "open detail" link both describe one real run; and +// model, variant, expected_turns, mature flag) come from a REPRESENTATIVE +// replicate — a passing one when any passed, else the lowest-index one — so +// the status pill and the "open detail" link both describe one real run; and // - quantitative columns (score, duration, cost, turns via actualCommands, // tokens) are the MEAN across ALL replicates, so the grid reflects the whole // repeat set rather than just the representative run. (Previously every // column was the representative's own value, so e.g. cost showed a single // run's price instead of the average over the repeats.) -// First-seen task order is preserved. With repeats disabled (one replicate per -// task) each mean is that single value, so the output is byte-identical. +// First-seen group order is preserved. With repeats disabled (one replicate per +// task/variant) each mean is that single value, so the output is byte-identical. export function collapseReplicates( rows: readonly TaskResultSummary[], ): TaskResultSummary[] { const groups = new Map(); for (const t of rows) { - const g = groups.get(t.taskId); + const key = taskGroupKey(t); + const g = groups.get(key); if (g) g.push(t); - else groups.set(t.taskId, [t]); + else groups.set(key, [t]); } const out: TaskResultSummary[] = []; for (const group of groups.values()) { From b5a4f02d3add296833fd3ff799806350b1a3c2aa Mon Sep 17 00:00:00 2001 From: CarlesUIPath Date: Thu, 30 Jul 2026 14:39:05 +0100 Subject: [PATCH 3/5] test(evalboard): cover variant path resolution; harden resolveSafePath traversal Add lib/__tests__/variant-paths.test.ts exercising the variant-aware readers against a temp local runs dir (the collect.test.ts env-stub + fresh-import harness): readTaskDetail picks the right model's row (not the first variant), returns null with no ?v on a run that has no "default" subdir, and sanitizes an unsafe ?v to "default"; readTaskReplicates / readLogTail / collectTaskFiles are scoped to the selected variant; resolveSafePath resolves a non-default variant artifact and rejects traversal. That last test caught a regression: the broadened resolveSafePath handed a "../.." segment to ensureTaskDir, which THROWS (isValidId admits dots), so the /api/file route would 500 instead of 403. Restrict the narrow-fetch prefetch to genuinely safe segments (reject "."/".."); traversal now falls through to the realpath containment check and returns null as before. Co-Authored-By: Claude Opus 4.8 (1M context) --- evalboard/lib/__tests__/variant-paths.test.ts | 151 ++++++++++++++++++ evalboard/lib/runs.ts | 13 +- 2 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 evalboard/lib/__tests__/variant-paths.test.ts diff --git a/evalboard/lib/__tests__/variant-paths.test.ts b/evalboard/lib/__tests__/variant-paths.test.ts new file mode 100644 index 00000000..984356cf --- /dev/null +++ b/evalboard/lib/__tests__/variant-paths.test.ts @@ -0,0 +1,151 @@ +import { promises as fs } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +// The variant-aware readers (readTaskDetail / readTaskReplicates / readLogTail / +// collectTaskFiles / resolveSafePath) all resolve a task's content under +// ////. RUNS_DIR is baked from EVALBOARD_LOCAL_RUNS_DIR +// at *import* time, so — like collect.test.ts — each test stubs the env to a +// throwaway runs dir and dynamically imports a fresh module copy so RUNS_DIR +// picks it up. LOCAL mode makes ensureTaskDir a no-op, so these exercise the +// pure on-disk path resolution deterministically. +let tmp: string; + +async function write(rel: string, body: string): Promise { + const abs = path.join(tmp, rel); + await fs.mkdir(path.dirname(abs), { recursive: true }); + await fs.writeFile(abs, body); +} + +async function loadRuns() { + vi.resetModules(); + vi.stubEnv("EVALBOARD_LOCAL_RUNS_DIR", tmp); + return import("../runs"); +} + +const RUN = "2026-01-01_00-00-00"; +const TASK = "demo-task"; + +// A multi-model (A/B) run: two variants ran the SAME task. They share the +// task_id at replicate 0 and differ only by variant_id / model_used — exactly +// the shape that used to collapse into one row and 404 on the detail page. +beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "evalboard-variant-")); + await write( + `${RUN}/run.json`, + JSON.stringify({ + run_id: "x", + task_results: [ + { + task_id: TASK, + variant_id: "kimi-k3", + model_used: "moonshotai/kimi-k3", + replicate_index: 0, + status: "SUCCESS", + }, + { + task_id: TASK, + variant_id: "glm-5-2", + model_used: "z-ai/glm-5.2", + replicate_index: 0, + status: "FAILURE", + }, + // A second replicate of ONE variant, to prove replicate listing + // is scoped to the selected variant. + { + task_id: TASK, + variant_id: "kimi-k3", + model_used: "moonshotai/kimi-k3", + replicate_index: 1, + status: "SUCCESS", + }, + ], + }), + ); + await write(`${RUN}/kimi-k3/${TASK}/00/task.json`, "{}"); + await write(`${RUN}/kimi-k3/${TASK}/00/task.log`, "kimi log"); + await write(`${RUN}/kimi-k3/${TASK}/01/task.json`, "{}"); + await write(`${RUN}/glm-5-2/${TASK}/00/task.json`, "{}"); + await write(`${RUN}/glm-5-2/${TASK}/00/task.log`, "glm log"); + await write(`${RUN}/glm-5-2/${TASK}/00/artifacts/out.txt`, "glm artifact"); +}); + +afterEach(async () => { + vi.unstubAllEnvs(); + await fs.rm(tmp, { recursive: true, force: true }); +}); + +describe("readTaskDetail — variant selects the model's own row + content", () => { + test("?v selects the matching model (not just the first variant)", async () => { + const { readTaskDetail } = await loadRuns(); + const kimi = await readTaskDetail(RUN, TASK, 0, "kimi-k3"); + const glm = await readTaskDetail(RUN, TASK, 0, "glm-5-2"); + expect(kimi?.model).toBe("moonshotai/kimi-k3"); + expect(kimi?.variant).toBe("kimi-k3"); + expect(kimi?.status).toBe("SUCCESS"); + // The SECOND variant resolves to its OWN row — before the fix every + // variant collapsed onto the first one at replicate 0. + expect(glm?.model).toBe("z-ai/glm-5.2"); + expect(glm?.status).toBe("FAILURE"); + }); + + test("a run with no 'default' variant returns null when ?v is omitted", async () => { + // Mirrors the live behavior: the grid only ever links multi-model rows + // with ?v=, and there is no default/ subdir to fall back to. + const { readTaskDetail } = await loadRuns(); + expect(await readTaskDetail(RUN, TASK, 0)).toBeNull(); + }); + + test("an unsafe variant is sanitized to 'default' (no path escape)", async () => { + const { readTaskDetail } = await loadRuns(); + // "../glm-5-2" is not a valid id → falls back to "default", which has + // no row here → null. It must NOT traverse into the glm-5-2 subtree. + expect(await readTaskDetail(RUN, TASK, 0, "../glm-5-2")).toBeNull(); + }); +}); + +describe("readTaskReplicates — scoped to the selected variant", () => { + test("lists only the chosen model's replicates", async () => { + const { readTaskReplicates } = await loadRuns(); + expect(await readTaskReplicates(RUN, TASK, "kimi-k3")).toEqual([0, 1]); + expect(await readTaskReplicates(RUN, TASK, "glm-5-2")).toEqual([0]); + }); +}); + +describe("readLogTail — reads the variant's log", () => { + test("each variant's task.log is read from its own subdir", async () => { + const { readLogTail } = await loadRuns(); + expect(await readLogTail(RUN, TASK, 0, "kimi-k3")).toBe("kimi log"); + expect(await readLogTail(RUN, TASK, 0, "glm-5-2")).toBe("glm log"); + }); +}); + +describe("collectTaskFiles — zips the variant's folder", () => { + test("collects files under the requested variant only", async () => { + const { collectTaskFiles } = await loadRuns(); + const files = await collectTaskFiles(RUN, TASK, "glm-5-2"); + const rels = files?.map((f) => f.relPath).sort(); + expect(rels).toEqual(["00/artifacts/out.txt", "00/task.json", "00/task.log"]); + for (const f of files ?? []) { + expect(f.abs).toContain(path.join("glm-5-2", TASK)); + } + }); +}); + +describe("resolveSafePath — variant-prefixed artifact URLs", () => { + test("resolves a non-default variant artifact path", async () => { + const { resolveSafePath } = await loadRuns(); + const abs = await resolveSafePath( + RUN, + `glm-5-2/${TASK}/00/artifacts/out.txt`, + ); + expect(abs).not.toBeNull(); + expect(abs).toContain(path.join(RUN, "glm-5-2", TASK)); + }); + + test("still rejects traversal outside the run dir", async () => { + const { resolveSafePath } = await loadRuns(); + expect(await resolveSafePath(RUN, "../../etc/passwd")).toBeNull(); + }); +}); diff --git a/evalboard/lib/runs.ts b/evalboard/lib/runs.ts index b5cd76e2..c7be66ec 100644 --- a/evalboard/lib/runs.ts +++ b/evalboard/lib/runs.ts @@ -2205,11 +2205,16 @@ export async function resolveSafePath( // `//artifacts/...` — the variant is "default" for a // single-config run and a model name (e.g. "kimi-k3") in an A/B run. // Extract both so the narrow fetch hits the right blobs without pulling the - // whole run. Anything that isn't that two-segment task shape (run-level - // files, or the nested activation layout) falls back to the run summary. + // whole run. This is only a prefetch optimization; the realpath containment + // check below is the actual security boundary, so an input that isn't that + // clean two-segment task shape (run-level files, the nested activation + // layout, OR any traversal like "../..") just falls back to the run summary + // and lets the containment check reject it — we must never hand a "."/".." + // segment to ensureTaskDir, which throws on it (isValidId admits dots). const parts = relPath.split("/"); - if (parts.length >= 2 && parts[0] !== "activation" && parts[1]) { - if (!isValidId(parts[0]) || !isValidId(parts[1])) return null; + const safeSeg = (s: string | undefined): s is string => + !!s && isValidId(s) && s !== "." && s !== ".."; + if (parts[0] !== "activation" && safeSeg(parts[0]) && safeSeg(parts[1])) { await ensureTaskDir(runId, parts[1], RUNS_DIR, parts[0]); } else { await ensureRunSummary(runId, RUNS_DIR); From dc0d308cad33f72993609a88b9c0f56cc273a7b1 Mon Sep 17 00:00:00 2001 From: CarlesUIPath Date: Mon, 3 Aug 2026 11:47:35 +0100 Subject: [PATCH 4/5] =?UTF-8?q?fix(evalboard):=20resolve=20PR=20#67=20revi?= =?UTF-8?q?ew=20=E2=80=94=20traversal,=20404,=20coherence,=20arm=20labelin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the blockers and non-blocking findings from the code review: Security (Axis 2/4/5 — path traversal): - isValidId now rejects "."/".." (blob.ts) — ID_RE `/^[\w.-]+$/` matched them, so variantSegment("..") returned ".." and `?v=..` escaped RUNS_DIR via /api/download. One guard fixes variantSegment, assertValidId, collectTaskFiles' runId check and clearRunCacheDir at once. - readTaskReview now runs variantSegment() on its variant (it was the one path reader taking the raw ?v=). Added a realpath containment guard in collectTaskFiles as defense in depth. Single sanitizer seam (Axis 5) + array param (Axis 2): - New leaf lib/variant.ts owns DEFAULT_VARIANT + the pure URL helpers (firstParam/variantFromParam/variantLinkParam); variantSegment is exported from runs.ts as the one path sanitizer. status.ts/blob.ts/task-grid.tsx/ page.tsx all import the single "default" const. firstParam collapses a repeated ?v=a&v=b (Next yields string[]) so it can't reach path.join as an array (500). searchParams typed string|string[]. 404 regression (Axis 7): - readTaskDetail reads run.json first, then resolveVariant() picks the arm when ?v= is absent (real "default" row → sole arm → first row) and returns it on task.variant; the page reuses it for every reader/link. Bare deep links to any run whose arm isn't literally "default" resolve instead of hard-404ing. Score/status coherence (Axis 8): - collapseReplicates keeps weightedScore on the representative (Status + Score + detail link now describe the same run); only cost/duration/turns/tokens are averaged, matching the PR's documented scope. Arm labeling (Axis 8): - The grid arm column gates on distinct VARIANT (not model), so a same-model A/B (skill on/off) shows a "Variant" column labeling each arm; header is "Model" only when models differ. Other: fmtCompact rounds averaged sub-1k means (no "500.666…"); reviews indexed by (task, variant) so each arm shows its own review; README download-route + layout contract updated. Tests: legacy/default fixture, all-9-column averaging + score-representative, download-route ?v wiring, same-model Variant column, dot-segment traversal negatives (bare "..", runId "..", absolute path). Co-Authored-By: Claude Opus 4.8 (1M context) --- evalboard/README.md | 10 +- .../app/api/download/__tests__/route.test.ts | 73 ++++++++++++ .../app/api/refresh/__tests__/route.test.ts | 7 +- evalboard/app/runs/[id]/[...task]/page.tsx | 32 +++--- .../runs/[id]/__tests__/task-grid.test.tsx | 37 ++++++- evalboard/app/runs/[id]/run-view.tsx | 6 +- evalboard/app/runs/[id]/task-grid.tsx | 77 +++++++++---- evalboard/lib/__tests__/status.test.ts | 34 +++++- evalboard/lib/__tests__/variant-paths.test.ts | 84 ++++++++++++-- evalboard/lib/blob.ts | 24 +++- evalboard/lib/format.ts | 5 +- evalboard/lib/reviews.ts | 24 +++- evalboard/lib/runs.ts | 104 ++++++++++++------ evalboard/lib/status.ts | 39 ++++--- evalboard/lib/variant.ts | 47 ++++++++ 15 files changed, 485 insertions(+), 118 deletions(-) create mode 100644 evalboard/app/api/download/__tests__/route.test.ts create mode 100644 evalboard/lib/variant.ts diff --git a/evalboard/README.md b/evalboard/README.md index 3596d6ff..1ff33795 100644 --- a/evalboard/README.md +++ b/evalboard/README.md @@ -36,14 +36,18 @@ show up in the index — empty shells and the `latest` symlink are filtered out. `` is the same string the eval framework writes to `task_results[].task_id` (e.g., `skill-flow-calculator`) and equals the -subdir name under `/default/`. +subdir name under `//`, where `` is the +experiment arm — `default` for a single-config run, or the arm name (e.g. +`opus`, `with-skill`) in an A/B run. The task page selects the arm via `?v=` +(mirroring `?r=` for replicates); a bare URL resolves the run's actual arm. ## Conventions - `/api/file?run=&path=` serves `.flow`, `.uipx`, etc. with path-traversal guard (`resolveSafePath`). -- `/api/download?run=[&task=]` streams a zip of a task folder (with - `task`) or the whole run (without). Files are gathered by `collectTaskFiles` +- `/api/download?run=[&task=][&v=]` streams a zip of a task + folder (with `task`; `v` selects the arm, default `default`) or the whole run + (without `task`). Files are gathered by `collectTaskFiles` / `collectRunFiles`, which reuse the `walkArtifacts` noise filter, and zipped by `lib/zip.ts` (a dependency-free DEFLATE writer). - Pass rows render green (`bg-green-50 text-green-700`), failures render red diff --git a/evalboard/app/api/download/__tests__/route.test.ts b/evalboard/app/api/download/__tests__/route.test.ts new file mode 100644 index 00000000..f61fe7b7 --- /dev/null +++ b/evalboard/app/api/download/__tests__/route.test.ts @@ -0,0 +1,73 @@ +import { promises as fs } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +// collectTaskFiles reads RUNS_DIR, resolved from EVALBOARD_LOCAL_RUNS_DIR at +// import time — so stub the env to a throwaway runs dir and import a fresh +// module copy (like collect.test.ts / the refresh route test). +let tmp: string; + +async function write(rel: string, body: string): Promise { + const abs = path.join(tmp, rel); + await fs.mkdir(path.dirname(abs), { recursive: true }); + await fs.writeFile(abs, body); +} + +async function loadGet() { + vi.resetModules(); + vi.stubEnv("EVALBOARD_LOCAL_RUNS_DIR", tmp); + return (await import("../route")).GET; +} + +function get(qs: string): Request { + return new Request(`http://test/api/download?${qs}`, { method: "GET" }); +} + +const RUN = "2026-01-01_00-00-00"; + +beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), "evalboard-download-")); + // A/B task: only under the glm-5-2 arm (no default/ subtree). + await write(`${RUN}/glm-5-2/ab-task/00/task.json`, "{}"); + await write(`${RUN}/glm-5-2/ab-task/00/artifacts/out.txt`, "glm out"); + // Single-config task: under default/. + await write(`${RUN}/default/solo-task/00/task.json`, "{}"); +}); + +afterEach(async () => { + vi.unstubAllEnvs(); + await fs.rm(tmp, { recursive: true, force: true }); +}); + +describe("GET /api/download — variant (?v=) wiring", () => { + test("zips the requested arm's subtree", async () => { + const GET = await loadGet(); + const res = await GET(get(`run=${RUN}&task=ab-task&v=glm-5-2`)); + expect(res.status).toBe(200); + expect(res.headers.get("Content-Type")).toBe("application/zip"); + // Non-empty archive → the arm's files were found. (Dropping the variant + // arg in the route would look in default/ab-task, which doesn't exist, + // and 404 — this assertion kills that mutation.) + expect(Number(res.headers.get("Content-Length"))).toBeGreaterThan(0); + }); + + test("a single-config task downloads with no ?v (default arm)", async () => { + const GET = await loadGet(); + const res = await GET(get(`run=${RUN}&task=solo-task`)); + expect(res.status).toBe(200); + expect(res.headers.get("Content-Type")).toBe("application/zip"); + }); + + test("an unknown arm 404s rather than zipping the wrong subtree", async () => { + const GET = await loadGet(); + const res = await GET(get(`run=${RUN}&task=ab-task&v=nope`)); + expect(res.status).toBe(404); + }); + + test("missing run -> 400", async () => { + const GET = await loadGet(); + const res = await GET(get(`task=ab-task`)); + expect(res.status).toBe(400); + }); +}); diff --git a/evalboard/app/api/refresh/__tests__/route.test.ts b/evalboard/app/api/refresh/__tests__/route.test.ts index cb06eb4d..c3bbf587 100644 --- a/evalboard/app/api/refresh/__tests__/route.test.ts +++ b/evalboard/app/api/refresh/__tests__/route.test.ts @@ -87,9 +87,10 @@ describe("POST /api/refresh", () => { }); test('traversal id ".." -> 400, cache root untouched', async () => { - // ".." passes isValidId (dots are word-ish) but clearRunCacheDir's - // strict-child check rejects it, so the route returns 400 and the - // cache root is never the rm target. + // isValidId now rejects "." / ".." outright, so clearRunCacheDir + // returns false and the route 400s — and even if that guard were + // loosened, its strict-child check still refuses to rm the cache + // root. Belt and suspenders; the cache root is never the rm target. const marker = path.join(cache, "keep.txt"); await fs.writeFile(marker, "x"); diff --git a/evalboard/app/runs/[id]/[...task]/page.tsx b/evalboard/app/runs/[id]/[...task]/page.tsx index c0db59b9..cbf028b5 100644 --- a/evalboard/app/runs/[id]/[...task]/page.tsx +++ b/evalboard/app/runs/[id]/[...task]/page.tsx @@ -9,6 +9,7 @@ import { replicateDirName, } from "@/lib/runs"; import { readTaskReview } from "@/lib/reviews"; +import { DEFAULT_VARIANT, firstParam, variantLinkParam } from "@/lib/variant"; import { fmtCompact, fmtRunTime, humanizeTaskId } from "@/lib/format"; import { StatusPill } from "@/lib/pills"; import { ChipButton } from "../chips"; @@ -33,7 +34,10 @@ export default async function TaskPage({ searchParams, }: { params: Promise<{ id: string; task: string[] }>; - searchParams: Promise<{ r?: string; v?: string }>; + // Next yields `string | string[]` for a repeated query key; declare it + // honestly and normalize with firstParam so `?v=a&v=b` can't reach a reader + // as an array (which would throw a 500 at path.join). + searchParams: Promise<{ r?: string | string[]; v?: string | string[] }>; }) { const { id, task: taskSegments } = await params; const { r, v } = await searchParams; @@ -42,16 +46,17 @@ export default async function TaskPage({ // path, so the query param is what selects which replicate's / dir to // open. Absent / non-numeric / negative → replicate 0 (the single result a // non-repeated or legacy run has). - const parsedR = Number(r); + const parsedR = Number(firstParam(r)); const replicate = - r != null && Number.isInteger(parsedR) && parsedR >= 0 ? parsedR : 0; - // Variant (model) from ?v=NAME — in a multi-model (A/B) run several variants - // share this task path, so ?v selects which model's / subdir to - // open. Absent → "default" (the single-config subdir); the readers sanitize - // and fall back to "default" for anything unsafe or unknown. - const variant = v ?? "default"; - const task = await readTaskDetail(id, taskId, replicate, variant); + Number.isInteger(parsedR) && parsedR >= 0 ? parsedR : 0; + // Variant (arm) from ?v=NAME — in an A/B run several variants share this task + // path, so ?v selects which arm's / subdir to open. When ABSENT, + // readTaskDetail resolves the run's actual arm (single-arm tasks just work; + // this is what keeps pre-existing ?v-less deep links from 404-ing). The + // resolved arm comes back on task.variant and drives every other reader. + const task = await readTaskDetail(id, taskId, replicate, firstParam(v)); if (!task) notFound(); + const variant = task.variant ?? DEFAULT_VARIANT; // Replicate indices available for this task/variant — drives the run // selector below. [0] (or fewer) for a non-repeated task, so it self-hides. @@ -73,12 +78,9 @@ export default async function TaskPage({ const { flowDebug } = task; // Preserve ?v= on in-page links (replicate selector, download) so switching - // replicates or downloading stays on the SAME model. "default" is the - // implicit fallback, so it's omitted to keep single-config URLs clean. - const variantParam = - variant && variant !== "default" - ? `&v=${encodeURIComponent(variant)}` - : ""; + // replicates or downloading stays on the SAME arm. "default" is the implicit + // fallback, so it's omitted to keep single-config URLs clean. + const variantParam = variantLinkParam(variant); return (
diff --git a/evalboard/app/runs/[id]/__tests__/task-grid.test.tsx b/evalboard/app/runs/[id]/__tests__/task-grid.test.tsx index f8807d70..f8dc05b7 100644 --- a/evalboard/app/runs/[id]/__tests__/task-grid.test.tsx +++ b/evalboard/app/runs/[id]/__tests__/task-grid.test.tsx @@ -449,7 +449,7 @@ describe("TaskGrid — multi-model (A/B) runs", () => { expect(hrefs).toEqual(["/runs/r1/t?v=glm-5-2", "/runs/r1/t?v=kimi-k3"]); }); - test("hides the Model column for a single-model run", () => { + test("hides the arm column for a single-arm run", () => { render( { expect( within(table).queryByRole("columnheader", { name: /Model/i }), ).toBeNull(); + expect( + within(table).queryByRole("columnheader", { name: /Variant/i }), + ).toBeNull(); + }); + + test("same-model A/B: shows a Variant column labeling each arm", () => { + // Skill on/off (or terse/detailed): same model, different variant. Rows + // split on variant, so they must be distinguishable — gating on distinct + // MODEL would render two identical unlabeled rows (the reported gap). + render( + , + ); + const table = screen.getByRole("table"); + // Header reads "Variant" (models don't differ), not "Model". + expect( + within(table).getByRole("columnheader", { name: /Variant/i }), + ).toBeInTheDocument(); + // Both arms are labeled and distinct. + expect(within(table).getByText("bare")).toBeInTheDocument(); + expect(within(table).getByText("with-skill")).toBeInTheDocument(); + // Each arm links to its own ?v=. + const hrefs = within(table) + .getAllByRole("link", { name: /^t/i }) + .map((l) => l.getAttribute("href")) + .sort(); + expect(hrefs).toEqual(["/runs/r1/t?v=bare", "/runs/r1/t?v=with-skill"]); }); }); diff --git a/evalboard/app/runs/[id]/run-view.tsx b/evalboard/app/runs/[id]/run-view.tsx index 160e1303..7c57717b 100644 --- a/evalboard/app/runs/[id]/run-view.tsx +++ b/evalboard/app/runs/[id]/run-view.tsx @@ -5,7 +5,7 @@ import { useCallback, useMemo, useState } from "react"; import type { ActivationScore, TaskResultSummary } from "@/lib/runs"; import type { ReviewIndexEntry } from "@/lib/reviews-types"; import { fmtDuration, humanizeTaskId } from "@/lib/format"; -import { perTaskPassCounts, statusCategory } from "@/lib/status"; +import { perTaskPassCounts, statusCategory, taskGroupKey } from "@/lib/status"; import { ChipLegend } from "@/app/_overview/tag-rail"; import { CollapsibleRail } from "@/app/_components/collapsible-rail"; import { ActivationCard } from "./activation-card"; @@ -285,7 +285,7 @@ export function RunView({ } if (selectedReviewTags.length > 0) { arr = arr.filter((t) => { - const rtags = reviewsByTask?.get(t.taskId)?.tags ?? []; + const rtags = reviewsByTask?.get(taskGroupKey(t))?.tags ?? []; return selectedReviewTags.every((tag) => rtags.includes(tag)); }); } @@ -300,7 +300,7 @@ export function RunView({ return true; if (t.tags.some((tag) => tag.toLowerCase().includes(qLower))) return true; - const rtags = reviewsByTask?.get(t.taskId)?.tags ?? []; + const rtags = reviewsByTask?.get(taskGroupKey(t))?.tags ?? []; if (rtags.some((tag) => tag.toLowerCase().includes(qLower))) return true; return false; diff --git a/evalboard/app/runs/[id]/task-grid.tsx b/evalboard/app/runs/[id]/task-grid.tsx index 619f6159..8702ee32 100644 --- a/evalboard/app/runs/[id]/task-grid.tsx +++ b/evalboard/app/runs/[id]/task-grid.tsx @@ -27,6 +27,7 @@ import { turnRatio, turnsCellClasses, } from "@/lib/turns"; +import { DEFAULT_VARIANT } from "@/lib/variant"; import { ChipButton } from "./chips"; import { TableScroll } from "@/app/_components/scroll-table"; import { @@ -220,7 +221,7 @@ function TaskIdCell({ // variant's detail. "default" is the implicit fallback, so it's omitted. const params = new URLSearchParams(); if (replicateCount > 1) params.set("r", String(t.replicateIndex ?? 0)); - if (t.variant && t.variant !== "default") params.set("v", t.variant); + if (t.variant && t.variant !== DEFAULT_VARIANT) params.set("v", t.variant); const qs = params.toString(); const href = `/runs/${runId}/${t.taskId}${qs ? `?${qs}` : ""}`; return ( @@ -295,7 +296,11 @@ function compare( case "task": return a.taskId.localeCompare(b.taskId); case "model": - return (a.model ?? "").localeCompare(b.model ?? ""); + // The arm column sorts by whichever label it shows (model when + // models differ, else the variant/arm id). + return (a.model ?? a.variant ?? "").localeCompare( + b.model ?? b.variant ?? "", + ); case "status": return statusSortRank(a.status) - statusSortRank(b.status); case "score": @@ -510,8 +515,10 @@ export function TaskGrid({ // separately rather than lumping all variants' rows together. const replicateCounts = useMemo(() => { const m = new Map(); - for (const t of tasks) - m.set(taskGroupKey(t), (m.get(taskGroupKey(t)) ?? 0) + 1); + for (const t of tasks) { + const k = taskGroupKey(t); + m.set(k, (m.get(k) ?? 0) + 1); + } return m; }, [tasks]); @@ -530,13 +537,27 @@ export function TaskGrid({ // Collapse BEFORE sorting so a metric-sorted view still shows one row per task. const collapsed = useMemo(() => collapseReplicates(tasks), [tasks]); - // Show the Model column only for multi-model (A/B) runs — a single-config - // run has one model across every row, so the column would be pure noise. - // The per-task detail page always names the model regardless (see page.tsx). - const showModel = useMemo( - () => new Set(collapsed.map((t) => t.model).filter(Boolean)).size > 1, - [collapsed], - ); + // Show the arm column only when a run actually has multiple ARMS — rows split + // on (taskId, variant), so gate on distinct VARIANT, not distinct model: a + // same-model A/B (skill on/off, terse/detailed) varies only the variant, and + // gating on model would render N identical unlabeled rows per task. Also show + // it if models differ (a genuine multi-model run). A single-config run has + // one arm across every row, so the column stays hidden (pure noise). When + // shown, prefer the model label if models differ (the informative axis), else + // the variant id (the arm name). The detail page always names the model too. + const { showArm, armByModel } = useMemo(() => { + const models = new Set(collapsed.map((t) => t.model).filter(Boolean)); + const variants = new Set( + collapsed.map((t) => t.variant ?? DEFAULT_VARIANT), + ); + return { + showArm: models.size > 1 || variants.size > 1, + armByModel: models.size > 1, + }; + }, [collapsed]); + // The label shown in the arm column / mobile card for one row. + const armLabel = (t: TaskResultSummary): string => + (armByModel ? t.model ?? t.variant : t.variant) ?? DEFAULT_VARIANT; const sorted = useMemo(() => { const arr = [...collapsed]; @@ -568,8 +589,12 @@ export function TaskGrid({ const visibleColumns = COLUMNS.filter( (c) => (showTokens || !TOKEN_KEYS.has(c.key)) && - (showModel || c.key !== "model"), + (showArm || c.key !== "model"), ); + // The arm column's header reads "Model" when models distinguish the arms, + // else "Variant" (same-model A/B). The SortKey stays "model". + const columnHeader = (col: (typeof COLUMNS)[number]): string => + col.key === "model" ? (armByModel ? "Model" : "Variant") : col.header; return (
@@ -636,7 +661,7 @@ export function TaskGrid({ onClick={() => onSort(col.key)} className="inline-flex items-center gap-1 hover:text-gray-900" > - {col.header} + {columnHeader(col)} {arrow} @@ -648,7 +673,7 @@ export function TaskGrid({ >
- {showModel && ( + {showArm && ( - {t.model ?? "—"} + {armLabel(t)} )} @@ -845,7 +874,7 @@ export function TaskGrid({ table's columns do. */}
{sorted.map((t) => { - const review = reviewsByTask?.get(t.taskId); + const review = reviewsByTask?.get(taskGroupKey(t)); const turnsTint = tintForRatio( turnRatio( displayedTurns(t.actualCommands, t.hasFinalReply), @@ -880,12 +909,16 @@ export function TaskGrid({ )}
- {showModel && t.model && ( + {showArm && (
- {t.model} + {armLabel(t)}
)} { - test("averages the numeric columns across a task's replicates", () => { + test("averages ALL resource columns across a task's replicates", () => { + // Distinct values per replicate so a mean is distinguishable from the + // representative's value on every averaged column (guards the whole set, + // per the review's mutation-survival finding). All SUCCESS → the + // representative is replicateIndex 0. const [collapsed] = collapseReplicates([ - row("t", { replicateIndex: 0, totalCostUsd: 0.1, durationSeconds: 10, weightedScore: 0.9, actualCommands: 4 }), - row("t", { replicateIndex: 1, totalCostUsd: 0.2, durationSeconds: 20, weightedScore: 0.6, actualCommands: 8 }), - row("t", { replicateIndex: 2, totalCostUsd: 0.3, durationSeconds: 30, weightedScore: 0.3, actualCommands: 6 }), + row("t", { replicateIndex: 0, totalCostUsd: 0.1, durationSeconds: 10, actualCommands: 4, totalTurns: 2, inputTokens: 100, outputTokens: 10, cacheCreationTokens: 1000, cacheReadTokens: 5000 }), + row("t", { replicateIndex: 1, totalCostUsd: 0.2, durationSeconds: 20, actualCommands: 8, totalTurns: 4, inputTokens: 200, outputTokens: 20, cacheCreationTokens: 2000, cacheReadTokens: 7000 }), + row("t", { replicateIndex: 2, totalCostUsd: 0.3, durationSeconds: 30, actualCommands: 6, totalTurns: 6, inputTokens: 300, outputTokens: 30, cacheCreationTokens: 3000, cacheReadTokens: 9000 }), ]); - // Cost is the MEAN of the three repeats — not the first run's 0.1. + // Every resource column is the MEAN of the three repeats — not the first + // run's value. expect(collapsed.totalCostUsd).toBeCloseTo(0.2, 10); expect(collapsed.durationSeconds).toBeCloseTo(20, 10); - expect(collapsed.weightedScore).toBeCloseTo(0.6, 10); expect(collapsed.actualCommands).toBeCloseTo(6, 10); + expect(collapsed.totalTurns).toBeCloseTo(4, 10); + expect(collapsed.inputTokens).toBeCloseTo(200, 10); + expect(collapsed.outputTokens).toBeCloseTo(20, 10); + expect(collapsed.cacheCreationTokens).toBeCloseTo(2000, 10); + expect(collapsed.cacheReadTokens).toBeCloseTo(7000, 10); + }); + + test("weightedScore stays on the representative, NOT averaged (score/status coherence)", () => { + // SUCCESS(1.0) + FAILURE(0.2): the representative is the passing row, so + // the row's Score must be that run's 1.0 — never the 0.6 mean, which would + // read "Passed · 0.60" and then show 1.00 on click-through. + const [collapsed] = collapseReplicates([ + row("t", { replicateIndex: 0, status: "FAILURE", weightedScore: 0.2 }), + row("t", { replicateIndex: 1, status: "SUCCESS", weightedScore: 1.0 }), + ]); + expect(collapsed.status).toBe("SUCCESS"); + expect(collapsed.replicateIndex).toBe(1); + expect(collapsed.weightedScore).toBe(1.0); }); test("keeps categorical fields from the representative (passing replicate wins, then lowest index)", () => { diff --git a/evalboard/lib/__tests__/variant-paths.test.ts b/evalboard/lib/__tests__/variant-paths.test.ts index 984356cf..8976ccfc 100644 --- a/evalboard/lib/__tests__/variant-paths.test.ts +++ b/evalboard/lib/__tests__/variant-paths.test.ts @@ -90,18 +90,60 @@ describe("readTaskDetail — variant selects the model's own row + content", () expect(glm?.status).toBe("FAILURE"); }); - test("a run with no 'default' variant returns null when ?v is omitted", async () => { - // Mirrors the live behavior: the grid only ever links multi-model rows - // with ?v=, and there is no default/ subdir to fall back to. + test("a bare URL (no ?v) resolves the run's actual arm instead of 404ing", async () => { + // The 404-regression fix: with no ?v and no "default" arm, readTaskDetail + // resolves to the run's first arm (kimi-k3 here) and renders it, rather + // than matching the literal "default" (zero rows → notFound). This is what + // keeps pre-existing ?v-less deep links / bookmarks working. const { readTaskDetail } = await loadRuns(); - expect(await readTaskDetail(RUN, TASK, 0)).toBeNull(); + const task = await readTaskDetail(RUN, TASK, 0); + expect(task).not.toBeNull(); + expect(task?.variant).toBe("kimi-k3"); }); - test("an unsafe variant is sanitized to 'default' (no path escape)", async () => { + test("an unsafe explicit variant is sanitized to 'default' (no path escape)", async () => { const { readTaskDetail } = await loadRuns(); - // "../glm-5-2" is not a valid id → falls back to "default", which has - // no row here → null. It must NOT traverse into the glm-5-2 subtree. + // "../glm-5-2" is not a valid id → sanitized to "default", which has no + // row here → null. It must NOT traverse into the glm-5-2 subtree. expect(await readTaskDetail(RUN, TASK, 0, "../glm-5-2")).toBeNull(); + // A bare ".." is now rejected by isValidId too (was the traversal hole). + expect(await readTaskDetail(RUN, TASK, 0, "..")).toBeNull(); + }); +}); + +describe("legacy / single-config layout (no variant_id, /default/)", () => { + const LRUN = "2026-02-02_00-00-00"; + const LTASK = "legacy-task"; + + async function loadLegacy() { + // A run whose rows omit variant_id entirely, content under default/ — + // the pre-variant on-disk shape the compat claim depends on. + await write( + `${LRUN}/run.json`, + JSON.stringify({ + run_id: "x", + task_results: [ + { task_id: LTASK, replicate_index: 0, status: "SUCCESS" }, + ], + }), + ); + await write(`${LRUN}/default/${LTASK}/00/task.json`, "{}"); + await write(`${LRUN}/default/${LTASK}/00/task.log`, "legacy log"); + return loadRuns(); + } + + test("readTaskDetail with no ?v resolves the default/ row + content", async () => { + const { readTaskDetail } = await loadLegacy(); + const task = await readTaskDetail(LRUN, LTASK, 0); + expect(task).not.toBeNull(); + expect(task?.variant).toBeNull(); // legacy row carries no variant_id + expect(task?.status).toBe("SUCCESS"); + }); + + test("readTaskReplicates / readLogTail resolve the default/ subdir", async () => { + const { readTaskReplicates, readLogTail } = await loadLegacy(); + expect(await readTaskReplicates(LRUN, LTASK)).toEqual([0]); + expect(await readLogTail(LRUN, LTASK, 0)).toBe("legacy log"); }); }); @@ -144,8 +186,34 @@ describe("resolveSafePath — variant-prefixed artifact URLs", () => { expect(abs).toContain(path.join(RUN, "glm-5-2", TASK)); }); - test("still rejects traversal outside the run dir", async () => { + test("rejects traversal outside the run dir (relative + absolute)", async () => { const { resolveSafePath } = await loadRuns(); expect(await resolveSafePath(RUN, "../../etc/passwd")).toBeNull(); + expect(await resolveSafePath(RUN, "/etc/passwd")).toBeNull(); + // dot-only variant segment: isValidId now rejects "..", so the prefetch + // falls through and the containment check nulls it. + expect(await resolveSafePath(RUN, `../${TASK}/00/task.json`)).toBeNull(); + }); +}); + +describe("dot-segment traversal is closed at the id guard", () => { + test("collectTaskFiles rejects a '..' runId / variant (no exfil via download)", async () => { + // Before the fix, isValidId admitted ".." so collectTaskFiles("..", …, "..") + // enumerated a sibling dir. Now every dot segment is rejected. + const { collectTaskFiles } = await loadRuns(); + // runId ".." → rejected outright. + expect(await collectTaskFiles("..", "secret", "..")).toBeNull(); + // variant ".." → sanitized to "default" (a nonexistent subtree here), so + // it resolves to nothing rather than escaping into "../". + expect(await collectTaskFiles(RUN, TASK, "..")).toBeNull(); + }); + + test("isValidId rejects '.' and '..' but accepts real ids", async () => { + const { isValidId } = await import("../blob"); + expect(isValidId("..")).toBe(false); + expect(isValidId(".")).toBe(false); + expect(isValidId("kimi-k3")).toBe(true); + expect(isValidId("default")).toBe(true); + expect(isValidId("gpt-5.6")).toBe(true); // dots inside a real id are fine }); }); diff --git a/evalboard/lib/blob.ts b/evalboard/lib/blob.ts index e4c656eb..4d50e42a 100644 --- a/evalboard/lib/blob.ts +++ b/evalboard/lib/blob.ts @@ -2,6 +2,7 @@ import { promises as fs } from "node:fs"; import path from "node:path"; import { randomBytes } from "node:crypto"; import type { ContainerClient } from "@azure/storage-blob"; +import { DEFAULT_VARIANT } from "./variant"; const ACCOUNT = "coderevaltests"; const CONTAINER = "runs"; @@ -13,19 +14,30 @@ const CONTAINER = "runs"; export const LOCAL_RUNS_DIR = process.env.EVALBOARD_LOCAL_RUNS_DIR || null; // Run / task IDs get reflected into filesystem paths and blob prefixes, so -// reject anything outside a narrow whitelist before any side effect. +// reject anything outside a narrow whitelist before any side effect. `.` and +// `..` match ID_RE (dots are word-ish) but are traversal segments, so they are +// rejected explicitly — a single guard that protects every id reflected into a +// path (run id, variant segment, task-id segment, blob prefix). Historically +// only isValidTaskId excluded them; isValidId did not, which let a variant of +// ".." escape the run dir via taskContentBase / collectTaskFiles. const ID_RE = /^[\w.-]+$/; // Task IDs from dataset-expanded tasks look like "sentiment-classification/r3" // (suite/row). Each segment must still pass the narrow ID_RE — only the slash // separator between segments is additionally allowed. const TASK_ID_RE = /^[\w.-]+(\/[\w.-]+)*$/; +// A path segment that is only dots ("." / "..") is a traversal, never a real id. +function isDotSegment(s: string): boolean { + return s === "." || s === ".."; +} + export function isValidId(id: unknown): id is string { return ( typeof id === "string" && id.length > 0 && id.length < 128 && - ID_RE.test(id) + ID_RE.test(id) && + !isDotSegment(id) ); } @@ -35,7 +47,7 @@ export function isValidTaskId(id: unknown): id is string { id.length > 0 && id.length < 256 && TASK_ID_RE.test(id) && - !id.split("/").some((s) => s === "." || s === "..") + !id.split("/").some(isDotSegment) ); } @@ -271,9 +283,9 @@ export async function ensureTaskDir( taskId: string, destRoot: string, // Experiment variant subdir. "default" for single-config runs; a model name - // (e.g. "kimi-k3") in A/B runs. Validated as a path segment so it can't - // escape the run prefix. - variant = "default", + // (e.g. "kimi-k3") in A/B runs. Validated as a path segment (assertValidId + // rejects "."/".." now) so it can't escape the run prefix. + variant: string = DEFAULT_VARIANT, ): Promise { assertValidId(runId, "runId"); assertValidTaskId(taskId, "taskId"); diff --git a/evalboard/lib/format.ts b/evalboard/lib/format.ts index 833c5a96..bd7da59c 100644 --- a/evalboard/lib/format.ts +++ b/evalboard/lib/format.ts @@ -37,7 +37,10 @@ export function fmtCompact(n: number | null | undefined): string { const abs = Math.abs(n); if (abs >= 1_000_000) return (n / 1_000_000).toFixed(1).replace(/\.0$/, "") + "M"; if (abs >= 1_000) return (n / 1_000).toFixed(1).replace(/\.0$/, "") + "k"; - return String(n); + // Cap sub-1k values at 2 decimals (dropping trailing zeros) — averaged + // token/command means across replicates are fractional (e.g. 500.666…), and + // the ≥1k branches already round via toFixed(1). Integers render unchanged. + return String(Number(n.toFixed(2))); } // USD with enough precision to read sub-cent differences as the thinking diff --git a/evalboard/lib/reviews.ts b/evalboard/lib/reviews.ts index be45119e..15d467d5 100644 --- a/evalboard/lib/reviews.ts +++ b/evalboard/lib/reviews.ts @@ -1,7 +1,8 @@ import { promises as fs } from "node:fs"; import path from "node:path"; import { ensureRunReviewIndex } from "./blob"; -import { RUNS_DIR, listRunIds } from "./runs"; +import { RUNS_DIR, listRunIds, variantSegment } from "./runs"; +import { taskGroupKey } from "./status"; import type { Review, ReviewIndex, @@ -46,10 +47,14 @@ export async function readTaskReview( taskId: string, replicate: string, ): Promise { + // Sanitize the variant here too — this is a path-consuming reader like the + // others, so it must not trust its caller (page.tsx forwards the run's arm, + // but a raw ?v= must never reach path.join). variantSegment collapses "."/ + // ".." / any non-id to "default". const p = path.join( RUNS_DIR, runId, - variantId, + variantSegment(variantId), taskId, replicate, "review.json", @@ -79,14 +84,23 @@ export function tagCountsForRun( .sort((a, b) => b.count - a.count || a.tag.localeCompare(b.tag)); } -// Per-task review entries indexed by task_id (collapsed across variant/replicate -// for the run grid — first occurrence wins for stable rendering). +// Per-arm review entries keyed by (task_id, variant) via taskGroupKey — the SAME +// key the grid rows collapse on — so in an A/B run each arm shows its OWN review +// instead of both arms inheriting the first arm's summary/tags. Collapsed across +// replicate only (first occurrence per arm wins for stable rendering). Look rows +// up with taskGroupKey(row), not row.taskId. export type EntriesByTask = Map; export function indexByTask(index: ReviewIndex): EntriesByTask { const out: EntriesByTask = new Map(); for (const e of index.reviews) { - if (!out.has(e.task_id)) out.set(e.task_id, e); + // Empty variant_id (legacy indexes) → null so it collapses to "default", + // matching a row whose variant is null/"default". + const key = taskGroupKey({ + taskId: e.task_id, + variant: e.variant_id || null, + }); + if (!out.has(key)) out.set(key, e); } return out; } diff --git a/evalboard/lib/runs.ts b/evalboard/lib/runs.ts index 81866238..149ccb01 100644 --- a/evalboard/lib/runs.ts +++ b/evalboard/lib/runs.ts @@ -14,6 +14,7 @@ import { } from "./blob"; import { DELIVERABLE_KINDS, DELIVERABLE_NAMES } from "./artifact-kinds"; import { messageCostUsd } from "./pricing"; +import { DEFAULT_VARIANT } from "./variant"; // Resolution order: // 1. EVALBOARD_LOCAL_RUNS_DIR — local mode, points at a coder_eval runs dir @@ -597,18 +598,35 @@ function isActivationTaskId(taskId: string): boolean { return taskId.startsWith("skill-activation/"); } -// The on-disk subdir a single-config run writes its tasks under. Multi-model -// (A/B) runs replace this with the variant id (e.g. "kimi-k3"); the fallback -// keeps legacy / single-config runs — whose rows carry no variant — resolving. -const DEFAULT_VARIANT = "default"; - -// Sanitize a variant into a safe single path segment. A variant id is reflected -// into a filesystem path and a blob prefix, so anything that isn't a plain id -// (null on legacy rows, or a would-be traversal) collapses to "default". -function variantSegment(variant: string | null | undefined): string { +// Sanitize a variant into a safe single path segment — THE seam that turns a +// variant (from run.json, or a raw ?v= query value) into something safe to +// splice into a filesystem path or blob prefix. isValidId rejects "."/".." and +// any non-id, so a traversal segment (or null on legacy rows) collapses to +// "default". Exported so every path consumer — including readTaskReview in +// lib/reviews.ts — routes through this one guard rather than re-implementing it. +export function variantSegment(variant: string | null | undefined): string { return variant && isValidId(variant) ? variant : DEFAULT_VARIANT; } +// Pick the variant to render for a task page when ?v= is absent. Explicit +// requests are sanitized and honored verbatim (a non-matching one 404s +// downstream); a bare URL resolves to the run's actual arm so pre-existing +// deep links / bookmarks / cross-page task links don't hard-404 on any run +// whose variant isn't literally "default": prefer a real "default" row, else +// the sole arm, else the first row's arm. `rows` are this run's task_results +// already filtered to the target task_id. +function resolveVariant( + rows: readonly RawTaskResult[], + requested: string | null | undefined, +): string { + if (requested != null && requested !== "") return variantSegment(requested); + if (rows.length === 0) return DEFAULT_VARIANT; + const variants = rows.map((t) => variantSegment(t.variant_id)); + if (variants.includes(DEFAULT_VARIANT)) return DEFAULT_VARIANT; + const uniq = [...new Set(variants)]; + return uniq.length === 1 ? uniq[0] : variants[0]; +} + // Filesystem base for a task's content (before the optional `00` replicate dir): // activation cases under /activation//, skills tasks under // //. `variant` defaults to "default" — the subdir a @@ -1845,28 +1863,31 @@ export async function readTaskDetail( runId: string, taskId: string, replicate = 0, - variant: string | null = DEFAULT_VARIANT, + // Explicit ?v= variant, or null/undefined for a bare URL → resolve the run's + // actual arm (see resolveVariant) so pre-PR deep links don't 404. + variant?: string | null, ): Promise { - const v = variantSegment(variant); - await ensureTaskDir(runId, taskId, RUNS_DIR, v); - + // Read run.json FIRST (readRunJson/readActivationRunJson fetch it in blob + // mode) so we can resolve the variant before prefetching its task subtree — + // a bare URL doesn't know which arm to fetch until it has seen the rows. // Activation cases live in the nested activation sub-run; skills tasks in the - // top-level run. Read the row from whichever run.json owns this task so the - // trace (linked from the activation page) still resolves. + // top-level run — read whichever run.json owns this task so the trace + // (linked from the activation page) still resolves. const data = isActivationTaskId(taskId) ? await readActivationRunJson(runId) : await readRunJson(runId); + const rows = (data?.task_results ?? []).filter((t) => t.task_id === taskId); + // Resolve the arm to show (explicit ?v=, else the run's real arm), then + // prefetch just that variant's subtree. + const v = resolveVariant(rows, variant); + await ensureTaskDir(runId, taskId, RUNS_DIR, v); + // Repeated runs share a task_id, so match on (task_id, replicate_index). In // a multi-model run several variants ALSO share the task_id at replicate 0, - // so filter on the variant too (rows carrying a variant_id) — otherwise - // every model would resolve to the first variant's row. Legacy rows carry - // neither field (null variant / null replicate_index) → treated as - // ("default", 0), so an old single-result run still resolves. - const matches = (data?.task_results ?? []).filter( - (t) => - t.task_id === taskId && - variantSegment(t.variant_id) === v, - ); + // so filter on the resolved variant too — otherwise every arm would resolve + // to the first row. Legacy rows carry no variant_id (→ "default"), so an old + // single-result run still resolves. + const matches = rows.filter((t) => variantSegment(t.variant_id) === v); const rawTask = matches.find((t) => (t.replicate_index ?? 0) === replicate) ?? (replicate === 0 ? matches[0] : undefined); @@ -2185,6 +2206,21 @@ export function parseConversation(raw: string): ConversationTurn[] { // symlink skip that drive the Artifacts list also shape the zip — plus // task.json / task.log at the task root, which aren't excluded by any pattern. // Returns null for an invalid id or a missing/empty task dir. +// True iff `target` resolves to the run's own directory or a descendant of it. +// Canonicalizes both sides so a symlink under the run that points outside is +// caught. A non-existent target (nothing to leak yet) is treated as contained — +// walkArtifacts will simply find nothing. Mirrors resolveSafePath's boundary. +async function isWithinRunDir(runId: string, target: string): Promise { + if (!isValidId(runId)) return false; + const baseReal = await fs + .realpath(path.join(RUNS_DIR, runId)) + .catch(() => null); + if (!baseReal) return false; + const targetReal = await fs.realpath(target).catch(() => null); + if (targetReal == null) return true; // doesn't exist → nothing to enumerate + return targetReal === baseReal || targetReal.startsWith(baseReal + path.sep); +} + export async function collectTaskFiles( runId: string, taskId: string, @@ -2194,6 +2230,11 @@ export async function collectTaskFiles( const v = variantSegment(variant); await ensureTaskDir(runId, taskId, RUNS_DIR, v); const taskDir = taskContentBase(runId, taskId, v); + // Defense in depth: runId/taskId/variant are all validated above (isValidId / + // isValidTaskId reject "."/".."), so taskDir can't escape — but confirm it + // resolves under the run dir before enumerating, so a future guard loosening + // (or a symlink under the run) can't turn this download into an exfil. + if (!(await isWithinRunDir(runId, taskDir))) return null; const refs = await walkArtifacts(taskDir); if (refs.length === 0) return null; return refs.map((r) => ({ relPath: r.relPath, abs: path.join(taskDir, r.relPath) })); @@ -2227,11 +2268,10 @@ export async function resolveSafePath( // check below is the actual security boundary, so an input that isn't that // clean two-segment task shape (run-level files, the nested activation // layout, OR any traversal like "../..") just falls back to the run summary - // and lets the containment check reject it — we must never hand a "."/".." - // segment to ensureTaskDir, which throws on it (isValidId admits dots). + // and lets the containment check reject it. isValidId rejects "."/".." (and + // any non-id), so a traversal segment never reaches ensureTaskDir. const parts = relPath.split("/"); - const safeSeg = (s: string | undefined): s is string => - !!s && isValidId(s) && s !== "." && s !== ".."; + const safeSeg = (s: string | undefined): s is string => isValidId(s); if (parts[0] !== "activation" && safeSeg(parts[0]) && safeSeg(parts[1])) { await ensureTaskDir(runId, parts[1], RUNS_DIR, parts[0]); } else { @@ -2263,10 +2303,10 @@ export async function resolveSafePath( // Delete a run's locally-cached blob copy under `root` so the next view // re-downloads it from storage. `force: true` makes a never-cached run a -// harmless no-op. Returns false (deleting nothing) for an unsafe id — note -// isValidId still admits "." and ".." (dots are word-ish), so require the -// resolved target to be a strict child of `root` before rm can run, or a "." -// id would nuke the cache root and ".." its parent. +// harmless no-op. Returns false (deleting nothing) for an unsafe id. isValidId +// rejects "." / ".." so a dot id can't reach here, but we still require the +// resolved target to be a strict child of `root` before rm can run — belt-and- +// suspenders against any future loosening of the id guard. export async function clearRunCacheDir( root: string, id: string, diff --git a/evalboard/lib/status.ts b/evalboard/lib/status.ts index e984410e..2aa1957c 100644 --- a/evalboard/lib/status.ts +++ b/evalboard/lib/status.ts @@ -9,6 +9,7 @@ // and "Faulted" and uses its own logic. import type { TaskResultSummary } from "./runs"; +import { DEFAULT_VARIANT } from "./variant"; export type StatusCategory = "passed" | "failed" | "error" | "unknown"; @@ -36,11 +37,16 @@ const KEY_SEP = "\u001f"; // model — so the variant is part of the key. Single-config runs carry variant // "default" (or null on legacy rows), so the key is effectively the taskId and // behavior is unchanged. +// `variant` is REQUIRED (matching TaskResultSummary.variant, a required +// `string | null`) — declaring it optional would let a row-shaped object that +// forgot the field type-check and silently fold every arm of an A/B run back +// into one "default" group, which is exactly the bug this key prevents. The +// `?? DEFAULT_VARIANT` still covers the legacy-null case. export function taskGroupKey(t: { taskId: string; - variant?: string | null; + variant: string | null; }): string { - return `${t.taskId}${KEY_SEP}${t.variant ?? "default"}`; + return `${t.taskId}${KEY_SEP}${t.variant ?? DEFAULT_VARIANT}`; } // Roll per-replicate rows up per (task, variant): key -> number of replicates @@ -49,7 +55,7 @@ export function taskGroupKey(t: { // tile AND the grid badge / collapse so they can never disagree. Keyed by // taskGroupKey so a multi-model run counts each model's attempt separately. export function perTaskPassCounts< - T extends { taskId: string; status: string | null; variant?: string | null }, + T extends { taskId: string; status: string | null; variant: string | null }, >(rows: readonly T[]): Map { const m = new Map(); for (const r of rows) { @@ -79,15 +85,21 @@ function meanOrNull(values: readonly (number | null)[]): number | null { // run keeps one row PER MODEL (the variant is part of the group key), so each // model's metrics stay separate instead of being averaged across models. Each // collapsed row's: -// - categorical fields (status, replicateIndex/detail link, tags, skill, -// model, variant, expected_turns, mature flag) come from a REPRESENTATIVE -// replicate — a passing one when any passed, else the lowest-index one — so -// the status pill and the "open detail" link both describe one real run; and -// - quantitative columns (score, duration, cost, turns via actualCommands, -// tokens) are the MEAN across ALL replicates, so the grid reflects the whole -// repeat set rather than just the representative run. (Previously every -// column was the representative's own value, so e.g. cost showed a single -// run's price instead of the average over the repeats.) +// - categorical + verdict fields (status, weightedScore, replicateIndex/detail +// link, tags, skill, model, variant, expected_turns, mature flag) come from +// a REPRESENTATIVE replicate — a passing one when any passed, else the +// lowest-index one — so the Status pill, the Score, and the "open detail" +// link ALL describe the SAME run (clicking a "Passed" row lands on a page +// showing that same score); and +// - resource columns (duration, cost, turns via actualCommands, tokens) are +// the MEAN across ALL replicates, so the grid reflects the whole repeat set +// rather than just the representative run. (Previously cost/tokens/etc. +// showed a single run's value instead of the average over the repeats.) +// weightedScore is DELIBERATELY kept on the representative, not averaged: the row +// shows one pass/fail verdict, so its score must be that run's score — an +// averaged score beside a representative Status pill reads "Passed · 0.60" for a +// SUCCESS/FAILURE pair and then shows 1.00 on click-through. Score aggregation, +// if wanted, belongs in a separately labeled column. // First-seen group order is preserved. With repeats disabled (one replicate per // task/variant) each mean is that single value, so the output is byte-identical. export function collapseReplicates( @@ -116,7 +128,8 @@ export function collapseReplicates( } out.push({ ...rep, - weightedScore: meanOrNull(group.map((t) => t.weightedScore)), + // weightedScore intentionally NOT averaged — see the note above; it + // stays the representative's so Status/Score/detail-link agree. durationSeconds: meanOrNull(group.map((t) => t.durationSeconds)), totalCostUsd: meanOrNull(group.map((t) => t.totalCostUsd)), // Turns render from displayedTurns(actualCommands, hasFinalReply); diff --git a/evalboard/lib/variant.ts b/evalboard/lib/variant.ts new file mode 100644 index 00000000..155d9ec7 --- /dev/null +++ b/evalboard/lib/variant.ts @@ -0,0 +1,47 @@ +// Variant (experiment / model) primitives shared across the dashboard. +// +// A "variant" is the on-disk subdir a run writes each task under +// (////). A single-config run uses the literal +// "default"; an A/B (multi-model) run uses one variant per arm, e.g. "kimi-k3". +// The URL carries it as ?v=, mirroring ?r=. +// +// This module is a dependency-free LEAF on purpose: it holds only the constant +// and pure URL helpers, so it is safe to import from client components +// (task-grid.tsx) and the client-safe status.ts. The PATH sanitizer that turns +// a variant into a validated filesystem segment (`variantSegment`) lives in +// lib/runs.ts instead — it depends on isValidId (which pulls the node-only blob +// layer), so it must stay server-side. + +// The subdir a single-config run writes its tasks under, and the safe fallback +// for legacy rows (no variant recorded) and for a missing ?v=. SINGLE SOURCE — +// import this instead of re-typing the "default" literal. +export const DEFAULT_VARIANT = "default"; + +// Next's App Router types a query value as `string | string[] | undefined` — a +// repeated key (`?v=a&v=b`) yields an array. Collapse to the first value so a +// repeated param can't reach a `path.join` as an array (which throws a 500). +export function firstParam( + v: string | string[] | null | undefined, +): string | undefined { + const first = Array.isArray(v) ? v[0] : v; + return first != null && first.length > 0 ? first : undefined; +} + +// Normalize a raw ?v= query value into the variant to read. Absent / empty → +// "default". Also collapses a repeated param to its first value. The path +// readers additionally run variantSegment() to reject unsafe values. +export function variantFromParam( + v: string | string[] | null | undefined, +): string { + return firstParam(v) ?? DEFAULT_VARIANT; +} + +// The query-string fragment that preserves the current variant on in-page links +// (the replicate selector, the download link, cross-page task links). Empty for +// the default variant so single-config URLs stay clean and unchanged. Returns a +// leading "&" so it appends after an existing query (`?r=2${variantLinkParam(v)}`). +export function variantLinkParam(variant: string | null | undefined): string { + return variant && variant !== DEFAULT_VARIANT + ? `&v=${encodeURIComponent(variant)}` + : ""; +} From 92f7e67e8f9bbe482b9313e56261b7ca4331d503 Mon Sep 17 00:00:00 2001 From: CarlesUIPath Date: Mon, 3 Aug 2026 12:13:44 +0100 Subject: [PATCH 5/5] =?UTF-8?q?refactor(evalboard):=20PR=20#67=20review=20?= =?UTF-8?q?cleanup=20=E2=80=94=20drop=20dead=20export,=20dedupe=20arm=20ti?= =?UTF-8?q?tle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the unused variantFromParam export from lib/variant.ts (zero importers; page.tsx uses firstParam + DEFAULT_VARIANT directly). Reads as a live seam and would drift. - Extract armTitle(t) in task-grid.tsx so the desktop cell and mobile card share one "model … · variant …" title expression instead of two copies. No behavior change; tsc + build clean, tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- evalboard/app/runs/[id]/task-grid.tsx | 19 +++++++++---------- evalboard/lib/variant.ts | 9 --------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/evalboard/app/runs/[id]/task-grid.tsx b/evalboard/app/runs/[id]/task-grid.tsx index 8702ee32..7cb1f96d 100644 --- a/evalboard/app/runs/[id]/task-grid.tsx +++ b/evalboard/app/runs/[id]/task-grid.tsx @@ -558,6 +558,13 @@ export function TaskGrid({ // The label shown in the arm column / mobile card for one row. const armLabel = (t: TaskResultSummary): string => (armByModel ? t.model ?? t.variant : t.variant) ?? DEFAULT_VARIANT; + // The hover title — names both the model and the variant so whichever the + // column shows, the other is one hover away. Shared by the desktop cell and + // the mobile card so the two can't drift. + const armTitle = (t: TaskResultSummary): string => + t.model + ? `model ${t.model} · variant ${t.variant ?? DEFAULT_VARIANT}` + : `variant ${t.variant ?? DEFAULT_VARIANT}`; const sorted = useMemo(() => { const arr = [...collapsed]; @@ -760,11 +767,7 @@ export function TaskGrid({ {showArm && ( {armLabel(t)} @@ -912,11 +915,7 @@ export function TaskGrid({ {showArm && (
{armLabel(t)}
diff --git a/evalboard/lib/variant.ts b/evalboard/lib/variant.ts index 155d9ec7..cd9203b0 100644 --- a/evalboard/lib/variant.ts +++ b/evalboard/lib/variant.ts @@ -27,15 +27,6 @@ export function firstParam( return first != null && first.length > 0 ? first : undefined; } -// Normalize a raw ?v= query value into the variant to read. Absent / empty → -// "default". Also collapses a repeated param to its first value. The path -// readers additionally run variantSegment() to reject unsafe values. -export function variantFromParam( - v: string | string[] | null | undefined, -): string { - return firstParam(v) ?? DEFAULT_VARIANT; -} - // The query-string fragment that preserves the current variant on in-page links // (the replicate selector, the download link, cross-page task links). Empty for // the default variant so single-config URLs stay clean and unchanged. Returns a