Repository navigation
Conversation
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): + added command `dokimi subjects evidence`
+ argument `scorecard`
+ option `--help/-h`
+ option `--out`
~ `dokimi runs list` parameters changed:
~ changed option `--kind`: choices: ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| scorecard = load_artifact("subject_scorecard", Path(args.scorecard)) | ||
| except (OSError, ValueError, ArtifactValidationError) as exc: | ||
| usage_exit(f"cannot read subject scorecard {args.scorecard}: {exc}") | ||
| evidence = build_skill_evidence(scorecard, source=str(args.scorecard)) |
There was a problem hiding this comment.
Must-fix: malformed cell data can escape input validation and crash this command. For example, take a valid subject_scorecard.v2 fixture, set cells to [null], and run dokimi subjects evidence sc.json: the scorecard schema only constrains cells to an array, so loading succeeds, but build_skill_evidence raises AttributeError at cell.get(...). Neither this handler nor the CLI's top-level handler catches it, resulting in a traceback instead of an actionable exit-2 error. Validate the cell/summary/per-arm shapes before conversion, translate malformed input into a usage error, and add a regression test for this input.
There was a problem hiding this comment.
🟡 Changes recommended
Malformed nested scorecard content can trigger an uncaught AttributeError instead of a diagnostic and exit code 2, so input validation or rendering error handling needs correction.
1 open issue
What changed in this PR
1 new must-fix finding.
Adds a free subjects evidence command that turns saved subject scorecards into publishable skill_evidence.v1 documents, using the distribution digest introduced by #343.
Changes:
- Exports paired statistics and significance-based verdicts without model calls.
- Carries baseline-arm metadata through rollups and scorecards.
- Registers the artifact and CLI route, with schema, fixtures, tests, and changelog updates.
| File | Description |
|---|---|
dokimi/tests/test_subjects_evidence.py |
Tests evidence rendering, statistics, and CLI safeguards. |
dokimi/tests/test_subjects_digest.py |
Tests distribution digests from the prerequisite change. |
dokimi/tests/test_runs_show.py |
Adds evidence artifact summary expectations. |
dokimi/tests/test_runs_scan.py |
Adds evidence artifact discovery expectations. |
dokimi/tests/fixtures/artifacts/skill_evidence.v1.json |
Provides a sample evidence document. |
dokimi/dokimi/subjects/scorecard.py |
Records baseline arms and distribution digests. |
dokimi/dokimi/subjects/evidence.py |
Renders scorecard statistics as evidence. |
dokimi/dokimi/schemas/subject_scorecard.v2.json |
Allows the subject distribution digest. |
dokimi/dokimi/schemas/skill_evidence.v1.json |
Defines the experimental evidence format. |
dokimi/dokimi/schemas/matrix_rollup.v1.json |
Allows cell baseline-arm metadata. |
dokimi/dokimi/runs/kinds.py |
Registers evidence discovery and summary fields. |
dokimi/dokimi/eval/matrix_rollup.py |
Preserves each cell’s baseline arm. |
dokimi/dokimi/commands/subjects.py |
Adds the evidence command and output handling. |
dokimi/dokimi/cli_routes.py |
Classifies evidence generation as free. |
dokimi/dokimi/artifacts.py |
Registers the evidence schema version. |
dokimi/docs/reference/cli-interface.json |
Updates the CLI interface snapshot. |
dokimi/changelog.d/20261031_130000_napetrov_skill-evidence.md |
Documents evidence export and baseline metadata. |
dokimi/changelog.d/20261031_120000_napetrov_distribution-digest.md |
Documents the prerequisite distribution digest. |
Review effort: Balanced
| scorecard = load_artifact("subject_scorecard", Path(args.scorecard)) | ||
| except (OSError, ValueError, ArtifactValidationError) as exc: | ||
| usage_exit(f"cannot read subject scorecard {args.scorecard}: {exc}") | ||
| evidence = build_skill_evidence(scorecard, source=str(args.scorecard)) |
…dex carries subject_scorecard.v2 gains subject.distribution_digest: sha256 of the SKILL.md bytes for a file ref or a directory holding only SKILL.md, the value a .well-known/agent-skills index.json entry of type skill-md carries (agentskills PR intel#254). An archive's bytes depend on how it was packed, so a skill with resources gets null, as does a non-local ref. ref_digest is unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ce.v1 A free leaf that reads a saved subject_scorecard and writes the document a publisher puts next to a skill: one result per (cell, non-baseline arm) with model, harness, effort, n, delta, CI, corrected p-value and a verdict, keyed by subject.distribution_digest so a client can match it to the .well-known/agent-skills index entry it installed from. Values the scorecard does not hold stay null. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…il a run records it Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
d25d6f1 to
d0f1704
Compare
| """ | ||
| if not ref: | ||
| return None | ||
| path = Path(ref.removeprefix("local:")) |
There was a problem hiding this comment.
Must-fix: non-local references can acquire a digest from unrelated local files. On POSIX, create base/context7:/org/lib/SKILL.md as the directory's only entry, then call distribution_digest("context7:/org/lib", base=base): this returns that file's SHA-256 instead of None. The descriptor loader accepts this reference, and the evidence renderer publishes the resulting digest as subject identity. Reject non-local reference schemes before filesystem lookup (while preserving Windows drive paths), and add a collision regression test; the existing non-local test only exercises a nonexistent path. This affects both subject and member digests through this helper.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
- Coverage 94.05% 94.04% -0.01%
==========================================
Files 297 298 +1
Lines 38018 38094 +76
Branches 6424 6439 +15
==========================================
+ Hits 35756 35827 +71
- Misses 1452 1456 +4
- Partials 810 811 +1
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d0f1704. The renderer is small, reads every number from the scorecard and leaves a missing one null, and the verdict rule (significant after correction, else inconclusive) is right. Two risks are worth fixing before merge, both gaps in what the tests pin: one inline, one below. One nit, and a note on the stacking.
What I ran, on a Linux review host at d0f1704:
make ci-local: 27 passed, 3 failed, 7 skipped; 11,986 tests passed, 1 failed; changed-line coverage passes. The 3 failures are not the PR's: one timing assertion (test_unbalanced_quote_in_hook_completes_quickly, 1.02 s against a 1 s limit) passes when rerun alone and the PR does not touch that code; the two gitleaks steps could not install or run the tool.- Mutation run on the new code and tests: 60 mutants, 29 pass
test_subjects_evidence.py,test_subjects_digest.py,test_runs_scan.pyandtest_runs_show.py. - Reproductions for the code findings, with the LLM transport mocked: malformed scorecards through
dokimi subjects evidence, anddistribution_digestwith a colliding directory.
Findings that cannot be anchored inline:
-
The baseline fallback and the interval null handling in
evidence.pyare unpinned._baseline_arm(evidence.py:80-84) picks the baseline for any scorecard recorded beforebaseline_armexisted. The only fallback test has two delta-less arms, so it covers the "skip the cell" branch only. Replacing the fallback withreturn None, and droppingand not row.get("significance"), both pass the four test files. Likewise forci(evidence.py:105-108): never returning null ({"lo": null, "hi": null, ...}for an arm with too few pairs) and keying the null check onci_hiboth pass. Add a cell with one delta-less arm and nobaseline_arm(that arm is the baseline, the other is published), a cell whose delta-less arm carriessignificance(not treated as baseline), and an arm withci_lo=None(expectci is None). Also unasserted, each passing when removed: thesubject kind is ... not 'skill'warning, the task-suite warning, and scorecard warnings carried through. -
The PR carries #343's commit. The first commit (
099c2b8,distribution_digest) belongs to that PR. Rebase ontomainonce #343 merges, so the diff holds only this PR's two commits.
Checked, no issues
- Pins: the verdict sign flip, ignoring
significant_adjusted, the delta source,arm != baseline, a recorded baseline winning over the fallback, the source basename, and the--out-equals-scorecard and empty-results exits each fail a test when broken. - Schemas:
subject_scorecard.v2andmatrix_rollup.v1only gain optional nullable properties in objects that allow extra keys, so older artifacts still validate.skill_evidence.v1is registered inSCHEMA_VERSIONSandruns/kinds.py, and the emitted document validates against it. - CLI: exit 2 with a message for a missing scorecard, a directory,
--outnaming the scorecard (also as./sc.json), and an unwritable target.source.pathkeeps only the file name. - Commits: all three carry
Signed-off-by.
| "model": cell.get("model"), | ||
| "provider": cell.get("provider"), | ||
| "harness": cell.get("harness"), | ||
| "baseline_arm": cell.get("baseline_arm"), |
There was a problem hiding this comment.
No test pins that scorecard cells carry baseline_arm. test_the_rollup_and_scorecard_cells_carry_the_baseline_arm (tests/test_subjects_evidence.py:155) only calls build_matrix_rollup; nothing calls build_subject_scorecard for this field.
Mutation check: deleting this line leaves test_subjects_evidence.py, test_subjects_digest.py, test_runs_scan.py and test_runs_show.py at 110 passed, 1 skipped, the same as the unmutated tree. Every new scorecard then loses baseline_arm and evidence.py falls back to the "one arm with no delta" guess, which is the guess the changelog says this field removes. An arm whose every answer failed has no delta either, so it can be published as the baseline or the cell skipped.
Suggestion: build a scorecard from a cell artifact that has baseline_arm: "none" and assert scorecard["cells"][0]["baseline_arm"] == "none"; rename the test or split it.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| if delta is None or not sig.get("significant_adjusted"): | ||
| return "inconclusive" |
There was a problem hiding this comment.
Must-fix: a schema-valid scorecard with significance: {"delta_mean": 12.0, "significant_adjusted": "false"} is published with verdict improved. The scorecard schema does not constrain nested cell statistics, and the string is truthy, so neither input validation nor the new exception handler rejects it. This silently turns malformed significance data into a positive claim. Validate this flag as boolean/null (reject other types with a handled input error), and add a regression test for the string "false"; only the boolean true should permit a directional verdict.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 778a1ac, which fix the nit from my review:
778a1ac:dokimi/dokimi/commands/subjects.py:178: wrappedbuild_skill_evidenceso AttributeError/TypeError exits 2 with a not-well-formed message; added acells: [null]test.
Left as is:
- The body's only nit-labelled point is not present; findings 1 and 2 are risks/stacking, left to the author.
make ci-local at 778a1ac: 28 passed, 2 failed (gitleaks, host), 7 skipped.
Not approving yet. Still open: the two risks from my review.
Summary
New
dokimi subjects evidence SCORECARD [--out FILE](free, no model calls) renders a savedsubject_scorecardasskill_evidence.v1(experimental schema): one row per (cell, non-baseline arm) with paired n, delta, CI, corrected p and a verdict (improved/regressedonly when significant after correction, elseinconclusive), keyed bysubject.distribution_digest. Nothing is measured; values the scorecard lacks stay null (e.g.effort). Rollup and scorecard cells now carrybaseline_arm.Stacked on #343 — review only the top commit until that merges.
CLI v2 (#94): maps to
report RUN --per-subject --format evidence.Evidence
tests/test_subjects_evidence.py, runs show/scan kind rows, CLI snapshot regenerated (py3.13); full suite 12018 passed locally.Merge danger
Low–medium: new artifact kind +
baseline_armadded tomatrix_rollup.v1cells.🤖 Generated with Claude Code