Repository navigation
fix(catalog): flag name conflicts, fail an empty root, keep spec errors out of routing - #328
Conversation
…spec errors from routing Three catalog-side bugs from the 10-30 rescan of public skill catalogs: - Two skills with one name and different content aborted `novelty routing --catalog` for the whole repo (exit 2). The routing field now skips that name, with a warning naming every path, and keeps the rest of the field. `catalog scan` reports each such skill as a `name-conflict` warning with the other path, where it used to say ok. - `catalog scan` on a root with no SKILL.md exited 0 silently. It now prints `no SKILL.md found under <root>` and exits 1. With `--kind all` a tasks-only root only warns, and `--strict` makes that a failure. - A skill with a spec error (for example a description over 1024 characters) entered the routing field. It is now left out with a warning naming the error; `novelty routing --include-invalid` keeps it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…r its copies catalog scan warns on every copy of a name whose SKILL.md files differ, including a copy that is disabled or has a spec error. Routing checked the conflict only after dropping those copies, so a name could stay in the field while the scan said it was skipped. The conflict is now found over the loaded, non-excluded copies before the disabled and spec-error filters. The note names every path of the name. The spec-error wording in the changelog and the docs now says it is an example list, not the whole list. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The saved routing result states the field rule, and it recorded include_disabled but not include_invalid, so a run that kept spec-error skills looked the same as a default run. The default exclusion of a spec-error skill is a change in what the field holds, so the changelog lists it under Changed. The docs list the flag with the other field-rule keys. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…-invalid The unreleased routing-catalog-manifest fragment still said same-name catalog skills are refused. They now leave the field with a warning. The --include-invalid entry and the docs say that a conflicting name stays out whatever the flag says. 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 `--include-invalid`Updated on every push; the |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
+ Coverage 94.29% 94.32% +0.02%
==========================================
Files 308 309 +1
Lines 39322 39535 +213
Branches 6651 6693 +42
==========================================
+ Hits 37079 37290 +211
- Misses 1430 1433 +3
+ Partials 813 812 -1
🚀 New features to boost your workflow:
|
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at ec204a1. The three fixes work as described: a same-name conflict no longer aborts routing, catalog scan warns about it and fails on an empty root, and spec-error skills leave the --catalog field. One point needs your decision: a disabled or spec-error copy of a name now also drops the enabled skill a harness still lists. The rest are small and in the line comments.
What I ran, at ec204a1:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network and Docker steps).- The PR's four touched test files on the merge-base
05f9d62: 14 of the new tests fail there, so they cover the change. - Repro on a scratch catalog (
skills/dbenabled,legacy/dbdisabled with other text;perfwith a differing plugin copy):catalog_competitorsdrops bothdbandperf;catalog scanwarnsname-conflicton all four copies. catalog scan --kind skillson a root whoseskills.yamllists no skill: exit 1 withno SKILL.md found.
Checked, no issues
- Empty field: when every catalog name is skipped,
check_fieldstill refuses a run with no competitors, so a conflict cannot turn into a 100% selection rate. - Mirrors and spec errors: a byte-identical copy is not a conflict, and a spec-error copy is filtered before the keeper is picked, so a valid deeper copy still stands for the card.
- Saved field rule:
field.include_invalidis written for both the single-artifact and the all-vs-all runs; the CLI snapshot and the flag allowlist are updated. - Docs:
external-content.mdandskill-novelty.mddescribe the new exit code, the warning and the flag.
| notes.append(f"left out catalog skill {skill.name!r}: " | ||
| "disable-model-invocation (--include-disabled keeps it)") | ||
| loaded.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest(), review)) | ||
| conflicting = _conflicting_names([(skill, digest) for skill, digest, _ in loaded], notes) |
There was a problem hiding this comment.
A disabled copy of a name now removes the enabled skill of that name from the field, though a harness still lists that skill.
Repro: a catalog with skills/db/SKILL.md (enabled) and legacy/db/SKILL.md (different text, disable-model-invocation: true). catalog_competitors returns no db, with the note skipped catalog name 'db'. On the base it returned db: the hidden copy was filtered first, and the one card a harness lists stayed. The field now lacks a real sibling, so the artifact's selection rate is measured against a smaller field than the one the model sees. The same holds for a copy with only a spec error under the default --include-invalid off.
The PR lists this as a decision to confirm; I think the conflict should be about the copies that can be in the field. Suggestion: run _left_out_by_flag first and _conflicting_names over what is left (so --include-disabled / --include-invalid bring the conflict back), and in catalog scan do not count a disable-model-invocation copy as a name-conflict (or say in the warning that routing skips the name only while both copies are listed).
There was a problem hiding this comment.
Pending the maintainer's decision; no change in this push. The field rule (a disabled copy of a name and an enabled copy) is the open item already listed in the PR body.
Options: (a) keep the current rule, the field drops a name whose copies differ, whatever the flags; (b) run the flag filters first and the conflict check over what is left. I will apply whichever the maintainer picks.
There was a problem hiding this comment.
Maintainer chose option (b): the flag filters run first, and the conflict check runs over what is left. Fixed in c848aec.
A disabled copy no longer makes its name a conflict, so the enabled skill stays in the field. --include-disabled brings the copy back into the check.
catalog scan now names only the listed copies in name-conflict (not disabled, not spec-error), so routing and scan agree. Tests: test_a_disabled_copy_of_a_name_is_no_conflict_with_the_enabled_copies, test_include_disabled_brings_the_disabled_copy_back_into_the_conflict, test_a_disabled_copy_of_a_name_is_not_a_name_conflict.
| - **`novelty routing --catalog` leaves out a skill with a spec error by default** (for example a | ||
| description over 1024 characters, or a name that is not kebab-case or not its directory name), with | ||
| a warning on stderr naming the error. Before, such a skill was in the field. `--include-invalid` | ||
| keeps it. | ||
| - **`catalog scan` reports a same-name skill with different content as a `name-conflict` warning** | ||
| naming the other path. Before, each of the two skills was reported `ok`. It is a warning, not an | ||
| error, so `--strict` does not fail on it. | ||
|
|
||
| ### Fixed | ||
|
|
||
| - **`novelty routing --catalog` skips a name that two different skills share, with a warning, | ||
| instead of aborting the whole run** (`catalog has X at A and B with different content`, exit 2). | ||
| Every copy of that name leaves the field, the rest of it stays, and stderr names every path. |
There was a problem hiding this comment.
The fragment does not say that earlier --catalog routing results stop being comparable, and its Fixed entry fixes an abort that no release had.
Leaving spec-error skills out by default, and dropping a name whose other copy is disabled, change the field for the same command, so the selection rates move (the PR body says "rerun to compare", the fragment does not). Under the project rules that is a change to defaults that moves published numbers. The abort catalog has X at A and B with different content came with the 20261021 fragment, which is not released yet and which this PR already rewrites, so the release notes would tell the same change twice.
Suggestion: add to the Changed entry "Rerun earlier --catalog results before comparing them", and drop the first Fixed bullet (the 20261021 fragment already says it).
There was a problem hiding this comment.
Fixed in d3ba18b. The changelog fragment now says a routing result saved before this change is not comparable, and the abort note is kept under Changed: it was refused (exit 2) in the unreleased 20261021 entry, and now it is a warning.
| routing_p.add_argument("--include-invalid", action="store_true", dest="include_invalid", | ||
| help="Keep catalog skills with a spec error in the field") |
There was a problem hiding this comment.
The spec-error filter covers only --catalog, so the same invalid skill is in the field or out of it depending on how it is passed.
--catalog cat/ leaves out cat/skills/long (description over 1024 characters) with a warning; --competitors skill:cat/skills/long keeps it with no warning. Both saved results record include_invalid: false. --include-disabled, by contrast, is applied to --competitors too.
Suggestion: either check skill: competitors the same way (review_skills on a one-entry list), or say in the help and in skill-novelty.md that --include-invalid and the filter are about --catalog only, so field.include_invalid is not read as covering the explicit competitors.
There was a problem hiding this comment.
Pending the maintainer's decision; no change in this push. skill: competitors and --catalog skills follow different filters, which the PR body lists as open.
Options: (a) apply the spec-error filter to explicit skill: competitors too (warn and drop); (b) keep it, and say in the help and skill-novelty.md that --include-invalid covers --catalog only.
There was a problem hiding this comment.
Maintainer chose option (a): an explicit skill: competitor with a spec error is skipped with a warning, as a catalog skill is. Fixed in c848aec.
The warning names the error and --include-invalid; the entry goes into the saved left_out list.
Tests: test_an_explicit_spec_error_competitor_is_skipped_with_a_warning; test_a_spec_error_copy_is_no_conflict_with_a_valid_copy_of_the_name (with --include-invalid: test_include_invalid_makes_the_spec_error_copy_a_conflict_again).
| reviews = review_skills(entries) | ||
| loaded: list[tuple[Skill, str, SkillReview | None]] = [] | ||
| for entry, review in zip(entries, reviews, strict=True): | ||
| skill = entry.skill | ||
| if skill is None or not entry.loaded: | ||
| if notes is not None: | ||
| notes.append(f"skipped catalog entry {entry.name!r}: {entry.error}") | ||
| continue | ||
| if skill.name in exclude: | ||
| continue | ||
| if not include_disabled and disables_model_invocation(skill.path): | ||
| if notes is not None: | ||
| notes.append(f"left out catalog skill {skill.name!r}: " | ||
| "disable-model-invocation (--include-disabled keeps it)") | ||
| loaded.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest(), review)) | ||
| conflicting = _conflicting_names([(skill, digest) for skill, digest, _ in loaded], notes) | ||
| eligible: list[tuple[Skill, str]] = [] | ||
| for skill, digest, review in loaded: | ||
| if skill.name in conflicting or _left_out_by_flag( | ||
| skill, review, include_disabled, include_invalid, notes): | ||
| continue | ||
| eligible.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest())) | ||
| eligible.append((skill, digest)) | ||
| return eligible | ||
|
|
||
|
|
||
| def _refuse_conflicting_names(eligible: list[tuple[Skill, str]]) -> None: | ||
| """Two skills with one name and different bytes cannot both stand for the card. | ||
| def _left_out_by_flag(skill: Skill, review: SkillReview | None, include_disabled: bool, | ||
| include_invalid: bool, notes: list[str] | None) -> bool: | ||
| """Whether a disabled or spec-error skill is left out, with the note that says why.""" | ||
| if not include_disabled and disables_model_invocation(skill.path): | ||
| if notes is not None: | ||
| notes.append(f"left out catalog skill {skill.name!r}: " | ||
| "disable-model-invocation (--include-disabled keeps it)") | ||
| return True | ||
| if not include_invalid and review is not None and review.state == "error": | ||
| if notes is not None: | ||
| problems = "; ".join(f.message for f in review.findings if f.level == "error") | ||
| notes.append(f"left out catalog skill {skill.name!r} at {skill.path}: " | ||
| f"spec error ({problems}; --include-invalid keeps it)") | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def _conflicting_names(loaded: list[tuple[Skill, str]], notes: list[str] | None) -> set[str]: |
There was a problem hiding this comment.
Routing now calls review_skills and then redoes what it computed: the digest, the disabled flag and the name-conflict check.
review_skills already holds content_sha256 and disable_model_invocation for each skill and adds the name-conflict findings; routing hashes each SKILL.md again (line 549), re-parses its frontmatter in disables_model_invocation (line 563), and has its own _conflicting_names. Two implementations of the rule the scan warns about can drift, which is what this PR is fixing.
Suggestion: use review.content_sha256 and review.disable_model_invocation, and take the conflicting names from the reviews (or move the grouping into one helper both use).
There was a problem hiding this comment.
Fixed in d3ba18b. Routing takes the digest, the disabled flag and the name-conflict set from review_skills (content_sha256, disable_model_invocation, name-conflict findings), so it no longer hashes or re-parses each SKILL.md. catalog scan and the field read one implementation.
| if args.kind in ("skills", "all") and not report["skills"]: | ||
| # Nothing to scan is a wrong root or an empty checkout, so a skills scan fails on it. A | ||
| # --kind all scan of a tasks-only catalog is a warning, which --strict makes a failure. | ||
| print(f"no SKILL.md found under {root}", file=sys.stderr) |
There was a problem hiding this comment.
Nit: The message says no SKILL.md found when there are SKILL.md files but the manifest lists none.
A root with skills/a/SKILL.md and skills.yaml set to skills: [] prints no SKILL.md found under <root> and exits 1. The exit is right; the message sends the user looking for missing files.
Suggestion: when a skills.yaml is present, say skills.yaml under <root> declares no skill.
There was a problem hiding this comment.
Fixed in d3ba18b. A root with a skills.yaml that has no skill that can be scanned now says so, instead of no SKILL.md found. Test: test_catalog_scan_with_a_manifest_that_declares_no_skill_says_so_and_exits_1.
| "warn", | ||
| "name-conflict", | ||
| f"name {name!r} is also used by {', '.join(others)}, with different content; " | ||
| "routing skips this name until one is renamed or removed", |
There was a problem hiding this comment.
Nit: The name-conflict warning says routing skips the name, but it does not when one copy is the artifact.
novelty routing --artifact skill:cat/skills/db --catalog cat/ with a different plugins/x/skills/db: the name is in exclude, so the fork is dropped by name before the conflict check, with no note and nothing in left_out, while the scan says routing skips db.
Suggestion: word it as "novelty routing --catalog leaves this name out of the field", or note in left_out a same-name catalog copy that differs from the artifact.
There was a problem hiding this comment.
Fixed in d3ba18b. The warning now says novelty routing --catalog leaves the name out of the field, which is what happens when the artifact is one of the copies.
…eview; reword the conflict and no-skill messages - Routing reads content_sha256, disable_model_invocation and the name-conflict findings of review_skills instead of hashing and re-parsing each SKILL.md again, so catalog scan and the field cannot drift (Thread: PRRT_kwDOVBg7g86yVfRA). - The name-conflict warning says `novelty routing --catalog` leaves the name out of the field, which is what happens when the artifact is one of the copies (Thread: PRRT_kwDOVBg7g86yVfRH). - A root whose skills.yaml declares no skill says so, instead of `no SKILL.md found` (Thread: PRRT_kwDOVBg7g86yVfRD). - The changelog fragment says a routing result saved before this change is not comparable, and drops the Fixed entry for an abort no release had (Thread: PRRT_kwDOVBg7g86yVfQ8). Thread: PRRT_kwDOVBg7g86yVfRA Thread: PRRT_kwDOVBg7g86yVfRH Thread: PRRT_kwDOVBg7g86yVfRD Thread: PRRT_kwDOVBg7g86yVfQ8 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the catalog diagnostics and routing eligibility changes.
No open issues 🎉
What changed in this PR
0 new findings; nothing to fix.
This PR aligns dokimi’s catalog diagnostics with routing eligibility, so conflicting names and invalid skills are handled consistently without unnecessarily aborting routing.
Changes:
- Warn about same-name skills with different content and exclude those names from catalog-derived routing fields.
- Exclude spec-error skills by default, add
--include-invalid, and record that flag in saved routing results. - Fail empty skill scans, with a tasks-only exception under non-strict
--kind all, and update tests and documentation.
| File | Description |
|---|---|
dokimi/tests/test_novelty_routing_field.py |
Tests conflict exclusion, invalid-skill filtering, warnings, and saved flags. |
dokimi/tests/test_novelty_routing_all_vs_all.py |
Updates the expected saved field metadata. |
dokimi/tests/test_catalog_validate.py |
Tests name-conflict warnings and affected paths. |
dokimi/tests/test_catalog_command.py |
Tests empty catalogs and tasks-only scan behavior. |
dokimi/tests/data/cli_flags_allowlist.txt |
Registers --include-invalid in the flag allowlist. |
dokimi/dokimi/novelty/routing.py |
Uses catalog reviews to filter conflicts and spec-error skills. |
dokimi/dokimi/external/validate.py |
Adds name-conflict findings for differing same-name skills. |
dokimi/dokimi/commands/novelty.py |
Wires --include-invalid into routing and saved metadata. |
dokimi/dokimi/commands/catalog.py |
Adds empty-scan diagnostics and failure rules. |
dokimi/docs/skill-novelty.md |
Documents routing exclusions and saved field rules. |
dokimi/docs/reference/cli-interface.json |
Adds the new flag to the CLI snapshot. |
dokimi/docs/external-content.md |
Documents empty scans and name-conflict warnings. |
dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md |
Records the catalog and routing behavior changes. |
dokimi/changelog.d/20261021_130000_napetrov_routing-catalog-manifest.md |
Updates the earlier fragment to describe conflict skipping. |
Review effort: Balanced
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d3ba18b. I found no new bug or risk. The merge waits on two design points that @vlad-perevezentsev raised and the author has handed back for a maintainer's decision.
Open until decided:
- @vlad-perevezentsev's point on
routing.py(a disabled or spec-error copy of a name removes the enabled same-name skill from the field) is unchanged at this head. The conflict check still runs over every loaded copy before the--include-disabled/--include-invalidfilters (_eligible_catalog_skills, by a reading of the code). It waits for a decision. - @vlad-perevezentsev's point on
commands/novelty.py(--include-invalidand the spec-error filter cover--catalogonly, whileskill:competitors are kept unfiltered) is unchanged:include_invalidis passed only tocatalog_competitors. The saved result still recordsinclude_invalid: falsefor such a run. It waits for a decision.
What I ran, on a Linux review host at d3ba18b:
make ci-local: 30 passed, 0 failed, 7 skipped (jobs that need a PR context, Docker or network).- 26 deliberate mutants of the new code against the four touched test files: 23 are caught. The 3 that pass change only the order of paths in a message, or are equivalent code (
args.kind == "skills"is implied bynot report["tasks"]). - The touched tests run against the base modules: 15 fail there, so they cover the change.
- Reproductions of
catalog scanon an empty root, a tasks-only root,--strict, and an emptyskills.yaml, and ofcatalog_competitorswith mirrors, disabled and spec-error copies, andexclude, all offline.
Checked, no issues
- Conflict rule: routing takes the conflicting names from the
name-conflictfindings ofreview_skills, so the scan warning and the field agree. Identical mirrors are not a conflict. - Exit codes:
--kind skillson an empty root exits 1;--kind allon a tasks-only root exits 0 and 1 under--strict; the pinned catalog in the agent-quality workflow hasSKILL.mdfiles, so it is not affected. - Saved field rule:
include_invalidis recorded for single-artifact and all-vs-all runs; the CLI snapshot and flag allowlist are updated. - Repo rules: changelog fragment present, commits signed off, no generated artifacts, no schema change.
…icit spec errors - The disable-model-invocation and spec-error filters now run before the same-name conflict check, so a copy no harness lists does not make its name a conflict. --include-disabled and --include-invalid bring the copy back into the check. - catalog scan names only the listed copies (not disabled, not a spec error) in name-conflict warnings, so routing and scan agree. - An explicit skill: competitor with a spec error is skipped with a warning, as a catalog skill is (competitor_left_out), instead of being kept silently. - growth_baseline.json lowered for novelty.py after the move. Covered by red-then-green tests in test_novelty_routing_field.py and test_catalog_validate.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| keeps it. A name with two different copies, or a copy that is `disable-model-invocation`, also changes | ||
| the field, so a routing result saved before this change is not comparable with one saved now for the |
There was a problem hiding this comment.
Nit: this still says a disabled copy changes the routing field, but the latest implementation filters disabled copies before checking conflicts. For example, an enabled skills/db/SKILL.md and a different-content disabled plugins/db/SKILL.md now keep the enabled db in the default field, just as before this PR. Remove the disabled-copy clause, or qualify it with --include-disabled, so the compatibility warning describes the current behavior.
There was a problem hiding this comment.
Fixed in a283eed. The changelog now says a copy that is disable-model-invocation counts only with --include-disabled. With the default flags it does not change the field, as you describe.
| entry = SkillCatalogEntry(name=skill.name, status=None, path=path.parent, | ||
| source_root=path.parent.parent, skill=skill) |
There was a problem hiding this comment.
Must-fix: the spec-error filter depends on how the same path is written. From /tmp/checkout with SKILL.md declaring name: db, competitor_left_out('skill:SKILL.md', include_disabled=False, include_invalid=False) keeps the skill, but the same call with skill:/tmp/checkout/SKILL.md drops it for name-mismatch. For the relative reference, both path.parent and path.parent.parent are ., so review_skills treats it as a root skill and exempts the directory-name check; the absolute reference does not get that exemption. Normalize the path before deriving the review root and apply a consistent root-skill policy. Add a regression comparing relative and absolute references (including skill:.) so path spelling cannot change the measured field.
There was a problem hiding this comment.
Fixed in a283eed. competitor_left_out now reads the absolute path, so skill:SKILL.md, skill:. and the absolute reference get the same verdict and the directory-name check applies to all three. Regression: test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict (relative gave None before).
| A name whose loaded copies differ in content leaves the field whole. That check runs | ||
| before the disabled and spec-error filters, so it sees the copies ``catalog scan`` warns | ||
| about, and the note names all of them. |
There was a problem hiding this comment.
Nit: this describes the previous filter order. The implementation now removes disabled and spec-error copies before checking conflicts, so a valid enabled db remains when its only differing copy is excluded. Update this docstring and catalog_competitors (which still says disabled and spec-error copies participate) to describe conflicts among copies admitted by the flags.
Suggestion:
| A name whose loaded copies differ in content leaves the field whole. That check runs | |
| before the disabled and spec-error filters, so it sees the copies ``catalog scan`` warns | |
| about, and the note names all of them. | |
| Disabled and spec-error copies are filtered according to the flags first. A name whose | |
| remaining copies differ in content leaves the field whole, with a note naming every | |
| remaining path of that name. |
There was a problem hiding this comment.
Fixed in a283eed. The docstrings of catalog_competitors and _eligible_catalog_skills now describe the flag-first order: conflicts are checked among the copies the flags admit, with a note naming every remaining path.
…elling competitor_left_out reads the absolute path. A relative skill:SKILL.md had parent.parent == "." and was taken for a root skill, so the directory-name check was skipped and the same skill got a different verdict. Also: the catalog_competitors and _eligible_catalog_skills docstrings and the changelog describe the flag-first order (a disabled copy counts only with --include-disabled). test_skill_repo_shapes drops its open-bug xfail table: the six entries XPASS on this branch, so the marks came off. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| another skill's, is a `warn`. So is a skill whose `name` another skill shares with | ||
| different content (`name-conflict`): the finding names the other path, and |
There was a problem hiding this comment.
Nit: qualify this as a conflict between enabled skills without spec errors. _name_conflicts excludes disabled and spec-invalid copies, so skills/x plus a different-content plugins/x with disable-model-invocation: true produces no name-conflict warning and keeps the enabled x in default routing (as test_a_disabled_copy_of_a_name_is_not_a_name_conflict verifies). The unqualified wording promises a warning for that input. Apply the same qualification to the catalog scan entry in changelog.d/20261031_120000_napetrov_rescan-catalog.md; routing inclusion flags can admit additional copies, but scan has no corresponding flags.
There was a problem hiding this comment.
Fixed in c936c71. The paragraph and the changelog entry now say name-conflict counts only among listed skills (no disable-model-invocation, no spec error).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a283eed. The flag-first order from c848aec holds: the scan warning and the routing field name the same copies, and the path-spelling fix works for relative references. One thing is worth fixing before merge: the same fix now rejects a root-level SKILL.md that --catalog keeps. One nit on the changelog.
What I ran, on a Linux review host at a283eed:
make ci-local: 30 passed, 0 failed, 7 skipped (jobs that need a PR context, Docker or network). Changed-line coverage gate passes.- 31 deliberate mutants of the new code against the four touched test files: 29 are caught. The 2 that pass are equivalent code (
args.kind == "skills"is implied bynot report["tasks"]; thereview is Noneguard) and one message-only filter in_name_conflicts. - Reproductions with the LLM transport mocked:
catalog scanon empty, tasks-only and empty-manifest roots;catalog_competitorswith mirrors, disabled and spec-error copies;competitor_left_outwith file, directory, relative andskill:.spellings.
Checked, no issues
- Conflict rule: disabled and spec-error copies are filtered first, then conflicts are taken over what is left. With default flags, an enabled
dbnext to a different disableddbstays in the field and gets no scan warning;--include-disabledbrings the conflict back. - Spelling:
skill:SKILL.md,skill:.,skill:dirand the absolute path give one verdict inside a skill directory. - Exit codes: empty root exits 1 for
--kind skillsand--kind all, 0 for--kind tasks; a tasks-only root under--kind allwarns and fails only with--strict. - Repo rules: all 7 non-merge commits are signed off, no generated artifacts, CLI snapshot and flag allowlist updated, changelog fragment present.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| | Command | Root missing | Exit | | ||
| |---|---|---| | ||
| | `catalog scan --root <missing>` | `--root is not a directory: <path>` | 1 | | ||
| | `catalog scan --root <no SKILL.md>` | `no SKILL.md found under <path>` | 1 | |
There was a problem hiding this comment.
Nit: this row promises exit 1 for every root without SKILL.md, but the default --kind all scan of a healthy tasks-only root prints this message and exits 0 without --strict (as the new tasks-only regression test verifies). Add --kind skills to this command, or qualify the exit column with the tasks-only exception.
Suggestion:
| | `catalog scan --root <no SKILL.md>` | `no SKILL.md found under <path>` | 1 | | |
| | `catalog scan --root <no SKILL.md> --kind skills` | `no SKILL.md found under <path>` | 1 | |
There was a problem hiding this comment.
Fixed in c936c71: the row runs catalog scan --root <no SKILL.md> --kind skills, which exits 1 (checked on this branch). With --kind all a tasks-only root still only warns, as the paragraph says.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to c3d32a1, which fix the nit from my review:
c3d32a1:dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md:11: added that a--competitors skill:<path>with a spec error is skipped the same way.
make ci-local at c3d32a1: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the risk from my review.
…kind skills The scan's name-conflict warning skips a disable-model-invocation copy and a copy with a spec error (validate._name_conflicts), so the docs and the changelog say so. The root-missing table row names --kind skills: with --kind all, a tasks-only root only warns. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
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 c936c71. The commits since my last review change docs only (external-content.md, the changelog fragment), and the doc fixes read right. The merge still waits on my earlier thread about a root-level SKILL.md, which this head leaves unchanged. One nit inline.
What I ran, on a Linux review host at c936c71:
make ci-local: 30 passed, 0 failed, 7 skipped (jobs that need a PR context, Docker or network).- 30 deliberate mutants of the new code against the five touched test files: 27 are caught. The 3 that pass change only note text or the scan warning for a spec-error copy (the nit inline).
- Reproductions with the LLM transport mocked:
catalog_competitorsandcompetitor_left_outon a root-levelSKILL.md;catalog scanon empty, tasks-only and empty-manifest roots; flag-first conflict checks with--include-disabledand--include-invalid.
Still open from my earlier review:
dokimi/dokimi/novelty/routing.py:472: a root-levelSKILL.mdis kept by--catalogbut dropped as an explicitskill:competitor. At c936c71,checkout/SKILL.mdwithname: db:catalog_competitors("checkout", set(), notes)returnsdb;competitor_left_out("skill:<abs>/checkout", ...)returnsspec error (name 'db' is not the directory name 'checkout'; --include-invalid keeps it). The author's option (a) calls this "the spec rule", but the catalog applies the same rule with the root exemption (validate.py:207), so the two paths still give different verdicts for one skill, and the checkout directory name decides the field. Please take option (b) in the narrow form from the thread: read aSKILL.mdwith noskills/parent as a root skill for the name check, and add a test forskill:<dir>andskill:SKILL.mdon a root-level skill.
Checked, no issues
- Exit codes: an empty root exits 1 for
--kind skillsand--kind all; a tasks-only root under--kind allwarns and exits 1 only with--strict; the new row inexternal-content.mduses--kind skills. - Conflict rule: disabled and spec-error copies are filtered first; the scan warns about the same copies routing drops. Verified with and without
--include-disabled/--include-invalid. - Repo rules: both changelog fragments present, all 9 non-merge commits signed off, CLI snapshot and flag allowlist updated, no schema change, no generated artifacts.
Bot points I checked that no longer apply at this head (1)
- exai-bot's point on the path spelling of
skill:references does not apply:skill:SKILL.md,skill:.and the absolute path now give one verdict (a283eed). The root-skill policy it also mentions is the thread above.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 5a61b4a. The merge still waits on my earlier point about a root-level SKILL.md, which this head leaves unchanged. The commits since c3d32a1 touch the docs and the changelog only, apart from merges of main, and the doc changes read right. I found no new bug or risk.
What I ran, on a Linux review host at 5a61b4a:
make ci-local: 28 passed, 2 failed, 7 skipped. The 2 failures are the gitleaks install and the commit scan, which need a download that is not available here; the PR does not cause them. 12,262 tests passed, 59 skipped; changed-line coverage 98%.- Reproductions with the LLM transport mocked:
catalog_competitorsandcompetitor_left_outon a root-levelSKILL.md. - One mutant of
_name_conflictsagainst the three touched test files (124 tests): it passes (the second point under "Still open").
Still open from my earlier review:
dokimi/dokimi/novelty/routing.py:472: a root-levelSKILL.mdis kept by--catalogbut dropped as an explicitskill:competitor. At 5a61b4a,checkout/SKILL.mdwithname: db:catalog_competitors("checkout", set(), notes)returnsdband no note.competitor_left_outonskill:<abs>/checkoutand onskill:<abs>/checkout/SKILL.mdreturnsspec error (name 'db' is not the directory name 'checkout'; --include-invalid keeps it). The checkout directory name still decides whether a single-skill repo is in the field. The author's option (a) keeps the check as "the spec rule", but the catalog applies the same rule with the root exemption (validate.py:207). Please take the narrow form of (b): read aSKILL.mdwith noskills/parent as a root skill for the name check, with a test forskill:<dir>andskill:SKILL.md.dokimi/dokimi/external/validate.py:245: nothing pins thestate != "error"clause in_name_conflicts. Removing it passes the touched test files (124 passed). The test from my thread,test_a_spec_error_copy_of_a_name_is_not_a_name_conflict, is still not intests/test_catalog_validate.py.
Checked, no issues
- Delta:
git diff c3d32a1..HEADfor the PR's files is the changelog wrapping and thename-conflictqualification inexternal-content.mdand the changelog, plus the--kind skillstable row. They match_name_conflictsand the exit codes. - Repo rules: all 9 non-merge commits are signed off, CLI snapshot and flag allowlist are current (
cli-surfacepasses), docs build, no schema change, no generated artifacts.
Bot points I checked that no longer apply at this head (1)
- exai-bot's point on the path spelling of
skill:references does not apply:skill:SKILL.md,skill:.and the absolute path give one verdict (a283eed). The root-skill policy it also mentions is the open point above.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 33d7692, which fix the nit from my review:
33d7692:dokimi/tests/test_catalog_validate.py:154: addedtest_a_spec_error_copy_of_a_name_is_not_a_name_conflictafter the disabled-copy test, as suggested.
make ci-local at 33d7692: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the earlier thread of mine above nit level.
…n explicit competitor's name check Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| field = _routing_field(args, {"perf"}, left_out) | ||
| assert [c.name for c in field] == ["cache"] |
There was a problem hiding this comment.
Nit (non-blocking): this regression now stops at _routing_field, so it no longer verifies the advertised change from command refusal to successful routing. A refusal introduced after field construction, or failure to persist the conflict in field.left_out, would still pass. Keep these filtering assertions, then call args.func(args) and inspect the saved JSON for status == "ok", field.competitors == ["cache"], and the conflict note in field.left_out. No other routing conflict test exercises that complete command path.
There was a problem hiding this comment.
Fixed in 6387fe6. The conflict test now runs the whole command: it checks the saved result is ok, competitors is [cache], and the conflict note is in field.left_out.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The root-skill fix is right, and my earlier thread on it is closed. One test gap came with it, and it is worth fixing before merge.
What I ran, on a Linux review host at c0bae6d:
make ci-local: 30 passed, 0 failed, 7 skipped (jobs that need Docker, network or a PR context).- 6 deliberate mutants of the
root = ...line againsttest_novelty_routing_field.py(36 tests): all 5 that change the rule are caught. The one that removes theabspathline passes (the inline comment). - Reproductions with the LLM transport mocked:
competitor_left_outoncheckout/SKILL.md(namedb) by file, directory,skill:SKILL.mdandskill:.; onskills/checkoutandplugins/x/skills/checkout.
Checked, no issues
- Root-skill rule: a
SKILL.mdwith noskills/parent gets no name-vs-directory error for any spelling; underskills/the check stays. Both matchcatalog_competitorsfor those layouts. - Repo rules: all non-merge commits are signed off, CLI snapshot and flag allowlist current, changelog fragments present, no schema change.
Bot points I checked that no longer apply at this head (1)
- exai-bot's point on the path spelling of
skill:references does not apply:skill:SKILL.md,skill:.and the absolute path now give one verdict, and the root-skill policy it also mentions is the one now applied.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at c0bae6d. The name check now works the same as --catalog for a skill at the root and under skills/. One more layout still differs; it is in the line comment.
Agreed with @antonwolfy on the missing relative-path test for the abspath line.
Agreed with @exai-bot on the conflict test that does not reach the saved JSON.
| skill = load_skill(path) | ||
| # A SKILL.md with no ``skills/`` parent is a root skill, as the catalog reads one at its scan root: | ||
| # its directory name says nothing about the skill, so the name check is skipped for it. | ||
| root = path.parent.parent if path.parent.parent.name == "skills" else path.parent |
There was a problem hiding this comment.
The skills/ parent rule from @antonwolfy's thread closes the root-level case, but a skill outside a skills/ directory now skips the name check only when passed explicitly.
The root-level and skills/ layouts agree with --catalog, as @antonwolfy checked; a third layout does not. Repro at c0bae6d: cat/tools/database/SKILL.md with name: db. catalog_competitors("cat", ...) leaves db out (name 'db' is not the directory name 'database'), and catalog scan reports the same error. competitor_left_out("skill:cat/tools/database", ...) returns None, so the same file is in the field as --competitors and out of it as --catalog. Any layout without skills/ (plugins/x/<dir>, tools/<dir>) gets this.
Suggestion: read a SKILL.md as a root skill only when nothing above it looks like a catalog, e.g. when its parent directory holds no other skill directory (a single-skill checkout), or when the directory is the top of a git checkout; and add the tools/database case to test_a_skill_under_a_skills_directory_keeps_the_directory_name_check. If the heuristic is meant to stay, say in skill-novelty.md which explicit skills are name-checked.
There was a problem hiding this comment.
Maintainer decision: keep the directory-name rule for explicit skill: outside skills/. Such a skill is read as a root skill and is not name-checked when passed explicitly.
Documented in docs/skill-novelty.md and pinned by a test. Done in 51ce87b.
…ative spelling under skills/ The conflict regression stopped at the field; it now runs the command and checks the saved result is ok, with the same note in left_out. A relative skill: reference under a skills/ directory is checked as its absolute one is, so removing the abspath in competitor_left_out fails it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A skill: competitor outside a skills/ tree is read as a root skill, so its name is not checked against its directory. Document that, and pin it with a test, so an explicit reference and a catalog scan are not read as agreeing. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ield The refusal for queries naming a skill outside the field only pointed at --include-disabled, which does nothing for the spec-error and name-conflict leave-outs this PR adds. Name all three, and say in the changelog that --all-vs-all still refuses such a query file. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Keeps intel#328's field rules (spec errors and name conflicts leave the field) and intel#329's estimate-only, all-vs-all repeats and cost; the refusal of a query file naming a left-out skill now names each skill with every reason that applies. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> intel#328's competitor rules (disable-model-invocation, spec errors, name conflicts) now live in routing_field, the module this branch moved them to. Routing tests follow main's notes and the InputError exit path.
Summary
A rescan of public skill catalogs found three catalog-side gaps: a name that two different skills share was silent in
catalog scanand abortednovelty routing --catalog, a root with noSKILL.mdexited 0 without a word, and a skill with a spec error (a description over 1024 characters) went into the routing field.The conflict check runs over every loaded copy before the disabled and spec-error filters, so the routing field and the
name-conflictwarnings agree: a name the scan warns about is never in the field.Shape tests in #327 (
tests/test_skill_repo_shapes.py) that this PR flips. Each is a strict xfail onmain(OPEN_BUGS). With this PR they XPASS, so their marks must come off when #327 is rebased on this change:drifted_mirrorsame_name_distinct_dirszero_skilldescription_too_longname_not_dirtitle_case_nameThe last three are labelled
fixed by fix/rescan-routingin #327. This PR also makes their field dimension hold, through the spec-error exclusion; the two changes overlap there, so whichever lands second should rebase on the other.Evidence
Tests were written first and fail on the base code (base run: 10 failed, 105 passed on the touched test files before the fix).
Full suite on the branch head
ec204a14, and onintel/main:The 13 failures are in the question-set pipeline and fail the same way without this change, so they are not part of this PR.
Local checks on this branch:
docs/reference/cli-interface.jsonis regenerated for--include-invalid, and the flag is intests/data/cli_flags_allowlist.txt.Self-review (local mode, plain pass and edge pass on each round): round 1 found a conflict check that ran after the disabled and spec-error filters, so the scan warning could be false, and a note that omitted copies; both fixed with a red test each. Round 2 found that the saved field rule did not record
include_invalid; fixed with a red test. Blocking findings left: 0.Merge Danger
Door: two-way. The only new surface is
novelty routing --include-invalid(default off). The saved routing result gains one additive key,field.include_invalid. Reverting restores the old field and the abort.Blast Radius: medium for routing and for CI, small elsewhere.
Ramifications:
--catalogby default. A non-kebab-case name (MyTool) or a name that is not its directory name is a spec error too, so it leaves the field as well. Numbers over such catalogs move; rerun to compare.catalog scanalready warns about.novelty routing --catalogexits 0 with a warning where it used to exit 2. The field then lacks the conflicting name, and the warning says how to route it: dedupe or rename.catalog scanon a root with noSKILL.md(for--kind skillsand--kind allwithout tasks) exits 1. Checked against the pinneduxl-skillscommit used by the agent-quality workflow (6feb3f3a, 8SKILL.mdfiles): exit 0, so that job is not affected. A tasks-only root under--kind allonly warns unless--strict.changelog.d/20261031_120000_napetrov_rescan-catalog.mdlists the three items.Decisions for the author to confirm:
--include-invaliddoes not keep a spec-error skill whose name conflicts; the name still leaves the field.--strictflag on routing for conflicts; the warning is the signal.Checklist
pytestpasses (13 failures are pre-existing onintel/main, see above)ruff,mypy, and schema checks pass (mypyhas the 4 pre-existingmcperrors)docs/external-content.md,docs/skill-novelty.md, CLI snapshot.github/.licenserc.yaml) — no new source files; the changelog fragment is Markdown🤖 Generated with Claude Code