Repository navigation
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 |
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 14 selected item(s). Review details
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
- Coverage 94.05% 94.04% -0.01%
==========================================
Files 297 297
Lines 38018 38052 +34
Branches 6424 6434 +10
==========================================
+ Hits 35756 35785 +29
- Misses 1453 1455 +2
- Partials 809 812 +3
🚀 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).
| - **`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).
| 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.
| 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).
| 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.
| "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.
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