ci: exclude measured-unstable questions from the KaiBench regression gate

[jordanrburger]

Follow-up to #641.

Why

The gate #641 added compares a single trial per question and fails a PR on any pass→fail flip. That only works if questions are deterministic. Q12 is not — it passes about half the time against an unmodified server:

	Q12, 10 trials	passed
main	[0,1,0,0,1,0,1,1,1,1]	6/10
an unrelated branch	[1,1,0,1,0,0,0,0,0,1]	4/10

Fisher exact p = 0.66 — the sides are indistinguishable, so the instability belongs to the question, not to any change. Q12 is a six-value all-or-nothing set_comparison over a multi-source join whose date-overlap semantics, weekday/weekend boundary, and aggregation grain are all undefined; Kai picks a different defensible reading per run and one of six values drifts.

Left as-is, the gate throws a spurious red check on roughly half the PRs it runs on. A gate that cries wolf at that rate gets learned as noise and ignored — which costs more than the coverage it provides. This already happened once during #640's validation: a single-trial run appeared to show a Q12 regression, and only --repeat disproved it.

What

• New .github/kaibench-flaky-questions.txt — one question ID per line, each with the measurement that justifies it. A question qualifies only once measured unstable against an unmodified server, not merely observed failing once. Currently one entry: 12.
• kaibench-parse-results.py splits regressions into regressions (gating) and flaky_regressions (reported only), and now emits the offending question IDs for both.
• The gate warns on every run that ignores something, naming the questions and the file. Coverage given up here stays visible rather than quietly shrinking what the check verifies.
• Real regressions now name the questions too — the previous message said only how many.

Putting the list in a reviewable file rather than a script constant is deliberate: exclusions should be easy to audit and easy to delete, and each one should have to justify itself in writing.

Verification

Parse script against fixtures:

fixture	result
Q12 + R01 both regress	regressions=1 regressed_qids=R01, flaky_regressions=1 flaky_regressed_qids=12
Q12 only	regressions=0, flaky_regressions=1

Gate script across all five paths:

scenario	exit	behaviour
flaky-only regression	0	passes, warns naming Q12
real regression	1	fails, names R01
both	1	fails on R01, still warns about Q12
clean	0	silent pass
no baseline	0	passes with the existing loud warning

Both workflows parse; the parse script compiles.

Follow-up

The right fix is making Q12 deterministic rather than excluding it. Tracked in keboola/KaiBench#80, which proposes modelling the missing business definitions in the semantic layer — a labor_cost_per_checkin metric with a fixed grain, a day_type dimension, and the co-covered-period rule as a constraint. That also upgrades what Q12 tests: from "did Kai guess the author's business rules" to "did Kai find and use the governed definition." This exclusion comes out at that point.

Worth noting the gate remains single-trial for everything else. That is fine for questions measured stable (R01–R08 were 3/3 on both sides), but the underlying limitation stands — a question that silently becomes flaky will produce false regressions until someone measures it. Running the gate at --repeat 3 and failing only on a majority-of-trials regression would be the more robust design, at roughly 3× the runtime and spend per gated PR.

🤖 Generated with Claude Code

[jordanrburger]

Folded in: questions/repeat dispatch inputs — plus a hazard they exposed

The inputs

Added questions and repeat dispatch inputs to kaibench.yml. These are how the Q12 instability justifying the exclusion above was measured in the first place — targeting one question and running it ten times per side. They're also a prerequisite for ever running the gate at --repeat N, which is the more robust design noted in the PR description.

Job timeout raised 60 → 120 minutes: --repeat runs trials sequentially, and ten trials of a slow question (Q12 takes 118–247s each) came uncomfortably close to the old cap.

The hazard they expose

Targeted runs create a problem the gate could not previously see, and it is already live in this repo.

The baseline lookup takes the most recent successful KaiBench artifact. The two most recent artifacts here are now single-question runs (the Q12 × 10 measurements). So the next full 33-question gated evaluation would:

1. select a 1-question baseline,
2. find exactly one shared question — which the exclusion list now skips anyway,
3. report regressions=0 with a non-empty baseline_run, so no warning fired,
4. go green having compared essentially nothing.

That is the same silent-pass failure mode baseline_run was added in #641 to prevent, reached by a different route. A valid artifact is not the same thing as a comparable baseline.

The guard

The parse script now emits baseline_overlap (percent) and baseline_shared (count), and the gate warns when coverage is under 50%:

Compared against baseline run: run_old (covers 1 of 25 questions, 4%)
::warning::Baseline covers only 4% of this run's questions (1/25) — most questions were
NOT compared against anything. Treat a pass here as unverified; run the full suite on
main to establish a comparable baseline.

