Repository navigation
feat(subjects): subjects evidence renders a scorecard as skill_evidence.v1 - #344
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'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'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'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'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.
Answered with evidence, no new change: 778a1ac already turns this into a usage exit. cmd_subjects_evidence maps AttributeError and TypeError from the build to usage_exit (exit 2), and test_subjects_evidence_refuses_a_scorecard_with_a_wrong_typed_cell now runs cells as [null], a non-object summary, a non-object per_arm, a non-object row, and a non-object significance.
Passes at 4eefca5. Left unresolved for your check.
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)) |
There was a problem hiding this comment.
…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.
There was a problem hiding this comment.
Fixed in 4eefca5. distribution_digest returns None for a URI-scheme ref (context7:/..., https:...) before any filesystem lookup, after the local: prefix is stripped. A drive letter is not a scheme, so Windows paths are unchanged.
Regression test: a context7:/org/lib directory holding a SKILL.md gives None (POSIX only).
Left unresolved for your check.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
- Coverage 94.21% 94.21% -0.01%
==========================================
Files 304 305 +1
Lines 38766 38840 +74
Branches 6567 6578 +11
==========================================
+ Hits 36524 36593 +69
- Misses 1429 1432 +3
- Partials 813 815 +2
🚀 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.
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.
There was a problem hiding this comment.
Fixed in 4eefca5: _verdict gives a direction only when significant_adjusted is True. The string "false" is inconclusive.
Regression test: test_a_significance_flag_that_is_not_a_boolean_gives_no_direction, which failed before the change.
Left unresolved for your check.
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.
…erdict distribution_digest looks a ref up on disk only for a local ref, so a scheme ref such as context7:/org/lib can no longer collide with a directory of that name. A verdict needs significant_adjusted to be the boolean true; the string "false" gives inconclusive. The baseline_arm of a scorecard cell and the wrong-typed-cell exits are pinned by tests. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| with exit_on_unwritable(out): | ||
| save_artifact("skill_evidence", evidence, out) |
There was a problem hiding this comment.
Must-fix: a scorecard with cells[0].summary.per_arm["skill:s"].significance.n set to "6" passes the scorecard schema and rendering, but fails the evidence schema here. save_artifact raises ArtifactValidationError; neither exit_on_unwritable (which catches only OSError) nor the top-level CLI handler catches it, so the command produces a traceback and exit 1 instead of an input diagnostic and exit 2. Catch output validation failures and route them through usage_exit, and add a regression test for a wrong-typed nested statistic that survives rendering.
There was a problem hiding this comment.
Fixed in ac2b33c: a value the evidence schema rejects at save (here a string n inside significance) now goes through usage_exit, exit 2 with a diagnostic, not a traceback. Regression test test_subjects_evidence_refuses_a_nested_statistic_the_evidence_schema_rejects was red before the change. Local: test_subjects_evidence.py 19 passed; full suite 12047 passed, 8 skipped.
…ceback A scorecard whose nested significance n is a string passes the scorecard schema and renders, then save_artifact raised ArtifactValidationError past both the output guard and the CLI handler. It now goes through usage_exit like the other input diagnostics. Regression test: red before. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if out.resolve() == Path(args.scorecard).resolve(): | ||
| usage_exit("--out names the scorecard itself; pick another file") |
There was a problem hiding this comment.
Must-fix: on supported Python 3.11–3.12 (requires-python >=3.11), Path.resolve() raises RuntimeError for a symlink loop. For example, create loop -> loop and run dokimi subjects evidence sc.json --out loop with a valid scorecard: this check fails before exit_on_unwritable, and the CLI does not catch RuntimeError, so it prints a traceback instead of an exit-2 diagnostic. Catch resolution failures (RuntimeError on these versions and OSError) around this check and add a symlink-loop regression test. Moving it inside the current output guard alone is insufficient because that guard catches only OSError.
There was a problem hiding this comment.
Fixed in 842720f: resolving --out now catches RuntimeError as well as OSError, so a symlink loop exits 2 with a diagnostic. Regression test test_subjects_evidence_refuses_an_out_that_is_a_symlink_loop, red before (traceback).
| if out.resolve() == Path(args.scorecard).resolve(): | ||
| usage_exit("--out names the scorecard itself; pick another file") |
There was a problem hiding this comment.
Must-fix: this protects only the output file, but save_artifact also writes or deletes <out>.rejected. With a valid scorecard saved as sc.json.rejected, subjects evidence sc.json.rejected --out sc.json succeeds and then deletes the input scorecard as a stale rejected payload (artifacts.py:188–190). If evidence validation fails, the same path is overwritten with the rejected evidence instead. Reject an output whose .rejected sibling resolves to the input as well, and add a regression test that the scorecard remains intact.
There was a problem hiding this comment.
Fixed in 842720f: --out is refused when its .rejected sibling resolves to the scorecard, since save_artifact removes or overwrites that sibling. The scorecard stays intact. Regression test test_subjects_evidence_refuses_an_out_whose_rejected_sibling_is_the_scorecard, red before (the valid save deleted the input). Local: test_subjects_evidence.py 21 passed; full suite 12049 passed, 8 skipped.
…sibling of the scorecard Path.resolve() raises RuntimeError on a symlink loop before Python 3.13, which escaped as a traceback. save_artifact also keeps and removes "<out>.rejected", so an --out whose sibling is the scorecard would delete or overwrite it. Both now exit 2; tests red before the change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| try: | ||
| out_real = out.resolve() | ||
| # save_artifact keeps and removes "<out>.rejected"; that sibling must not be the scorecard. | ||
| stale_real = out.with_name(out.name + ".rejected").resolve() |
There was a problem hiding this comment.
Must-fix: with a valid scorecard, dokimi subjects evidence sc.json --out . (or --out / on POSIX) reaches out.with_name(...) with an empty out.name, which raises ValueError. This handler catches only OSError and RuntimeError, and the CLI does not catch this ValueError, so an invalid output path produces a traceback instead of a usage diagnostic (exit 2). Reject paths without a filename before constructing the sibling, or catch ValueError here, and add regression tests for current-directory and root output paths.
There was a problem hiding this comment.
Fixed in e5fa6df: the --out guard now catches ValueError too. with_name on . or / (no file name) exits 2 with a usage diagnostic, no traceback. Tests --out . and --out / (red before).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
A bug and a nit in the new --out handling; the rest of the delta since 778a1ac holds. The path checks added for the bot findings crash on --out . and --out /, which exited 2 before.
What I ran, on a Linux review host at 842720f:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, conda, semgrep and other-Python legs not run locally); changed-line coverage passes.- Reproductions for the code findings, with the LLM transport mocked:
dokimi subjects evidencewith--outof.,/,..and an empty string. - Mutation run on the new delta: 11 mutants, 8 killed by the test files. The survivors are the
RuntimeErrorcatch (the symlink-loop test passes without it on Python 3.13) and the one-letter drive-letter exclusion in the scheme regex.
Checked, no issues
- Pins: dropping
is True, the.rejectedsibling check, theArtifactValidationErrorhandler and the scheme check each fail a test. - Baseline field: the scorecard test now calls
build_subject_scorecardand assertsbaseline_arm. - Commits: all 7 carry
Signed-off-by.
Bot points I checked that no longer apply at this head (7)
- Copilot's and exai-bot's points on a
cells: [null]or other wrong-typed nested value do not apply: the build is wrapped and exits 2 with a diagnostic. - Copilot's point on non-object
summary,per_arm, row orsignificancevalues does not apply: they exit 2 with a diagnostic at this head. - exai-bot's point on a non-local ref taking a digest from a local path does not apply at this head: a URI-scheme ref returns None before any filesystem lookup.
- exai-bot's point on
significant_adjusted: "false"givingimproveddoes not apply: the verdict needs the booleantrue. - exai-bot's point on a string
nfailing the evidence schema with a traceback does not apply:ArtifactValidationErrornow exits 2. - exai-bot's point on a symlink-loop
--outdoes not apply:RuntimeErroris caught and exits 2. - exai-bot's point on the
.rejectedsibling deleting the scorecard does not apply: that--outis refused.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
with_name() on a path with no file name raises ValueError, which the --out guard did not catch. It now exits 2 with a usage diagnostic. Tests: --out . and --out / refused (red before). The drive-letter scheme pin is Anton Volkov's test on the branch (541b928). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The delta since 842720f is one except tuple and two tests, and it fixes both open threads: --out . and --out / now exit 2 with a usage diagnostic, and the drive-letter exclusion in the scheme regex is pinned.
What I ran, on a Linux review host at e5fa6df:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, conda, semgrep and other-Python legs not run locally); changed-line coverage passes.- Mutation run on the delta: removing
ValueErrorfrom theexceptfails both--outcases; changing+to*in the scheme regex fails the drive-letter test. - Reproductions with the LLM transport mocked:
dokimi subjects evidencewith--outof.,/and..all exit 2 without a traceback.
Checked, no issues
- Commits: all 9 non-merge commits carry
Signed-off-by. - Schemas and CLI snapshot:
schema_compat, the CLI snapshot check and the registry checks pass.
Bot points I checked that no longer apply at this head (8)
- Copilot's and exai-bot's points on a
cells: [null]or other wrong-typed nested value do not apply: the build is wrapped and exits 2 with a diagnostic. - exai-bot's point on a non-local ref taking a digest from a local path does not apply at this head: a URI-scheme ref returns None before any filesystem lookup.
- exai-bot's point on
significant_adjusted: "false"givingimproveddoes not apply: the verdict needs the booleantrue. - exai-bot's point on a string
nfailing the evidence schema with a traceback does not apply:ArtifactValidationErrornow exits 2. - exai-bot's point on a symlink-loop
--outdoes not apply:RuntimeErroris caught and exits 2. - exai-bot's point on the
.rejectedsibling deleting the scorecard does not apply: that--outis refused. - exai-bot's point on
--out .or--out /raisingValueErrordoes not apply:ValueErroris caught and exits 2.
# Conflicts: # dokimi/changelog.d/20261031_120000_napetrov_distribution-digest.md # dokimi/dokimi/subjects/scorecard.py # dokimi/tests/test_subjects_digest.py
| if stale_real == card_real: | ||
| usage_exit(f"{out}.rejected is the scorecard; a save to {out} would delete or overwrite it; pick another --out") |
There was a problem hiding this comment.
Must-fix: the rejected-sibling protection still misses hard links. Take the schema-valid scorecard used by the string-n rejection test, run ln sc.json evidence.json.rejected, then dokimi subjects evidence sc.json --out evidence.json. The resolved paths differ, so this passes the guard; validation fails and save_artifact writes the rejected payload with rejected.write_text(...), truncating the shared inode and replacing the input scorecard with evidence. Check existing rejected siblings with samefile() as well as resolved-path equality, and add a regression asserting the scorecard bytes remain unchanged. The main output write uses atomic replacement, so an ordinary hard-linked --out does not have this problem.
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