Repository navigation
Conversation
…yze headline The headline printed the added and already-knew counts but not how much of the artifact the run read, so an entry-point run of a progressive-disclosure skill quoted rates that describe the entry point as if they described the skill. The headline now prints the measured share of readable words on stdout, and warns on stdout when a run read half or less of them. The novelty.v1 bundle record stamps the share as bundle.measured_share. The stdout warning uses the line the analyzer has always warned at (an outside share of 0.5 or more), so the stdout and stderr warnings agree and the coverage parity golden keeps its value. The settings parity module leaves out the new stdout lines when it compares with main; the journey test pins them instead. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ity checks the headline
- novelty.v1 bundle: declare measured_share (number or null, 0 to 1) beside
measurable_outside_share; the record already stamps it.
- Add the scriv fragment the package change needs: Added for the headline
coverage line and bundle.measured_share, Changed for the stdout warning.
- docs/skill-novelty.md: "a run that read half or less ... says so on stdout"
was ungrammatical ("an run"); split it into its own sentence.
- settings parity: the coverage and WARNING lines are no longer dropped
unchecked. Main's golden cannot hold them, so each analyze step checks its
stdout coverage lines against the measured_share its own novelty.json
stamped: the percentage, and the WARNING exactly when the share is 0.5 or less.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
=======================================
Coverage 91.98% 91.99%
=======================================
Files 223 223
Lines 27743 27768 +25
Branches 4560 4565 +5
=======================================
+ Hits 25520 25545 +25
+ Misses 1574 1573 -1
- Partials 649 650 +1
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
There was a problem hiding this comment.
Review at db55ba0. The record field and the schema widening are right, and the stderr output is unchanged from the base. The stdout percentage is wrong for three shares, and the new stdout warning fires for runs that never warned before. The percentage bug and the risk inline are worth fixing before merge, as are the three points below.
What I ran, on a Linux review host at db55ba0:
make ci-local: 20 passed, 8 failed, 7 skipped. All 8 failures are exit 127 (ruff, zizmor, pre-commit, bandit, mypy, deptry, mkdocs, pip-audit missing from 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. The changelog check and the test jobs passed.- The four touched test files: 158 passed.
- Reproductions for the code findings with synthetic bundle records, no model calls.
- Mutation run on the new tests: the
<=/<boundary,int/round, the threshold, the scope suffix and the>=/>stderr condition are all killed. Five mutants pass the full suite: always appending the--include-resourcesadvice, droppingcoverage.include_resourcesfrom the advice condition, dropping the zero-words guard, droppinground(..., 4)inmeasured_share, and dropping"null"from the schema type.
Findings that cannot be anchored inline:
- The stdout warning is not "the same runs as before" for
--include-resourcesruns. Before the PR only an entry-point run withoutside_share >= 0.5warned (bundle.py:461)._print_coveragewarns on any run withmeasured_share <= 0.5, bundle runs included. A--include-resourcesrun withmax_file_words=50, a 100-word entry and a 500-word reference measures 100 of 600 words. Head printscoverage: 16% ...andWARNING: only 16% .... The base printed nothing about coverage, and thewarningslist is identical on both. So the changelog line "theWARNING: only N% ...line goes to stdout as well as the stderr warning" and "the same runs warn as before" (changelog.d/...measured-share.md:11-14) are false for budget-truncated bundle runs, and the PR description's "only the channel changed" too. Warning there is arguably right, but say so in the changelog, or restrict the warning to what the stderr one covers. docs/skill-novelty.md:492-496and the schema description do not cover the new cases. The docs say a run that read half or less "says so on stdout", but not thatcoverage: unknownexists, thatbundle.measured_sharecan benull, or that bundle runs now warn. Thebundledescription string innovelty.v1.jsonlists the record's fields and does not namemeasured_share.- Test gap on the advice and the null type. The
; pass --include-resources ...suffix is asserted only where it appears. Mutation check: always appending it, droppingcoverage.include_resourcesfrom its condition, and droppingor not coverage.measurable_resource_wordsall pass the full suite (158 passed in the four touched files). A budget-capped--include-resourcesrun could then tell the user to pass the flag they already passed. Removing"null"from themeasured_sharetype in the schema also passes, although the code emitsNonefor an undecodable file. Add a CLI test for a bundle run at 30% that asserts the line has no--include-resourcesadvice, and a schema validation of a record withmeasured_share: null.
Checked, no issues
- Consistency:
measured_shareequals1 - measurable_outside_sharefor entry-point runs, as the docs say, and(entry + measured) / (entry + measurable)for truncated bundle runs. - Stderr: the
warningslist and stderr output are identical to the base in every scenario I ran.1 - 0.5is exactly 0.5, so>=has no float edge. - Schema: an optional property added in place; old artifacts still validate. No version bump is needed under AGENTS.md.
- Consumers: nothing in the repo parses the headline by position.
_print_headlinehas one caller. - Repo rules: the changelog fragment follows
changelog.d/README.md, and the new tests are offline.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/novelty/bundle.py
|
Review-body items from the review at db55ba0, in 3872e1c:
|
…are keeps scope and warning 0.29, 0.57 and 0.58 printed one too low (float product). An undecodable file printed coverage: unknown with no scope and no warning. Docs, changelog and the schema description now cover the bundle-run warning and the null share. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…e; chore(growth): raise commands/novelty.py for W5 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
There was a problem hiding this comment.
The three points from my earlier review are fixed: the percentage floors the stored 4-place share, the coverage: unknown line keeps its scope and warns, and the comment is accurate. One risk and three nits remain, all inline.
What I ran, on a Linux review host at 62b87f9:
make ci-local: 28 passed, 1 failed, 7 skipped. The failure ispip-audit(gitpython 3.2.0, CVE-2026-114145); the PR changes no dependency or lock file, so it is not from this PR. Tests, changed-line coverage, ruff, mypy, schema_compat, the docs build and the changelog check pass.- Reproductions with synthetic bundle records, no model calls.
- A mutation run over
_print_coverage,Coverage.measured_share, thebundle.pythreshold and the schema: 26 mutants, 6 of which pass the five touched test files.
Checked, no issues
- Test move:
test_novelty_bundle_key.pyholds all 22 items removed fromtest_novelty_bundle.pyunchanged (compared withast), plus the three new tests. - Percentage:
round(share * 10000) // 100is right for 29, 57, 58 and 49; the stored share is rounded once, so there is no double rounding. - Stderr and schema: the
warningslist is unchanged;measured_shareis an optional nullable property added in place, so no version bump is needed. - Docs claims: the changelog now names the two cases where stdout warns more widely than stderr, and the code agrees.
|
|
||
| # A file that did not decode leaves the share unknowable, and the headline says so, | ||
| # with the scope and, while the entry point is half or less of the words, the warning. | ||
| novelty_cmd._print_headline(dict(artifact, bundle=dict(record, unreadable_files=1))) |
There was a problem hiding this comment.
The unknown-share branch and its advice guards are only half pinned. Mutation check: each of these passes the five touched test files (165 passed):
- replacing
not coverage.include_resourceswithTruein thecoverage: unknownwarning (novelty.py:850-852). An--include-resourcesrun with an undecodable file and a small entry point would then print "the entry point is half or less of the artifact's readable words", which is false for a bundle run; - changing
>=to>in the same condition. No test uses an entry point of exactly half the words, so stdout would stop warning where the stderr warning (bundle.py:224,>=) still fires; - dropping
or not coverage.measurable_resource_wordsfromrerun. An entry-point run whose bundled files are all policy-excluded would be told to pass a flag that reads nothing more; - dropping
round(..., 4)inCoverage.measured_share. The record then stores 0.2857142857 and every test follows it.
Suggestion: add cases to this test and the parametrized one: unreadable_files=1 with include_resources=True and entry_point_words=10 asserts no WARNING; unreadable_files=1 with 50 entry words and 50 on disk asserts the warning; an entry-point run with policy_excluded_words equal to the resource words asserts no pass --include-resources; and a record with 2 entry words and 7 resource words asserts bundle["measured_share"] == 0.2222.
There was a problem hiding this comment.
Added in 8739308: test_an_undecodable_file_in_a_bundle_run_does_not_claim_an_entry_point_only_warning (include-resources with an undecodable file prints no WARNING), test_an_unknown_share_warns_at_exactly_half_as_the_stderr_warning_does (50 of 100 words warns), and test_the_stored_measured_share_is_rounded_to_four_places (2 entry + 7 resource words stores 0.2222). Verified each fails with the guard changed (True for the include-resources check, > for >=, no round). For the policy-excluded 'rerun' term: it cannot matter, since a known share only warns when readable words lie outside the entry point, and the unknown-share warning needs the same, so measurable_resource_words is never 0 there; I removed the dead condition instead of testing it.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
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 c9e5a77, which fix the three nits from my review:
d7db0f4:dokimi/dokimi/commands/novelty.py:861: a share above 0 and under 0.01 now prints<1%, with a test for 1 entry word in 10,000.95b8380:dokimi/scripts/growth_baseline.json:3: the rationale now reads "stdout coverage line and warning (_print_coverage); split tracked separately".c9e5a77:dokimi/docs/skill-novelty.md:499: the doc says1 - measurable_outside_shareto 4 places, and the schema saysmeasured_shareis null when a file did not decode or there are no readable words.
make ci-local at c9e5a77: 28 passed, 1 failed, 7 skipped (the failure is pip-audit, as at the reviewed head).
Not approving yet. Still open: the risk from my review.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/scripts/growth_baseline.json
…asured_share Cover the include-resources guard and the exact-half boundary of the coverage: unknown warning, and the rounding of the stored share. Drop the unreachable measurable_resource_words term from the rerun advice. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/scripts/growth_baseline.json
Part of #4.
Depends on #166: both touch
commands/novelty.py, so this rebases cleanly only after #166 merges.Summary
novelty analyzestates on stdout how much of the artifact's readable words its run measured, and warns on stdout when that is half or less.The threshold is the one the analyzer already used for its stderr warning, so the same runs warn as before. Only the channel changed.
Evidence
Five new tests fail against
intel/main(commands/novelty.pyandnovelty/bundle.pyreverted, the rest of the branch kept); the other 153 in the four touched test files pass there:Local checks on the rebased head:
Merge Danger
Door: two-way. The change adds a line and a record field; nothing is computed differently.
Blast Radius: low. The stderr warning and the threshold are unchanged.
analyzeheadline gains acoverage:line, and a run at or under half gains a secondWARNING:line on stdout. A parser that reads the headline by position needs updating.bundle.measured_shareis added to the novelty.v1 bundle record and to the schema. It isnullwhen a bundled file did not decode, so readers must allow null.--include-resourcesdefault. The PR proposes it as a follow-up and does not make it. Turning it on would change every entry-point number and the cost of every analyze run, so it needs a decision of its own.Checklist
pytestpassesruff,mypy, and schema checks passdocs/skill-novelty.md(bundle section)🤖 Generated with Claude Code