Repository navigation
fix: fail incomplete task runs and add opt-in baseline regression gates - #300
vchamarthi wants to merge 21 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>
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 2 issue(s) in this PR. By severity: 2 medium
Review details
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 94.04% 94.06% +0.01%
==========================================
Files 294 298 +4
Lines 38011 38127 +116
Branches 6432 6444 +12
==========================================
+ Hits 35747 35863 +116
- Misses 1454 1455 +1
+ Partials 810 809 -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>
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.
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>
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
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.
napetrov
left a comment
There was a problem hiding this comment.
Review of #300 at head 1de60da (base 3cfe49c).
Provenance: one changed finding (inline); one changed nit (docs, below). Other rows are questions, see below.
Checks: CI is red on Tests (py3.11), Tests (py3.14) and mypy at head 1de60da. I could not read the job logs with gh from this box, so I did not confirm the failing tests.
Nit (docs, no line here): .github/workflows/agent-quality.yml lines 41-44 still say tasks run's exit code "does not reflect pass/fail" and serve "callers that want partial results". This PR makes that step exit 1 on incomplete cells, so the comment is now wrong.
Question: --harnesses codex,codex or --tasks a,a now exit 1 with invalid identities: N duplicate instead of running twice. The PR body says duplicates count as incomplete, so this looks intended; it is a behaviour change for existing scripts. Probe from the edge pass: probes/completion_edges.py.
Open threads from other reviewers that I confirmed or did not repeat: the duplicate-identity run-then-reject at tasks.py:211 (vlad-perevezentsev), --min-matched silently ignored without a drop gate at baseline.py:352 (vlad-perevezentsev), the no-test mutants in baseline_regression.py:61 and tasks.py:197 (antonwolfy), and the CLI flag registry failure on main after #254 (vlad-perevezentsev). I did not re-probe those.
Coverage: read the diff and the probes from the plain and edge passes. Did not run the test suite (the sandbox pytest plugins do not load).
Maintainer view on the scoring question: for command-harness cells, treat exit 126/127 and signal exits as no verdict (an infra error, not counted) and keep every other exit code scored. Please add a test for that and a changelog line saying avg_reward changes for command-harness runs.
napetrov
left a comment
There was a problem hiding this comment.
Review of #300 at 1de60da against intel main 72d7add.
Merge: clean. git merge-tree with main has no conflicts (only growth_baseline.json auto-merges). No command is added, so no route-table row is needed. baseline compare stays free, which is right: the gate reads saved files.
Mismatches with the new conventions:
- Flag registry (still open, already raised).
dokimi/dokimi/commands/baseline.py:348,:350,:352add--max-context-drop,--max-lift-dropand--min-matched. None is incli_flags.pyortests/data/cli_flags_allowlist.txt, sotest_every_spelling_on_the_tree_is_canonical_or_allowlistedfails. Fix: add three rows todokimi/tests/data/cli_flags_allowlist.txtwith a reason, e.g.--min-matched # baseline compare gate: no registry concept yet, then runmake cli-snapshot. I checked this on a merge with main:test_cli_flags.py,test_cli_routes.py,test_baseline_compare.pyandtest_tasks_command.pypass (123). If you want the gate flags canonical now, add a concept tocli_flags.pyinstead. - CLI snapshot (same root cause).
dokimi/docs/reference/cli-interface.jsonlacks the three flags, sotest_the_cli_interface_snapshot_is_currentand the mypy job fail. Fix: the regeneration in item 1. - Exit rule, duplicate identities (the run-then-reject point is already raised; this adds the exit code).
--harnesses codex,codexor--tasks a,ais a usage error, so under the rule it exits 2 and nothing runs. Now the cells run, then_check_completioncallssys.exit(1)atdokimi/dokimi/commands/tasks.py:223. Fix: reject duplicates where the harness list is built (tasks.py:231-233) and where tasks are selected, withSystemExit(2). Keep exit 1 at:223for errored, unscored or missing cells, which are runtime results. - Exit rule, partial evidence (decide).
dokimi/dokimi/commands/baseline_regression.py:54exits 1 when matched evidence is below--min-matchedor is not complete and unique. This PR already exits 2 for a malformed file (baseline.py:281). Recommendation: exit 2 for partial or duplicate baseline and current evidence, since the user can fix the input. Keep exit 1 for REGRESSION atbaseline_regression.py:66, so CI can tell the two apart. Your call.
Not repeated here: --min-matched ignored without a drop flag, the command-harness reward fallback, the mutant gaps, the doc nits and the agent-quality.yml comment. Those are already open in the threads.
Pre-existing on main, not this PR: tasks.py:131, :162 and :233 ("No tasks/harnesses selected") and baseline.py:210 ("Baseline not found") exit 1. Under the rule these are usage errors and should be 2. Not blocking for #300; a follow-up.
napetrov
left a comment
There was a problem hiding this comment.
CLI API check against #94 v2.1, which was accepted on 10-28. The goal fits: an opt-in regression gate, and "not measured is never pass" (G3). The placement and exit codes conflict with the accepted design.
- The thresholds belong to
gate, notcompare. In §2.1,comparedoes the paired comparison andgate <comparison> --policyapplies the thresholds and sets the exit code. The §2.9pr-checkrecipe iscompare $RUN baseline:main→gate --policy policy.yaml. With--max-context-drop/--max-lift-drop/--min-matched,baseline comparebecomes a second gate. - Exit codes (§2.6). The PR exits 1 in every case. Under the table, a regression beyond the threshold exits 1. Not comparable (models differ or unrecorded, questions unmatched) is
invalidand exits 4. Below--min-matchedisinconclusiveand exits 3. As written, CI cannot tell "regressed" from "could not compare". A2 #285 is setting up this split. baseline compareis being moved. §2.10 maps it tocompare $RUN baseline:NAME, so three new flags here mean three more migrations or tombstones.- Flag registry.
cli_flags.pyand the ratchet contract test (F1a, #254) are on main, but this PR does not register the new flags. They may be whyTestsis failing; I have not checked the logs. - Minor: the
--max-*stem means a spend cap elsewhere (--max-usd). These flags are score tolerances, so check the name against the F1c stem rule.
Suggestion: keep this as a stopgap if (a) the names and meanings match the policy.v1 fields that gate will read, so the later move is mechanical; (b) invalid exits 4 and inconclusive exits 3, with 1 only for a real regression; (c) the flags are registered in cli_flags.py; and (d) a line in the changelog or docs says that #94 moves these to gate.
The tasks run incomplete-grid half fits G3, subject to the same 1 vs 3/4 question for errored or missing cells.
Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
A command that exited 126 or 127, or was killed by a signal, never produced a verdict, and a reward file that cannot be read is not an absent one. Both stay unscored so tasks run reports them instead of reading them as a pass or a fail. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
A task or harness listed twice used to run both cells and then fail the run as incomplete. It now exits 2 before anything runs. The cell state reuses the runner's number check, so the exit code and the summary agree on what is scored. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
… evidence exits 2 --max-context-drop, --max-lift-drop and --min-matched join the CLI flag allowlist. --min-matched alone is a usage error instead of a silent no-op. Partial or insufficient evidence is an input the user can fix, so it exits 2; a regression and a model mismatch keep exit 1. The fragment and docs say so, and avg_reward of command harnesses changing is noted. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
The two functions were over the complexity baseline. The smoke job comment is cut to two lines as the comment-block check requires. Signed-off-by: vchamarthi <123653452+vchamarthi@users.noreply.github.com>
The repeat check compared --tasks strings as typed, while cells and output dirs are keyed by the loaded task name. A name plus its path, a trailing slash, or two tasks sharing a directory name ran every cell, then exited 1. Check the loaded names too, before anything runs. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Exit 126/127, a signal exit or an unreadable reward file left reward None but passed from the exit code, so task_runs counted the cell as a scored pass or failure in pass_rate and the paired test. Mark it solver_error, which the summary already leaves out. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
|
|
||
| def pair(provider: Any, model: Any) -> Optional[Tuple[str, str]]: | ||
| if not provider or not model or "unknown" in (provider, model): | ||
| if any(not str(v or "").strip() or str(v).strip().lower() == "unknown" for v in (provider, model)): |
There was a problem hiding this comment.
Must-fix: non-string identities can satisfy the new promotion gate. For example, compare(_eval(judge=(123, 456)), _eval(judge=(123, 456)), "--max-context-drop", "0") prints Regression gate: PASS: this check accepts the numbers, and pair() then stringifies them into matching identities. _read_json_object() only checks the outer object and records() does not validate metadata; the eval schema requires judge identities to be strings or null. Reject non-string provider/model values before normalization (for both answer and judge identities), and add a test ensuring malformed identities cannot promote a candidate.
There was a problem hiding this comment.
Reproduced (judge=(123, 456) on both sides printed Regression gate: PASS). Fixed in f19aff1: pair() treats a non-string provider or model as not recorded, so the gate exits 1 with "cannot verify the judge model"; the test covers both answer and judge.
| if reward is None: | ||
| passed = returncode == 0 | ||
| reward = _exit_code_reward(returncode, output_dir) | ||
| harbor_metrics = _no_verdict(reward, returncode, output_dir) |
There was a problem hiding this comment.
Must-fix: an invalid reward file can still be counted as scored when stdout/stderr contains a reward marker. For example, extend the new unreadable-file test so reward.txt contains 2.0 and the handler returns reply("reward: 1"): _find_reward rejects the file but falls through to stdout and returns 1.0, so this guard skips the no-verdict check. The cell passes, has no solver_error, and tasks run can exit 0 despite the documented unreadable-file policy. Distinguish an absent reward file from an invalid/unreadable one before accepting the output fallback, and cover this combination in the test.
There was a problem hiding this comment.
Reproduced: reward.txt holding 2.0, or an empty one, plus reward: 1 on stdout gave a pass. Fixed in d010a4d: once a reward file exists, the output marker is no longer consulted, so the cell is errored ("reward file unreadable"). The existing test now covers both cases.
A reward file that failed to parse fell through to a reward: x marker on stdout or stderr, so the cell passed instead of being errored as documented. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
pair() stringified numbers, so a schema-invalid identity of 123/456 on both sides verified the model and let the regression gate print PASS. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Two new claims on 97e5dfd, both reproduced and fixed:
|
| if reward is None: | ||
| passed = returncode == 0 | ||
| reward = _exit_code_reward(returncode, output_dir) | ||
| harbor_metrics = _no_verdict(reward, returncode, output_dir) |
There was a problem hiding this comment.
Must-fix: the no-verdict exit fix still has an output-marker bypass. With no reward file, reply("reward: 1", "", 127) (also 126, 137 or -9) makes _find_reward return 1.0, so this branch never checks the exit and never sets solver_error. The cell is consequently scored as a pass and tasks run can exit 0, contrary to the documented policy. Reject these exits before accepting stdout/stderr rewards, and extend the no-verdict test to include reward markers on both streams.
There was a problem hiding this comment.
Reproduced and fixed in 3af1472: after exit 126, 127 or a signal exit the output reward markers are not read, so the cell is errored (no verdict: exit N). Only a readable reward file still decides such a cell. The no-verdict test now has markers on stdout, on stderr, and on both.
…illed With no reward file, a `reward: 1` on stdout or stderr scored exit 126, 127 or a signal exit as a pass. Read output markers only after an exit that finished; a readable reward file still outranks such an exit. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Conflicts in commands/baseline.py: keep intel#273's same-model rule (raw panel strings, model_identity) with this PR's regression-gate policy and non-string identity rule; growth_baseline.json keeps both caps. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Pushed 3af1472 for the exai-bot finding on |
| ### Fixed | ||
|
|
||
| - `baseline compare` now treats a blank, non-string 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 with no reward file records its exit code as reward 1.0 or 0.0, so `tasks run` counts it as scored; exit 126, 127, signal exits and an unreadable reward file are no verdict, even with a `reward: x` on the output: the cell is errored and stays out of `pass_rate` and the paired test. `avg_reward` of command harnesses now includes these exit-code cells, so it does not compare with older `task_runs` artifacts. |
There was a problem hiding this comment.
Nit: this overstates the exit-code fallback. For a non-Harbor command with no reward file, stdout reward: 0.5 and exit 0 still records reward 0.5, not 1.0: _find_reward reads output before _exit_code_reward is called. Also, a readable reward file can supply a verdict even after exit 126/127 or a signal. Please qualify the fallback as applying when neither a reward file nor a valid output marker supplies a reward, and qualify the no-verdict exits as applying without a readable reward file, matching docs/coding-harnesses.md.
| def _never_finished(returncode: int) -> bool: | ||
| return returncode < 0 or returncode in (126, 127) or 128 < returncode <= 192 |
There was a problem hiding this comment.
Must-fix: native Windows interruption statuses bypass this check. process.run() forwards proc.returncode unchanged for non-timeout exits (process.py:235), and Windows reports Control-C termination as positive 0xC000013A (3221225786). With reply("", "", 3221225786) and no reward file, this produces reward 0.0 with no solver_error, so an interrupted cell counts as completed evidence and tasks run can exit 0. With reward: 1 on stdout, it even counts as a pass. Recognize native Windows abnormal-termination statuses here (or normalize them in the runner), and extend the no-verdict parametrization to cover them while preserving readable reward-file precedence.
There was a problem hiding this comment.
Reproduced on Windows (Server 2025, Python 3.13) with a real process, no fake runner. command:"<python>" -c "import os,sys; print('reward: 1'); sys.stdout.flush(); os.abort()" exits 3221226505 (0xC0000409) and is saved as reward=1.0, passed=True, with no solver_error. Without the printed marker it is a scored 0.0. On Linux the same command exits -6 and is no verdict: exit -6. So the same crash is no verdict on one OS and a pass on the other.
Suggestion: on win32, treat an NTSTATUS error code as not finished too, for example returncode >= 0xC0000000 (this covers Ctrl-C 0xC000013A, access violation 0xC0000005 and fail-fast 0xC0000409). Add these to the no-verdict parametrization.
Review: re-review since 1de60da (head 7b6da2e)This PR makes Findings:
Coverage: checked the 10 commits since 1de60da (round diff, main merges excluded), a reward-path probe on head and on intel/main, the 6 touched test files (384 passed), and ruff F-rules on changed .py files. Not checked: the full test suite, Docker and native Windows runs. No blocking findings. |
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 7b6da2e. Every point from my first round is fixed. The reward-file rule, the repeated-task refusal, --min-matched, the flag registry and the changelog all hold, and the new tests catch the bugs they cover. What is left is on Windows: a crashed command still counts as a verdict there, and the new CLI test fails there. Both are in the line comments.
Agreed with @napetrov on the doc wording: coding-harnesses.md and the fragment say "a reward file it cannot read", but a readable reward.txt that holds PASS is no verdict too. On the 3/4 exit codes from #94 I disagree for this PR: exit_codes.py on main has only 1 and 2, and the PR follows it (2 for partial evidence, 1 for a regression or a model mismatch). The move to gate is noted at benchmarking-and-comparison.md:838.
Agreed with @exai-bot on the Windows exit statuses (I added a repro in its thread) and on the changelog wording for the exit-code fallback.
What I ran, at 7b6da2e:
make ci-local --offline(lint, types, tests, coverage of changed lines, docs): 19 passed, 0 failed, 13 skipped.- This head merged with current
main: the PR's test files plustest_cli_flags.pyandtest_growth_ratchet.py, 700 passed. - Mutation: with the reward-file check removed from
_exit_code_reward, 3 tests intest_harbor_results.pyfail. - On Windows (Server 2025, Python 3.13):
pytest tests/test_harbor_results.py tests/test_tasks_command.py tests/test_baseline_compare.py: 181 passed, 2 failed. Both failures are cases oftest_command_harness_exit_code_verdict_is_a_complete_run, a test this PR adds. A realos.abort()there exits0xC0000409and is scored, while on Linux it isno verdict: exit -6.
Checked, no issues
- No-verdict rule: exit 126, 127 and signal exits ignore a printed reward, a readable reward file still decides, and an unreadable one blocks both the exit code and the output marker. On Linux, a real segfault and a real abort are errored cells.
- Repeated tasks:
tasks run --allcannot repeat a name, since task names are directory names one level under the root.--taskschecks both the spelling and the loaded name. - Gate exits: partial evidence and
--min-matchedwithout a threshold exit 2, a regression exits 1. Merging #273 kept the same-model rule and the stricterpair().
| @pytest.mark.parametrize("script, expected", [("pass", 0), ("raise SystemExit(1)", 0)]) | ||
| def test_command_harness_exit_code_verdict_is_a_complete_run(tmp_path, capsys, script, expected): | ||
| import sys | ||
|
|
There was a problem hiding this comment.
This test fails on Windows, and on Linux it writes its cells into the work tree.
On Windows (Server 2025, Python 3.13), both cases fail: the cell is errored with OSError: [WinError 123] ... results\task-runs\toy-task\command_"C__..._python.exe"_-c_"pass". runner._safe_id replaces only /, : and space, so the quote now in the harness id ends up in the cell's directory name, and Windows does not allow " in a path. So any quoted command: harness cannot run on Windows (this part is older than this PR). The test also passes no --output-dir, so on Linux it leaves dokimi/results/task-runs/toy-task/command_"..._python"_-c_"pass"/ in the checkout (git-ignored). The other tests in this file pass on Windows. CI does not catch this, because windows-smoke runs only two test files.
Suggestion: pass --output-dir str(tmp_path / "cells"), and make _safe_id also replace the characters Windows refuses ("<>|?*\).
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 7b6da2e. The delta fixes everything raised in the last round, but one command-harness hole is still open: a crash on Windows is scored as a verdict. One nit inline, and two small ones below.
Open bot points I confirm:
- I agree with exai-bot and @vlad-perevezentsev that a Windows crash status still counts as a verdict:
_never_finishedstops at 192, so an exit such as 3221226505 (0xC0000409) is recorded as reward 0.0, or as a pass withreward: 1printed, andtasks runcan exit 0. New evidence: with the fake runner, exits 3221226505, 3221225786 and 3221225477 each givesolver_error=None, while 192 givesno verdict: exit 192.
Other points:
- Nit:
tests/test_tasks_command.py:249and three neighbours pass no--output-dir. I agree with @vlad-perevezentsev on the quoted-harness test. New evidence:test_unsupported_dry_run_preserves_artifacts,test_partial_suite_writes_remaining_results_before_refusingandtest_native_completion_preserves_grade_failuresalso leaveresults/task-runs/toy-task/...in the working directory. - Nit:
docs/coding-harnesses.mdand the changelog fragment say "a reward file it cannot read". A readablereward.txtholdingPASSat exit 0 also givesreward=Noneandsolver_error: no verdict: exit 0, reward file unreadable. I agree with @napetrov and @vlad-perevezentsev on the wording; say "a reward file with no reward in it" and reword thesolver_errortext.
What I ran, on a Linux review host at 7b6da2e:
make ci-local(with Docker): 35 passed, 0 failed, 7 skipped.- Mutation run of the new tests over
command.py,tasks.pyandbaseline_regression.py: 91 mutants, 25 survived the three new test files. The survivors that decide a verdict are in the inline comment and in the exit-code edges below. - Reproductions with the LLM transport mocked.
Checked, no issues
- Completion check: every
_check_completionmutant is killed (missing, duplicate, unplanned, errored, unscored cells, empty plan, repeats). - No-verdict rule: 126, 127, -1, 129 and 192 are errored cells; 125, 128, 193 and 255 stay scored. A readable reward file still decides, and an unreadable one blocks the output marker.
- Repo rules: all 16 non-merge commits carry
Signed-off-by;cli-interface.json, the flag allowlist and the CLI snapshot test agree with the parser. - Gate exits: regression and model mismatch exit 1, partial evidence and
--min-matchedwithout a gate exit 2, matching the docs.
Bot points I checked that no longer apply at this head (1)
- exai-bot's points on non-string identities, on an unreadable reward file with a printed marker, and on exit 126, 127 or a signal with a printed marker do not apply: each is fixed at this head and pinned by a test.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| number = float(value) | ||
| if math.isfinite(number) and number >= 0: | ||
| return number |
There was a problem hiding this comment.
Must-fix: converting the CLI threshold to float loses precision before the Decimal comparison. For one matched question with saved context score 0.2, current score 0.09999999999999999, and both no-context scores 0, the computed context/lift drop is Decimal('0.10000000000000001'). Passing that exact value to either threshold flag converts it to 0.1, so an equal drop incorrectly exits 1. Preserve the threshold as a Decimal parsed directly from the argument, validate its finiteness/non-negativity, and add this case to the equality tests for both gates.
| if any(not isinstance(v, str) or not v.strip() or v.strip().lower() == "unknown" | ||
| for v in (provider, model)): |
There was a problem hiding this comment.
Must-fix: reject the unknown sentinel after removing model routes as well. With otherwise valid scored artifacts, compare(_eval(judge=("openai", "openai/unknown")), _eval(judge=("openai", "openai/unknown")), "--max-context-drop", "0") reaches Regression gate: PASS: the raw string is not unknown, but model_identity() reduces it to ('unknown', None), which matches on both sides. The same bypass affects answer identities and panel members such as openai/openai/unknown. Check the normalized model id for unknown and add regression cases for each identity path so missing model identity cannot authorize promotion.
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 3d350ed, which fix the three nits from my review:
4fff339:tests/test_baseline_compare.py:375: added tests for the mean drop (two questions down 1), an improvement larger than the limit, and an empty baseline and current exiting 2.b002f48:tests/test_tasks_command.py:190: the quoted-harness test,_run_tasks(used by the native-completion and partial-suite tests) and the unsupported dry-run test now pass--output-dirundertmp_path.3d350ed:docs/coding-harnesses.md:611: the docs, the changelog fragment and thesolver_errortext now say "a reward file with no reward in it" / "reward file has no reward".
make ci-local at 3d350ed: 35 passed, 0 failed, 7 skipped.
Not approving yet. Still open: @vlad-perevezentsev on Windows NTSTATUS exits (0xC0000409, 0xC000013A) are scored as a verdict by _never_finished (command.py:404); also raised by exai-bot.


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.