Skip to content

fix(evalboard): variant-run coverage, averaged repeat stats, and per-task model name - #67

Open
CarlesUIPath wants to merge 4 commits into
mainfrom
fix/evalboard-multimodel-and-model-tag
Open

fix(evalboard): variant-run coverage, averaged repeat stats, and per-task model name#67
CarlesUIPath wants to merge 4 commits into
mainfrom
fix/evalboard-multimodel-and-model-tag

Conversation

@CarlesUIPath

Copy link
Copy Markdown
Contributor

Summary

This PR implements 2 fixes and proposes a small feature/addon.

  1. Evalboard has no coverage for variant type runs. Current version shows no information:
Screenshot 2026-07-30 at 14 38 00
  1. Evalboard repeats type runs show stats of the first run, when it should show the average (cost, tokens, wall clock time, turns).

  2. Evalboard shows no model name or id within a task, which can become problematic when combining several models/versions across runs and tasks.

Fixes

Variant runs coverage

Screenshot 2026-07-30 at 14 36 40

Evalboard repeats

Now correctly displaying average stats
Screenshot 2026-07-30 at 14 37 33

Model name display

Now displaying name next to task verdict
Screenshot 2026-07-30 at 14 35 49

Root cause (variant runs)

The dashboard had no concept of an experiment variant. In an A/B run each model produces a task_result sharing the task_id, so it broke on two surfaces:

  • Grid — the replicate collapse was keyed on task_id alone, folding every model into one row with metrics averaged across models; the distinct models were never shown.
  • 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.

Changes

Variant is now a first-class dimension, end to end:

  • Data model (lib/runs.ts) — 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 (lib/blob.ts), readTaskDetail (now also matches the row on variant), readTaskReplicates, readLogTail, readConversationLog, collectTaskFiles, resolveSafePath, and the download API.
  • Grid (lib/status.ts, task-grid.tsx) — collapse + pass-counts key on (taskId, variant) via a new taskGroupKey, so each model keeps its own row and its own metrics. New Model column, shown only when a run has >1 distinct model. Detail links carry ?v=<variant>.
  • Detail page ([...task]/page.tsx) — parse ?v=, thread it to every reader, preserve it on the replicate selector + download link, and show the model chip in the header (fix chore: Bump actions/cache from 5.0.5 to 6.1.0 #3) so a task always names the LLM it ran on — single-model runs included.
  • Repeats (fix chore: Bump actions/setup-node from 4.4.0 to 6.4.0 #2) — collapseReplicates now averages the quantitative columns (score, duration, cost, turns, tokens) across a task's replicates instead of showing the first/representative run's values.

Variant ids are validated as a single path segment (they land in filesystem paths and blob prefixes); anything unsafe or unknown falls back to "default".

Backward compatibility

Single-config runs carry variant "default", so their grid, links, and paths are byte-for-behavior unchanged — verified live: no Model column, no ?v=, model chip still shown.

Testing

Result: all suites greentsc --noEmit clean, next build succeeds, 363/365 unit tests pass. The only 2 failures are a pre-existing, unrelated pricing-table parity check (see Not in this PR), which fails identically on the base branch. 19 new tests were added for this change; all pass.

  • Logic: variant grouping keeps one row per model, taskGroupKey collision-safety, perTaskPassCounts per-variant, grid Model-column show/hide + ?v= links, toTaskRow variant/model mapping.
  • Path resolution (lib/__tests__/variant-paths.test.ts, against a temp local runs dir): readTaskDetail selects the right model's row (not the first variant) and returns null / sanitizes an unsafe ?v; readTaskReplicates / readLogTail / collectTaskFiles are scoped to the selected variant; resolveSafePath resolves a non-default variant artifact and rejects traversal. This suite caught (and now guards) a regression where a broadened resolveSafePath handed a .. segment to ensureTaskDir and threw instead of returning null — fixed by restricting the narrow-fetch prefetch to safe segments.
  • Live E2E against a real 3-model run (kimi-k3 / glm-5.2 / deepseek-v4-pro): all three render as distinct rows with the Model column; each links to its own ?v=; all three per-model detail pages load (HTTP 200) with the correct model chip — these all 404'd before. Single-model regression clean.

Known coverage gap

The task-detail page ([...task]/page.tsx) is a React Server Component and has no unit test for its ?v= parsing / model-chip render — there's no RSC test harness in the repo. That logic is simple and is covered by the live E2E above.

Not in this PR

  • Pre-existing, unrelated failure in lib/__tests__/pricing-parity.test.ts (2 cases — claude-sonnet-5, gpt-5.6-terra missing from lib/pricing.ts). Confirmed failing identically before these changes.

CarlesUIPath and others added 4 commits July 27, 2026 18:23
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) <noreply@anthropic.com>
…odel runs

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) <noreply@anthropic.com>
…h 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) <noreply@anthropic.com>
@CarlesUIPath
CarlesUIPath marked this pull request as ready for review July 31, 2026 09:14
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @CarlesUIPath's task in 1m 26s —— View job


Code Review In Progress

Todo List:

  • Read code review guidelines (.github/code_review.md)
  • Read project conventions (CLAUDE.md)
  • Review full diff (git diff origin/main...HEAD)
  • Read and analyze changed files with full context
  • Check cross-file consistency
  • Perform design-level scrutiny
  • Check for missing edge cases and error handling
  • Verify test coverage
  • Write comprehensive review feedback

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: coder_eval — pr:67

Scope: pr:67 · branch fix/evalboard-multimodel-and-model-tag · 7c1f25d · 2026-07-31T20:32Z · workflow variant

Change class: complex — reworks the evalboard grouping key, path resolution (incl. traversal guard), and replicate-stat aggregation; correctness requires reasoning about control flow and data shape

The PR's core mechanics are strong — error handling, style, and merge/canonicalization discipline hold up (8.7/10 overall, spotless Error Handling at 10) — but the variant threading it introduces has three real risks: display semantics that change a task's shown score for identical run data, bare task URLs that now hard-404 on every multi-variant run, and an unvalidated ?v= segment that reaches path.join on two paths (including /api/download, where isValidId admits ..); combined with a Test Health axis at 6.9 where five separate mutations reintroducing the exact bugs this PR fixes leave the suite green, the change is sound in direction but should not merge until the 404 regression, the sanitizer seam, and the score/status coherence question are resolved.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9.8 / 10 0 0 0 2 The detail page hand-concatenates &v= inline instead of reusing the grid's URLSearchParams link builder — inconsistent and untestable in place
2. Type Safety 7.8 / 10 0 1 2 2 Raw ?v= query param reaches readTaskReview's filesystem path unsanitized — the one variant consumer that skips variantSegment() (traversal out of RUNS_DIR)
3. Test Health 6.9 / 10 0 1 4 1 readTaskDetail's single-config / legacy row path (variant_id absent or "default") has no assertion — a mutation that breaks it keeps the suite green
4. Security 9 / 10 0 1 0 0 variantSegment/isValidId admits . and .. (dots are word chars), so ?v=.. escapes the run dir via /api/download; the doc comment promising traversal-collapsing is false
5. Architecture & Design 8.7 / 10 0 1 0 3 Variant canonicalization has no single validated seam: module-private DEFAULT_VARIANT/variantSegment, re-typed "default" literals at six call sites, unvalidated pass-through in status.ts and the detail page, and silent lossy collapse of absent/out-of-whitelist variant_id
6. Error Handling & Resilience 10 / 10 0 0 0 0
7. API Surface & Maintainability 9 / 10 0 1 0 0 Bare (?v=-less) task URLs hard-404 on multi-variant runs because the detail page defaults to the literal "default" and readTaskDetail returns null when no such row exists — mature-source, trends and watchlist links all emit that URL
8. Evaluation Harness Quality 8 / 10 0 2 0 0 collapseReplicates averages weightedScore while Status pill + detail link stay on the pass-first representative, so a repeats row can read "Passed" next to a sub-threshold mean score (and clicking through shows a different score) — a disclosed, documented, tested change, not a silent one

Overall Score: 8.7 / 10 · Weakest Axis: Test Health at 6.9 / 10
Totals: 🔴 0 · 🟠 7 · 🟡 6 · 🔵 8 across 8 axes.

