Skip to content

fix: fail incomplete task runs and add opt-in baseline regression gates - #300

Open
vchamarthi wants to merge 21 commits into
intel:mainfrom
vchamarthi:fix/task-completion-baseline-gates
Open

vchamarthi wants to merge 21 commits into
intel:mainfrom
vchamarthi:fix/task-completion-baseline-gates

Conversation

@vchamarthi

Copy link
Copy Markdown
Contributor

tasks run exits 0 when some cells errored, were never run, or have no score, so a partly broken grid reads as a result. baseline compare can check that the models match (--strict) but has no way to fail on a score regression.

Changes

  • tasks run checks the result grid against the planned task x harness x repeat cells after writing runs.json and the report. Any errored, unscored (non-finite or missing reward), missing, duplicate or unplanned cell prints an incomplete line 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-run prints the plan and exits 0.
  • baseline compare takes opt-in gates: --max-context-drop and --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. --strict alone is unchanged: an identity check only.
  • An undecided judge-validity check reads INCONCLUSIVE in the result line and in evaluate's warnings. Exit codes are unchanged.

Behaviour change

This reverses part of #181. That PR made tasks run exit 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_counts becomes test_tasks_run_with_some_cells_errored_is_incomplete. A CI job that runs tasks run and tolerates some errored cells now fails. The artifacts are still written.

Testing

  • Breaking the code on purpose fails the tests each time: drop check > to >= (7 fail), --min-matched check removed (1 fails), unscored cells not counted (1 fails).
  • Two oneTBB tasks run in real Docker against the oracle: 4/4 cells completed, exit 0. A run with a missing harness wrote its results and exited 1.

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.

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>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 02:55
@vchamarthi
vchamarthi requested a review from a team as a code owner October 30, 2026 02:55
@github-actions github-actions Bot added the cli-interface-change Changes dokimi's user-facing CLI surface label Oct 30, 2026
@github-actions

Copy link
Copy Markdown

CLI interface change detected

This 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 cli-interface-change label is on while the diff is non-empty.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 INCONCLUSIVE judge-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

Comment thread dokimi/dokimi/commands/tasks.py Resolved
@exai-bot

exai-bot commented Oct 30, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 2 issue(s) in this PR.

By severity: 2 medium

Severity Category Location Finding
medium bug baseline_regression.py:19-21 Must-fix: converting the CLI threshold to float loses precision before the Decimal comparison.
medium bug baseline.py:103-104 Must-fix: reject the unknown sentinel after removing model routes as well.
Review details
  • Commit reviewed: 3d350ed against merge base 63c9cdb
  • Coverage: 23 files in 6 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 943k tokens (604k from cache), 1m28s
  • Tool calls: 85 (64 file_read, 17 code_search, 2 code_comment, 2 file_read_diff)
  • Baseline comparison and regression reporting: 4 files
  • Evaluation completion and judge reliability: 5 files
  • Task commands and CLI contract: 4 files
  • Harness execution and result records: 4 files
  • User journeys and outcome completeness: 3 files
  • Agent quality workflow and smoke-run checks: 3 files

Comment thread dokimi/dokimi/commands/baseline.py Resolved
Comment thread dokimi/dokimi/commands/baseline.py Resolved
Comment thread dokimi/dokimi/eval/judge_validity.py Resolved
Comment thread dokimi/tests/outcomes/journeys.yaml Resolved
Comment thread dokimi/tests/test_tasks_command.py Resolved
@codecov-commenter

codecov-commenter commented Oct 30, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.38710% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.06%. Comparing base (63c9cdb) to head (3d350ed).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
dokimi/dokimi/harnesses/command.py 90.90% 1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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     
Files with missing lines Coverage Δ
dokimi/dokimi/commands/baseline.py 94.75% <100.00%> (+0.54%) ⬆️
dokimi/dokimi/commands/baseline_regression.py 100.00% <100.00%> (ø)
dokimi/dokimi/commands/evaluate.py 89.93% <ø> (ø)
dokimi/dokimi/commands/tasks.py 93.66% <100.00%> (+0.91%) ⬆️
dokimi/dokimi/eval/judge_validity.py 98.33% <100.00%> (ø)
dokimi/dokimi/records.py 100.00% <100.00%> (ø)
dokimi/dokimi/harnesses/command.py 97.09% <90.90%> (-0.21%) ⬇️

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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>
Comment thread dokimi/dokimi/commands/baseline.py Resolved

@antonwolfy antonwolfy left a comment

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.

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 compare and tasks run cases 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:

  1. Nit: .github/workflows/agent-quality.yml:42 and dokimi/scripts/check_smoke_runs.py:7 still say tasks 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.
  2. 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 ### Changed fits better than ### Fixed.

