Repository navigation
fix(novelty-gate): measure the unmeasured share over readable words, and stamp the basis - #101
Conversation
…and stamp the basis The gate's artifact_coverage divided by every bundled word on disk, role decisions included, while the analyzer's warning divides by readable words: an entry-point run of 400 entry words, a 500-word reference and 1,000 words of evals passed the gate at 26% while the analyzer warned 56%. - Bundle records are written with coverage_basis: "readable", and the gate judges the basis the record names; its check carries it as `basis`. - A record without the field (every artifact stored before it) is read as all_bundled_words, so re-gating a stored artifact keeps its value. Any other value the gate does not know is reported as not measured. - On the readable basis the gate's message names the files the word budgets excluded and the role decisions separately. - schemas/novelty.v1.json: bundle.coverage_basis, optional, enum. - The coverage parity test keeps its golden capture of main for unstamped records and pins the stamped gate values separately; the record counts, analyzer warnings and report still equal the golden. The guards test's policy-rows case pins all three bases (unstamped and legacy 0.4945, readable 0.9783). On 71 public skills, freshly measured entry-point runs move up on 10 (all with their own evals), by +0.040 to +0.134; none crosses the 0.5 ceiling and no bundle run moves. With the stamp removed the gate's output on the same 142 records equals main's. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 90.30% 90.29% -0.01%
==========================================
Files 206 206
Lines 24356 24376 +20
Branches 3923 3927 +4
==========================================
+ Hits 21995 22011 +16
- Misses 1695 1698 +3
- Partials 666 667 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Explicit null coverage stamps produce measured legacy results instead of withholding judgment.
1 open issue
What changed in this PR
Aligns the novelty gate with readable-word coverage, completing the basis change deferred from #44 while preserving legacy artifact values.
Changes:
- Stamps coverage records and scorecard checks with their basis.
- Updates gate arithmetic and exclusion messages.
- Adds regression coverage, schema documentation, and release notes.
| File | Description |
|---|---|
| dokimi/tests/test_novelty_coverage_parity.py | Checks legacy parity and readable-basis results. |
| dokimi/tests/test_novelty_bundle.py | Tests stamps, gate messages, and schema validation. |
| dokimi/tests/test_novelty_bundle_guards.py | Tests basis handling and policy exclusions. |
| dokimi/dokimi/schemas/novelty.v1.json | Defines the optional coverage basis. |
| dokimi/dokimi/novelty/bundle.py | Writes and reads coverage stamps. |
| dokimi/dokimi/gate/novelty_gate.py | Judges and reports the recorded basis. |
| dokimi/docs/skill-novelty.md | Explains coverage and legacy compatibility. |
| dokimi/changelog.d/20261018_120000_napetrov_novelty-coverage-basis.md | Documents the numerical change. |
Review effort: Balanced
…sis key Reserve the legacy basis fallback for truly absent coverage_basis keys only. Treat explicit `None` as an invalid stamp to prevent ambiguity between missing data and intentional null values. Add regression test for None stamp. 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.
Review at 8d8804e. The change does what it says: the stamp round-trips through the bundle block, an absent stamp reads as all_bundled_words, and an unknown or explicit null stamp is reported as not measured. One test gap is worth closing before merge: the gap/role-decision counts in the new message are pinned by a single symmetric fixture. Three nits inline.
What I ran, on a Linux review host at 8d8804e:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network or not run on a PR);schema_compatnotice only for the new optionalbundle.coverage_basis.- Reproductions for the gate behaviour, with the LLM transport mocked: 400 entry words, 500-word reference, 1,000 words of evals gives 0.2632 unstamped and 0.5556 stamped
readable, as the description says; bundle run with a 5,000-word reference gives 0.8475 and 0.7246. - A mutation run: 44 mutants, 9 survive the three changed test files (the ones described inline; one is equivalent).
Checked, no issues
- Stamp values:
None,"",5,["readable"],{},Trueand"READABLE"all givemeasured=False, passed=Truewith a message naming the stored value; none is read as legacy. An unknown stamp wins over the unreadable-file message and never reachesdenominator(). - Stamp coverage:
Bundle.summary()writes it, andatoms.jsonandprobes.jsontake their bundle block from it. - Schema: the new property is optional with an enum, the object stays open, and older records still validate.
- Consumers: the scorecard's
gateobject is open,basisis emitted only when set, and no reference toGATE_COVERAGE_BASISremains.
| decided = (f", {coverage.decision_files} not measured by role" | ||
| if coverage.decision_files else "") | ||
| mode = (f"the bundle was measured ({coverage.n_files} files, " | ||
| f"{coverage.resource_words_measured} of " | ||
| f"{coverage.measurable_resource_words} readable bundled words, " | ||
| f"{coverage.gap_files} files excluded by the word budgets{decided})") |
There was a problem hiding this comment.
The readable-basis message counts are pinned by one symmetric fixture, so swapping gaps and role decisions passes the suite.
The only assertion on this message is test_the_gate_counts_only_gaps_as_excluded_files (test_novelty_bundle.py:605), whose bundle has exactly 1 word-budget gap and 1 role decision. The parity cases do not assert the message.
Mutation check, each run against test_novelty_bundle.py, test_novelty_bundle_guards.py and test_novelty_coverage_parity.py (135 passed every time):
- print
coverage.decision_fileswheregap_filesis printed; - print
gap_filesin thedecidedsuffix; - set
gap_files=len(decisions)inCoverage.from_record; - change the guard to
if Trueondecision_files, which prints ", 0 not measured by role" for a bundle with no evals.
Miscounting these two is the bug this message change exists to fix.
Suggestion: add a case with 2 gaps and 1 decision, and one with 1 gap and no decision, and assert the full parenthesis.
There was a problem hiding this comment.
Fixed in f3007a5: the single symmetric fixture could not tell the counts apart, so test_the_gate_message_keeps_gaps_and_role_decisions_apart uses 3 word-budget gaps and 2 role decisions and asserts '3 files excluded by the word budgets, 2 not measured by role'. Swapping the two counts now fails.
There was a problem hiding this comment.
Already addressed in f3007a5: the test test_the_gate_message_keeps_gaps_and_role_decisions_apart uses 3 gaps and 2 role decisions and asserts the full parenthesis, so swapping the counts fails. No change in this pass.
The readable-basis message now correctly uses 'file' singular for gap_files==1 and 'files' plural otherwise, instead of always saying '1 files excluded...' Fixes grammar issue reported in code review. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Add tests to verify that: - Non-measured checks (unknown basis) omit the 'basis' key from as_dict() - Measured coverage checks include the stored basis value in as_dict() These mutations were previously undetected by the test suite. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…stamp Qualify the line about records being written 'exactly as before' to clarify it applies only until the coverage-basis change in this release. This resolves the contradiction: a freshly written entry-point record now gates at 0.5556 (readable basis) instead of 0.2632 (all words). Both fragments ship in the same release, so marking the timeline boundary. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The test for as_dict() basis serialization was checking the wrong behavior. The actual contract is: - When basis is None, omit the 'basis' key from as_dict() - When basis has any value (including unknown), include it Tests now correctly verify both cases: one with basis=None and one with basis=<value>, covering the mutation detection gap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The gate message now correctly pluralizes 'file(s)' based on count. Update the test assertion to expect 'file' instead of 'files' when count==1. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ate message; untrack .venv Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
Review effort: Lite
| def _stored_basis(value: Any) -> str: | ||
| """A record's ``coverage_basis``: :data:`LEGACY_COVERAGE_BASIS` only when it is absent. | ||
|
|
||
| Any other value is kept as written, so a stamp this version does not know -- an empty | ||
| string, a number -- is reported as unknown rather than read on the legacy basis. | ||
| """ | ||
| if value is None: | ||
| return LEGACY_COVERAGE_BASIS | ||
| return value if isinstance(value, str) else repr(value) |
There was a problem hiding this comment.
Declined. Tracking the raw stamp type separately (e.g., basis_raw_repr, basis_raw_type) would require extending the Coverage, record schema, and gate logic—a more invasive refactoring than fits this PR's scope of measuring the unmeasured share. This is a good candidate for a follow-up enhancement when more type-aware coverage reporting is desired.
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 three code and doc threads from my last review are fixed: the gap and role-decision counts are pinned apart, the singular is handled, as_dict serialization of basis is tested, and the older changelog fragment no longer contradicts the new one. One test gap from the first thread is still open, and one nit inline.
What I ran, on a Linux review host at 17bdfb3:
make ci-local: 19 passed, 0 failed, 4 skipped (docker/network or not run on a PR);schema_compatnotice only for the new optionalbundle.coverage_basis.- Mutation run on the three changed test files (baseline 138 passed): 12 mutants, 9 caught. Swapping the gap and decision counts,
gap_files=len(decisions), inverting or removing the pluralization, and bothas_dictmutants from my first review now fail. The three survivors are listed below.
Still open from my earlier review:
dokimi/dokimi/gate/novelty_gate.py:322: the "no role decisions" branch of the message is still unpinned. Replacingif coverage.decision_files else ""withif True else ""keeps all 138 tests green, so a skill withoutevals/would print ", 0 not measured by role". Every message assertion has at least one decision. Add the 1 gap and no decision case and assert the whole parenthesis.
Checked, no issues
- Stamp values:
None,"",5, lists, dicts,True,"READABLE"and" readable"all give an unmeasured info check with the stored value in the message; none is read as legacy and none raises. - Legacy path: a record without
coverage_basisstill gates at 0.2632 for 400 entry words, a 500-word reference and 1,000 words of evals; stampedreadablegives 0.5556. - Consumers: the scorecard readers use only
passed,severity,nameandmessage,gateis an open object in the scorecard schema, andGATE_COVERAGE_BASIShas no remaining references in code or docs. - Writers:
Bundle._counts()stampsreadable, andatoms.jsonandprobes.jsontake it throughsummary().
| A record without the stamp, which is every artifact stored before this release, is still | ||
| gated on all bundled words, so re-gating it gives the value it had; a stamp this version | ||
| does not know is reported as not measured. Do not compare `artifact_coverage` values |
There was a problem hiding this comment.
Nit: "A stamp this version does not know is reported as not measured" only holds for a record that skips schema validation. novelty.v1.json limits bundle.coverage_basis to readable or all_bundled_words, and load_artifact("novelty", path) validates by default (novelty_track.py:108). Validating a stored record with coverage_basis: "tokens" against the schema gives bundle/coverage_basis: 'tokens' is not one of ['readable', 'all_bundled_words'] so the loader should reject it before the gate sees it. The "not measured" row is reached only by calling evaluate_novelty directly.
Suggestion: say so in the sentence ("evaluate_novelty reports it as not measured; schema validation rejects it"), or drop the enum if a later version is meant to add a basis that older readers should degrade on.
There was a problem hiding this comment.
Already reflected on this branch: the fragment now says the stamp is "reported as not measured by evaluate_novelty, and rejected by schema validation when the record is loaded". No change in this pass.
…no role decisions The changelog now says a stamp this version does not know is rejected on load, not only reported as not measured. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…agment Main's intel#101 grew dokimi/novelty/bundle.py to 1016 lines and evaluate_novelty to complexity 29 after the baseline was cut, so the ratchet failed on code this PR does not touch. Raise both entries with a rationale, as the intel#72 entry already does. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Part of #4.
Follow-up to #44.
Summary
The gate's
artifact_coveragenow judges the share the analyzer warns about: readable words, so a skill's ownevals/(and any--resource-rolesfilter) are left out of the denominator. #44 moved the coverage arithmetic into oneCoverageand kept the gate's numbers. This PR is the number change it left out, and every record now says which basis it was written for.Evidence
Before (
main). The skill has 400 entry-point words, a 500-word reference and 1,000 words of evals. The bundle run adds a 5,000-word reference, which is over the per-file budget.After:
71 public skills (intel/skills, mattpocock/skills), measured fresh on
mainand on this branch:evals/The largest move is
onetbb-quickstart, from 0.1509 to 0.2852 (968 words of evals). The other nine are eight dpnp skills (+0.040 to +0.069) andmkl-extension-advisor(0.6506 to 0.6943, which was a WARN before and still is).Stored artifacts do not move. On the same 142 records with the stamp removed, which is how every artifact stored before this PR looks, the gate's output equals
main's. The only difference is that each check now also carriesbasis: "all_bundled_words".Tests. On
main, the stamped cases fail because the gate reads demo/entry as (True, 0.2632). On this branch they pass.test, docs: leftovers from the reviews of #43, #44, #59, #69 and #75 #87's other pinned gate figures are on unstamped records, so they hold unchanged. That is the guarantee this PR makes.
Local run of every PR CI step (
make ci-local --offline): 0 failed, 14 passed, 7 skipped (the skipped ones need network or docker, or do not run on a PR).ruff, strictmypy,schema_check,mkdocs build --strict, the changelog check, andpre-commiton the changed files: all passschema_compatagainstmain: OK. Its only notice is the new optionalbundle.coverage_basis.Overlap with fix(novelty): a partial conflict check is not a cleared harmful_atoms pass #72:
git merge-treeagainst its head is clean, and the novelty tests pass on the merged tree (228 passed).PR CI: 19 checks pass, including Tests on py3.11 to py3.14, Schema compatibility, the static checks and the changelog check; 2 are skipped (Pages deploy and latest-deps run only on main).
Merge Danger
Door: one-way for stored comparisons, mitigated by the stamp. Once records are written on the readable basis, their
artifact_coveragecannot be compared with values on the old basis. The stamp makes that visible: an unstamped record reads as before, and each check'sbasissays which basis a value is on. Reverting this PR would leave stamped records that an older gate reads on all bundled words.Blast Radius: gate
artifact_coverageon newly written artifacts divides by readable words only. Entry-point runs of skills with their own evals move up: on the 71-skill sweep, 10 of 71 moved by +0.040 to +0.134, and no pass/fail changed. A skill whose evals are a larger share of its words can flip an entry-point run from pass to WARN, as in the example above. The gate never blocks, so this changes a warning, not a failure.coverage_basisis read asall_bundled_words. Any other unknown value, such as"", a number or a misspelling, is reported as not measured and is not read as legacy.novelty.v1.jsongainsbundle.coverage_basis, which is optional and limited toreadableorall_bundled_words.atoms.jsonandprobes.jsoncarry it too, throughBundle.summary(). The scorecard's gate check gainsbasis.Changed.Checklist
pytestpassesruff,mypy, and schema checks pass.github/.licenserc.yaml)🤖 Generated with Claude Code