Blockers

  1. [Axis 2] Raw ?v= query param reaches readTaskReview's filesystem path unsanitized — the one variant consumer that skips variantSegment() (traversal out of RUNS_DIR) (evalboard/app/runs/[id]/[...task]/page.tsx:64) — page.tsx:52 does const variant = v ?? "default"; and then passes that RAW value at line 62-67:
    const review = await readTaskReview(
        id,
        variant,
        taskId,
        replicateDirName(replicate),
    );

Every other new consumer receives the value only after variantSegment() sanitizes it (readTaskDetail/readTaskReplicates/readLogTail/readConversationLog/collectTaskFiles all start with const v = variantSegment(variant); — lib/runs.ts:1830, 1850, 2114, 2141, 2194). readTaskReview(runId: string, variantId: string, ...) (lib/reviews.ts:43-56) declares variantId as a bare string and splices it straight into path.join(RUNS_DIR, runId, variantId, taskId, replicate, "review.json") — outside the try. Nothing in the type system distinguishes "raw query-param string" from "validated path segment", so the omission compiles cleanly. Verified: path.join("/runs","20260101","../../../../etc","t","00","review.json") === /etc/t/00/review.json. Fix: hoist the sanitizer — export variantSegment from lib/runs.ts (or a shared lib/variant.ts) and compute const variant = variantSegment(v) ONCE at page.tsx:52, passing the sanitized value to every reader including readTaskReview; better still, make it return a branded type VariantSegment = string & { __variant: true } and type readTaskReview's parameter as that, so an unsanitized string is a compile error. CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:N/A:N
2. [Axis 3] readTaskDetail's single-config / legacy row path (variant_id absent or "default") has no assertion — a mutation that breaks it keeps the suite green (evalboard/lib/__tests__/variant-paths.test.ts:96) — The PR's headline compat claim is at evalboard/lib/runs.ts:1865-1869: const matches = (data?.task_results ?? []).filter((t) => t.task_id === taskId && variantSegment(t.variant_id) === v,); with the comment "Legacy rows carry neither field (null variant / null replicate_index) → treated as ("default", 0), so an old single-result run still resolves." Nothing asserts that. The new fixture (variant-paths.test.ts:60-77) gives EVERY row a variant_id and creates NO default/ subdir, so the only default-variant assertion is the negative one at line 96-97: expect(await readTaskDetail(RUN, TASK, 0)).toBeNull();. grep -rn readTaskDetail over lib/__tests__ + app shows variant-paths.test.ts is its ONLY caller in tests. Verified by mutation: rewriting the filter to (t) => t.task_id === taskId && t.variant_id === v (i.e. legacy rows with no variant_id never match, so every pre-existing run's task page 404s) leaves npx vitest run at 3 failed | 369 passed — the same 3 pre-existing pricing-parity failures, zero new. Fix: add a second fixture run whose rows omit variant_id with content under <RUN>/default/<TASK>/00/, and assert readTaskDetail(RUN, TASK) (no ?v) returns the row + content, plus readTaskReplicates(RUN, TASK)[0] and readLogTail(RUN, TASK, 0) reads that dir.
3. [Axis 4] variantSegment/isValidId admits . and .. (dots are word chars), so ?v=.. escapes the run dir via /api/download; the doc comment promising traversal-collapsing is false (evalboard/lib/runs.ts:609) — variantSegment (evalboard/lib/runs.ts:608-610) is the only guard on the new attacker-controlled variant segment:

// ...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;   // line 609
}

The comment's traversal claim is false: isValidId is ID_RE = /^[\w.-]+$/ (evalboard/lib/blob.ts:17) which MATCHES "." and ".." — the codebase already knows this (clearRunCacheDir comment, runs.ts:2267: "isValidId still admits '.' and '..' (dots are word-ish)"; resolveSafePath explicitly adds s !== "." && s !== ".." at runs.ts:2234). assertValidId(variant, "variant") in blob.ts:280 (commented "Validated as a path segment so it can't escape the run prefix") passes ".." for the same reason. So taskContentBase (runs.ts:624) evaluates path.join(RUNS_DIR, runId, "..", taskId) and collectTaskFiles (runs.ts:2193-2199) accepts runId=".." too (isValidId("..") === true), giving two levels of escape; walkArtifacts then enumerates that directory and app/api/download/route.ts:36-38 reads and zips every file.

Verified by running the real modules under vitest: with RUNS_DIR = <tmp>/a/b/runs-remote, collectTaskFiles("..", "secret", "..") returned [{relPath:"flag.txt", abs:"<tmp>/a/secret/flag.txt"}] — in BOTH blob mode and EVALBOARD_LOCAL_RUNS_DIR mode (local mode returns from ensureTaskDir before any blob call, so only the on-disk join runs). With a container WORKDIR of /app, RUNS_DIR is /app/runs-remote, so GET /api/download?run=..&task=etc&v=.. resolves to /etc and streams it as a zip; task=proc/self etc. reach process environment/secrets. This is NEW: on origin/main taskContentBase hardcoded the literal segment (path.join(RUNS_DIR, runId, "default", taskId)), which neutralised runId=".." — the PR replaced that literal with the user-controlled variant.

Fix: reject dot-only segments in the shared validator — e.g. add && id !== "." && id !== ".." inside isValidId (blob.ts:23-30), which fixes variantSegment, assertValidId, collectTaskFiles's runId check and clearRunCacheDir at once — and, defence in depth, apply the same realpath containment check resolveSafePath uses (runs.ts:2240-2261) to the directory returned by taskContentBase before walkArtifacts runs. Note the new test at evalboard/lib/tests/variant-paths.test.ts:100-105 gives false assurance: it only exercises "../glm-5-2" (contains /, correctly rejected) — add cases for bare "..", "." and for runId=".." on collectTaskFiles. CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
4. [Axis 5] Variant canonicalization has no single validated seam: module-private DEFAULT_VARIANT/variantSegment, re-typed "default" literals at six call sites, unvalidated pass-through in status.ts and the detail page, and silent lossy collapse of absent/out-of-whitelist variant_id (evalboard/lib/runs.ts:608) — The PR threads a new defaulted variant param through 9 signatures, but the rule for "turn a variant into a path segment" is re-implemented four different ways instead of living in one exported seam:

  1. evalboard/lib/runs.ts:608-610 (module-private, the main path):
function variantSegment(variant: string | null | undefined): string {
    return variant && isValidId(variant) ? variant : DEFAULT_VARIANT;
}
  1. evalboard/lib/runs.ts:2233-2234 — the SAME file's other rule, which adds the guard the main path is missing:
const safeSeg = (s: string | undefined): s is string =>
    !!s && isValidId(s) && s !== "." && s !== "..";

with the author's own comment two lines above (runs.ts:2231): "… segment to ensureTaskDir, which throws on it (isValidId admits dots)".
3. evalboard/lib/status.ts:43 — no validation at all: return ${t.taskId}${KEY_SEP}${t.variant ?? "default"};
4. evalboard/app/runs/[id]/[...task]/page.tsx:62-67 — the raw ?v= value is handed straight to readTaskReview(id, variant, taskId, …), which does zero validation and interpolates it into path.join(RUNS_DIR, runId, variantId, taskId, replicate, "review.json") (evalboard/lib/reviews.ts:45-56).

Because ID_RE = /^[\w.-]+$/ (evalboard/lib/blob.ts:17) matches "..", rule 1 lets ".." through as a path segment while rule 2 explicitly rejects it. I verified this empirically with a throwaway vitest probe against the PR worktree (since deleted): with EVALBOARD_LOCAL_RUNS_DIR=/tmp/probe-runs, readLogTail("2026-01-01_00-00-00", "mytask", 0) returned "" while readLogTail("2026-01-01_00-00-00", "mytask", 0, "..") returned "ESCAPED-SIBLING\n" — i.e. taskContentBase (runs.ts:616-625) joined .. and read <RUNS_DIR>/mytask/task.log, outside the run dir. The same holds for readConversationLog (runs.ts:2134) and collectTaskFiles (runs.ts:2188), which is reachable from /api/download?run=…&task=…&v=...

