Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jordanrburger wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8df6bc
ci: exclude measured-unstable questions from the KaiBench regression …
Jul 29, 2026
e0a7642
ci: add questions/repeat dispatch inputs and guard against thin basel…
Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/kaibench-flaky-questions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Questions excluded from the KaiBench regression gate.
#
# A question belongs here only when it has been MEASURED as unstable against an unmodified
# server — not merely observed failing once. The gate compares a single trial per question,
# so a question with a base pass rate meaningfully below 100% produces phantom regressions
# at that rate and trains reviewers to ignore the check.
#
# Exclusions are reported as warnings on every gated run, so coverage lost here stays
# visible rather than silently narrowing. Remove an entry once the underlying question is
# made deterministic.
#
# Format: one question ID per line. Blank lines and #-comments ignored.

# ~50% pass rate on unmodified main (6/10 vs 4/10 on a branch, Fisher exact p=0.66 — the
# instability is the question's own, not any PR's). Six-value all-or-nothing set_comparison
# over an underspecified multi-source join: date-overlap semantics, weekday/weekend boundary,
# and aggregation grain are all undefined. Tracked in keboola/KaiBench#80 as the reference
# case for the semantic layer; remove this entry once those definitions are modelled.
12
36 changes: 35 additions & 1 deletion .github/scripts/kaibench-parse-results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,31 @@
status = 'passed' if m['failed'] == 0 and m.get('errors', 0) == 0 and partial_count == 0 else 'failed'
print(f"status={status}")

# Questions measured as unstable against an unmodified server. A single-trial comparison on a
# question whose base pass rate is well under 100% produces phantom regressions at that rate, so
# these are counted separately instead of failing the gate. Reported, never silently dropped.
flaky_path = Path(__file__).parent.parent / 'kaibench-flaky-questions.txt'
flaky_qids = set()
if flaky_path.exists():
for raw in flaky_path.read_text().splitlines():
entry = raw.split('#', 1)[0].strip()
if entry:
flaky_qids.add(entry)

# Count regressions vs previous run (downloaded into prev-results/)
# `baseline_run` stays empty when no comparison happened, so callers can tell "0 regressions"
# apart from "never compared" — the two look identical otherwise.
regressions = 0
regressed_qids = []
flaky_regressions = 0
flaky_regressed_qids = []
baseline_run = ''
# Share of this run's questions the baseline actually covers. A targeted run (say a single
# question dispatched with --questions) produces a perfectly valid artifact that nonetheless
# makes a near-empty baseline, which would otherwise yield "0 regressions" and a green check
# while verifying almost nothing.
baseline_overlap = 0
baseline_shared = 0
Comment on lines +53 to +58

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

prev_runs = sorted(Path('prev-results').glob('run_*'), key=lambda p: p.stat().st_mtime) if Path('prev-results').exists() else []
if prev_runs:
prev_file = prev_runs[-1] / 'results.jsonl'
Expand All @@ -49,10 +69,24 @@
except json.JSONDecodeError:
continue
prev_by_qid[str(pr.get('question_id', ''))] = pr

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

candidate_qids = {str(r.get('question_id', '')) for r in evaluated}
baseline_shared = len(candidate_qids & set(prev_by_qid))
if candidate_qids:
baseline_overlap = round(100 * baseline_shared / len(candidate_qids))
for r in evaluated:
qid = str(r.get('question_id', ''))
if qid in prev_by_qid:
if prev_by_qid[qid].get('status') == 'passed' and r.get('status') not in ('passed', 'skipped'):
regressions += 1
if qid in flaky_qids:
flaky_regressions += 1
flaky_regressed_qids.append(qid)
else:
regressions += 1
regressed_qids.append(qid)
Comment on lines 76 to +85

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

print(f"regressions={regressions}")
print(f"regressed_qids={','.join(sorted(regressed_qids))}")
print(f"flaky_regressions={flaky_regressions}")
print(f"flaky_regressed_qids={','.join(sorted(flaky_regressed_qids))}")
print(f"baseline_overlap={baseline_overlap}")
print(f"baseline_shared={baseline_shared}")
print(f"baseline_run={baseline_run}")
20 changes: 18 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,12 @@ jobs:
env:
EVAL_RESULT: ${{ needs.kaibench.result }}
REGRESSIONS: ${{ needs.kaibench.outputs.regressions }}
REGRESSED_QIDS: ${{ needs.kaibench.outputs.regressed_qids }}
FLAKY_REGRESSIONS: ${{ needs.kaibench.outputs.flaky_regressions }}
FLAKY_REGRESSED_QIDS: ${{ needs.kaibench.outputs.flaky_regressed_qids }}
BASELINE_RUN: ${{ needs.kaibench.outputs.baseline_run }}
BASELINE_OVERLAP: ${{ needs.kaibench.outputs.baseline_overlap }}
BASELINE_SHARED: ${{ needs.kaibench.outputs.baseline_shared }}
PASSED: ${{ needs.kaibench.outputs.passed }}
TOTAL: ${{ needs.kaibench.outputs.total }}
PASS_RATE: ${{ needs.kaibench.outputs.pass_rate }}
Expand All @@ -206,9 +211,20 @@ jobs:
echo "::warning::No baseline artifact available, so no regression comparison was made — this check passing does NOT mean the PR is regression-free"
exit 0
fi
echo "Compared against baseline run: $BASELINE_RUN"
echo "Compared against baseline run: $BASELINE_RUN (covers ${BASELINE_SHARED:-?} of $TOTAL questions, ${BASELINE_OVERLAP:-?}%)"
# A targeted run (dispatched with `questions`) yields a valid artifact that is nonetheless a
# near-empty baseline. Real regressions inside a thin overlap are still worth failing on, but a
# pass must not read as full coverage.
if [ "${BASELINE_OVERLAP:-0}" -lt 50 ]; then
echo "::warning::Baseline covers only ${BASELINE_OVERLAP:-0}% of this run's questions (${BASELINE_SHARED:-0}/$TOTAL) — most questions were NOT compared against anything. Treat a pass here as unverified; run the full suite on main to establish a comparable baseline."
fi
Comment on lines +214 to +220

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