Checked, no issues

  • Completion check: every non-error reward=None row in harnesses/* is a dry-run row or carries metrics.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 2 with 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.

Comment thread dokimi/dokimi/commands/tasks.py Resolved
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>
Comment thread dokimi/tests/test_tasks_command.py Resolved

@antonwolfy antonwolfy left a comment

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.

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 prints RESULT: INCONCLUSIVE above the gate and RESULT: 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 says tasks run exits 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 vlad-perevezentsev left a comment

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.

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.py fails 2 tests, the registry check (on --max-context-drop, --max-lift-drop and --min-matched) and the CLI snapshot.
  • The external-catalog smoke (docker-oracle + docker-probe on the pinned task): 2/2 cells measured and passed, the isolation probe is closed.
  • tasks run repros with a command: harness: a reward.txt holding 2.0 gives reward 1.0, passed, exit 0; a repeated harness or task runs both cells, then exits 1 with "1 duplicate".

Checked, no issues

  • Gate logic: _scored drops 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, and FAIL (INCONCLUSIVE, see above) otherwise.
  • Callers of tasks run: the only one in CI is agent-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.

Comment thread dokimi/dokimi/harnesses/command.py Resolved
Comment thread dokimi/dokimi/commands/baseline.py Resolved
Comment thread dokimi/dokimi/commands/tasks.py Resolved
Comment thread dokimi/dokimi/commands/baseline.py Resolved
Comment thread dokimi/scripts/check_smoke_runs.py Resolved
Comment thread dokimi/dokimi/commands/tasks.py Resolved

@napetrov napetrov left a comment

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.

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.

Comment thread dokimi/dokimi/harnesses/command.py Resolved

@napetrov napetrov left a comment

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.

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:

  1. Flag registry (still open, already raised). dokimi/dokimi/commands/baseline.py:348, :350, :352 add --max-context-drop, --max-lift-drop and --min-matched. None is in cli_flags.py or tests/data/cli_flags_allowlist.txt, so test_every_spelling_on_the_tree_is_canonical_or_allowlisted fails. Fix: add three rows to dokimi/tests/data/cli_flags_allowlist.txt with a reason, e.g. --min-matched # baseline compare gate: no registry concept yet, then run make cli-snapshot. I checked this on a merge with main: test_cli_flags.py, test_cli_routes.py, test_baseline_compare.py and test_tasks_command.py pass (123). If you want the gate flags canonical now, add a concept to cli_flags.py instead.
  2. CLI snapshot (same root cause). dokimi/docs/reference/cli-interface.json lacks the three flags, so test_the_cli_interface_snapshot_is_current and the mypy job fail. Fix: the regeneration in item 1.
  3. Exit rule, duplicate identities (the run-then-reject point is already raised; this adds the exit code). --harnesses codex,codex or --tasks a,a is a usage error, so under the rule it exits 2 and nothing runs. Now the cells run, then _check_completion calls sys.exit(1) at dokimi/dokimi/commands/tasks.py:223. Fix: reject duplicates where the harness list is built (tasks.py:231-233) and where tasks are selected, with SystemExit(2). Keep exit 1 at :223 for errored, unscored or missing cells, which are runtime results.
  4. Exit rule, partial evidence (decide). dokimi/dokimi/commands/baseline_regression.py:54 exits 1 when matched evidence is below --min-matched or 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 at baseline_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 napetrov left a comment

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.

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.

  1. The thresholds belong to gate, not compare. In §2.1, compare does the paired comparison and gate <comparison> --policy applies the thresholds and sets the exit code. The §2.9 pr-check recipe is compare $RUN baseline:main → gate --policy policy.yaml. With --max-context-drop / --max-lift-drop / --min-matched, baseline compare becomes a second gate.
  2. 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 invalid and exits 4. Below --min-matched is inconclusive and exits 3. As written, CI cannot tell "regressed" from "could not compare". A2 #285 is setting up this split.
  3. baseline compare is being moved. §2.10 maps it to compare $RUN baseline:NAME, so three new flags here mean three more migrations or tombstones.
  4. Flag registry. cli_flags.py and the ratchet contract test (F1a, #254) are on main, but this PR does not register the new flags. They may be why Tests is failing; I have not checked the logs.
  5. 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.

vchamarthi and others added 6 commits October 30, 2026 23:02
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>
@xaleryb

xaleryb commented Oct 31, 2026

Copy link
Copy Markdown
Contributor
  • Ran: the test suite, lint, types, security and the benchmark gate on the head merged with current main, plus the CLI inputs from the open threads.
  • tasks run --tasks toy-task,<root>/toy-task (also toy-task,toy-task/, and two different */toy-task dirs) -> every cell runs, then exit 1 1 duplicate; the shared-basename case writes both cells into one output dir -> expected: exit 2 before anything runs, the same as toy-task,toy-task.
  • command:sh -c "exit 127", a signal exit, or an unreadable reward.txt -> stderr says unscored, but task_runs.json counts the cell as scored (pass_rate 0.0, or 1.0 when the unreadable file comes with exit 0) -> expected: errored, out of pass_rate and the paired test.
  • Pushed: 592dd42 checks the loaded task names for repeats as well as the raw strings; 97e5dfd marks a no-verdict command cell solver_error, with docs and changelog updated.
  • Nothing is left for the author.