Fix: export ONE resolver from lib/runs.ts (e.g. resolveVariant() returning a sanitized segment, or a TaskLocation object built once and passed instead of a bare string), give it the s !== "." && s !== ".." guard that safeSeg already has, and route every consumer through it — taskGroupKey, readTaskReview, and the download route. Also export DEFAULT_VARIANT (runs.ts:603): the literal "default" is currently hardcoded in status.ts:43, page.tsx:52, page.tsx:79, task-grid.tsx:223 and blob.ts:276, and the "omit default from the URL" rule is written twice in two different shapes (page.tsx:78-81 template string vs task-grid.tsx:221-225 URLSearchParams).
5. [Axis 7] Bare (?v=-less) task URLs hard-404 on multi-variant runs because the detail page defaults to the literal "default" and readTaskDetail returns null when no such row exists — mature-source, trends and watchlist links all emit that URL (evalboard/app/runs/[id]/[...task]/page.tsx:52) — const variant = v ?? "default"; (page.tsx:52) hardcodes the fallback instead of resolving the run's actual variant, and lib/runs.ts:1866-1869 now filters rows on it:

    const matches = (data?.task_results ?? []).filter(
        (t) =>
            t.task_id === taskId &&
            variantSegment(t.variant_id) === v,
    );

with if (!rawTask) return null;if (!task) notFound();. Only experiments/default.yaml:85 uses variant_id: default; real in-repo experiments use other ids (experiments/permissions-smoke.yaml:23 with-deny, experiments/plugin-comparison.yaml:33 with-plugin, experiments/model-comparison.yaml:21,24 sonnet/opus). For any such run a URL without ?v= matches zero rows and 404s, where before the PR (.filter((t) => t.task_id === taskId)) it resolved the row and rendered the header/status/score/cost from run.json. Three in-repo link builders still emit ?v=-less hrefs — app/runs/[id]/task-grid.tsx:158 (href={/runs/${sourceRun}/${t.taskId}}, the mature-task popover), app/trends/trends-view.tsx:267, app/watchlist/watchlist-view.tsx:25 — plus every existing bookmark/report deep link. The new test at lib/__tests__/variant-paths.test.ts:94-98 encodes this as intended ("the grid only ever links multi-model rows with ?v="), which is not true of those three call sites. Fix: when v is absent, derive it from run.json — if the task's rows all share one variant use it, and only require ?v= to disambiguate a genuine multi-variant task; and thread the variant through the trends/watchlist/mature links.
6. [Axis 8] collapseReplicates averages weightedScore while Status pill + detail link stay on the pass-first representative, so a repeats row can read "Passed" next to a sub-threshold mean score (and clicking through shows a different score) — a disclosed, documented, tested change, not a silent one (evalboard/lib/status.ts:119) — evalboard/lib/status.ts:117-121 spreads the representative and then overwrites the score: out.push({ ...rep, weightedScore: meanOrNull(group.map((t) => t.weightedScore)), ... }). The representative is still chosen pass-first (if (repPass !== tPass) { if (tPass) rep = t; }, lines 111-112), so for a 2-replicate task with SUCCESS/score 1.0 and FAILURE/score 0.2 the row now renders Status=Passed (task-grid.tsx:747 <StatusPill status={t.status} relabel />) next to Score=0.60 (task-grid.tsx:751-753 t.weightedScore.toFixed(2)) and badge "1/2 ✓". On main the same run.json rendered Score=1.00. Two problems: (1) this is a displayed-score change for identical run data that the PR description does NOT claim — it lists only "cost, tokens, wall clock, turns"; (2) it breaks the invariant main's own comment asserted at task-grid.tsx (removed by this diff): "The representative is chosen so its status, score, cost, duration AND detail link all describe the SAME run". The new comment at status.ts:82-90 silently reclassifies score from a categorical (representative) field to a quantitative (mean) one. Either keep weightedScore on the representative so status/score/link stay coherent, or make the whole row an aggregate (render a mean-derived status, or add a separate "mean score" column and label the existing one), and state the score change in the PR description. evalboard/lib/__tests__/status.test.ts:44 (expect(collapsed.weightedScore).toBeCloseTo(0.6, 10)) locks in the current behavior, so this needs a deliberate decision, not just a code tweak.
7. [Axis 8] Grid rows split on (taskId, variant) but the only distinguishing column is Model, gated on distinct model — same-model A/B runs render N visually identical, unlabeled rows per task (evalboard/app/runs/[id]/task-grid.tsx:537) — app/runs/[id]/task-grid.tsx:536-539 reads const showModel = useMemo(() => new Set(collapsed.map((t) => t.model).filter(Boolean)).size > 1, [collapsed]); and the column is dropped when that is false (line 568-571: (showModel || c.key !== "model")). But the row split is by variant (taskGroupKey = taskId+variant, status.ts:39-45). docs/AB_EXPERIMENTS.md:164-179 — the "Recipe: A/B a Skill" example — sets model: claude-sonnet-4-6 in the shared defaults and varies only agent.plugins between variant_id: bare and variant_id: with-skill; docs/AB_EXPERIMENTS.md:223-228 ("terse" vs "detailed") likewise varies only prompt_mutations. For both, every row has the same model_used, so showModel is false and the grid renders two visually identical rows per task with no column naming the arm — the user cannot tell bare from with-skill. (The same happens in an actual multi-model run when one variant's rows carry model_used: null, since .filter(Boolean) drops them.) Gate on distinct VARIANT instead of distinct model, and render the variant id (falling back to / alongside the model) — e.g. new Set(collapsed.map((t) => t.variant ?? "default")).size > 1 with a "Variant" column, so a same-model A/B is attributable. The new tests at app/runs/[id]/__tests__/task-grid.test.tsx:400-441 only cover the differing-model case, so this gap is untested.

Non-blocking, but please consider before merge

  1. [Axis 2] searchParams is typed v?: string but Next yields string[] for a repeated param, so ?v=a&v=b bypasses tsc and reaches readTaskReview / the in-page links as an array (evalboard/app/runs/[id]/[...task]/page.tsx:36) — Line 35-36 declares:
    params: Promise<{ id: string; task: string[] }>;
    searchParams: Promise<{ r?: string; v?: string }>;

Next 15 App Router resolves searchParams to { [key: string]: string | string[] | undefined } — a repeated query key yields an array. Next's generated page-props validator types searchParams as Promise<any>, which is assignable to this narrower shape, so tsc --noEmit (verified clean, exit 0) cannot catch the lie. At runtime ?v=a&v=b makes variant an array while typed string; it flows unsanitized into readTaskReviewpath.join (lib/reviews.ts:49) which throws TypeError: The "path" argument must be of type string. Received an instance of Array (verified with node), producing an unhandled 500 on a page that would otherwise render. (?r=a&r=b is harmless — Number(["a","b"]) is NaN → replicate 0 — so v is the newly exposed one.) Fix: declare searchParams: Promise<{ r?: string | string[]; v?: string | string[] }> and normalize (const v0 = Array.isArray(v) ? v[0] : v;) before use, or feed it through variantSegment, whose isValidId already rejects non-strings.
2. [Axis 2] Averaged token buckets render unrounded in the grid: fmtCompact's < 1000 branch is String(n), so a repeats run shows e.g. "440.6666666666667" in the token columns (the turns column was patched, the token columns were not) (evalboard/lib/status.ts:126) — collapseReplicates now writes means into integer-semantics fields (lib/status.ts:124-131):

            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)),