# Surface excluded questions every time, so coverage given up here stays visible
# instead of quietly shrinking what this check actually verifies.
if [ "${FLAKY_REGRESSIONS:-0}" -gt 0 ]; then
echo "::warning::Ignored $FLAKY_REGRESSIONS regression(s) on known-unstable question(s): ${FLAKY_REGRESSED_QIDS} — these are excluded via .github/kaibench-flaky-questions.txt and are NOT verified by this check"
fi
if [ "${REGRESSIONS:-0}" -gt 0 ]; then
echo "::error::$REGRESSIONS question(s) regressed vs the previous run — see the step summary for details"
echo "::error::$REGRESSIONS question(s) regressed vs the previous run: ${REGRESSED_QIDS} — see the step summary for details"
exit 1
fi
echo "No regressions."
Expand Down
30 changes: 29 additions & 1 deletion .github/workflows/kaibench.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,23 @@ on:
required: false
type: string
default: ''
questions:
description: 'Specific question IDs (comma-separated). Intersects with the other filters.'
required: false
type: string
default: ''
repeat:
description: 'Trials per question, for consistency/noise measurement (default 1)'
required: false
type: string
default: ''
Comment on lines +28 to +37

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

workflow_call:

jobs:
evaluate:
name: Run KaiBench evaluation
runs-on: ubuntu-latest
timeout-minutes: 60
timeout-minutes: 120
continue-on-error: ${{ github.event_name == 'workflow_call' }}
outputs:
status: ${{ steps.parse.outputs.status }}
Expand All @@ -41,7 +51,12 @@ jobs:
pass_rate: ${{ steps.parse.outputs.pass_rate }}
duration: ${{ steps.parse.outputs.duration }}
regressions: ${{ steps.parse.outputs.regressions }}
regressed_qids: ${{ steps.parse.outputs.regressed_qids }}
flaky_regressions: ${{ steps.parse.outputs.flaky_regressions }}
flaky_regressed_qids: ${{ steps.parse.outputs.flaky_regressed_qids }}
baseline_run: ${{ steps.parse.outputs.baseline_run }}
baseline_overlap: ${{ steps.parse.outputs.baseline_overlap }}
baseline_shared: ${{ steps.parse.outputs.baseline_shared }}

services:
postgres:
Expand Down Expand Up @@ -209,6 +224,8 @@ jobs:
KAIBENCH_EVAL_PARALLEL_WORKERS: '4'
KAIBENCH_EVAL_KAI_BACKEND_URL: http://localhost:3000
QUESTION_TYPES: ${{ inputs.question_types }}
QUESTION_IDS: ${{ inputs.questions }}
REPEAT: ${{ inputs.repeat }}
run: |
CMD_ARGS=()
if [ -n "$QUESTION_TYPES" ]; then
Expand All @@ -220,11 +237,22 @@ jobs:
else
CMD_ARGS=(-t "Data Analysis Query" -t "Configuration Reasoning" -t "Storage Object Reasoning" -t "MCP Tool Validation")
fi
if [ -n "$QUESTION_IDS" ]; then
IFS=',' read -ra QIDS <<< "$QUESTION_IDS"
for q in "${QIDS[@]}"; do
trimmed=$(echo "$q" | xargs)
CMD_ARGS+=(--question "$trimmed")
done
fi
if [ -n "$REPEAT" ]; then
CMD_ARGS+=(--repeat "$REPEAT")
fi
Comment on lines +240 to +249

@tomasfejfar tomasfejfar Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

if [ "${{ inputs.regression_only }}" = "true" ]; then
# 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
CMD_ARGS+=(--regression)
fi
echo "Running: kaibench run ${CMD_ARGS[*]}"
uv run kaibench run "${CMD_ARGS[@]}"
continue-on-error: true

Expand Down
Loading