Comment thread dokimi/dokimi/commands/baseline.py Outdated

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

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.

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.

Comment on lines 159 to +161
if reward is None:
passed = returncode == 0
reward = _exit_code_reward(returncode, output_dir)
harbor_metrics = _no_verdict(reward, returncode, output_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

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.

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>
@xaleryb

xaleryb commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Two new claims on 97e5dfd, both reproduced and fixed:

  • d010a4d: a reward file that fails to parse no longer falls through to a reward: x marker on stdout/stderr; an empty or out-of-range reward.txt next to reward: 1 output is now an errored cell, not a pass. Docs and changelog say so.
  • f19aff1: a non-string provider or model counts as not recorded, so 123/456 on both sides no longer verifies the model; the regression gate and --strict exit 1 with "cannot verify". Covers both the answer and judge roles.

Comment on lines 159 to +161
if reward is None:
passed = returncode == 0
reward = _exit_code_reward(returncode, output_dir)
harbor_metrics = _no_verdict(reward, returncode, output_dir)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

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.

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>
@xaleryb

xaleryb commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Pushed 3af1472 for the exai-bot finding on command.py:161: with no reward file, a reward: x on stdout or stderr is no longer read after exit 126, 127 or a signal exit, so the cell is errored as the docs say. A readable reward file still decides the cell, and a printed reward followed by exit 1 still scores. Then merged main (#273, #301) in 7b6da2e: in baseline.py the same-model rule and the regression gates both stay.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

documentation · low
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.

Comment on lines +404 to +405
def _never_finished(returncode: int) -> bool:
return returncode < 0 or returncode in (126, 127) or 128 < returncode <= 192

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

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.

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.

@napetrov

Copy link
Copy Markdown
Contributor

Review: re-review since 1de60da (head 7b6da2e)

This PR makes tasks run and baseline compare refuse incomplete evidence. A command cell with no verdict (exit 126/127, a signal exit, or an unreadable reward file) is errored instead of scored, a task or harness listed twice exits 2 before running, and baseline compare gets opt-in score-drop gates where a regression exits 1 and insufficient evidence exits 2. Since 1de60da it adds: a printed reward: no longer counts for a command that never ran or was killed, a reward file that yields no number blocks the output fallback, repeated tasks are caught after name resolution, --min-matched needs a gate, and the CI smoke comment is shortened.

Findings:

  • nit: dokimi/docs/coding-harnesses.md (command harness rule) and the changelog say "a reward file it cannot read". The code also errors a readable file that holds no number: probe with reward.txt = PASS at exit 0 gives reward None and solver_error: no verdict: exit 0, reward file unreadable (intel/main gave passed=True with reward None). Fix: say "a reward file with no reward in it" in both places, and reword the solver_error text to match.

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. test_agent_runner.py and test_arm_runner.py fail with ModuleNotFoundError: litellm on both head and intel/main in this environment, so they are not a signal about this PR.

No blocking findings.

@vlad-perevezentsev vlad-perevezentsev left a comment

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.

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 plus test_cli_flags.py and test_growth_ratchet.py, 700 passed.
  • Mutation: with the reward-file check removed from _exit_code_reward, 3 tests in test_harbor_results.py fail.
  • 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 of test_command_harness_exit_code_verdict_is_a_complete_run, a test this PR adds. A real os.abort() there exits 0xC0000409 and is scored, while on Linux it is no 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 --all cannot repeat a name, since task names are directory names one level under the root. --tasks checks both the spelling and the loaded name.
  • Gate exits: partial evidence and --min-matched without a threshold exit 2, a regression exits 1. Merging #273 kept the same-model rule and the stricter pair().

@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

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 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 antonwolfy left a comment

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.

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_finished stops at 192, so an exit such as 3221226505 (0xC0000409) is recorded as reward 0.0, or as a pass with reward: 1 printed, and tasks run can exit 0. New evidence: with the fake runner, exits 3221226505, 3221225786 and 3221225477 each give solver_error=None, while 192 gives no verdict: exit 192.

Other points:

  1. Nit: tests/test_tasks_command.py:249 and 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_refusing and test_native_completion_preserves_grade_failures also leave results/task-runs/toy-task/... in the working directory.
  2. Nit: docs/coding-harnesses.md and the changelog fragment say "a reward file it cannot read". A readable reward.txt holding PASS at exit 0 also gives reward=None and solver_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 the solver_error text.

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.py and baseline_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_completion mutant 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-matched without 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>
Comment on lines +19 to +21
number = float(value)
if math.isfinite(number) and number >= 0:
return number

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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.

Comment on lines +103 to +104
if any(not isinstance(v, str) or not v.strip() or v.strip().lower() == "unknown"
for v in (provider, model)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
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 antonwolfy left a comment

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.

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-dir under tmp_path.
  • 3d350ed: docs/coding-harnesses.md:611: the docs, the changelog fragment and the solver_error text 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.

This branch has not been deployed

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

Labels

cli-interface-change Changes dokimi's user-facing CLI surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants