Repository navigation
Conversation
…ll-vs-all estimate Four gaps in `novelty routing`: - Same-name catalog copies with different content emptied the field with no hint. The error now names the conflicting skills, the directory of each copy, and the `--catalog <dir>` to route with. `--prefer-path <dir>` keeps the one copy under <dir> when the copies there agree. - `--all-vs-all --queries-by-skill` aborted when the queries named a skill the field left out. Those queries are dropped with a WARNING naming the flag that keeps the skill; the run is refused only when no query is left. A name no rule explains is still refused, as a likely typo. - `--all-vs-all --estimate-only` was refused. It now prints the same JSON shape as the single-artifact estimate, with zero model calls. - A missing `--queries-by-skill` and `--out` are reported in one error. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ped queries
Self-review fixes to the routing gaps change:
- Rename --prefer-path to --keep-copies-under DIR. It keeps only the copies under DIR,
relative to --catalog; an empty, absolute, escaping or non-directory value exits 2.
- Record keep_copies_under in the saved field, and list dropped all-vs-all queries as
field.dropped_queries ({skill, reason, keep_flag, queries}) instead of free-text left_out.
- The "leaves out" list names only skills the field does not have.
- A copy at the catalog root is named as the catalog root, not as <catalog>/.
- A queried catalog skill that failed to load is refused with the load error, not as a
misspelling.
- The conflict note points to --keep-copies-under.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi novelty routing` parameters changed:
+ added option `--keep-copies-under`Updated on every push; the |
|
OpenCodeReview: Commit 027e4d3 was not reviewed. This PR has reached its review limit (6 reviews). CI at `027e4d3`: 7 still running, 7 passing, 2 skipped
To review this commit: re-request review from exai-bot in the Reviewers list. <details><summary>Last review (5d38e30): no findings</summary> Open threads from the bot: 0 </details> |
There was a problem hiding this comment.
🟡 Changes recommended
Estimate-only preflight accepts duplicate skills that measured runs reject, while root-copy diagnostics and documentation incorrectly rule out a valid copy-selection option.
2 open issues
What changed in this PR
2 new findings.
This PR improves novelty routing when catalog copies conflict or queries target excluded skills, and adds cost estimates for all-vs-all runs.
Changes:
- Adds
--keep-copies-underand more specific catalog conflict diagnostics. - Drops queries for excluded skills with warnings and saved reasons.
- Enables all-vs-all estimates and reports missing required flags together.
| File | Description |
|---|---|
dokimi/tests/test_skill_repo_shapes.py |
Updates excluded-skill warning expectations. |
dokimi/tests/test_novelty_routing_gaps.py |
Adds routing regression coverage. |
dokimi/tests/test_novelty_routing_field.py |
Updates field exclusion checks. |
dokimi/tests/test_novelty_routing_all_vs_all.py |
Updates query handling and saved-field tests. |
dokimi/tests/data/cli_flags_allowlist.txt |
Registers the new copy-selection flag. |
dokimi/scripts/growth_baseline.json |
Lowers the command module’s size baseline. |
dokimi/dokimi/novelty/routing.py |
Integrates copy selection and exclusion diagnostics. |
dokimi/dokimi/novelty/catalog_keep.py |
Validates and resolves copy-selection rules. |
dokimi/dokimi/commands/novelty.py |
Wires the flag, estimates, and query filtering. |
dokimi/dokimi/commands/novelty_routing_report.py |
Adds diagnostics, estimates, and field metadata. |
dokimi/dokimi/commands/novelty_routing_inputs.py |
Passes copy rules and diagnostic details through loaders. |
dokimi/docs/skill-novelty.md |
Documents the updated routing behavior. |
dokimi/docs/reference/cli-interface.json |
Records the new CLI option. |
dokimi/changelog.d/20261104_120000_napetrov_routing-gaps2.md |
Summarizes routing additions and fixes. |
Review effort: Balanced
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 94.78% 94.80% +0.02%
==========================================
Files 353 355 +2
Lines 46527 46765 +238
Branches 7967 8011 +44
==========================================
+ Hits 44100 44335 +235
- Misses 1540 1541 +1
- Partials 887 889 +2
🚀 New features to boost your workflow:
|
A SKILL.md one directory beneath the catalog (review/SKILL.md) was reported as a copy at the catalog root, so the conflict hint said to dedupe or rename it. --keep-copies-under review selects it. The hint now names that directory as the keep path and gives the dedupe advice only for a SKILL.md in the catalog root. The root-copy test wrote a skill directory, not a root SKILL.md, so it pinned the bug; it now writes the root file, and a test routes the suggested keep. --all-vs-all --estimate-only priced a field that names a skill twice, which the measured run refuses. The estimate now runs the same check, check_field_names, before it prices the calls. Thread: PRRT_kwDOVBg7g86zv_GG Thread: PRRT_kwDOVBg7g86zwAGs Thread: PRRT_kwDOVBg7g86zwAH5 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
No change here: this review is only the bot marker. Its finding is the thread above, fixed in 9545b30. No change here: this overview restates the two threads above, fixed in 9545b30 and 9545b30. No change: this is the automated CLI notice. The flag it lists is registered in the CLI allowlist (dokimi/tests/data/cli_flags_allowlist.txt:96). No change here: this summary restates the thread above, fixed in 9545b30. No change: this is a Codecov setup notice, not a code claim. No Codecov check runs on this PR, and the PR changes no Codecov setup. |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 631fca0. The routing changes hold up: --keep-copies-under validates its value, the conflict error names each copy's directory, and --all-vs-all --estimate-only prices the same field the run measures. One risk before merge: no test checks which copy --keep-copies-under keeps. Two nits inline, and one more on the empty-query-file message.
What I ran, on a Linux review host at 631fca0:
make ci-local: 30 passed, 0 failed, 7 skipped (jobs that need another Python, a conda or semgrep install, or a push event). Changed-line coverage gate passes.- Reproductions for the findings, with the LLM transport mocked.
- A mutation run: 40 mutants of the new source, 36 killed, 4 survive the full routing suite, and so does one more (a duplicated validation call). The two that matter are inline.
--help,docs/reference/cli-interface.jsonregenerated from the head tree (identical), changelog fragment format, growth baseline fornovelty.py(1019 lines, equal to the new baseline), all three commits haveSigned-off-by.
Nit on the empty-query-file message. routing --all-vs-all --queries-by-skill q.json with q.json = {} fails with no skill in the field has queries to route: the queries name only skills the field leaves out (`off` sets disable-model-invocation; ...; `review` has copies with different content; ...). The file names none of those skills, and with no skill left out the parenthesis is empty. The base printed only the first clause. Suggestion: add the parenthesis only when queries_by_skill is non-empty and names a dropped skill (novelty_routing_report.py:260).
Checked, no issues
- Validation: an empty, absolute,
.., file or missing--keep-copies-undervalue, and the flag without--catalog, each exit 2 with the documented reason. A trailing slash and a skill directory directly under the catalog work. - Saved field:
field.keep_copies_underandfield.dropped_queriesappear; comparing JSON key paths of base and head, nothing is removed.schemas/novelty.v1.jsonhas no routing definition, so the schema-versioning rule does not apply. - Estimate:
--all-vs-all --estimate-onlyexits 0 without--out, makes no model call and writes no file; a duplicate name is refused as in the run. - Earlier bot and reviewer threads: the root-copy hint and the duplicate-name check in the estimate are both fixed at this head.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| ``{"skill", "reason", "keep_flag", "queries"}``. | ||
| """ | ||
| names = {c.name for c in field} | ||
| unexplained = sorted(n for n in queries_by_skill if n not in names and n not in dropped) |
There was a problem hiding this comment.
Must-fix: preserve load-error refusal when an excluded copy shares the name. Trigger: a catalog with two enabled skills (cache, queue), malformed YAML at broken/review/SKILL.md, a disabled off/review/SKILL.md, and queries for cache and review. Evidence: catalog scanning records the malformed copy in unloaded['review'] and the disabled copy in dropped['review']; this condition excludes review from validation solely because it is in dropped, so its queries are discarded and the run succeeds instead of reporting the load error promised in the docs. Impact: incomplete query coverage with a misleading disabled-only explanation. Check absent field names in unloaded before applying rule-based dropping, and add a mixed-copy regression test. Workaround: repair the malformed copy before running. Severity is medium because this is a bounded input-validation and saved-result correctness failure.
| raise ValueError("--keep-copies-under is an absolute path: give a directory relative to " | ||
| f"--catalog: {keep}") | ||
| base = Path(root).resolve() | ||
| target = (base / keep).resolve() |
There was a problem hiding this comment.
Must-fix (medium). Impact: invalid copy-selection input escapes the CLI's input-error handling and produces a traceback instead of an exit-2 validation message. Trigger: on supported Python 3.11, create catalog/loop -> loop and pass --catalog catalog --keep-copies-under loop. Evidence: Path.resolve() raises RuntimeError for a symlink loop; routing_field() catches only ValueError here, its loading wrappers catch OSError, ValueError and RecursionError, and the CLI catches InputError, so this exception is not translated. Wrap resolution and translate its symlink-loop RuntimeError to ValueError, with a regression test. Workaround: choose a non-cyclic directory. Severity justification: this is a bounded, concrete invalid-input failure affecting both measured and estimate paths, not normal routing.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 5b5047c, which fix the three nits from my review:
00f6ebf:dokimi/tests/test_novelty_routing_gaps.py:207: the dropped skill now has two queries; the test assertsqueries == 2in the record anddropped 2 queries ofin stderr.36c27c9:dokimi/tests/test_novelty_routing_gaps.py:139: the second_parsecall passes--outundertmp_path, so noresults/is written to the working directory.5b5047c:dokimi/dokimi/commands/novelty_routing_report.py:259: the left-out clause is added to the error only when the queries file names skills.
make ci-local at 5b5047c: 29 passed, 1 failed (timing flake, passes on rerun), 7 skipped.
Not approving yet. Still open: the risk from my review.
A mutant that keeps the copies outside the directory passed the whole routing suite: the competitor names are the same for both copies. Thread: PRRT_kwDOVBg7g860DRpM Review: PRR_kwDOVBg7g88AAAABWNTUWQ Review: PRR_kwDOVBg7g88AAAABWMe6eQ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A name with a malformed copy and a copy a rule left out was exempt from the unexplained-name refusal, so its queries were dropped and the run succeeded with a disabled-only explanation. A name in the unloaded map is refused with its load error. Thread: PRRT_kwDOVBg7g860Dnta Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
On Python 3.11 and 3.12 Path.resolve() raises RuntimeError for a symlink loop, which the CLI does not translate, so the run printed a traceback. The check now reports a loop as a ValueError (exit 2) on every version. Thread: PRRT_kwDOVBg7g860Dnth Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 8050502. --keep-copies-under keeps the right copy, the all-vs-all estimate prices the field the run measures, and the fixes for the earlier threads hold. The main gap is the conflict hint: for nested folders and for symlinked copies it suggests flags that change nothing, and the flag then says nothing.
What I ran, at 8050502:
make ci-local --offline: 19 passed, 0 failed, 13 skipped (network, Docker and push-only jobs).novelty routing(lexical and--estimate-only, no model call) on scratch catalogs: nested copies (skills/review+skills/legacy/review), aSKILL.mdsymlinked from outside, two conflicting names in four folders, a disabled + spec-error skill, a broken copy next to a loaded one, a missing--catalog. The results are in the line comments.
Checked, no issues
- Earlier threads: a broken copy next to a disabled one is refused with its load error (29c1605); a symlink loop in
--keep-copies-underexits 2 on 3.11/3.12 (8050502); the test pins which copy is kept (f0795d4). - Validation: an empty, absolute,
..or file value, and the flag without--catalog, exit 2 with a clear reason. - Saved field and docs:
keep_copies_underanddropped_queriesonly add keys; no schema covers the routing field.skill-novelty.mdand the changelog fragment describe the new flag and the dropped-query behaviour.
…ng-gaps2 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…r name The conflict hint now names, for each conflicting name, only the directories that hold exactly one of its copies, and says that --keep-copies-under takes one directory. Thread: PRRT_kwDOVBg7g860Lia8 Thread: PRRT_kwDOVBg7g860LibM Review: PRR_kwDOVBg7g88AAAABWQyWpQ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860LibD Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860LibN Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…even when one loaded Thread: PRRT_kwDOVBg7g860LibV Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860Libd Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860Libj Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Thanks: all seven fixed this round, from 337c641. |
…talog copy A broken catalog copy of a name in --competitors was recorded as unloaded, so --all-vs-all refused the run as "half loaded", although the explicit competitor shadows every catalog copy of its name. Main measured it. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A name whose field copy is under --keep-copies-under was still refused as "half loaded" when a copy outside the directory failed to load, although the user named the copy to measure. That broken copy is now left out with a note; a broken copy under the directory, or one with no path, still refuses. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The conflict hint placed a copy by its unresolved path, while the keep rule resolves it, so a copy whose SKILL.md links outside the catalog got a --keep-copies-under suggestion that the run then answered with "kept no copy". Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Covers a name whose copies agree (left alone) and the "kept no copy" note, which must not appear once a name was resolved. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
skill-novelty.md keeps --keep-copies-under under intel#396's `catalog check` name; the growth baseline records novelty.py at its 926 merged lines. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
LGTM
I checked 027e4d3. All seven points from my first round are fixed, and the fixes by @xaleryb (explicit competitor beside a broken copy, a broken copy outside --keep-copies-under, the hint for a symlinked SKILL.md) are right.
Merge: safe now: it merges cleanly into current main, 0 commits behind, and its tests pass at this head. Once it is in, #405, #406 and #403 conflict with it (novelty.py, novelty_routing_report.py, novelty_routing_inputs.py, routing.py, skill-novelty.md, the growth baseline) and need a rebase.
What I ran, at 027e4d3:
make ci-local --offline: 19 passed, 0 failed, 13 skipped (network, Docker and push-only jobs).novelty routing(lexical and--estimate-only, no model call) on the scratch catalogs from my first round: nested copies, two conflicting names, a symlinkedSKILL.md, a broken copy beside a loaded one (with and without--keep-copies-under), a missing--catalog. Each now gives the fixed behaviour.
Checked, no issues
- Conflict hint: for nested copies it suggests only
skills/legacy; for two names it gives the folders per name and says the flag takes one directory; for a symlinked copy it no longer offers the keep that resolves nothing. - Broken copies: a queried name with a broken copy beside a loaded one is refused with the load error;
--keep-copies-underleaves out a broken copy outside it with a note, and still refuses one under it. - Docs and changelog:
skill-novelty.mdand the fragment describe the per-skilldropped_queriesrecord and the broken-copy rule.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 027e4d3. The fixes since my last review hold up: the three nits I raised are fixed, the earlier bot and reviewer points are fixed, and the new rules for a broken copy beside a loaded one behave as documented. One test gap on the broken-copy gate is worth fixing before merge.
What I ran, on a Linux review host at 027e4d3:
make ci-local: 29 passed, 1 failed, 7 skipped; 15,954 tests; the changed-line coverage gate passes. The one failed step is the test run: 4 tests hit time limits (test_the_interval_excludes_zero_exactly_when_p_is_below_005,test_unbalanced_quote_in_hook_completes_quicklyat 1.17 s against a 1 s limit, twotest_audit_test_qualitysetups). None of those files is in this PR's diff, and all three files pass together on rerun (701 passed).- Reproductions for the code findings, with the LLM transport mocked.
- A mutation run: 34 mutants of the new source, 28 killed, 6 survive the four routing test files. One matters (inline); the others change only message text, the order of a saved list, or are unreachable.
- All 19 non-merge commits carry
Signed-off-by.
Checked, no issues
- Broken copy beside a loaded one: a queried name with a malformed copy and a disabled copy (the case in exai-bot's earlier point) is refused with the load error. An explicit
--competitors skill:Xstill shadows a broken catalog copy. A broken copy under--keep-copies-underis refused, and one outside it is left out with a note. - Symlink loop:
--keep-copies-under loopwithcatalog/loop -> loopexits 2 with a usage error and no traceback (run on Python 3.13 only; the 3.11/3.12RuntimeErrorbranch is not exercised in this run). - Conflict hint: for three copies, nested copies, a root
SKILL.mdplus a nested copy, direct skill directories and symlinkedSKILL.mdcopies, every--keep-copies-undervalue the hint offers keeps exactly one copy. - Saved field:
dropped_querieshas one record per skill; with a disabled skill of 3 queries and another of 1,queriesis 3 and 1 and matches the two warnings.docs/skill-novelty.mdand the changelog fragment describereasonsandkeep_flags. The PR description still shows the olderreason/keep_flagshape. - Estimate:
--all-vs-all --estimate-onlygives the call count the measured run makes (4 queries,--repeats 2: 8 calls), needs no--outand writes no file.
Bot points I checked that no longer apply at this head (2)
- exai-bot's point on a disabled copy hiding a malformed copy of the same name does not apply: a name with a copy that failed to load is now refused with its load error even when another copy was left out by a rule, and when another copy loaded.
- exai-bot's point on a symlink loop in
--keep-copies-underdoes not apply: it is translated to a usage error and exits 2 without a traceback.
| return | ||
| prefix = (Path(root) / keep_copies_under).resolve() | ||
| for name, paths in sorted(broken.items()): | ||
| if (name in unloaded and all(p is not None and not under(p, prefix) for p in paths) |
There was a problem hiding this comment.
No test notices if a broken copy under --keep-copies-under is cleared because another broken copy of the name sits outside it.
Mutation check: changing all(...) to any(...) on this line passes all 192 tests in test_novelty_routing_gaps.py, _field.py, _all_vs_all.py and test_skill_repo_shapes.py. Every existing test has one broken copy per name, so all and any give the same answer.
Case: skills/review loads, skills/legacy/review is broken, and .openclaw/skills/review is broken. With --keep-copies-under skills, the broken copy under skills should still refuse the name ("failed to load beside one that loaded"). With the any mutant the name is cleared, and --all-vs-all measures review and exits 0 with a broken copy inside the directory the user chose.
Suggestion: add this test next to the other broken-copy tests:
def test_keep_copies_under_still_refuses_when_a_broken_copy_is_under_it_and_another_outside(tmp_path):
root = tmp_path / "catalog"
_skill(root, "skills", "cache", "cache slow lookups")
_skill(root, "skills", "review", "review a change for bugs")
_broken(root, "skills/legacy", "review")
_broken(root, ".openclaw/skills", "review")
args = _keep_run(root, tmp_path, "--keep-copies-under", "skills")
with pytest.raises(InputError, match="failed to load"):
args.func(args)It passes on the head and fails on the any mutant.
Summary
Evidence
--all-vs-all --queries-by-skillnaming a spec-error skill aborts with exit 2; aponytail-shaped catalog (
skills/and.openclaw/skills/with differing copies) gives 0competitors and "routing needs competitors" with no cause;
--all-vs-all --estimate-onlyisrefused.
After:
tests/test_novelty_routing_gaps.py(19 tests) and the all-vs-all and field testspass. The new tests failed before the change (18 failures across the routing test files).
pytest tests/test_novelty*.py tests/test_skill_repo_shapes.py tests/test_cli_flags.py tests/test_cli_surface.py: 1455 passed. mypy and ruff clean on the changed modules.Merge Danger
Door: two-way. The flags are additive;
--keep-copies-underand the estimate path have notshipped before. The saved routing JSON gains keys and no existing key is removed.
Blast radius:
novelty routingonly. Behaviour changes in two places: an--all-vs-allrunwhose queries name a skill the field leaves out now exits 0 with a WARNING (was exit 2), and the
conflict error text changed. Runs without a same-name conflict or a left-out skill are unchanged.
Local self-review: 2 fresh-context passes, 0 blocking.
Follow-ups (not in this PR):
field.dropped_queriesdo.--keep-copies-under .is a no-op: it names the root, so every copy stays and nothing resolves.--keep-copies-underhelp does not say that names with no copy under DIR are left outwith a note.
check_all_vs_all_argsanddrop_queries_left_outintocommands/novelty_routing_inputs.py.🤖 Generated with Claude Code