Repository navigation
fix: nothing measured is not a pass, second batch (tasks run, novelty, run, compares) - #181
Conversation
…, run, compares) Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
+ Coverage 91.64% 91.72% +0.07%
==========================================
Files 222 223 +1
Lines 27409 27565 +156
Branches 4508 4536 +28
==========================================
+ Hits 25118 25283 +165
+ Misses 1620 1616 -4
+ Partials 671 666 -5
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a1d4436. The tasks run guard, the NOT MEASURED status and the DocLoadError path hold up. Three things inline: one wrong headline in the compare report that I'd fix before merge, one test gap, and one nit.
What I ran, on a Linux review host at a1d4436:
make ci-local: the full pytest run passed (7078 passed, 21 skipped). The lint, type and docs legs (ruff, mypy, deptry, bandit, zizmor, mkdocs, pip-audit) did not run in my venv:make devstops at the tools install on this base, which #179 fixed onmain. Static checks, mypy and the strict docs build pass on GitHub's CI at this head.- Reproduction of the compare-report finding with the real
compare_snapshotsandwrite_compare_report, no LLM involved. - Mutations of the new code: the
tasks runguard, theanyinNoveltyVerdict.status, thenot n_comparedbranch, both sides of theunscoredfilter and theDocLoadErrorhandler are each caught by the new tests. The count in thebaseline compareline is not (see inline).
Checked, no issues
- Novelty status consumers:
format_novelty_block,report/subject_report.py:_gate_statusandsubjects/novelty_track.pytreat the status as an opaque string, andsubject_scorecard.v2.jsonhas no enum for it, soNOT MEASUREDrenders asNOT MEASURED (k not measured)and breaks nothing. runon an unreadable doc: a doc with mode 000 gives one❌ could not read ...: Permission deniedline on stderr, exit 1, and no snapshot. Invalid UTF-8 does not raise (load_textreplaces), andrun_benchmarkhas no other caller.tasks runcounts:nanderroredinper_harnessmatchsummarize_task_results, soerrored == nmeans no cell has a verdict.
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 5410417, which fix the nit from my review:
5410417:dokimi/docs/skill-novelty.md:757: ended the sentence after "cleared one." and put theNOT MEASUREDsentence and thewarningssentence on their own lines, as suggested.
make ci-local at 5410417: 19 passed, 9 failed (host), 7 skipped.
Not approving yet. Still open: the bug and the risk from my review.
The compare report put 'Nothing to compare' before the CRITICAL/WARN headline, so a candidate that lost every document read as no regression. The regression headline now comes first and the unscored note follows. Also pins the 'N of M questions compared' count in baseline compare. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…_tasks_run to hold the complexity ratchet Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… the baseline Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ss-2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 0d84e46. Both inline points from my earlier review are fixed. One test gap is left in the compare report, inline.
What I ran, on a Linux review host at 0d84e46:
make ci-local: 28 passed, 1 failed, 7 skipped; 7218 tests passed. The failure ispip-auditreporting gitpython 3.2.0 (CVE-2026-114145) from the lock files, which this PR does not touch.- Reproductions with the LLM transport mocked; the
baseline comparerefactor diffed against the base over 11 edge inputs (empty, unscored on either side, question missing from the baseline, duplicate ids, noevaluationskey, malformedevaluations): exit codes and output are identical except for the two intended wording lines. - A mutation run of the new report and compare code: 12 mutants, one passes the full suite (inline).
Checked, no issues
- Headline order: the CRITICAL/WARN line now comes first and the
Nothing to compareline follows it; the empty-candidate case is pinned bytest_write_compare_report_empty_candidate_keeps_the_critical_headline. - Question count:
n_comparedis pinned by the 1-of-3 test; usinglen(current_evals)as the numerator fails it. - Novelty status:
format_novelty_blockandsubject_reportprint the status as an opaque string, andsubject_scorecard.v2.jsonhas no enum for it.
The empty-candidate test covered only the CRITICAL headline, so narrowing the guard to has_critical stayed green and dropped the "no document in candidate" line on a WARN-level diff. Parametrize over both severities. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The warn-level empty-side test closes my earlier gap, and the rest of the PR holds up. One nit inline.
What I ran, on a Linux review host at 99feff2:
make ci-local: 28 passed, 1 failed, 7 skipped; 7219 tests passed. The failure ispip-auditreporting gitpython 3.2.0 from the lock files, which this PR does not touch.- Mutants of the new compare-report branch: dropping
has_warnings, droppinghas_critical,ortoand, removing the unscored line, putting theelifbefore the headline, and wrong side lists are all caught. Two survive (inline, and the line order). - The new tests against the base modules: the five
baseline compareandtasks runtests fail there and pass at the head.
Checked, no issues
baseline comparerefactor: 12 edge inputs (empty, unscored on either side, disjoint ids, duplicate ids, non-listevaluations, delta without an arm score) give the same exit codes, stderr and tables as the base, except the two intended wording lines.tasks run: the artifact and the report are written before the exit 1, and partial errors still exit 0.- Docs and changelog:
NOT MEASUREDand thetasks runexit match the code; no schema or nav change is needed.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. I pushed a commit per nit, up to cc7397f, which fix the nit from my review:
cc7397f:dokimi/tests/test_runner_and_report.py:278: the critical and warn compare-report tests now assertNothing to compareis absent when both snapshots have documents.
make ci-local at cc7397f: 28 passed, 1 failed (pip-audit, same as at the reviewed head), 7 skipped.
Follow-up to #176 (independent of it: based on main, no shared code, merges cleanly in either order).
Summary
More places where nothing measured read as green. Each now says so, with counts, using the
incomplete/NOT MEASURED/ "not measured" wording from #176. An exit code is added only where the command already ends a run with a verdict; report-only lines get new wording and no exit code.The changes are in
commands/tasks.cmd_tasks_run,gate/novelty_gate.NoveltyVerdict.status,runner/run._load_found_docs(newDocLoadError, caught incommands/run.cmd_run),commands/baseline.cmd_baseline_compareandreport/markdown_report.write_compare_report. Docs:coding-harnesses.md(thetasks runexit) andskill-novelty.md(the new status).Still deferred: the run report's "minimal impact" on empty input, and the trust gate not counting unscored rows on partial input.
Evidence
Each new test fails with the source changes stashed and passes now:
tests/test_tasks_command.py::test_tasks_run_with_every_cell_errored_is_incomplete: every harness raises. Main exits 0; now exits 1 and still writesruns.json.…_with_some_cells_errored_keeps_exit_0_and_countscovers the partial case, which is unchanged (it passes on main too).tests/test_novelty_gate.py::test_an_artifact_with_no_check_measured_is_not_a_pass:evaluate_novelty({})isPASSon main andNOT MEASUREDnow; one measured check still givesPASS.tests/test_runner_run.py::test_a_found_doc_that_did_not_load_is_a_clean_error: a KeyError and a PermissionError on main,DocLoadErrornow.…::test_run_command_exits_1_without_a_traceback_when_a_doc_does_not_loadchecks the CLI: exit 1, one line, no snapshot.tests/test_baseline_compare.py::test_compare_with_nothing_scored_on_both_sides_is_not_no_change[unscored|empty], plus…_with_a_scored_pair_and_no_move_says_how_many_it_compared.tests/test_runner_and_report.py::test_write_compare_report_with_no_scored_doc_is_not_no_regressions.One existing fixture changed: the toy task in
tests/test_tasks_command.pyhad noenvironment/ortests/test.sh, so every dry-run cell errored and the two dry-run tests passed only because an all-errored run exited 0. The fixture now makes a task Harbor can run, so those tests still exit 0, for the right reason.Local CI: 17 passed, 0 failed, 12 skipped (network or docker). That covers
ruff check,mypy(222 files), deptry, bandit, zizmor, schema and registry checks, the import-architecture test,audit_test_quality.py, the changelog fragment, docs review triggers and mkdocs. The CLI surface diff is empty, and changed-line coverage is 100% (35 lines).Merge Danger
Door: two-way. Every change is an exit code or a wording change for input where nothing was measured; a revert restores the old behaviour.
Blast Radius: gates
tasks runwhere every cell errors (missing credentials, a broken image) now fails; before, it passed with nothing scored. Runs with some cells scored are unchanged.statusof the novelty gate verdict (novelty.cells[].gate.statusinsubject_scorecard.v2, which does not enumerate it) can now beNOT MEASURED. No field is added or removed. A reader that switches onPASS/WARN/FAILneeds a case for it.runover a tree where a selected doc cannot be read now exits 1 without writing a snapshot, instead of crashing with a traceback.baseline compareand the snapshotcompareMarkdown change wording only; their exit codes are unchanged. A scraper of "No notable changes vs baseline." must allow the new(k of N questions compared)suffix.commands/run.cmd_run, in separate hunks.git merge-treeshows no conflict either way.Checklist
pytestpassesruff,mypy, and schema checks pass.github/.licenserc.yaml)🤖 Generated with Claude Code