A real regression inside a thin overlap still fails the check — a true positive is a true positive regardless of how narrow the comparison was. What changes is that a pass can no longer be mistaken for full coverage.

Verified against a fixture reproducing the exact situation above (25-question run vs a Q12-only baseline → baseline_overlap=4, baseline_shared=1), and across three gate paths:

scenario	exit	behaviour
thin baseline (4%), clean	0	passes, flagged unverified
thin baseline (4%) + real regression	1	still fails, names the question
full baseline (100%), clean	0	passes quietly, no warning

Both workflows parse; the parse script compiles.

Note on threshold choice

50% matches the overlap requirement KaiBench already applies to its own baseline comparability rules, so the two agree rather than each inventing a number. It's a warning rather than a hard failure because refusing to compare at all would be a worse outcome than comparing partially and saying so.

[tomasfejfar]

Review: three things to fix before merge

Context for whoever reads this later. This gate compares two runs: a baseline (a previous successful run's results.jsonl, downloaded into prev-results/) and the current run. A regression is one question that said passed in the baseline and says something other than passed/skipped now. More than zero regressions turns the check red.

Everything below is about that comparison being trustworthy. Three separate things make it untrustworthy right now.

1. The new dispatch inputs poison the baseline for the next PR

find-prev-run takes the newest kaibench-results-* artifact from any successful run. It does not care what that run covered. So the moment you dispatch questions: 12, repeat: 20 — which is exactly the run kaibench-flaky-questions.txt asks for before an entry is added — that run's artifact becomes the baseline for the next labelled PR. The PR then compares 20 questions against a baseline holding 1, finds nothing, and goes green.

The branch spots this and adds a warning when baseline_overlap < 50. A warning does not stop a merge. So we ship the tool that breaks the gate, plus a note saying the gate may be broken.

Fix it where the baseline is chosen, not after the fact. Detail in the line comment on kaibench.yml. That also deletes about 20 lines of plumbing whose only job is to measure the damage.

2. repeat breaks the counting in two places, in opposite directions

The scripts assume one row in results.jsonl = one question. --repeat 20 writes 20 rows for the same question, and both places that assumption is load-bearing break:

• the baseline dict silently keeps 1 of N rows, so it hides a real regression (fail-open)
• the regression loop silently counts N rows as N regressions, so it reports one question as several (fail-closed)

Both verified by running the script against synthetic payloads. Details in the two line comments on kaibench-parse-results.py.

The real fix is to give the code the concept it lacks: a question's verdict across its trials. Then both bugs go away by construction, and "what does 2-out-of-3 mean?" is answered in one place, on purpose, instead of emerging from dict insertion order.

3. A bad input value fails silently — the exact trap documented six lines below it

The block that builds --question/--repeat sits under continue-on-error: true with no exit-code check, six lines above the comment recording that --regression-only "is rejected by typer as an unknown option, and because this step is continue-on-error it failed silently." A wrong flag name now means: no results, empty outputs, ${VAR:-0} fallbacks, and a green "No regressions." after a two-hour job. The tokenising also has no guards — I reproduced --question '' from three different inputs. Detail in the line comment.

Suggested order

1. Artifact-name fix. Kills #1, and removes the need to decide what a multi-trial baseline even means.
2. Dedupe the regression count. Cheap, and it is a live trap for whoever wires repeat through workflow_call later.
3. Input validation + drop continue-on-error on the eval step.
4. Baseline aggregation or refusal as belt and braces behind step 1.

Separately, and needed regardless: this branch is 20 commits behind main, and its pyproject.toml is at 1.74.0 against 1.74.3 on main. It needs a rebase onto origin/main, then a version bump plus uv lock, per CLAUDE.md.

[tomasfejfar]

These two inputs need to make the run ineligible as a baseline.

Context: find-prev-run (line ~270) looks for artifacts named kaibench-results-*, takes the newest one whose run concluded success, and downloads it into prev-results/. Nothing anywhere records what a run covered, so a run that measured one question looks exactly like a run that measured all of them.

What happens step by step:

1. You dispatch questions: 12, repeat: 20 to find out whether Q12 is unstable — the run kaibench-flaky-questions.txt asks for before an entry gets added.
2. The run succeeds and uploads kaibench-results-<run_id> (line 337).
3. Tomorrow a PR gets the run-kaibench label. find-prev-run picks that artifact, because it is the newest.
4. The PR runs 20 questions. prev_by_qid holds one. The other 19 are simply not in it, so they are skipped with no comment.
5. regressions=0 → green check.

The check verified 1 question out of 20 and reported that nothing was wrong.

Suggested fix — one line, and no new logic, because find-prev-run already filters on the name prefix:

# line 337
name: kaibench-${{ (inputs.questions == '' && inputs.repeat == '' && inputs.regression_only != true) && 'results' || 'adhoc' }}-${{ github.run_id }}

Narrowed runs then upload as kaibench-adhoc-… and drop out of candidacy on their own. On the workflow_call route all three inputs are empty or false, so the PR gate and the release route behave exactly as they do today. And ci.yml already has an honest branch for "nothing eligible was found" — it warns that a pass proves nothing.

Why this rather than filtering candidates on .event != workflow_dispatch: a full-suite manual dispatch is a perfectly good baseline and should stay eligible. The thing that actually matters is whether the run was narrowed, so key on that instead of on how it was triggered.

[tomasfejfar]

This becomes redundant once narrowed runs cannot be baselines.

These two variables exist to measure a problem after it has already happened. They are computed here, printed as two outputs (lines 90-91), declared as two job outputs in kaibench.yml (58-59), passed as two env vars in ci.yml (198-199), and turned into a -lt 50 warning in ci.yml (218-220). Five layers of plumbing to emit a log line that does not stop a merge. Fixing baseline selection instead deletes all of it.

Two problems while it is still here.

The guard only catches one of the two narrowing shapes. A questions: 12 dispatch produces a thin overlap, so the warning fires. A repeat: 10 dispatch has the same question set, so overlap is ~100% and no warning fires at all — and it is the more dangerous baseline of the two, because of the last-row-wins problem on line 71.

baseline_overlap is printed against a different denominator than it was computed from. Here it divides by len(candidate_qids): unique, non-skipped questions. Both messages in ci.yml print it against $TOTAL, which is summary.metrics.total_questions — that counts skipped questions, and with repeat it counts trials. A 20-question run with repeat: 2 prints:

Compared against baseline run: run_B (covers 20 of 40 questions, 100%)

Self-contradicting on its face. Any run with a skipped question does the same thing on the PR route today.

If you keep the guard rather than fixing selection: export the denominator instead of the percentage (baseline_candidates=len(candidate_qids)) and divide in the shell. One denominator, one source of truth, one fewer output.

[tomasfejfar]

Delete this together with the selection fix — see the comment on kaibench.yml line 28.

Two notes while it exists.

Wrong denominator. $TOTAL is summary.metrics.total_questions, but BASELINE_SHARED/BASELINE_OVERLAP were computed over unique non-skipped questions. The two halves of that sentence count different things, so with any skipped question the line reads like "covers 5 of 20 questions, 63%". See the comment on kaibench-parse-results.py line 53.

A warning is not a guard. The comment on 215-217 says: "Real regressions inside a thin overlap are still worth failing on, but a pass must not read as full coverage." The first half is right and already works. The second half is implemented as ::warning::, which does not change the check's colour — so the PR still merges on a baseline that verified almost nothing. If a thin baseline genuinely means a pass proves nothing, the honest behaviour is exit 1. Better still is never selecting that baseline, which is what the kaibench.yml comment proposes.

[tomasfejfar]

Fail-open: with a multi-trial baseline this keeps whichever trial happens to be last in the file.

Plain =, not append. Ten rows for question 12 means this line runs ten times and the dict keeps only the tenth. Nine measurements are thrown away, and which one survives is decided by nothing more meaningful than line order.

Concretely. Question 5 is genuinely borderline on main, and a measurement run recorded:

trial	result
1	passed
2	passed
3	failed

A PR picks that run as its baseline, and in the PR's own run question 5 fails outright:

prev_by_qid['5'] = trial 3    → 'failed'
current q5                    → 'failed'
line 79: prev == 'passed'?    → no
→ not counted

regressions=0. Green check. A question that passed 2 out of 3 times on main and now fails every time is recorded as "no change". I confirmed this by running the script against exactly that payload.

Note the direction is not even stable. Had the trials landed failed, failed, passed, the baseline would read passed and the same PR would be blocked. Same data, different order, opposite verdict.

Why this one is the urgent bug: a gate can be wrong in two ways. Fail closed blocks a PR it should not — annoying, but you find out at once and nothing bad ships. Fail open passes a PR it should have blocked — you find out later, or never. This fails open.

And it is reachable through the normal workflow, not a corner case: the measurement run described in kaibench-flaky-questions.txt is what puts multi-trial rows into prev-results/ in the first place.

Fixes, in order of preference:

1. Stop narrowed and repeated runs from becoming baselines (comment on kaibench.yml line 28). Then multi-trial data never reaches this line and no aggregation policy is needed at all.
2. Belt and braces: if the baseline file contains duplicate question ids, set baseline_run = ''. That routes into the existing "no comparison was made" branch, which already warns that a pass proves nothing. Better to say "I cannot compare these" than to quietly guess.
3. If you do want to compare: aggregate on purpose — passed if all(t == 'passed' for t in trials), or any(...). Either is defensible. Silently keeping the last row is not.

[tomasfejfar]

One question counted once per trial.

This is the mirror image of the line 71 problem. There, N rows collapse into 1. Here, 1 question expands into N counts. prev_by_qid holds one entry per question, but this loop walks rows — so every failing trial increments the counter and appends the same id again.

Baseline had question 5 passing; the current run uses repeat: 3 and question 5 fails all three times:

regressions=3
regressed_qids=5,5,5

and the gate prints:

::error::3 question(s) regressed vs the previous run: 5,5,5

One question, reported as three. Confirmed by running the script.

Two reasons this is less urgent than line 71, so you can schedule it rather than rush it:

• It fails closed. It inflates the count, so it errs toward blocking. Wrong and loud beats wrong and quiet.
• It cannot reach a PR check today. repeat is declared only under workflow_dispatch; the workflow_call: block on line 38 declares no inputs, and ci.yml calls this workflow with no with: at all. So on the PR route inputs.repeat is always empty and one-row-per-question still holds.

It becomes a live gating bug the moment somebody plumbs repeat through workflow_call — a two-line change that is easy to make without knowing any of this.

Minimal fix: count questions, not rows. sorted(set(regressed_qids)) with a numeric key, and len() of that for the count. (Related: line 87's plain sorted() on id strings gives 10,12,2,9. Both sibling scripts already carry a numeric-aware sort_key — this would be a third, worse ordering for the same identifiers.)

Better fix, which also solves line 71 — build a per-question verdict for both sides first, then compare sets:

prev_trials, curr_trials = defaultdict(list), defaultdict(list)
...
regressed = {q for q in curr_trials
             if verdict(prev_trials.get(q)) == 'passed'
             and verdict(curr_trials[q]) != 'passed'}
regressions = len(regressed)

verdict() is then the single place where "what does 2-out-of-3 mean?" gets answered deliberately, instead of emerging from dict insertion order.

[tomasfejfar]

No validation, and a wrong value fails silently.

Six lines below this, on 251-252, is the lesson from last time:

the CLI option is --regression (see kaibench/cli.py); --regression-only is rejected by typer as an unknown option, and because this step is continue-on-error it failed silently

This block adds two more flags — --question and --repeat — under identical conditions: continue-on-error: true on line 257, and no check of the exit code anywhere. If either name is wrong, the eval writes no results, kaibench-parse-results.py hits raise SystemExit(0) on line 6, every job output comes back empty, and the ${VAR:-0} fallbacks in ci.yml turn that into a green "No regressions." after a two-hour job. The echo "Running: …" on 255 shows a human the command, but it does not make the job fail.

I could not verify that --question and --repeat exist in keboola/KaiBench's CLI — the repo is private and not vendored here — so this trap is live right now. Worth one check before merge.

Separately, the tokenising has no guards. I ran this block as-is:

questions input	resulting args
" "	--question ''
12,,13	--question 12 --question '' --question 13
,12	--question '' --question 12

repeat is type: string, so 0, abc and " " all pass straight through to --repeat.

Minimal fix:

for q in "${QIDS[@]}"; do
  trimmed=$(echo "$q" | xargs)
  [ -n "$trimmed" ] || continue                 # skip empty tokens
  CMD_ARGS+=(--question "$trimmed")
done
if [ -n "$REPEAT" ]; then
  [[ "$REPEAT" =~ ^[1-9][0-9]*$ ]] || { echo "::error::repeat must be a positive integer, got '$REPEAT'"; exit 1; }
  CMD_ARGS+=(--repeat "$REPEAT")
fi

And on line 256, stop the silence:

uv run kaibench run "${CMD_ARGS[@]}" || { echo "::error::kaibench run exited non-zero — check the flags above"; exit 1; }

continue-on-error: true on 257 is not buying anything here: every downstream step is already if: always(), and the job-level continue-on-error on line 45 covers the non-blocking case. Dropping it converts a silent green into a visible failure.

(While you are in the area — line 45's github.event_name == 'workflow_call' is never true. Inside a reusable workflow the github context belongs to the caller, so event_name is pull_request or push. Pre-existing, and a separate fix.)

[Matovidlo]

@jordanrburger ping