TaskResultSummary's number | null cannot express "integer count", so tsc sees no change. The author recognised the hazard for turns and patched it (lib/turns.ts:76-79, fmtTurnsCount${Number(n.toFixed(2))}), but the token columns route through tokenCellfmtCompact (lib/format.ts:35-41), whose final branch is return String(n); for |n| < 1000. Verified: fmtCompact((500+501+501)/3) === "500.6666666666667" — a 3-repeat task with sub-1k output tokens renders that string in the Output column (values ≥1000 are masked by the .toFixed(1) k-branch). Fix: round in collapseReplicates for count-typed fields (e.g. Math.round(meanOrNull(...) ?? NaN) guarded for null), or mirror the turns fix in fmtCompact's tail branch: return String(Number(n.toFixed(2)));.
3. [Axis 3] 5 of the 9 columns averaged by collapseReplicates (status.ts:119-131) have no averaging assertion — reverting them to the representative's value keeps the whole vitest suite green (evalboard/lib/__tests__/status.test.ts:34) — The averaging test at status.test.ts:34-45 varies only 4 fields (totalCostUsd, durationSeconds, weightedScore, actualCommands) and asserts only those 4 means. evalboard/lib/status.ts:119-131 averages nine: the four above plus totalTurns, inputTokens, outputTokens, cacheCreationTokens, cacheReadTokens. The only other token assertion (status.test.ts:64-72) uses all-null values, which cannot distinguish a mean from a representative. Verified by mutation: replacing those five with totalTurns: rep.totalTurns, inputTokens: rep.inputTokens, outputTokens: rep.outputTokens, cacheCreationTokens: rep.cacheCreationTokens, cacheReadTokens: rep.cacheReadTokens — i.e. reintroducing exactly the bug this PR fixes, for the token and turn columns — leaves npx vitest run at 3 failed | 369 passed (only the pre-existing pricing-parity failures). Fix: give each replicate in the status.test.ts:34 case distinct token/turn values and assert all nine means; and add a parity guard that iterates the numeric keys of TaskResultSummary so a future column added to the type but not to the mean list at status.ts:119 fails a test instead of silently taking the representative's value.
4. [Axis 3] ensureTaskDir's variant blob prefix + dedupe key have zero test coverage, and resolveSafePath's prefetch branch is only half-covered (LOCAL mode no-ops the fetch) (evalboard/lib/__tests__/variant-paths.test.ts:136) — Both new resolveSafePath tests (variant-paths.test.ts:136-150) run with EVALBOARD_LOCAL_RUNS_DIR stubbed, and ensureTaskDir short-circuits on if (LOCAL_RUNS_DIR) return; (evalboard/lib/blob.ts:281) as does ensureRunSummary (blob.ts:164). So the rewritten branch at evalboard/lib/runs.ts:2233-2237if (parts[0] !== "activation" && safeSeg(parts[0]) && safeSeg(parts[1])) { await ensureTaskDir(runId, parts[1], RUNS_DIR, parts[0]); } — has no behavioral coverage: verified by mutation, replacing the condition with parts.length > 99 (always take the ensureRunSummary fallback) leaves npx vitest run at 3 failed | 369 passed, only the pre-existing pricing-parity failures. The same holds for ensureTaskDir's new dedupe key task:${runId}/${variant}/${taskId} (blob.ts:282) and prefix ${runId}/${variant}/${taskId}/ (blob.ts:296-298) — lib/__tests__/blob.test.ts only tests isValidId/isValidTaskId, so a wrong prefix means artifacts silently never download in production blob mode while CI stays green. Negative coverage is also thin: only resolveSafePath(RUN, "../../etc/passwd") (line 149) is asserted — no absolute path, no default/../../etc/passwd, no empty relPath, no ./.. variant segment. Fix: unit-test ensureTaskDir against a stubbed container asserting the exact listBlobsFlat prefix per variant, and add the missing traversal negatives.
5. [Axis 3] Download route's ?v= variant param is untested — dropping the argument survives vitest, tsc, and (absent) lint (evalboard/app/api/download/route.ts:26) — const variant = url.searchParams.get("v") ?? undefined; (route.ts:17) and ? await collectTaskFiles(runId, taskId, variant) (route.ts:26) are the whole new contract, and nothing exercises it. Verified by mutation: reverting line 26 to await collectTaskFiles(runId, taskId) — so every A/B download zips the non-existent default/ folder and 404s — leaves npx vitest run at 3 failed | 366 passed with no new failures, and npx tsc --noEmit clean (no eslint in this package to flag the now-unused variant). A route-test precedent already exists at evalboard/app/api/refresh/__tests__/route.test.ts. Fix: add app/api/download/__tests__/route.test.ts asserting ?run=..&task=..&v=glm-5-2 zips the glm-5-2/ subtree, that an absent v resolves the default/ subtree, and that an unknown variant returns 404.
6. [Axis 3] Re-keying perTaskPassCounts on (taskId, variant) flips the run-page headline tile for multi-variant runs, and no computeRunMetrics test uses a non-null variant (evalboard/app/runs/[id]/__tests__/run-view.test.ts:98) — perTaskPassCounts is now keyed by taskGroupKey (evalboard/lib/status.ts:56), and run-view.tsx:100 derives the headline tile from its size: taskTotal: perTask.size, with const hasRepeats = metrics.taskTotal !== metrics.total; at run-view.tsx:325 switching the tile between the per-task rate and the plain rate. For an A/B run with two variants of one task this flips: before, taskTotal 1 vs total 2 → repeats-style "1 of 1 tasks"; after, taskTotal 2 === total 2 → plain 2/2. The PR's only edit to this test file is variant: null, in the row factory (run-view.test.ts:22-23 region), so both metric assertions (expect(m.taskTotal).toBe(2) at line 98 and expect(m.taskTotal).toBe(m.total) at line 112) still only exercise null-variant rows — the changed multi-variant semantics of the tile are unasserted. Fix: add a computeRunMetrics case with two variants sharing one taskId asserting taskTotal/taskPassed/taskFailed and the resulting hasRepeats semantics.

Nits

  1. [Axis 1] The detail page hand-concatenates &v= inline instead of reusing the grid's URLSearchParams link builder — inconsistent and untestable in place (evalboard/app/runs/[id]/[...task]/page.tsx:78) — app/runs/[id]/task-grid.tsx:221-225 builds the link properly:
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);

but app/runs/[id]/[...task]/page.tsx:78-81 produces a fragment that silently assumes its consumers already emitted a ?:

const variantParam =
    variant && variant !== "default" ? `&v=${encodeURIComponent(variant)}` : "";

spliced into page.tsx:116 (...?r=${ri}${variantParam}) and page.tsx:165 (...&task=…${variantParam}). It works today only because both URLs happen to carry a prior param. Use URLSearchParams in both places so the two new link builders read the same way and cannot break when a param is dropped.
2. [Axis 1] taskGroupKey(t) is computed twice per row in the replicateCounts reducer (evalboard/app/runs/[id]/task-grid.tsx:514) — app/runs/[id]/task-grid.tsx:513-514:

for (const t of tasks)
    m.set(taskGroupKey(t), (m.get(taskGroupKey(t)) ?? 0) + 1);

Hoist it — const k = taskGroupKey(t); m.set(k, (m.get(k) ?? 0) + 1); — matching the shape already used in perTaskPassCounts (lib/status.ts:56-57), which does bind the key to a local.
3. [Axis 2] taskGroupKey/perTaskPassCounts declare variant optional while TaskResultSummary.variant is required, re-opening the cross-model merge hole (evalboard/lib/status.ts:41) — lib/status.ts:39-43 and 51-53 widen the contract:

