Repository navigation
fix: fail incomplete task runs and add opt-in baseline regression gates - #300
vchamarthi wants to merge 7 commits into
Conversation
Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi baseline compare` parameters changed:
+ added option `--max-context-drop`
+ added option `--max-lift-drop`
+ added option `--min-matched`Updated on every push; the |
There was a problem hiding this comment.
🟡 Changes recommended
Incomplete panel votes can falsely pass baseline regression gates, while command harness runs with valid pass/fail verdicts are incorrectly rejected as unscored.
2 open issues
What changed in this PR
2 new findings.
This PR strengthens evaluation gates: incomplete task grids fail after saving artifacts, baseline comparisons gain opt-in regression thresholds, and undecided judge-validity results are labeled clearly.
Changes:
- Reject errored, unscored, missing, duplicate, or unplanned task cells while preserving supported dry runs.
- Add context-score and paired-lift regression gates requiring complete matched scores and known matching models.
- Display
INCONCLUSIVEjudge-validity results without changing exit policies, and update tests and documentation.
| File | Description |
|---|---|
dokimi/tests/test_tasks_command.py |
Tests completion checks and artifact preservation. |
dokimi/tests/test_judge_validity.py |
Updates inconclusive-result assertions. |
dokimi/tests/test_judge_reliability.py |
Checks the explicit inconclusive verdict. |
dokimi/tests/test_baseline_compare.py |
Tests regression thresholds and evidence requirements. |
dokimi/tests/outcomes/journeys.yaml |
Updates capability status and outcome evidence. |
dokimi/scripts/growth_baseline.json |
Lowers the reliability test file’s size baseline. |
dokimi/dokimi/eval/judge_validity.py |
Labels undecided results INCONCLUSIVE. |
dokimi/dokimi/commands/tasks.py |
Checks task-grid completeness after saving artifacts. |
dokimi/dokimi/commands/evaluate.py |
Updates inconclusive warnings and help text. |
dokimi/dokimi/commands/baseline.py |
Registers regression flags and enforces model checks. |
dokimi/dokimi/commands/baseline_regression.py |
Implements matched-score regression gates. |
dokimi/docs/user-journeys/journeys.md |
Updates gating capabilities and acceptance criteria. |
dokimi/docs/coding-harnesses.md |
Documents stricter task completion behavior. |
dokimi/docs/benchmarking-and-comparison.md |
Explains regression gates and inconclusive exit policies. |
dokimi/changelog.d/20261026_evaluation_completion.md |
Records completion, verdict, and regression-gate changes. |
Review effort: Balanced
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 93.50% 93.53% +0.03%
==========================================
Files 273 275 +2
Lines 35569 35709 +140
Branches 6040 6067 +27
==========================================
+ Hits 33260 33402 +142
+ Misses 1492 1491 -1
+ Partials 817 816 -1
🚀 New features to boost your workflow:
|
Reject panel scores with a failed vote in the regression gate, score the command harness exit-code fallback, print the gate verdict after the comparison output, keep FAIL visible on an undecided judge-validity failure, document the stricter model parsing, and tie the journey evidence to the exit codes. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
| and pair(*m.split("/", 1)) is not None for m in models)): | ||
| models = [] | ||
| judge = ("panel", ",".join(sorted( | ||
| normalize_model_ref(*m.split("/", 1)) if "/" in m else m for m in models))) if models else None |
There was a problem hiding this comment.
Dead branch: the check above empties models unless every member contains "/", so else m can never run. Drop the conditional (nit).
Suggestion:
| normalize_model_ref(*m.split("/", 1)) if "/" in m else m for m in models))) if models else None | |
| normalize_model_ref(*m.split("/", 1)) for m in models))) if models else None |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 003edba. The completion check and the baseline gates behave correctly in every case I ran, and the earlier bot points are fixed. Two of the new decisions are not pinned by any test, and there are three doc nits.
What I ran, on a Linux review host at 003edba:
make ci-local(with Docker): 34 passed, 0 failed, 6 skipped.- Mutation runs on a copy of the head against the new test files, plus hand-built
baseline compareandtasks runcases with the LLM transport mocked. 28 of 35 mutants of the gate code were killed; the survivors that decide a verdict or exit code are in the two inline comments.
Nits outside this diff:
- Nit:
.github/workflows/agent-quality.yml:42anddokimi/scripts/check_smoke_runs.py:7still saytasks run's exit code "does not reflect pass/fail" and that it is "used by callers that want partial results". Since this PR any errored, unscored or missing cell exits 1. Suggestion: reword both comments to say the exit code now flags incomplete runs and the explicit pass-rate check covers failed verification. - Nit:
changelog.d/20261021_180000_napetrov_outcome-completeness-2.md:3(unreleased) still says "partly errored runs keep exit 0". Both fragments land in one release, so the notes would state both rules. Suggestion: drop that clause from the older fragment. This PR's bullet also changes behaviour that CI jobs depend on, so### Changedfits better than### Fixed.
Checked, no issues
- Completion check: every non-error
reward=Nonerow inharnesses/*is a dry-run row or carriesmetrics.error, so real runs never hit "unscored" by accident. A nonexistent binary exits 1, exit code 3 is a scored 0 with exit 0, and--repeats 2with a command harness exits 0. - Gate logic:
>vs>=, lift vs context index, completeness on each side,--min-matched, Decimal handling at the boundary (6.9 vs 0.1) and duplicate question ids are all pinned by killed mutants. - Earlier bot points: Copilot's panel-vote and command-fallback points are fixed at this head and covered by tests.
- Repo rules: the schema, nav and growth-baseline checks pass, the commits all carry
Signed-off-by, and journeys.yaml evidence ids exist.
| if limit is None: | ||
| continue | ||
| drop = sum((base[key][index] - cur[key][index] for key in sorted(matched)), Decimal(0)) / len(matched) | ||
| failed |= drop > Decimal(str(limit)) |
There was a problem hiding this comment.
No test makes exactly one of the two gates fail, so a last-metric-wins bug would pass CI.
Mutation check: changing failed |= drop > Decimal(str(limit)) to failed = drop > Decimal(str(limit)) passes the full test_baseline_compare.py (49 passed). With that change, --max-context-drop 0 --max-lift-drop 10 on a run whose context score fell 9 -> 5 and whose lift fell 5 -> 0 has the lift check, evaluated last and passing, overwrite the context failure. The run would print Regression gate: PASS and exit 0 instead of 1. The existing tests use one flag, or two flags that both pass. The current code exits 1 for this input.
Suggestion: add a test with both flags where only the context drop exceeds its limit, and one where only the lift drop does, each expecting REGRESSION and exit 1.
| if isinstance(reward, bool) or not isinstance(reward, (int, float)): | ||
| return "unscored" | ||
| return "scored" if isfinite(reward) else "unscored" |
There was a problem hiding this comment.
The unscored rule for bool and non-finite rewards, and the repeat identity on --repeats > 1, have no test.
Mutation check, each passing all 30 tests in test_tasks_command.py:
return "scored" if isfinite(reward) else "unscored"->return "scored": aNaNreward counts as scored andtasks runexits 0.- dropping
isinstance(reward, bool) orat line 195:reward=Truecounts as scored. r.get("repeat", 1)->1at line 207: every complete--repeats 2run counts its second repeat as a duplicate and exits 1. A manual--repeats 2run with a command harness exits 0 today; only the missing-cell case usesrepeats=2in tests.
These decide the exit code of tasks run. Suggestion: add one _check_completion test per case: a NaN reward and a True reward each expecting exit 1, and a complete two-repeat grid expecting exit 0.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…hanged Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| import sys | ||
|
|
||
| _fixture_task(tmp_path) | ||
| harness = f'command:{Path(sys.executable).as_posix()} -c "{script}"' |
There was a problem hiding this comment.
Must-fix: quote the executable path before embedding it in the command. CommandHarness.build_command() uses shlex.split(), so an interpreter at C:/Program Files/Python/python.exe is split into C:/Program and Files/Python/python.exe. Both parameter cases then record an errored cell and fail this test despite correct completion behavior. Keep .as_posix() and quote the resulting path (as the other command-harness tests do).
Suggestion:
| harness = f'command:{Path(sys.executable).as_posix()} -c "{script}"' | |
| harness = f'command:"{Path(sys.executable).as_posix()}" -c "{script}"' |
There was a problem hiding this comment.
Nit: This is not a must-fix: the windows-smoke job runs only tests/test_cli_help.py and tests/test_console_encoding.py, so this test never runs on Windows in CI. Its neighbour in test_task_formats.py uses sh -c, which is POSIX-only as well. Quoting still helps, because a venv path with a space breaks this test on Linux too, so command:"{Path(sys.executable).as_posix()}" -c ... is a cheap fix.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 1de60da, which fix the three nits from my review:
1fc52af:dokimi/docs/benchmarking-and-comparison.md:487: states that an undecided result printsRESULT: INCONCLUSIVEabove the gate andRESULT: FAIL (INCONCLUSIVE, see above)below it.57e06f2:dokimi/changelog.d/20261021_180000_napetrov_outcome-completeness-2.md:3: dropped the stale "partly errored runs keep exit 0" clause, and moved this PR's task-run exit bullet under### Changed.1de60da:dokimi/scripts/check_smoke_runs.py:7: docstring now saystasks runexits 1 for an incomplete run, not for failed verification, which this gate adds.
make ci-local at 1de60da: 31 passed, 3 failed (host: gitleaks download, gitleaks scan, one timing-flaky test), 6 skipped.
Tests (py3.11, py3.14) fail in test_cli_flags.py::test_every_spelling_on_the_tree_is_canonical_or_allowlisted, and the mypy job fails at CLI snapshot is current. Both checks came with #254, which merged into main after my review: the new baseline compare flags (--max-context-drop, --max-lift-drop) are not in cli_flags.py or tests/data/cli_flags_allowlist.txt, and cli-interface.json lacks them. The fix commits touch only docs. Fix: allowlist the new flags with a reason (or use the registry spelling), then make cli-snapshot.
Not approving yet. Still open: the two risks from my review.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 1de60da. The completion check and the baseline gates work in the cases I ran. Two things need a change before merge: on current main the three new baseline compare flags fail the CLI flag registry check, and the command-harness exit-code fallback also turns an unreadable reward file into a scored 1.0. The rest is small and in the line comments.
What I ran, at 1de60da:
make ci-local --offline: 18 passed, 0 failed, 13 skipped (network-only jobs). It runs against the PR base, so it does not see #254.- This head merged with
main(e97ff7a):tests/test_cli_flags.pyfails 2 tests, the registry check (on--max-context-drop,--max-lift-dropand--min-matched) and the CLI snapshot. - The external-catalog smoke (
docker-oracle+docker-probeon the pinned task): 2/2 cells measured and passed, the isolation probe is closed. tasks runrepros with acommand:harness: areward.txtholding2.0givesreward 1.0, passed, exit 0; a repeated harness or task runs both cells, then exits 1 with "1 duplicate".
Checked, no issues
- Gate logic:
_scoreddrops errored rows, errored scores and incomplete panel votes on both sides. Coverage has to be complete and unique, the model check turns strict under a gate, and the gate runs after the comparison tables. - Judge validity: the
RESULT:line matches the exit code:INCONCLUSIVE (see above)only for a passing undecided result, andFAIL (INCONCLUSIVE, see above)otherwise. - Callers of
tasks run: the only one in CI isagent-quality.yml, which checks the pass rate in its own step, so an exit 1 on an incomplete run does not hide anything. - Earlier review points: Copilot's panel-vote and command-fallback threads and the exai-bot threads resolved in 003edba are fixed in the code.
| passed = reward is not None and reward >= 1.0 | ||
| if reward is None: | ||
| passed = returncode == 0 | ||
| reward = 1.0 if passed else 0.0 |
There was a problem hiding this comment.
A reward file that cannot be read now becomes a scored reward 1.0 (or 0.0) from the exit code.
_find_reward returns None both when there is no reward file and when there is one it cannot parse (2.0, nan, error). This fallback covers both cases. Repro: a command: harness that writes 2.0 to $DOKIMI_OUTPUT_DIR/reward.txt and exits 0 is saved as reward: 1.0, passed: true, and tasks run exits 0 with 1/1 pass. The verifier wrote a reward, but nobody can read it, so the new completion check should call this cell unscored. Before this PR the reward stayed None. Only the exit-code pass was there.
Suggestion: use the exit code only when no reward.txt / reward.json exists. When a reward file exists but does not parse, keep reward = None, so _check_completion reports the cell as unscored. Add a test with an out-of-range reward file.
| bl_cmp.add_argument("--max-context-drop", type=baseline_regression.threshold, default=None, | ||
| help="Gate on the mean matched context-score drop in score points (equality passes)") | ||
| bl_cmp.add_argument("--max-lift-drop", type=baseline_regression.threshold, default=None, | ||
| help="Gate on the mean matched (context minus baseline) lift drop in score points") | ||
| bl_cmp.add_argument("--min-matched", type=positive_int, default=1, | ||
| help="Minimum matched scored questions under a regression gate (default: 1); " | ||
| "complete coverage and matching known models are always required") |
There was a problem hiding this comment.
On current main (with #254), all three new flags fail the CLI flag registry check, and the CLI snapshot is out of date.
I merged this head with main (e97ff7a) and ran tests/test_cli_flags.py. Two tests fail: test_every_spelling_on_the_tree_is_canonical_or_allowlisted and test_the_cli_interface_snapshot_is_current. The first one lists --max-context-drop and --max-lift-drop, and also --min-matched. The note in the earlier review named only the first two.
Suggestion: add all three to dokimi/tests/data/cli_flags_allowlist.txt with a reason, or use the registry spelling where one exists. Then run make cli-snapshot.
| planned = {(task["name"], harness, rep) | ||
| for task in output["tasks"] for harness in output["harnesses"] | ||
| for rep in range(1, output.get("repeats", 1) + 1)} | ||
| seen = {(r["task"], r["harness"], r.get("repeat", 1)) for r in output["results"]} | ||
| counts = Counter(_cell_state(row, dry_run) for row in output["results"]) | ||
| missing = planned - seen | ||
| duplicate = len(output["results"]) - len(seen) | ||
| unplanned = seen - planned |
There was a problem hiding this comment.
A repeated task or harness on the command line runs every cell, then the run is rejected as incomplete.
planned is a set, so --harnesses command:true,command:true (or --tasks toy-task,toy-task) plans one cell but runs two. Both pass (2/2 pass (100.00%)), and then the command exits 1 with ✗ incomplete: 2 scored, 0 errored, 0 unscored, 0 missing of 1 planned cells / invalid identities: 1 duplicate. Before this PR this exited 0. With docker or model harnesses the time and cost are spent before the refusal, and the message does not say what the user did wrong.
Suggestion: refuse a repeated task or harness in cmd_tasks_run before anything runs (exit 2, naming the repeated entry), or drop the repeats.
| help="Gate on the mean matched context-score drop in score points (equality passes)") | ||
| bl_cmp.add_argument("--max-lift-drop", type=baseline_regression.threshold, default=None, | ||
| help="Gate on the mean matched (context minus baseline) lift drop in score points") | ||
| bl_cmp.add_argument("--min-matched", type=positive_int, default=1, |
There was a problem hiding this comment.
--min-matched without --max-context-drop or --max-lift-drop is silently ignored.
baseline_regression.enabled() looks only at the two drop flags, so baseline compare --min-matched 20 with 3 matched questions exits 0 and prints no gate line. A check the user asked for that never ran reads as a pass.
Suggestion: make the default None, and if --min-matched is set without a drop flag, refuse it with parser.error (exit 2): "--min-matched needs --max-context-drop or --max-lift-drop".
| ### Fixed | ||
|
|
||
| - `baseline compare` now treats a blank or case-variant "Unknown" provider or model, and a panel member without a `provider/` prefix, as not recorded, for every run: the "not compared" warning appears and `--strict` exits 1 where it used to pass. | ||
| - A command harness without a reward file records its exit code as reward 1.0 or 0.0, so `tasks run` counts it as scored. |
There was a problem hiding this comment.
This changes avg_reward for command-harness runs, so it no longer matches earlier artifacts, and the fragment does not say so.
Before, a cell with no reward was left out of reward_n, so avg_reward was the mean of the cells that printed a reward. Now every such cell adds 1.0 or 0.0. For a --command-template run where some tasks print reward: 0.5 and others print nothing, the same suite now gives a different avg_reward.
Suggestion: add to this line that avg_reward of command harnesses now includes exit-code cells, so it cannot be compared with older task_runs artifacts.
|
|
||
| ``tasks run`` is a reporting command: its exit code does not say whether the | ||
| task passed. This gate does, for ``docker-oracle`` and ``docker-probe``: | ||
| ``tasks run`` exits 1 for an incomplete run (an errored, unscored or missing |
There was a problem hiding this comment.
Nit: This docstring is fixed, but the same old text is still in .github/workflows/agent-quality.yml:42-44.
It still says tasks run's exit code "does not reflect pass/fail (it is a reporting command, used by callers that want partial results)". Since this PR, an errored, unscored or missing cell exits 1. Suggestion: reword it the same way as here.
| if dry_run and row.get("metrics", {}).get("dry_run"): | ||
| return "planned" | ||
| reward = row.get("reward") | ||
| if isinstance(reward, bool) or not isinstance(reward, (int, float)): |
There was a problem hiding this comment.
Nit: _cell_state repeats the finite-non-bool-number check that dokimi.harnesses.runner._num already does for the summary.
If one copy changes, the exit code and summarize_task_results stop agreeing on what a scored reward is. Suggestion: return "unscored" if _num(row.get("reward")) is None else "scored".
tasks runexits 0 when some cells errored, were never run, or have no score, so a partly broken grid reads as a result.baseline comparecan check that the models match (--strict) but has no way to fail on a score regression.Changes
tasks runchecks the result grid against the planned task x harness x repeat cells after writingruns.jsonand the report. Any errored, unscored (non-finite or missing reward), missing, duplicate or unplanned cell prints anincompleteline with counts and lists the missing cells, then exits 1. A verifier failure (reward 0.0) is a scored outcome and keeps exit 0. A supported--dry-runprints the plan and exits 0.baseline comparetakes opt-in gates:--max-context-dropand--max-lift-drop(score points, finite and >= 0, equality passes) and--min-matched(default 1). With a gate on, every scored question must match on both sides, and both answer and judge models must be recorded and equal. Otherwise it exits 1.--strictalone is unchanged: an identity check only.INCONCLUSIVEin the result line and inevaluate's warnings. Exit codes are unchanged.Behaviour change
This reverses part of #181. That PR made
tasks runexit 1 only when every cell errored and kept partly errored runs at exit 0. Here a partly errored run also exits 1.test_tasks_run_with_some_cells_errored_keeps_exit_0_and_countsbecomestest_tasks_run_with_some_cells_errored_is_incomplete. A CI job that runstasks runand tolerates some errored cells now fails. The artifacts are still written.Testing
>to>=(7 fail),--min-matchedcheck removed (1 fails), unscored cells not counted (1 fails).Not run: a Docker run on this rebased branch (the Docker results above are from before the rebase onto main), and a full native Windows suite.