export function taskGroupKey(t: {
    taskId: string;
    variant?: string | null;
}): string {
    return `${t.taskId}${KEY_SEP}${t.variant ?? "default"}`;
export function perTaskPassCounts<
    T extends { taskId: string; status: string | null; variant?: string | null },
>(rows: readonly T[]): Map<string, number> {

The source of truth, TaskResultSummary.variant, is a REQUIRED string | null (lib/runs.ts:96) — making it optional here means a future row-shaped object that simply forgets the field type-checks and folds every model of an A/B run back into one group under "default", which is exactly the bug this PR fixes, with no compile error and no test failure. Today's only callers pass real TaskResultSummary rows (app/runs/[id]/run-view.tsx:97, app/runs/[id]/task-grid.tsx:514/522/531), so this is latent. Fix: declare variant: string | null (required) in both signatures; the ?? "default" still covers the legacy-null case.
4. [Axis 2] let rep = group[0] relies on noUncheckedIndexedAccess being off to type an indexed access as non-undefined (evalboard/lib/status.ts:107) — lib/status.ts:107 reads let rep = group[0]; and immediately dereferences it (isPassStatus(rep.status) at line 109). This is safe by construction — lib/status.ts:99-101 only creates a group via groups.set(key, [t]), so no group is ever empty — but the safety is invisible to the type checker because evalboard/tsconfig.json enables "strict": true without noUncheckedIndexedAccess (verified: neither noUncheckedIndexedAccess nor exactOptionalPropertyTypes appears in tsconfig.json). Fix: either destructure with an explicit guard (const [first, ...rest] = group; if (!first) continue;) so the invariant is checked rather than assumed, or enable noUncheckedIndexedAccess package-wide to make this class of assumption a compile error everywhere.
5. [Axis 3] turnsCellFor hardcodes cell index 5 against the pre-PR column layout, which the new Model column shifts (evalboard/app/runs/[id]/__tests__/task-grid.test.tsx:51) — The helper comments // Layout: Task, Status, Score, Duration, Cost, Turns, Out, Cache+, Cache↺ (line 51) then return cells[5]!; (line 52). The PR inserts a Model column right after Task ({ key: "model", header: "Model" }, task-grid.tsx:345) whenever a run has >1 distinct model, which shifts Turns to index 6. Today's Turns tests pass only because their rows have model: null so showModel is false; any future turns assertion on multi-model rows would silently read the Cost cell instead. Relatedly, no grid-level test asserts a collapsed repeats row renders the AVERAGED Cost/Turns — the fractional fmtTurnsCount path is unit-tested (turns.test.ts:137-142) but never through the grid. Fix: derive the cell index from the rendered column headers rather than a literal, and add one grid test that a 2-replicate task renders the mean cost and a fractional turn count.
6. [Axis 5] lib/status.ts outgrows its stated purpose: a status-categorization leaf now owns replicate collapsing / numeric averaging and takes a new dependency on the server-only lib/runs.ts (header no longer describes the module) (evalboard/lib/status.ts:93) — lib/status.ts was a dependency-free status classifier (its only exports were statusCategory / isPassStatus / statusSortRank / perTaskPassCounts), imported by the client component app/runs/[id]/task-grid.tsx:1 ("use client") and by lib/pills.tsx:1. It now opens with import type { TaskResultSummary } from "./runs"; (status.ts:11) and hosts collapseReplicates (status.ts:93-129), which averages weightedScore / durationSeconds / totalCostUsd / actualCommands / totalTurns and the four token buckets — run-grid row shaping, not status classification. The new lib/status → lib/runs edge is only safe because it is import type (erased by tsc); lib/runs.ts:1-16 pulls node:fs, node:path and the blob layer, so a future value import from the same module would drag server-only code into the client bundle. Consider moving collapseReplicates + taskGroupKey into a small lib/task-rows.ts that owns TaskResultSummary shaping, leaving status.ts as the leaf classifier.
7. [Axis 5] variant inserted as a middle positional parameter ahead of the pre-existing maxBytes default on readLogTail/readConversationLog (evalboard/lib/runs.ts:2111) — readLogTail and readConversationLog gain variant in position 4, shifting maxBytes:

export async function readLogTail(
    runId: string,
    taskId: string,
    replicate = 0,
    variant: string | null = DEFAULT_VARIANT,   // runs.ts:2111 (and :2138 for readConversationLog)
    maxBytes = 200_000,
)

No current caller passes maxBytes (only app/runs/[id]/[...task]/page.tsx:69,71 call these, with 4 args), so nothing breaks today — but a caller that did would now silently pass a number as the variant, and variantSegment would swallow it into "default" rather than failing. Prefer an options object ({ replicate, variant, maxBytes }) or append variant after maxBytes so an existing positional call can't be reinterpreted.
8. [Axis 5] Download route's documented contract in evalboard/README.md is now stale (evalboard/app/api/download/route.ts:9) — The route gains a third query param (route.ts:9-11, ?run=<id>&task=<id>[&v=<variant>]; read at route.ts:20 const variant = url.searchParams.get("v") ?? undefined;), but evalboard/README.md — the only architecture doc for this package — still documents the old contract and the old layout invariant:

  • README.md:45 — - `/api/download?run=<id>[&task=<id>]` streams a zip of a task folder …
  • README.md:37-39 — <task-id>` … equals the subdir name under `<run-id>/default/`.
    The second line is now false for any A/B run (the subdir is <variant_id>/). Update both README lines in the same change so the package's stated conventions match the code.

What's Missing

Parallel paths:

  • 🟠 Reviews were not re-keyed alongside the row split: the grid now renders one row per (taskId, variant) but indexByTask (evalboard/lib/reviews.ts:86-90) still indexes review entries by task_id alone, first-occurrence-wins, and task-grid.tsx:691/848 does reviewsByTask?.get(t.taskId) — so in an A/B run BOTH arms display the first arm's review summary/tags, and run-view.tsx:288/303 filters both arms on those same tags. The producer already emits the full triple ((task_id, variant_id, replicate) per .claude/commands/coder-eval-review.md step 4; variant_id is on ReviewIndexEntry, lib/reviews-types.ts:13), so only the consumer index drops it. Fix: key EntriesByTask on taskGroupKey-style (task, variant) and look rows up with the row's variant. (trigger: evalboard/app/runs/[id]/task-grid.tsx)
  • 🟠 The sibling row shape in the SAME changed file was not taught about variants: readRunOverview/RunOverviewTask (lib/runs.ts:804-940) is built from the identical run.json task_results rows but still has no variant field, so lib/trends.ts:127-131 collapses an A/B run to a single first-occurrence-per-taskId sample (its comment only reasons about repeats, not variants) and lib/watchlist.ts inherits the same fold. Result: a 2-arm run contributes one arbitrary arm's status to every task's pass-rate trend and watchlist streak, and neither surface can emit ?v= on its links. Either carry variant onto RunOverviewTask and bucket trends per (task, variant), or state explicitly that trends deliberately track one arm. (trigger: evalboard/lib/runs.ts)
  • 🟡 Mature-task carry-forward was left variant-blind: resolveMatureSourceRuns (lib/runs.ts:740-766) resolves "the run this task last executed in" by task_id only, and MatureTaskLink (task-grid.tsx:158) links there with no ?v= — so on a multi-variant run the popover can name a source run in which only the OTHER arm executed, and the link then 404s under the new row filter. Thread the row's variant into both the lookup and the href. (trigger: evalboard/lib/runs.ts) (restates: Axis 7: Bare (?v=-less) task URLs hard-404 on multi-variant runs)
  • 🟡 The activation surface was half-threaded: taskContentBase/ensureTaskDir now build <run>/activation/<variant>/<taskId>/ (lib/runs.ts:622-625, lib/blob.ts:296-298), but readActivationTasks (lib/runs.ts:917-940) still produces rows with no variant and app/runs/[id]/activation/page.tsx:233 links tasks without ?v=. Today every sampled activation row carries variant_id: "default" so nothing breaks, but the moment an activation suite is run under an experiment those links 404 exactly like the skills links do — decide and document whether activation is variant-scoped or pin it to default. (trigger: evalboard/lib/blob.ts) (restates: Axis 7: Bare (?v=-less) task URLs hard-404 on multi-variant runs)

Tests:

  • 🟠 The most-changed UI file has no test of any kind: app/runs/[id]/[...task]/__tests__/ holds only conversation/message-timeline/turns-stat tests, nothing that renders page.tsx. All 54 changed lines — ?v= parsing (line 52), the variantParam fragment used on the replicate selector (116) and download link (165), the model chip (151-163), and the six reader call sites — are unexercised; a page-level test passing ?v=.. or ?v=a&v=b is precisely what would have caught the one reader that skips variantSegment. (trigger: evalboard/app/runs/[id]/[...task]/page.tsx) (restates: Axis 2: Raw ?v= query param reaches readTaskReview's filesystem path unsanitized)
  • 🟡 There is no blob-mode test harness at all: every new variant test stubs EVALBOARD_LOCAL_RUNS_DIR, and both ensureRunSummary and ensureTaskDir return at if (LOCAL_RUNS_DIR) return; (blob.ts:164/281), so the entire production code path this PR changed (variant prefix at blob.ts:296-298, dedupe key at :282, and the prefetch branch at runs.ts:2233-2237) is CI-invisible — a wrong prefix ships green and silently yields artifact-less task pages in the deployed dashboard. Add a stubbed-container unit test asserting the exact listBlobsFlat prefix per variant. (trigger: evalboard/lib/blob.ts) (restates: Axis 3: ensureTaskDir's variant blob prefix + dedupe key have zero test coverage)
  • 🟡 The new sanitizer suite only asserts the easy negative ("../glm-5-2", rejected because it contains /, variant-paths.test.ts:100-105). Missing negatives for the cases that actually pass ID_RE: bare "..", ".", an absolute path, an empty relPath, and collectTaskFiles(runId="..") — plus the positive legacy shape (a row with no variant_id resolving under <run>/default/), which no fixture in the file creates. (trigger: evalboard/lib/tests/variant-paths.test.ts) (restates: Axis 4: variantSegment/isValidId admits . and ..)

Downstream consumers:

  • 🟠 perTaskPassCounts was re-keyed on (taskId, variant) but its non-grid consumer was neither changed nor re-verified: run-view.tsx:97-103 derives taskTotal/taskPassed/taskFailed from perTask.size, and run-view.tsx:325 flips the headline tile on taskTotal !== total — so for a 2-arm A/B run the tile switches from "1 / 1 tasks" plus a "N / M replicate runs" sub-line to a plain "2 / 2" for byte-identical run.json. run-view.tsx is not in the PR's changed-file set and the PR text does not mention the tile at all; either state the new semantics or keep the tile's denominator on distinct task ids. (trigger: evalboard/lib/status.ts) (restates: Axis 3: Re-keying perTaskPassCounts on (taskId, variant) flips the run-page headline tile)
  • 🟡 Turning six integer-semantics columns into means (status.ts:119-131) required a sweep of every formatter that renders them, and only one was done: fmtTurnsCount got the Number(n.toFixed(2)) guard (turns.ts:76-79) while the four token columns still route through fmtCompact's return String(n) tail branch (lib/format.ts:35-41), so a 3-repeat task with sub-1k tokens prints e.g. "500.6666666666667". Round count-typed means at the source in collapseReplicates rather than patching formatters one at a time. (trigger: evalboard/lib/status.ts) (restates: Axis 2: Averaged token buckets render unrounded in the grid (fmtCompact))
  • 🟡 The collapsed row is now half aggregate (mean score/cost/duration/turns/tokens) and half representative (status, model, detail link), but nothing downstream was updated to say so: the Score cell (task-grid.tsx:751-753) and mobile Stat (:903) carry no "mean of N" label or tooltip, COLUMN_HELP (task-grid.tsx:54-62) has no entry for Score or Turns explaining the averaging, and clicking through lands on one replicate whose own page shows a different score. Label the aggregated columns, or derive the pill from the aggregate. (trigger: evalboard/lib/status.ts) (restates: Axis 8: collapseReplicates averages weightedScore while the Status pill stays on the representative)

Display & mapping dicts:

  • 🟡 The new grouping axis (variant) got no display mapping of its own — everything keys off model_used instead: the table column is gated on distinct model (task-grid.tsx:537), the mobile card chip on the same flag (:883), the detail-page chip only renders task.model && (page.tsx:151) so a row with null model_used shows no arm identifier at all, the variant appears only inside the href, and COLUMN_HELP (Partial<Record<SortKey, ColHelp>>, task-grid.tsx:54) has no entry for the new Model column so its ⓘ is absent without any compile error. Add a Variant label/column gated on distinct variant. (trigger: evalboard/app/runs/[id]/task-grid.tsx) (restates: Axis 8: Grid rows split on (taskId, variant) but the only distinguishing column is Model)

Daily/nightly:

  • 🟠 The cross-repo contract this PR now depends on is unpinned on the writer side: ExperimentVariant.variant_id has NO charset validator (src/coder_eval/models/experiment.py:25 — contrast experiment_id's kebab-case field_validator at :155-160), yet the Python core uses it verbatim as a run-dir segment (path_utils.build_task_run_dir, <run_dir>/<variant_id>/<task_id>/<NN>/) and evalboard now reflects it into a filesystem path AND an Azure blob prefix, silently collapsing anything outside /^[\w.-]+$/ to "default" (runs.ts:608-610). A variant id containing a space or / therefore produces a run the dashboard can never resolve: the row matches only when ?v= is omitted, then taskContentBase looks in a nonexistent default/ and the page renders with no artifacts, logs or task.json and no error. Either add a kebab/slug validator to variant_id in the core (and document it as the evalboard contract) or make evalboard surface a hard "unknown variant" state instead of falling back. (trigger: evalboard/lib/runs.ts)
  • 🟠 Blast radius on the deployed dashboard / nightly is unstated: the new row filter (runs.ts:1865-1869) and blob prefetch prefix (blob.ts:296-298) apply retroactively to every historical run already in blob storage, and non-default variant ids are not exotic — every experiment arm (sonnet/opus, with-deny, with-plugin, terse/detailed) plus every coder-eval evaluate run, which hardcodes variant_id="evaluate" (src/coder_eval/cli/evaluate_command.py:115). All existing deep links/bookmarks into those runs now hard-404 rather than degrading, and the change is exercised only in LOCAL mode by tests. The PR should state the prod impact and ship a fallback (resolve the variant from run.json when ?v= is absent and the task has exactly one arm). (trigger: evalboard/lib/runs.ts) (restates: Axis 7: Bare (?v=-less) task URLs hard-404 on multi-variant runs)

Harness & Lint Improvements

Static checks (lint / type):

  • [bandit-codeql] Add javascript-typescript to the CodeQL language matrix in .github/workflows/codeql.yml (today: languages: python only, so the Next.js app — the repo's ONLY network-facing HTTP surface — has never been statically scanned). With queries: security-and-quality, the default js/path-injection ("Uncontrolled data used in path expression") query models exactly this shape: source = url.searchParams.get("v") / Next searchParams, sink = path.join(RUNS_DIR, …)fs.readFile/fs.readdir in evalboard/lib/reviews.ts:49 and evalboard/lib/runs.ts:624. Run it as a second matrix entry on the existing analyze job (strategy.matrix.language: [python, javascript-typescript]); no build step is needed for JS/TS. Prevents: The whole traversal cluster: raw ?v= reaching readTaskReview's path.join unsanitized (app/runs/[id]/[...task]/page.tsx:62-67, reported by axes 2/4/6/7), and isValidId admitting ./.. so /api/download?run=..&task=..&v=.. escapes RUNS_DIR (evalboard/lib/runs.ts:609 + lib/blob.ts:17, axes 1/2/4/6). Both are dataflow-from-request-to-fs-path, the canonical CodeQL JS query — no bespoke rule needed.
  • [pyright] Flip noUnusedLocals + noUnusedParameters in evalboard/tsconfig.json. Verified free: npx tsc --noEmit --noUnusedLocals --noUnusedParameters on the current tree emits 0 errors, so this is a one-line config change with no migration. (The type-checker bucket for this repo's TS package — tsc --noEmit is evalboard's pyright.) Prevents: The Axis-3 download-route finding's exact verification: reverting collectTaskFiles(runId, taskId, variant) to collectTaskFiles(runId, taskId) in app/api/download/route.ts:26 orphans the variant local at line 20 and is currently flagged by NOTHING (no eslint in the package, noUnusedLocals off). With this flag the mutation becomes a compile error, i.e. a dropped-argument regression can no longer ship green.
  • [pyright] Make "validated path segment" a type, not a convention: export ONE resolver from evalboard/lib/runs.ts (export function resolveVariant(v: string | string[] | null | undefined): VariantSegment) returning a branded export type VariantSegment = string & { readonly __variantSegment: unique symbol }, with the s !== "." && s !== ".." guard that runs.ts:2234's safeSeg already carries but variantSegment (runs.ts:608-610) lacks. Then re-type every path-consuming parameter — readTaskReview(runId, variantId: VariantSegment, …) in lib/reviews.ts:43, taskContentBase, collectTaskFiles, ensureTaskDir — so passing a raw query string is a tsc error rather than a silent omission. Prevents: Axis-2 high / Axis-4 high / Axis-5 high simultaneously: the one consumer that skipped variantSegment() compiles cleanly today precisely because nothing in the type system distinguishes a raw ?v= string from a sanitized segment; the brand converts that omission into a build failure, and hoisting the guard into the single resolver also closes the ./.. hole for assertValidId, collectTaskFiles's runId check and clearRunCacheDir at once.
  • [pyright] Adopt noUncheckedIndexedAccess in evalboard/tsconfig.json — as a tracked migration, not a flip: measured 282 errors across the package today (npx tsc --noEmit --noUncheckedIndexedAccess, mostly lib/*.ts regex-match and array-index reads). Land it behind a scoped tsconfig (e.g. a tsconfig.strict.json including lib/status.ts, lib/runs.ts, lib/blob.ts first, widened file-by-file) so the invariant is checked where path/aggregation logic lives. Prevents: Axis-2 low let rep = group[0] in lib/status.ts:107 dereferenced without a guard (safe only by construction, invisible to the checker), and the same latent class in every other indexed read. Honest caveat: at 282 errors this is the one proposal here that is not a same-PR fix.
  • [ce-lint] CE034 — one DEFAULT_VARIANT literal. Grep gate (CE027 retired-token shape) over evalboard/{lib,app}/**/*.{ts,tsx} excluding __tests__: the bare literal "default" in a variant position may appear only at its single exported definition in lib/runs.ts (allowlist by file+line-context; comments exempt). Wire as a @pytest.mark.lint test class in tests/test_custom_lint.py alongside CE027-CE031 (it reasons over a non-Python file tree, so it is not a BaseRule). Numbering note: CE026, CE032 and CE033 are already claimed as proposals in .claude/harness-candidates.md — claim the next id unused in tests/lint/rules/ and let the uniqueness assert in tests/lint/runner.py arbitrate. Prevents: Axis-5 high (no single canonicalization seam): the literal is currently hardcoded at lib/status.ts:43, app/runs/[id]/[...task]/page.tsx:52 and :79, app/runs/[id]/task-grid.tsx:223, lib/blob.ts:276 and lib/runs.ts:603, and the "omit default from the URL" rule is written twice in two different shapes. Forcing every site through the exported const is what makes the branded-type proposal above enforceable rather than advisory.
  • [ce-lint] CE035 — Next.js searchParams keys must be typed string | string[] | undefined. Text/AST-lite rule over evalboard/app/**/page.tsx: any searchParams: Promise<{ … }> type literal whose members are declared k?: string fails, unless the value is normalized through a shared firstParam() helper. Next's generated page-props validator types searchParams as Promise<any>, so the narrower annotation is assignable and tsc --noEmit can never catch the lie — a lint rule is the only mechanical gate. Prevents: Axis-2 medium: ?v=a&v=b yields an array at runtime while typed string, reaching path.join in lib/reviews.ts:49 and throwing TypeError: The "path" argument must be of type string. Not a one-off — the same untrue annotation exists at app/page.tsx:100, app/trends/page.tsx:39, app/watchlist/page.tsx:15, app/path-to-ga/page.tsx:36, so the rule retires a repo-wide class.
  • [ce-lint] CE036 — no hand-built query strings in link builders. Forbid template literals under evalboard/app/**/*.tsx that splice an interpolation directly after ?k= / &k= (regex: `[^`]*[?&]\w+=\$\{), requiring new URLSearchParams() instead; exempt __tests__. ~15 lines, same test-class wiring as CE034. Prevents: Axis-1/Axis-3 low: app/runs/[id]/[...task]/page.tsx:78-81 emits a bare &v=… fragment that silently assumes its two consumers (lines 116 and 165) already emitted a ?, while app/runs/[id]/task-grid.tsx:221-225 does the same job correctly with URLSearchParams. The rule makes the correct shape the only shape and removes the latent break when a preceding param is dropped.
  • [ce-lint] CE037 — every route handler has a route test. Assert that each evalboard/app/api/**/route.ts has a sibling __tests__/route.test.ts. Pure filesystem parity check (~10 lines), precedent shape: CE028's index-parity class. Precedent for the test itself already exists at app/api/refresh/__tests__/route.test.ts. Prevents: Axis-3 medium: app/api/download/route.ts's entire new ?v= contract (read at line 20, passed at line 26) is untested — dropping the argument survives vitest, tsc, and (absent) lint, and would 404 every A/B download. app/api/file/route.ts is in the same state. A missing-test-file check is fully static; what those tests assert is the harness item below.
  • [ce-lint] CE038 — replicate-collapse field parity. Parse the TaskResultSummary interface in evalboard/lib/runs.ts and the object literal built by collapseReplicates in lib/status.ts; every field typed number | null must appear either in the averaged set or in an explicit, commented CARRIED_FROM_REPRESENTATIVE allowlist. Source-text parity in the CE030 (doc-schema-parity) style. Prevents: Axis-3 medium (5 of the 9 averaged columns have no assertion; a future column added to the type but not to the mean list silently takes the representative's value — the exact bug this PR fixes) and forces Axis-8 high (weightedScore averaged while the Status pill and detail link stay on the pass-first representative) to be a declared entry in one of the two lists rather than an implicit consequence of a spread.
  • [ce-lint] CE039 — API route contract doc parity. For each evalboard/app/api/**/route.ts, collect every url.searchParams.get("<k>") key and assert each appears in the route's documented contract line in evalboard/README.md. Same mechanical shape as CE027/CE030 (extract from source, require a doc mention), ~20 lines. Prevents: Axis-5 low: the download route gained ?v= while evalboard/README.md:45 still documents /api/download?run=<id>[&task=<id>], and README:37-39's "subdir name under <run-id>/default/" invariant is now false for every A/B run. The Python side already enforces this discipline for models (CE030); the TS package has no equivalent.
  • [ruff] The evalboard/ package has no linter at all — no eslint config, no lint script in package.json, nothing in .pre-commit-config.yaml or Makefile. Add a flat ESLint config (eslint-config-next + typescript-eslint with type-aware linting) as the missing counterpart of ruff check, enabling at minimum @typescript-eslint/no-unused-vars, @typescript-eslint/no-floating-promises, and eslint-plugin-security's detect-non-literal-fs-filename, and wire pnpm lint into the verify script. Prevents: Provides the standing gate under which several findings here would have been mechanical rather than human catches (the orphaned variant local in the download route; non-literal fs/path sinks in lib/runs.ts/lib/reviews.ts flagged for review). Honest scoping: the tsconfig flags above cover the unused-local case more cheaply and CodeQL covers the fs-sink case more precisely — the real value of this item is that the package currently has zero lint coverage of any kind, so the next defect class has nowhere to land.

Harness improvements (not statically reachable):

  • Run evalboard's own gate in CI and in make verify. evalboard/package.json already defines verify: tsc --noEmit && vitest run && next build, and nothing anywhere invokes it: grep -rn evalboard .github/workflows/ .pre-commit-config.yaml Makefile returns exactly one hit — a comment in pr-checks.yml:102 saying evalboard/pnpm-lock.yaml is out of scope. Add a evalboard-gate job (setup-node + pnpm install --frozen-lockfile + pnpm verify, working-directory: evalboard, paths: [evalboard/**]) and a make verify-evalboard target folded into make verify. Why not static: It is the execution vehicle for every static check proposed above, not itself a rule: tsc --noEmit, the new tsconfig flags and any eslint config are inert until something runs them. Every 'verified clean, exit 0' / 'suite stays green' claim in this review came from a reviewer running these commands by hand on a scratch worktree. Prevents: Prerequisite for the tsconfig/eslint/CE proposals; and the whole Axis-3 cluster (findings 8-13) is currently invisible to CI because the suite that would host those tests is never executed on a PR.
  • Mutation smoke on the variant + collapse seams. Add StrykerJS (or a scripted mutation harness) scoped to evalboard/lib/status.ts, the variant helpers in lib/runs.ts (variantSegment, taskContentBase, the readTaskDetail/readTaskReplicates row filters) and lib/blob.ts::ensureTaskDir, with a per-file survived-mutant budget, run nightly rather than per-PR. Why not static: Requires executing the suite against mutated builds. Line coverage alone would not have caught these — the lines ARE executed by the new tests, they are simply unasserted, which only a killed/survived signal distinguishes. Prevents: Every Axis-3 finding was proven by exactly this method and every mutant survived: the legacy/default row filter (runs.ts:1865-1869), 5 of 9 averaged columns in collapseReplicates, ensureTaskDir's variant blob prefix + dedupe key, the download route's ?v= argument, and the resolveSafePath prefetch branch.
  • Coverage thresholds for evalboard, per-directory. @vitest/coverage-v8 is installed but vitest.config.ts configures no coverage block and no thresholds at all, while the Python side enforces --cov-fail-under=80. Add coverage.thresholds with a global floor plus stricter per-glob floors for lib/** and app/api/**. Why not static: Coverage is a runtime measurement of the executed suite. Prevents: Axis-3 medium items where whole new surfaces shipped with zero executed lines — app/api/download/route.ts's query wiring and lib/blob.ts::ensureTaskDir (lib/__tests__/blob.test.ts only exercises isValidId/isValidTaskId).
  • A blob-mode test double. ensureTaskDir and ensureRunSummary both start with if (LOCAL_RUNS_DIR) return; (lib/blob.ts:164, :281), and every test in the package runs with EVALBOARD_LOCAL_RUNS_DIR stubbed — so the entire production storage path is unexecuted by construction. Introduce an injectable container client (fake listBlobsFlat/downloadBlob) and assert the exact blob prefix per variant plus the dedupe-key composition. Why not static: Needs a fake network/storage client and executed control flow; the prefix string is assembled at runtime from three components. Prevents: Axis-3 medium: mutating the prefix to ${runId}/WRONG-${variant}/${taskId}/ and dropping ${variant} from the dedupe key both leave the suite green, i.e. artifacts silently never download in production blob mode (and variants collide in the dedupe cache) while CI is green.
  • A shared golden run-tree fixture covering all three on-disk layouts — legacy rows with no variant_id, single-config rows with variant_id: "default", and a genuine multi-variant A/B — checked into evalboard/lib/__tests__/fixtures/ and regenerated by a make target that runs the Python writer, with every reader test (readTaskDetail, readTaskReplicates, readLogTail, readConversationLog, collectTaskFiles) parameterized over all three. Why not static: The contract spans two languages and a directory tree: Python's EvaluationResult.variant_id (src/coder_eval/models/results.py:473) and the <run>/<variant>/<task>/<NN>/ layout are consumed by TS readers, and no type system sees both ends. Prevents: Axis-3 high (the legacy/default compat claim at runs.ts:1859-1869 is asserted only negatively — the new fixture gives every row a variant_id and creates no default/ dir) and it is the fixture Axis-7 high would have failed against.
  • A cross-surface link-resolution test: enumerate every in-repo builder of a /runs/<id>/<task> href (app/runs/[id]/task-grid.tsx:158, app/trends/trends-view.tsx:267, app/watchlist/watchlist-view.tsx:25, app/runs/[id]/activation/page.tsx:233) and assert each emitted URL resolves under the detail page's actual row-matching rules for a multi-variant run. Why not static: Requires rendering the producers and evaluating the consumer's run.json row filter — a producer/consumer agreement across component boundaries that no type expresses. Prevents: Axis-7 high: bare (?v=-less) task URLs hard-404 on any run whose variant_id is not literally default — i.e. every real in-repo experiment (sonnet/opus, with-deny, with-plugin, terse/detailed) plus every existing bookmark, while the PR's own test encodes the 404 as intended behavior.
  • Property tests for the display formatters in lib/format.ts / lib/turns.ts: for any finite input, the rendered string must match a bounded-precision shape (≤2 decimals, or ≤1 with a k/M suffix). Pair with a grid-level render test that a 2-replicate task shows the mean cost and a fractional turn count. Why not static: Needs the formatter evaluated over an input domain; the defect is in the output string, and the field types (number | null) cannot express "integer count". Prevents: Axis-2 medium: fmtCompact's tail branch is return String(n) (lib/format.ts:41), so an averaged sub-1k token bucket renders as "500.6666666666667". The author hit the same hazard on turns and patched only fmtTurnsCount; a property test covers the family rather than the instance.
  • Kill positional cell indices in grid tests: turnsCellFor hardcodes cells[5] against a comment-documented column layout (app/runs/[id]/__tests__/task-grid.test.tsx:51-52). Replace with a helper that derives the index from the rendered <th> text, and add the missing same-model-different-variant case. Why not static: The coupling is between a literal and a runtime-rendered DOM whose column set is conditional (showModel, showTokens). Prevents: Axis-3 low (the new Model column shifts Turns to index 6 whenever a run has >1 distinct model, so a future multi-model turns assertion silently reads the Cost cell) and Axis-8 high (same-model A/B runs render N visually identical unlabeled rows — untested because the only Model-column tests use differing models).

Top 5 Priority Actions

  1. Resolve the score/status incoherence in collapseReplicates (/Users/religa/src/coder_eval/evalboard/lib/status.ts:119): the row spreads a pass-first representative but overwrites weightedScore with the mean, so a 2-replicate task (SUCCESS 1.0 / FAILURE 0.2) renders "Passed" beside Score 0.60 and links to a detail page showing 1.00 — either keep the score on the representative or make the pill/label aggregate-derived, and round the count-typed means (totalTurns/inputTokens/outputTokens/cache buckets, status.ts:124-131) so fmtCompact's String(n) tail branch stops rendering "500.6666666666667".
  2. Fix the hard-404 regression on bare task URLs (/Users/religa/src/coder_eval/evalboard/app/runs/[id]/[...task]/page.tsx:52 with the new row filter at /Users/religa/src/coder_eval/evalboard/lib/runs.ts:1865): const variant = v ?? "default" matches zero rows on any run whose variants are sonnet/opus/with-skill/etc., so derive the variant from run.json when ?v= is absent (unambiguous single-variant tasks resolve; only genuinely multi-variant tasks require the param) and thread ?v= through the three in-repo builders that still emit bare hrefs — task-grid.tsx:158, trends-view.tsx:267, watchlist-view.tsx:25.
  3. Close the traversal in the shared validator by rejecting dot-only segments in isValidId (/Users/religa/src/coder_eval/evalboard/lib/blob.ts:23), since ID_RE = /^[\w.-]+$/ matches .. and makes variantSegment's "collapses to default" comment (/Users/religa/src/coder_eval/evalboard/lib/runs.ts:609) false — GET /api/download?run=..&task=..&v=.. was verified to read outside the run dir — and add a realpath containment check to taskContentBase before walkArtifacts, plus negative tests for bare .., ., and runId="..".
  4. Collapse the four ad-hoc variant-canonicalization rules into one exported, guarded seam (resolveVariant/DEFAULT_VARIANT out of /Users/religa/src/coder_eval/evalboard/lib/runs.ts:603-610, ideally returning a branded VariantSegment) and route every consumer through it — notably /Users/religa/src/coder_eval/evalboard/app/runs/[id]/[...task]/page.tsx:64, the one caller that hands the raw query value to readTaskReview's path.join (lib/reviews.ts:49), which also 500s on a repeated ?v=a&v=b because searchParams is mistyped as v?: string (page.tsx:36).
  5. Kill the mutation-surviving test gaps on the weakest axis — assert all nine averaged columns in collapseReplicates plus a numeric-key parity guard (/Users/religa/src/coder_eval/evalboard/lib/tests/status.test.ts:34), add a legacy fixture whose rows omit variant_id so readTaskDetail/readTaskReplicates/readLogTail resolve the default/ layout (/Users/religa/src/coder_eval/evalboard/lib/tests/variant-paths.test.ts), cover the download route's ?v= wiring (/Users/religa/src/coder_eval/evalboard/app/api/download/route.ts:26) and ensureTaskDir's variant blob prefix/dedupe key (blob.ts:282,296), and add a same-model A/B case — since showModel gates on distinct model (/Users/religa/src/coder_eval/evalboard/app/runs/[id]/task-grid.tsx:537) while rows split on variant, a skill-on/skill-off run renders two identical unlabeled rows per task and should gate on distinct variant instead.

Stats: 0 🔴 · 7 🟠 · 6 🟡 · 8 🔵 across 8 axes reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants