From bb696878a388f00f7f05ee72a6f41d831cb7fd76 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 31 Oct 2026 10:04:59 -0700 Subject: [PATCH 01/15] fix(catalog): skip conflicting names, flag them in scan, and exclude 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 ` 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 --- ...20261031_120000_napetrov_rescan-catalog.md | 20 +++++ dokimi/docs/external-content.md | 9 ++- dokimi/docs/reference/cli-interface.json | 16 ++++ dokimi/docs/skill-novelty.md | 11 ++- dokimi/dokimi/commands/catalog.py | 9 ++- dokimi/dokimi/commands/novelty.py | 16 ++-- dokimi/dokimi/external/validate.py | 27 +++++++ dokimi/dokimi/novelty/routing.py | 63 ++++++++++------ dokimi/tests/data/cli_flags_allowlist.txt | 1 + dokimi/tests/test_catalog_command.py | 29 ++++++++ dokimi/tests/test_catalog_validate.py | 26 ++++++- dokimi/tests/test_novelty_routing_field.py | 74 ++++++++++++++----- 12 files changed, 245 insertions(+), 56 deletions(-) create mode 100644 dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md new file mode 100644 index 000000000..1a9ea3c7f --- /dev/null +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -0,0 +1,20 @@ +### Added + +- **`novelty routing --include-invalid`** keeps a catalog skill with a spec error (a description + over 1024 characters, a name that is not its directory name) in the `--catalog` field. Off by + default: such a skill is left out, with a warning on stderr naming the error. + +### Changed + +- **`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). + Both skills of that name leave the field, the rest of it stays, and stderr names every path. +- **`catalog scan` on a root with no `SKILL.md` exits 1 with `no SKILL.md found under `** + instead of exiting 0 silently. With `--kind all`, a tasks-only root only warns, and `--strict` + makes that a failure. diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index 4eb88f845..5a299350a 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -113,7 +113,9 @@ Read that output as three outcomes, not two: Only load errors change the exit code, so `catalog scan` exiting 0 means "everything declared as runnable is runnable". A spec error (below) changes it -only under `--strict`. Note the asymmetry above: a `planned` *task* +only under `--strict`. A root with no `SKILL.md` at all (and no skills.yaml entry) +prints `no SKILL.md found under ` to stderr and exits 1. With `--kind all`, a +tasks-only root only warns, and exits 1 under `--strict`. Note the asymmetry above: a `planned` *task* with no directory is skipped, while a `planned` *skill* with no directory is an error — the skill reader has no notion of a not-yet-written entry. Either drop such a skill from `skills.yaml` or expect the non-zero exit. @@ -123,7 +125,9 @@ its directory name and kebab-case (at most 64 characters; a `SKILL.md` directly at most 1024 characters: a violation is an `error`, printed under the skill. It exits 1 only with `--strict`; without it, the scan reports the error and still exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to -another skill's, is a `warn`. Of identical copies, the one nearest the root (fewest +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 +`dokimi novelty routing --catalog` skips that name rather than the whole catalog. Of identical copies, the one nearest the root (fewest path parts, ties to path order) is the original, the copy the `skill-catalog:` arm offers; each other copy is the `warn` and names it. Warnings never change the exit code. `disable-model-invocation: true` is printed as a `note`, because @@ -392,6 +396,7 @@ Worth knowing before you read a suspiciously clean result: | Command | Root missing | Exit | |---|---|---| | `catalog scan --root ` | `--root is not a directory: ` | 1 | +| `catalog scan --root ` | `no SKILL.md found under ` | 1 | | `tasks run --tasks-root ` | `FileNotFoundError: Task directory not found: demo-widget` | 1 | | `tasks list --tasks-root ` | prints nothing | 0 | | `explore list --tasks-root ` | `--tasks-root is not a directory: ` | 2 | diff --git a/dokimi/docs/reference/cli-interface.json b/dokimi/docs/reference/cli-interface.json index c9eb437d1..435910269 100644 --- a/dokimi/docs/reference/cli-interface.json +++ b/dokimi/docs/reference/cli-interface.json @@ -8309,6 +8309,22 @@ "required": false, "type": null }, + { + "action": "_StoreTrueAction", + "choices": null, + "const": true, + "default": false, + "group_key": null, + "group_required": null, + "hidden": false, + "is_flag": true, + "key": "--include-invalid", + "kind": "option", + "nargs": 0, + "position": null, + "required": false, + "type": null + }, { "action": "_StoreAction", "choices": null, diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index 0894076c7..e99241a1a 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -605,8 +605,12 @@ lists except the artifact itself. A `skills.yaml` at the root decides what the c without one, every `SKILL.md` under it counts, except a `template/` copy and vendored or VCS directories such as `.git` and `node_modules`, which are not walked. A byte-identical copy under another path counts once. A declared skill that cannot be loaded is skipped with a warning. Two -catalog skills with the same name and different content are refused, with both paths named: -rename one, or delete the copy you do not mean to route against. +catalog skills with the same name and different content are both skipped, with a warning that +names every path of that name: the rest of the field stays. Rename one, or delete the copy you do +not mean to route against, to route the name. `dokimi catalog scan` lists the same names as +`name-conflict` warnings. A skill with a spec error (a description over 1024 characters, a name +that is not its directory name) is also left out, with a warning naming the error; pass +`--include-invalid` to keep it, as a harness that refuses to list such a skill would not. A `skill:` competitor that is not a `SKILL.md`, or a directory without one, is skipped with a warning. A shell glob must be expanded first, with the prefix on each path (`skill:dir/a/SKILL.md skill:dir/b/SKILL.md`): an unexpanded `skill:dir/*` is refused as a missing @@ -673,7 +677,8 @@ is no attractor, and a tied skill's own rate is lower by the picks inside its gr `n_scored` and `n_unscored` per skill say how many, a query with no content word or a failed model call is unscored, and a skill with none scored has an own rate of `null`, not 0.0. A run with any failed model call writes the result and exits non-zero. The saved file records the -field rule under `field` (`include_disabled`, the `competitors` and what was `left_out`); a +field rule under `field` (`include_disabled`, the `competitors` and what was `left_out`, which +also names each skill left out for a conflicting name or a spec error); a single-artifact run records it too. It takes no `--artifact`, `--queries`, `--queries-from-headings` or `--repeats` (one model call per query). The model arm also records the selector `model`, so two runs with different models can be told apart. It is refused with fewer than two skills, a name repeated diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index fa7cfb29a..9e61fc3e6 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -131,6 +131,12 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: any_failure = _scan_skills(args, root, report, as_json) if args.kind in ("tasks", "all"): any_failure = _scan_tasks(args, root, report, as_json) or any_failure + 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) + if args.kind == "skills" or args.strict or not report["tasks"]: + any_failure = True if as_json: print(json.dumps(report, indent=2)) if any_failure: @@ -335,7 +341,8 @@ def register(sub, positive_int) -> None: "--strict", action="store_true", help="Exit 1 on a skill spec error (name not the directory name or not kebab-case, " - "description over 1024 characters) as well as on a skill that does not load", + "description over 1024 characters) as well as on a skill that does not load; with " + "--kind all, also on a tasks-only root with no SKILL.md", ) scan_p.add_argument( "--format", diff --git a/dokimi/dokimi/commands/novelty.py b/dokimi/dokimi/commands/novelty.py index ae625fd9c..2d7ec2c4c 100644 --- a/dokimi/dokimi/commands/novelty.py +++ b/dokimi/dokimi/commands/novelty.py @@ -329,9 +329,9 @@ def _routing_field(args: argparse.Namespace, exclude: set[str], competitors.append(candidate) if args.catalog: notes: list[str] = [] - catalog = catalog_competitors(args.catalog, - exclude | {c.name for c in competitors}, notes, - include_disabled=args.include_disabled) + catalog = catalog_competitors(args.catalog, exclude | {c.name for c in competitors}, + notes, include_disabled=args.include_disabled, + include_invalid=args.include_invalid) for note in notes: print(f"WARNING: {note}", file=sys.stderr) if left_out is not None: @@ -855,16 +855,16 @@ def register(sub, positive_int) -> None: help="Directory of skills to take the competitors from: every " "skill the catalog lists (skills.yaml when present, else every " "SKILL.md under it; a template/ copy and vendored or VCS " - "directories like .git are left out, and a byte-identical " - "copy under another path counts once) except " - "the artifact itself, i.e. the " - "siblings a harness lists next to it. Combines with " - "--competitors") + "directories like .git are left out, and a byte-identical copy " + "under another path counts once) except the artifact itself, the " + "siblings a harness lists next to it. Combines with --competitors") routing_p.add_argument("--include-disabled", action="store_true", dest="include_disabled", help="Keep skills that set disable-model-invocation in the field. " "Off by default: no harness lists them, so the model cannot " "select them and a card for one measures a choice that does " "not exist") + routing_p.add_argument("--include-invalid", action="store_true", dest="include_invalid", + help="Keep catalog skills with a spec error in the field") routing_p.add_argument("--queries", default=None, help="JSON list (or one per line) of user-shaped requests that " "should select this artifact, written independently of it") diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index ff13c2c71..949978fcc 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -27,6 +27,9 @@ Mirrors are common (one skill checked in under two plugin directories), and counting them twice inflates routing and coverage numbers. +Same name, different content: a skill whose ``name`` another skill shares, with other bytes, is a +``warn`` (``name-conflict``) naming the other path. Routing skips that name rather than aborting. + ``disable-model-invocation: true`` is not a finding: the model is never offered that skill, so it is reported as a flag (the same reading as the skill arm's check in :mod:`dokimi.harnesses.skill_treatment`). @@ -193,6 +196,7 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe """ reviews: list[Optional[SkillReview]] = [] copies: list[tuple[SkillReview, str, str]] = [] # (review, skill directory, digest) per loaded skill + by_name: dict[str, list[tuple[SkillReview, str, str]]] = {} for entry in entries: if not entry.loaded or entry.skill is None: reviews.append(_unloaded_review(entry)) @@ -205,6 +209,7 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe review.content_sha256 = _skill_digest(skill) where = _relative_to_root(skill.path.parent, entry.source_root) copies.append((review, where, review.content_sha256)) + by_name.setdefault(skill.name, []).append((review, where, review.content_sha256)) reviews.append(review) original_of: dict[str, tuple[SkillReview, str]] = {} @@ -222,4 +227,26 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe f"SKILL.md is identical to {where}; the same skill is counted twice", ) ) + _name_conflicts(by_name) return reviews + + +def _name_conflicts(by_name: dict[str, list[tuple[SkillReview, str, str]]]) -> None: + """A warning on each skill that shares its name with a skill of different content, naming that path. + + Identical copies are not a conflict (they are the duplicate above). The routing field skips + such a name, so the scan names both paths to say which skills are affected. + """ + for name, group in by_name.items(): + for review, _, digest in group: + others = sorted({other for _, other, other_digest in group if other_digest != digest}) + if not others: + continue + review.findings.append( + Finding( + "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", + ) + ) diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 021ba4475..919f9bfbe 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -484,27 +484,29 @@ def load_candidate(ref: str) -> tuple[Candidate, str, str | None]: def catalog_competitors(root: str, exclude: set[str], notes: list[str] | None = None, *, - include_disabled: bool = False) -> list[Candidate]: + include_disabled: bool = False, + include_invalid: bool = False) -> list[Candidate]: """Every skill under ``root`` whose name is not in ``exclude``. The field a harness would list next to the artifact, rather than one the caller picks. Skills come from the catalog reader, so a ``skills.yaml`` manifest decides which skills the catalog has. A declared skill that cannot be loaded is left out and described in ``notes`` (when given) rather than aborting the run. A skill with - ``disable-model-invocation`` is left out too, unless ``include_disabled``. A mirror, a - byte-identical ``SKILL.md`` under another path (a plugin copy of the ``skills/`` tree), is - left out too: the copy nearest the catalog root stands for the card. + ``disable-model-invocation`` is left out too, unless ``include_disabled``. A skill with a + spec error (``catalog scan`` reports it as an error, e.g. a description over 1024 + characters) is left out too, unless ``include_invalid``. A mirror, a byte-identical + ``SKILL.md`` under another path (a plugin copy of the ``skills/`` tree), is left out too: + the copy nearest the catalog root stands for the card. Two skills that share a name with + different content are both left out, and ``notes`` names both paths. Raises: ValueError: ``root`` is not a directory, or its ``skills.yaml`` is invalid (with no manifest every ``SKILL.md`` is read, and a broken one is skipped with a note). - RoutingInputError: Two skills in the field share a name with different content; the - message names both paths. """ if not Path(root).is_dir(): raise ValueError(f"--catalog is not a directory: {root}") - eligible = _eligible_catalog_skills(root, exclude, notes, include_disabled) - _refuse_conflicting_names(eligible) + eligible = _eligible_catalog_skills(root, exclude, notes, include_disabled, include_invalid) + eligible = _skip_conflicting_names(eligible, notes) keeper = _keepers_by_copy(eligible) candidates: list[Candidate] = [] for skill, digest in eligible: @@ -518,12 +520,16 @@ def catalog_competitors(root: str, exclude: set[str], def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | None, - include_disabled: bool) -> list[tuple[Skill, str]]: + include_disabled: bool, + include_invalid: bool = False) -> list[tuple[Skill, str]]: """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``.""" from dokimi.external.catalog import read_skill_manifest + from dokimi.external.validate import review_skills + entries = read_skill_manifest(Path(root), notes) + reviews = review_skills(entries) eligible: list[tuple[Skill, str]] = [] - for entry in read_skill_manifest(Path(root), notes): + 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: @@ -536,27 +542,36 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No notes.append(f"left out catalog skill {skill.name!r}: " "disable-model-invocation (--include-disabled keeps it)") continue + 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)") + continue eligible.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest())) 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 _skip_conflicting_names(eligible: list[tuple[Skill, str]], + notes: list[str] | None) -> list[tuple[Skill, str]]: + """``eligible`` without the skills of a name that two different files share. - check_field would only say "pass each competitor once", which is not the problem. + Two skills with one name and different bytes cannot both stand for the card, so the name + leaves the field, and ``notes`` names every path of it. The rest of the field stays. """ - copies_by_name: dict[str, list[tuple[str, str]]] = {} + by_name: dict[str, list[tuple[Skill, str]]] = {} for skill, digest in eligible: - copies_by_name.setdefault(skill.name, []).append((skill.path.as_posix(), digest)) - for name in sorted(copies_by_name): - copies = sorted(copies_by_name[name]) - first_path, first_digest = copies[0] - other = next((path for path, digest in copies if digest != first_digest), None) - if other is not None: - raise RoutingInputError( - f"catalog has {name} at {first_path} and {other} with " - "different content; dedupe or rename" - ) + by_name.setdefault(skill.name, []).append((skill, digest)) + skipped: set[str] = set() + for name in sorted(by_name): + copies = by_name[name] + if len({digest for _, digest in copies}) > 1: + skipped.add(name) + if notes is not None: + paths = ", ".join(sorted(skill.path.as_posix() for skill, _ in copies)) + notes.append(f"skipped catalog name {name!r}: it has different content at " + f"{paths}; dedupe or rename them to route them") + return [(skill, digest) for skill, digest in eligible if skill.name not in skipped] def _keepers_by_copy(eligible: list[tuple[Skill, str]]) -> dict[tuple[str, str], Skill]: diff --git a/dokimi/tests/data/cli_flags_allowlist.txt b/dokimi/tests/data/cli_flags_allowlist.txt index f8e037fda..0db2b4374 100644 --- a/dokimi/tests/data/cli_flags_allowlist.txt +++ b/dokimi/tests/data/cli_flags_allowlist.txt @@ -79,6 +79,7 @@ --hits # repo report replay hits (#297) adds it: a view switch has no registry concept yet --image --include-disabled # novelty routing (#172) adds it: no registry concept yet; a F1b migration names it +--include-invalid # novelty routing: keeps a skill with a spec error in the field; no registry concept yet --include-resources --input --intents diff --git a/dokimi/tests/test_catalog_command.py b/dokimi/tests/test_catalog_command.py index 9d0d79410..cb2e3a0e2 100644 --- a/dokimi/tests/test_catalog_command.py +++ b/dokimi/tests/test_catalog_command.py @@ -195,6 +195,35 @@ def test_catalog_scan_exits_zero_when_every_not_loaded_entry_is_a_benign_skip(tm assert "ERROR" not in out +def test_catalog_scan_on_a_root_with_no_skill_md_says_so_and_exits_1(tmp_path, capsys): + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path)]) + with pytest.raises(SystemExit) as exc_info: + args.func(args) + + assert exc_info.value.code == 1 + assert f"no SKILL.md found under {tmp_path}" in capsys.readouterr().err + + +def test_catalog_scan_with_tasks_but_no_skill_md_warns_and_exits_0_unless_strict(tmp_path, capsys): + _write_task(tmp_path, "onetbb-real-task") + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path)]) + args.func(args) # no SystemExit: the tasks are a catalog the scan can run + + assert f"no SKILL.md found under {tmp_path}" in capsys.readouterr().err + strict = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path), "--strict"]) + with pytest.raises(SystemExit) as exc_info: + strict.func(strict) + assert exc_info.value.code == 1 + + +def test_catalog_scan_kind_tasks_does_not_ask_for_a_skill_md(tmp_path, capsys): + _write_task(tmp_path, "onetbb-real-task") + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path), "--kind", "tasks"]) + args.func(args) + + assert "no SKILL.md" not in capsys.readouterr().err + + def test_catalog_scan_rejects_missing_root(tmp_path): parser = build_parser() args = parser.parse_args( diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 2313223a6..931a01200 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -114,7 +114,31 @@ def test_different_skill_md_with_the_same_name_is_not_a_duplicate(tmp_path): reviews = review_skills(read_skill_manifest(tmp_path)) assert all(r.duplicate_of is None for r in reviews) - assert all(r.state == "ok" for r in reviews) + assert all(r.state == "warn" for r in reviews) + assert all(r.findings[-1].code == "name-conflict" for r in reviews) + + +def test_same_name_with_different_content_names_the_other_path_on_each_skill(tmp_path): + # The routing field refuses such a name (or skips it); the scan must say so, with both paths. + _skill(tmp_path / "plugins", "x", name="x", description="Use for x.") + _skill(tmp_path / "skills", "x", name="x", description="Use for x, a different text.") + + reviews = review_skills(read_skill_manifest(tmp_path)) + messages = sorted(f.message for r in reviews for f in r.findings if f.code == "name-conflict") + assert len(messages) == 2 + # Each skill names the other one's path, and only that one. + assert any("used by plugins/x," in m for m in messages) + assert any("used by skills/x," in m for m in messages) + + +def test_same_name_different_content_is_listed_by_catalog_scan_as_a_warning(tmp_path, capsys): + _skill(tmp_path / "plugins", "x", name="x", description="Use for x.") + _skill(tmp_path / "skills", "x", name="x", description="Use for x, a different text.") + + report, code = _scan_json(tmp_path, capsys) + assert code == 0 + assert [s["state"] for s in report["skills"]] == ["warn", "warn"] + assert all(s["findings"][-1]["code"] == "name-conflict" for s in report["skills"]) def test_disable_model_invocation_is_reported_as_a_flag_not_a_finding(tmp_path): diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 2de05770d..4f11a121e 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -10,8 +10,8 @@ import pytest from dokimi.cli import build_parser +from dokimi.commands.novelty import _routing_field from dokimi.novelty.routing import ( - RoutingInputError, catalog_competitors, disables_model_invocation, ) @@ -213,22 +213,29 @@ def test_mirrors_at_one_depth_keep_the_path_that_sorts_first(tmp_path): assert f"catalog skill 'db' at {root / 'b' / 'skills' / 'db' / 'SKILL.md'}" in notes[0] -def test_two_catalog_skills_with_one_name_and_different_content_are_refused_by_name( +def test_two_catalog_skills_with_one_name_and_different_content_are_skipped_with_a_warning( tmp_path, capsys ): - # Not "pass each competitor once": the catalog holds two different cards under one name, - # and the refusal names both copies rather than the duplicate-competitor check. + # The catalog holds two different cards under one name. Routing does not abort the repo: + # both copies of that name leave the field, the rest of it stays, and stderr names both paths. root = tmp_path / "catalog" _catalog_skill(root, "perf", "profile a slow binary with perf") + _catalog_skill(root, "cache", "warm a cold cache before a benchmark") _catalog_skill(root, "db", "tune a slow database query") fork = root / "plugins" / "x" / "skills" / "db" fork.mkdir(parents=True) (fork / "SKILL.md").write_text( "---\nname: db\ndescription: a different card\n---\n\n# B\n", encoding="utf-8" ) + notes: list[str] = [] + names = [c.name for c in catalog_competitors(str(root), {"perf"}, notes)] + assert names == ["cache"] + assert len(notes) == 1 and "db" in notes[0] and "different content" in notes[0] + assert str(root / "skills" / "db" / "SKILL.md") in notes[0] + assert str(fork / "SKILL.md") in notes[0] + queries = tmp_path / "q.json" queries.write_text(json.dumps(["my binary is slow"]), encoding="utf-8") - out = tmp_path / "routing.json" args = _parse( "novelty", "routing", @@ -239,15 +246,13 @@ def test_two_catalog_skills_with_one_name_and_different_content_are_refused_by_n "--queries", str(queries), "--out", - str(out), + str(tmp_path / "routing.json"), ) - with pytest.raises(SystemExit) as refused: - args.func(args) - message = str(refused.value) - assert message.startswith("catalog has db at ") and "dedupe or rename" in message - assert str(root / "skills" / "db" / "SKILL.md") in message - assert str(fork / "SKILL.md") in message - assert "more than once" not in message and not out.exists() + left_out: list[str] = [] + field = _routing_field(args, {"perf"}, left_out) + assert [c.name for c in field] == ["cache"] + assert "WARNING: " + notes[0] in capsys.readouterr().err + assert left_out == notes def test_two_differently_named_skills_with_identical_text_both_stay_in_the_field(tmp_path): @@ -276,13 +281,48 @@ def test_two_manifest_entries_with_identical_text_and_different_names_both_stay( assert not any("identical to" in note for note in notes) -def test_two_same_name_copies_that_differ_only_late_in_the_body_are_refused(tmp_path): +def test_two_same_name_copies_that_differ_only_late_in_the_body_are_skipped(tmp_path): # The copies share the frontmatter and the first 100+ bytes, so a digest of a prefix would - # call them mirrors; the whole file decides, and they clash. + # call them mirrors; the whole file decides, and they clash, so the name leaves the field. head = "---\nname: db\ndescription: tune a slow database query\n---\n\n" + "x" * 120 for branch, tail in (("a", "\nfirst ending\n"), ("b", "\nsecond ending\n")): copy = tmp_path / branch / "skills" / "db" copy.mkdir(parents=True) (copy / "SKILL.md").write_text(head + tail, encoding="utf-8") - with pytest.raises(RoutingInputError, match="catalog has db at .* different content"): - catalog_competitors(str(tmp_path), set(), []) + notes: list[str] = [] + assert catalog_competitors(str(tmp_path), set(), notes) == [] + assert len(notes) == 1 and "db" in notes[0] + + +def _long_description_catalog(tmp_path): + root = tmp_path / "catalog" + _catalog_skill(root, "perf", "profile a slow binary with perf") + _catalog_skill(root, "long", "x" * 1100) + return root + + +def test_a_catalog_skill_over_the_spec_limit_is_left_out_with_a_note(tmp_path): + notes: list[str] = [] + names = [c.name for c in catalog_competitors(str(_long_description_catalog(tmp_path)), set(), notes)] + assert names == ["perf"] + assert len(notes) == 1 and "long" in notes[0] and "description" in notes[0] + + +def test_include_invalid_keeps_a_catalog_skill_over_the_spec_limit(tmp_path): + root = _long_description_catalog(tmp_path) + names = [c.name for c in catalog_competitors(str(root), set(), [], include_invalid=True)] + assert sorted(names) == ["long", "perf"] + + +def test_the_include_invalid_flag_reaches_the_routing_field(tmp_path, capsys): + root = _long_description_catalog(tmp_path) + queries = tmp_path / "q.json" + queries.write_text(json.dumps(["profile my binary"]), encoding="utf-8") + argv = ["novelty", "routing", "--artifact", f"skill:{root / 'skills' / 'perf'}", + "--catalog", str(root), "--queries", str(queries), "--out", str(tmp_path / "r.json")] + left_out: list[str] = [] + field = _routing_field(_parse(*argv), {"perf"}, left_out) + assert field == [] + assert "long" in capsys.readouterr().err and left_out + field = _routing_field(_parse(*argv, "--include-invalid"), {"perf"}, []) + assert [c.name for c in field] == ["long"] From 5466a5a36e1639fe3af93a31de5daa17123ab63a Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 31 Oct 2026 10:16:25 -0700 Subject: [PATCH 02/15] fix(routing): a name with different content leaves the field, whatever 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 --- ...20261031_120000_napetrov_rescan-catalog.md | 9 +-- dokimi/docs/skill-novelty.md | 7 +- dokimi/dokimi/novelty/routing.py | 69 ++++++++++++------- dokimi/tests/test_novelty_routing_field.py | 44 +++++++++++- 4 files changed, 97 insertions(+), 32 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index 1a9ea3c7f..141d56911 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -1,8 +1,9 @@ ### Added -- **`novelty routing --include-invalid`** keeps a catalog skill with a spec error (a description - over 1024 characters, a name that is not its directory name) in the `--catalog` field. Off by - default: such a skill is left out, with a warning on stderr naming the error. +- **`novelty routing --include-invalid`** keeps a catalog skill with a spec error (for example a + description over 1024 characters, or a name that is not kebab-case or not its directory name) in + the `--catalog` field. Off by default: such a skill is left out, with a warning on stderr naming + the error. ### Changed @@ -14,7 +15,7 @@ - **`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). - Both skills of that name leave the field, the rest of it stays, and stderr names every path. + Every copy of that name leaves the field, the rest of it stays, and stderr names every path. - **`catalog scan` on a root with no `SKILL.md` exits 1 with `no SKILL.md found under `** instead of exiting 0 silently. With `--kind all`, a tasks-only root only warns, and `--strict` makes that a failure. diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index e99241a1a..842ddc9e2 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -606,10 +606,11 @@ without one, every `SKILL.md` under it counts, except a `template/` copy and ven directories such as `.git` and `node_modules`, which are not walked. A byte-identical copy under another path counts once. A declared skill that cannot be loaded is skipped with a warning. Two catalog skills with the same name and different content are both skipped, with a warning that -names every path of that name: the rest of the field stays. Rename one, or delete the copy you do +names every path of that name (a disabled or spec-error copy counts too, so the field and +`catalog scan` agree): the rest of the field stays. Rename one, or delete the copy you do not mean to route against, to route the name. `dokimi catalog scan` lists the same names as -`name-conflict` warnings. A skill with a spec error (a description over 1024 characters, a name -that is not its directory name) is also left out, with a warning naming the error; pass +`name-conflict` warnings. A skill with a spec error (for example a description over 1024 characters, or a +name that is not kebab-case or not its directory name) is also left out, with a warning naming the error; pass `--include-invalid` to keep it, as a harness that refuses to list such a skill would not. A `skill:` competitor that is not a `SKILL.md`, or a directory without one, is skipped with a warning. A shell glob must be expanded first, with the prefix on each path diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 919f9bfbe..fc5e0e09d 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -39,12 +39,16 @@ from dataclasses import dataclass from functools import partial from pathlib import Path +from typing import TYPE_CHECKING from dokimi.skills import Skill from dokimi.utils import parse_frontmatter from .client import LLMClient, parse_json_object +if TYPE_CHECKING: + from dokimi.external.validate import SkillReview + # Words, with sentence punctuation left out. A version like `2.1` stays one token, and a # hyphenated name yields its parts as well as the whole: `dpnp-array-creation` could not # otherwise be matched by a query that says "array". @@ -496,8 +500,9 @@ def catalog_competitors(root: str, exclude: set[str], spec error (``catalog scan`` reports it as an error, e.g. a description over 1024 characters) is left out too, unless ``include_invalid``. A mirror, a byte-identical ``SKILL.md`` under another path (a plugin copy of the ``skills/`` tree), is left out too: - the copy nearest the catalog root stands for the card. Two skills that share a name with - different content are both left out, and ``notes`` names both paths. + the copy nearest the catalog root stands for the card. Every copy of a name whose loaded + skills differ in content is left out, with the disabled and spec-error ones too, and + ``notes`` names every path of that name. Raises: ValueError: ``root`` is not a directory, or its ``skills.yaml`` is invalid (with no @@ -506,7 +511,6 @@ def catalog_competitors(root: str, exclude: set[str], if not Path(root).is_dir(): raise ValueError(f"--catalog is not a directory: {root}") eligible = _eligible_catalog_skills(root, exclude, notes, include_disabled, include_invalid) - eligible = _skip_conflicting_names(eligible, notes) keeper = _keepers_by_copy(eligible) candidates: list[Candidate] = [] for skill, digest in eligible: @@ -522,13 +526,18 @@ def catalog_competitors(root: str, exclude: set[str], def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | None, include_disabled: bool, include_invalid: bool = False) -> list[tuple[Skill, str]]: - """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``.""" + """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``. + + 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. + """ from dokimi.external.catalog import read_skill_manifest from dokimi.external.validate import review_skills entries = read_skill_manifest(Path(root), notes) reviews = review_skills(entries) - eligible: list[tuple[Skill, str]] = [] + 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: @@ -537,41 +546,53 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No 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)") - continue - 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)") + 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 _skip_conflicting_names(eligible: list[tuple[Skill, str]], - notes: list[str] | None) -> list[tuple[Skill, str]]: - """``eligible`` without the skills of a name that two different files share. +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]: + """The names that two different files among ``loaded`` share, with a note naming each path. Two skills with one name and different bytes cannot both stand for the card, so the name - leaves the field, and ``notes`` names every path of it. The rest of the field stays. + leaves the field. The rest of the field stays. """ by_name: dict[str, list[tuple[Skill, str]]] = {} - for skill, digest in eligible: + for skill, digest in loaded: by_name.setdefault(skill.name, []).append((skill, digest)) - skipped: set[str] = set() + conflicting: set[str] = set() for name in sorted(by_name): copies = by_name[name] if len({digest for _, digest in copies}) > 1: - skipped.add(name) + conflicting.add(name) if notes is not None: paths = ", ".join(sorted(skill.path.as_posix() for skill, _ in copies)) notes.append(f"skipped catalog name {name!r}: it has different content at " f"{paths}; dedupe or rename them to route them") - return [(skill, digest) for skill, digest in eligible if skill.name not in skipped] + return conflicting def _keepers_by_copy(eligible: list[tuple[Skill, str]]) -> dict[tuple[str, str], Skill]: diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 4f11a121e..daddb07b0 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -255,6 +255,47 @@ def test_two_catalog_skills_with_one_name_and_different_content_are_skipped_with assert left_out == notes +def test_a_name_with_a_spec_error_copy_leaves_the_field_as_the_scan_warns(tmp_path): + # catalog scan warns on both copies of a name with different content, the spec-error one + # included. Routing must agree: the name leaves the field, not only when both copies pass. + root = tmp_path / "catalog" + _catalog_skill(root, "perf", "profile a slow binary with perf") + _catalog_skill(root, "db", "x" * 1100) + fork = root / "plugins" / "x" / "skills" / "db" + fork.mkdir(parents=True) + (fork / "SKILL.md").write_text( + "---\nname: db\ndescription: a valid card\n---\n\n# B\n", encoding="utf-8" + ) + notes: list[str] = [] + names = [c.name for c in catalog_competitors(str(root), {"perf"}, notes)] + assert "db" not in names + assert len(notes) == 1 and "different content" in notes[0] + assert str(root / "skills" / "db" / "SKILL.md") in notes[0] + assert str(fork / "SKILL.md") in notes[0] + + +def test_the_conflict_note_names_a_disabled_copy_of_the_name_too(tmp_path): + catalog = tmp_path / "catalog" + catalog.mkdir() + _catalog_skill(catalog, "db", "tune a slow database query") + other = catalog / "plugins" / "b" / "skills" + other.mkdir(parents=True) + (other / "db").mkdir() + (other / "db" / "SKILL.md").write_text( + "---\nname: db\ndescription: a different card\n---\n\n# B\n", encoding="utf-8") + hidden = catalog / "plugins" / "c" / "skills" / "db" + hidden.mkdir(parents=True) + (hidden / "SKILL.md").write_text( + "---\nname: db\ndescription: a hidden card\ndisable-model-invocation: true\n---\n\n# B\n", + encoding="utf-8") + notes: list[str] = [] + assert catalog_competitors(str(catalog), set(), notes) == [] + conflict = [note for note in notes if "different content" in note] + assert len(conflict) == 1 + for path in (catalog / "skills" / "db", other / "db", hidden): + assert str(path / "SKILL.md") in conflict[0] + + def test_two_differently_named_skills_with_identical_text_both_stay_in_the_field(tmp_path): # Without a frontmatter name the name is the directory's, so identical text under skills/a # and skills/b is two skills, not a mirror: both are siblings the field must show. @@ -303,7 +344,8 @@ def _long_description_catalog(tmp_path): def test_a_catalog_skill_over_the_spec_limit_is_left_out_with_a_note(tmp_path): notes: list[str] = [] - names = [c.name for c in catalog_competitors(str(_long_description_catalog(tmp_path)), set(), notes)] + root = _long_description_catalog(tmp_path) + names = [c.name for c in catalog_competitors(str(root), set(), notes)] assert names == ["perf"] assert len(notes) == 1 and "long" in notes[0] and "description" in notes[0] From 4815a1150717f7788055c01bf143fcbb032c45d7 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 31 Oct 2026 10:28:38 -0700 Subject: [PATCH 03/15] fix(routing): record include_invalid in the saved field rule 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 --- .../20261031_120000_napetrov_rescan-catalog.md | 10 ++++++---- dokimi/docs/skill-novelty.md | 2 +- dokimi/dokimi/commands/novelty.py | 2 +- dokimi/tests/test_novelty_routing_all_vs_all.py | 3 ++- dokimi/tests/test_novelty_routing_field.py | 17 +++++++++++++++++ 5 files changed, 27 insertions(+), 7 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index 141d56911..f8d657b0d 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -1,12 +1,14 @@ ### Added -- **`novelty routing --include-invalid`** keeps a catalog skill with a spec error (for example a - description over 1024 characters, or a name that is not kebab-case or not its directory name) in - the `--catalog` field. Off by default: such a skill is left out, with a warning on stderr naming - the error. +- **`novelty routing --include-invalid`** keeps a catalog skill with a spec error in the `--catalog` + field, and the saved result records the flag under `field`. ### Changed +- **`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. diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index 842ddc9e2..d3ba24b91 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -678,7 +678,7 @@ is no attractor, and a tied skill's own rate is lower by the picks inside its gr `n_scored` and `n_unscored` per skill say how many, a query with no content word or a failed model call is unscored, and a skill with none scored has an own rate of `null`, not 0.0. A run with any failed model call writes the result and exits non-zero. The saved file records the -field rule under `field` (`include_disabled`, the `competitors` and what was `left_out`, which +field rule under `field` (`include_disabled`, `include_invalid`, the `competitors` and what was `left_out`, which also names each skill left out for a conflicting name or a spec error); a single-artifact run records it too. It takes no `--artifact`, `--queries`, `--queries-from-headings` or `--repeats` (one model call per query). The model arm also records the selector `model`, so two runs with diff --git a/dokimi/dokimi/commands/novelty.py b/dokimi/dokimi/commands/novelty.py index 2d7ec2c4c..5f7e184f4 100644 --- a/dokimi/dokimi/commands/novelty.py +++ b/dokimi/dokimi/commands/novelty.py @@ -408,7 +408,7 @@ def _cmd_routing_all_vs_all(args: argparse.Namespace) -> None: def _field_record(args: argparse.Namespace, competitors: list, left_out: list[str]) -> dict: """The field rule a routing result was measured under, saved next to the result.""" - return {"include_disabled": bool(args.include_disabled), + return {**{name: bool(getattr(args, name)) for name in ("include_disabled", "include_invalid")}, "competitors": [c.name for c in competitors], "left_out": left_out} diff --git a/dokimi/tests/test_novelty_routing_all_vs_all.py b/dokimi/tests/test_novelty_routing_all_vs_all.py index f3076861b..e13f0f540 100644 --- a/dokimi/tests/test_novelty_routing_all_vs_all.py +++ b/dokimi/tests/test_novelty_routing_all_vs_all.py @@ -596,7 +596,8 @@ def test_a_single_artifact_routing_run_records_the_field_rule_too(tmp_path): "--out", str(out)) args.func(args) field = json.loads(out.read_text(encoding="utf-8"))["field"] - assert field == {"include_disabled": True, "competitors": ["db"], "left_out": []} + assert field == {"include_disabled": True, "include_invalid": False, + "competitors": ["db"], "left_out": []} def test_console_says_how_many_tied_and_unscored_queries_are_not_counted_as_picks( diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index daddb07b0..3ceb31078 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -356,6 +356,23 @@ def test_include_invalid_keeps_a_catalog_skill_over_the_spec_limit(tmp_path): assert sorted(names) == ["long", "perf"] +def test_the_saved_field_rule_records_include_invalid(tmp_path): + # The saved result states the field rule; a run that kept spec-error skills must say so. + root = _long_description_catalog(tmp_path) + _catalog_skill(root, "db", "tune a slow database query") + queries = tmp_path / "q.json" + queries.write_text(json.dumps(["profile my binary"]), encoding="utf-8") + out = tmp_path / "r.json" + argv = ["novelty", "routing", "--artifact", f"skill:{root / 'skills' / 'perf'}", + "--catalog", str(root), "--queries", str(queries), "--out", str(out)] + args = _parse(*argv) + args.func(args) + assert json.loads(out.read_text(encoding="utf-8"))["field"]["include_invalid"] is False + args = _parse(*argv, "--include-invalid") + args.func(args) + assert json.loads(out.read_text(encoding="utf-8"))["field"]["include_invalid"] is True + + def test_the_include_invalid_flag_reaches_the_routing_field(tmp_path, capsys): root = _long_description_catalog(tmp_path) queries = tmp_path / "q.json" From ec204a14e88c15825ebf5136dfb0390711bb75ad Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 31 Oct 2026 10:36:12 -0700 Subject: [PATCH 04/15] docs(routing): one changelog story for same-name skills and --include-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 --- .../20261021_130000_napetrov_routing-catalog-manifest.md | 4 ++-- dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md | 3 ++- dokimi/docs/skill-novelty.md | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dokimi/changelog.d/20261021_130000_napetrov_routing-catalog-manifest.md b/dokimi/changelog.d/20261021_130000_napetrov_routing-catalog-manifest.md index 53e0e44c6..a7f569949 100644 --- a/dokimi/changelog.d/20261021_130000_napetrov_routing-catalog-manifest.md +++ b/dokimi/changelog.d/20261021_130000_napetrov_routing-catalog-manifest.md @@ -5,5 +5,5 @@ does for `catalog scan`, so a `SKILL.md` it does not list is no longer a sibling. Without a manifest, every `SKILL.md` under the directory counts as before. A declared skill that cannot be loaded is skipped with a warning on stderr instead of making the whole field unreadable. - Two catalog skills with the same name and different content are refused, naming both paths, - rather than the run failing on a duplicate competitor name. + Two catalog skills with the same name and different content leave the field with a warning + naming every path, rather than the run being refused. diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index f8d657b0d..5fdf69314 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -1,7 +1,8 @@ ### Added - **`novelty routing --include-invalid`** keeps a catalog skill with a spec error in the `--catalog` - field, and the saved result records the flag under `field`. + field, and the saved result records the flag under `field`. A name that conflicts with another + catalog skill still leaves the field. ### Changed diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index d3ba24b91..8f0bfdddd 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -612,6 +612,7 @@ not mean to route against, to route the name. `dokimi catalog scan` lists the sa `name-conflict` warnings. A skill with a spec error (for example a description over 1024 characters, or a name that is not kebab-case or not its directory name) is also left out, with a warning naming the error; pass `--include-invalid` to keep it, as a harness that refuses to list such a skill would not. +A name that conflicts with another catalog skill leaves the field whatever the flag says. A `skill:` competitor that is not a `SKILL.md`, or a directory without one, is skipped with a warning. A shell glob must be expanded first, with the prefix on each path (`skill:dir/a/SKILL.md skill:dir/b/SKILL.md`): an unexpanded `skill:dir/*` is refused as a missing From d3ba18b583b481175ac3c1f8160edea8b21aad48 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sat, 31 Oct 2026 22:30:31 -0700 Subject: [PATCH 05/15] fix(routing): take the digest, disabled flag and conflicts from the review; 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 --- ...20261031_120000_napetrov_rescan-catalog.md | 17 ++++---- dokimi/dokimi/commands/catalog.py | 5 ++- dokimi/dokimi/external/validate.py | 5 ++- dokimi/dokimi/novelty/routing.py | 42 +++++++++---------- dokimi/tests/test_catalog_command.py | 13 ++++++ 5 files changed, 50 insertions(+), 32 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index 5fdf69314..f16857c83 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -9,16 +9,19 @@ - **`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. + 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 + same `--catalog`: rerun it to compare. +- **`novelty routing --catalog` skips a name that two different skills share, with a warning, instead of + refusing the run** (exit 2). Every copy of that name leaves the field, the rest of it stays, and stderr + names every path. - **`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. + naming the other path and that `novelty routing --catalog` leaves the name out of the field. 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. - **`catalog scan` on a root with no `SKILL.md` exits 1 with `no SKILL.md found under `** instead of exiting 0 silently. With `--kind all`, a tasks-only root only warns, and `--strict` - makes that a failure. + makes that a failure. A root with a `skills.yaml` that has no skill that can be scanned says + `skills.yaml under has no skill that can be scanned` instead. diff --git a/dokimi/dokimi/commands/catalog.py b/dokimi/dokimi/commands/catalog.py index 9e61fc3e6..a6629967b 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -134,7 +134,10 @@ def cmd_catalog_scan(args: argparse.Namespace) -> None: 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) + if (root / "skills.yaml").is_file(): + print(f"skills.yaml under {root} has no skill that can be scanned", file=sys.stderr) + else: + print(f"no SKILL.md found under {root}", file=sys.stderr) if args.kind == "skills" or args.strict or not report["tasks"]: any_failure = True if as_json: diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 949978fcc..8dc7946e2 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -28,7 +28,7 @@ counting them twice inflates routing and coverage numbers. Same name, different content: a skill whose ``name`` another skill shares, with other bytes, is a -``warn`` (``name-conflict``) naming the other path. Routing skips that name rather than aborting. +``warn`` (``name-conflict``) naming the other path. Routing leaves that name out of the field rather than aborting. ``disable-model-invocation: true`` is not a finding: the model is never offered that skill, so it is reported as a flag (the same reading as the skill arm's @@ -247,6 +247,7 @@ def _name_conflicts(by_name: dict[str, list[tuple[SkillReview, str, str]]]) -> N "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", + "`novelty routing --catalog` leaves this name out of the field " + "until one is renamed or removed", ) ) diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index fc5e0e09d..7b7e7e876 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -30,7 +30,6 @@ from __future__ import annotations -import hashlib import json import math import random @@ -537,17 +536,18 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No entries = read_skill_manifest(Path(root), notes) reviews = review_skills(entries) - loaded: list[tuple[Skill, str, SkillReview | None]] = [] + loaded: list[tuple[Skill, str, SkillReview]] = [] for entry, review in zip(entries, reviews, strict=True): skill = entry.skill - if skill is None or not entry.loaded: + if skill is None or not entry.loaded or review is None: if notes is not None: notes.append(f"skipped catalog entry {entry.name!r}: {entry.error}") continue if skill.name in exclude: continue - loaded.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest(), review)) - conflicting = _conflicting_names([(skill, digest) for skill, digest, _ in loaded], notes) + # the digest and the disabled flag are the review's, so routing and catalog scan cannot disagree + loaded.append((skill, review.content_sha256 or "", review)) + conflicting = _conflicting_names(loaded, notes) eligible: list[tuple[Skill, str]] = [] for skill, digest, review in loaded: if skill.name in conflicting or _left_out_by_flag( @@ -557,15 +557,15 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No return eligible -def _left_out_by_flag(skill: Skill, review: SkillReview | None, include_disabled: bool, +def _left_out_by_flag(skill: Skill, review: SkillReview, 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 not include_disabled and review.disable_model_invocation: 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 not include_invalid 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}: " @@ -574,24 +574,22 @@ def _left_out_by_flag(skill: Skill, review: SkillReview | None, include_disabled return False -def _conflicting_names(loaded: list[tuple[Skill, str]], notes: list[str] | None) -> set[str]: +def _conflicting_names(loaded: list[tuple[Skill, str, SkillReview]], + notes: list[str] | None) -> set[str]: """The names that two different files among ``loaded`` share, with a note naming each path. Two skills with one name and different bytes cannot both stand for the card, so the name - leaves the field. The rest of the field stays. + leaves the field. The rest of the field stays. The names are the ``name-conflict`` findings + of the reviews, which ``catalog scan`` reports too. """ - by_name: dict[str, list[tuple[Skill, str]]] = {} - for skill, digest in loaded: - by_name.setdefault(skill.name, []).append((skill, digest)) - conflicting: set[str] = set() - for name in sorted(by_name): - copies = by_name[name] - if len({digest for _, digest in copies}) > 1: - conflicting.add(name) - if notes is not None: - paths = ", ".join(sorted(skill.path.as_posix() for skill, _ in copies)) - notes.append(f"skipped catalog name {name!r}: it has different content at " - f"{paths}; dedupe or rename them to route them") + conflicting = {skill.name for skill, _, review in loaded + if any(f.code == "name-conflict" for f in review.findings)} + if notes is not None: + for name in sorted(conflicting): + paths = ", ".join(sorted(skill.path.as_posix() for skill, _, _ in loaded + if skill.name == name)) + notes.append(f"skipped catalog name {name!r}: it has different content at " + f"{paths}; dedupe or rename them to route them") return conflicting diff --git a/dokimi/tests/test_catalog_command.py b/dokimi/tests/test_catalog_command.py index cb2e3a0e2..f3066343d 100644 --- a/dokimi/tests/test_catalog_command.py +++ b/dokimi/tests/test_catalog_command.py @@ -204,6 +204,19 @@ def test_catalog_scan_on_a_root_with_no_skill_md_says_so_and_exits_1(tmp_path, c assert f"no SKILL.md found under {tmp_path}" in capsys.readouterr().err +def test_catalog_scan_with_a_manifest_that_declares_no_skill_says_so_and_exits_1(tmp_path, capsys): + _write_skill(tmp_path, "lint") + (tmp_path / "skills.yaml").write_text("skills: []\n", encoding="utf-8") + args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path)]) + with pytest.raises(SystemExit) as exc_info: + args.func(args) + + assert exc_info.value.code == 1 + err = capsys.readouterr().err + assert f"skills.yaml under {tmp_path} has no skill that can be scanned" in err + assert "no SKILL.md found" not in err + + def test_catalog_scan_with_tasks_but_no_skill_md_warns_and_exits_0_unless_strict(tmp_path, capsys): _write_task(tmp_path, "onetbb-real-task") args = build_parser().parse_args(["catalog", "scan", "--root", str(tmp_path)]) From c848aeccbdf77105c26c8f939f091edbfa107d08 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 1 Nov 2026 10:51:13 -0800 Subject: [PATCH 06/15] fix(routing): flag filters before the name-conflict check; check explicit 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 --- dokimi/docs/skill-novelty.md | 10 +-- dokimi/dokimi/commands/novelty.py | 16 +++-- dokimi/dokimi/external/validate.py | 15 +++-- dokimi/dokimi/novelty/routing.py | 57 ++++++++++++----- dokimi/scripts/growth_baseline.json | 2 +- dokimi/tests/test_catalog_validate.py | 10 +++ dokimi/tests/test_novelty_routing_field.py | 72 ++++++++++++++++++---- 7 files changed, 137 insertions(+), 45 deletions(-) diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index 8f0bfdddd..339f359b8 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -606,12 +606,14 @@ without one, every `SKILL.md` under it counts, except a `template/` copy and ven directories such as `.git` and `node_modules`, which are not walked. A byte-identical copy under another path counts once. A declared skill that cannot be loaded is skipped with a warning. Two catalog skills with the same name and different content are both skipped, with a warning that -names every path of that name (a disabled or spec-error copy counts too, so the field and -`catalog scan` agree): the rest of the field stays. Rename one, or delete the copy you do +names every path of that name: the rest of the field stays. Only the copies the field lists count: +a disabled copy (unless `--include-disabled`) or a spec-error copy (unless `--include-invalid`) is +left out first, so it does not make its name a conflict. Rename one, or delete the copy you do not mean to route against, to route the name. `dokimi catalog scan` lists the same names as -`name-conflict` warnings. A skill with a spec error (for example a description over 1024 characters, or a +`name-conflict` warnings, for the copies it lists. A skill with a spec error (for example a description over 1024 characters, or a name that is not kebab-case or not its directory name) is also left out, with a warning naming the error; pass -`--include-invalid` to keep it, as a harness that refuses to list such a skill would not. +`--include-invalid` to keep it, as a harness that refuses to list such a skill would not. A `skill:` +competitor with a spec error is skipped with the same warning. A name that conflicts with another catalog skill leaves the field whatever the flag says. A `skill:` competitor that is not a `SKILL.md`, or a directory without one, is skipped with a warning. A shell glob must be expanded first, with the prefix on each path diff --git a/dokimi/dokimi/commands/novelty.py b/dokimi/dokimi/commands/novelty.py index 5f7e184f4..f46d663d7 100644 --- a/dokimi/dokimi/commands/novelty.py +++ b/dokimi/dokimi/commands/novelty.py @@ -304,9 +304,8 @@ def _routing_field(args: argparse.Namespace, exclude: set[str], """ from dokimi.novelty.routing import ( catalog_competitors, - disables_model_invocation, + competitor_left_out, non_skill_reason, - skill_file, ) competitors = [] @@ -318,13 +317,12 @@ def _routing_field(args: argparse.Namespace, exclude: set[str], left_out.append(f"{ref}: {reason}") continue candidate = _routing_card(ref)[0] - ref_file = skill_file(ref) - if (ref_file and not args.include_disabled - and disables_model_invocation(ref_file)): - print(f"WARNING: skipped competitor {ref}: disable-model-invocation " - "(--include-disabled keeps it)", file=sys.stderr) + reason = competitor_left_out(ref, include_disabled=args.include_disabled, + include_invalid=args.include_invalid) + if reason: + print(f"WARNING: skipped competitor {ref}: {reason}", file=sys.stderr) if left_out is not None: - left_out.append(f"{ref}: disable-model-invocation") + left_out.append(f"{ref}: {reason}") continue competitors.append(candidate) if args.catalog: @@ -864,7 +862,7 @@ def register(sub, positive_int) -> None: "select them and a card for one measures a choice that does " "not exist") routing_p.add_argument("--include-invalid", action="store_true", dest="include_invalid", - help="Keep catalog skills with a spec error in the field") + help="Keep catalog and skill: competitors with a spec error in the field") routing_p.add_argument("--queries", default=None, help="JSON list (or one per line) of user-shaped requests that " "should select this artifact, written independently of it") diff --git a/dokimi/dokimi/external/validate.py b/dokimi/dokimi/external/validate.py index 8dc7946e2..8344c7d8e 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -27,8 +27,9 @@ Mirrors are common (one skill checked in under two plugin directories), and counting them twice inflates routing and coverage numbers. -Same name, different content: a skill whose ``name`` another skill shares, with other bytes, is a -``warn`` (``name-conflict``) naming the other path. Routing leaves that name out of the field rather than aborting. +Same name, different content: a skill whose ``name`` another listed skill shares, with other bytes, is a +``warn`` (``name-conflict``) naming the other path. A listed skill is one a harness offers (no +``disable-model-invocation``, no spec error). Routing leaves that name out of the field rather than aborting. ``disable-model-invocation: true`` is not a finding: the model is never offered that skill, so it is reported as a flag (the same reading as the skill arm's @@ -232,12 +233,16 @@ def review_skills(entries: Sequence[SkillCatalogEntry]) -> list[Optional[SkillRe def _name_conflicts(by_name: dict[str, list[tuple[SkillReview, str, str]]]) -> None: - """A warning on each skill that shares its name with a skill of different content, naming that path. + """A warning on each listed skill that shares its name with a listed skill of different content. - Identical copies are not a conflict (they are the duplicate above). The routing field skips - such a name, so the scan names both paths to say which skills are affected. + Listed means a harness offers it: a skill with ``disable-model-invocation`` or a spec error is + left out of the routing field by default, so its copy is no conflict. Identical copies are not + a conflict (they are the duplicate above). The routing field skips such a name, so the scan + names both paths to say which skills are affected. """ for name, group in by_name.items(): + group = [entry for entry in group + if not entry[0].disable_model_invocation and entry[0].state != "error"] for review, _, digest in group: others = sorted({other for _, other, other_digest in group if other_digest != digest}) if not others: diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 7b7e7e876..7eff0bbd3 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -446,6 +446,33 @@ def non_skill_reason(ref: str) -> str | None: return None +def competitor_left_out(ref: str, *, include_disabled: bool, include_invalid: bool) -> str | None: + """Why an explicit ``skill:`` competitor is left out of the field, as the note says, or None. + + The same two rules as a catalog skill: a ``disable-model-invocation`` skill (unless + ``include_disabled``) and a skill with a spec error (unless ``include_invalid``). + """ + from dokimi.external.catalog import SkillCatalogEntry + from dokimi.external.validate import review_skills + from dokimi.skills import load_skill + + path = skill_file(ref) + if path is None: + return None + if not include_disabled and disables_model_invocation(path): + return "disable-model-invocation (--include-disabled keeps it)" + if include_invalid: + return None + skill = load_skill(path) + entry = SkillCatalogEntry(name=skill.name, status=None, path=path.parent, + source_root=path.parent.parent, skill=skill) + review = review_skills([entry])[0] + if review is None or review.state != "error": + return None + problems = "; ".join(f.message for f in review.findings if f.level == "error") + return f"spec error ({problems}; --include-invalid keeps it)" + + def skill_file(ref: str) -> Path | None: """The ``SKILL.md`` a ``skill:`` reference names, or ``None`` for any other reference.""" if not ref.startswith("skill:"): @@ -547,14 +574,13 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No continue # the digest and the disabled flag are the review's, so routing and catalog scan cannot disagree loaded.append((skill, review.content_sha256 or "", review)) - conflicting = _conflicting_names(loaded, notes) - eligible: list[tuple[Skill, str]] = [] + # the flags first: a copy no harness lists (disabled, or a spec error) does not make its name a conflict + listed: 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, digest)) - return eligible + if not _left_out_by_flag(skill, review, include_disabled, include_invalid, notes): + listed.append((skill, digest)) + conflicting = _conflicting_names(listed, notes) + return [(skill, digest) for skill, digest in listed if skill.name not in conflicting] def _left_out_by_flag(skill: Skill, review: SkillReview, include_disabled: bool, @@ -574,19 +600,20 @@ def _left_out_by_flag(skill: Skill, review: SkillReview, include_disabled: bool, return False -def _conflicting_names(loaded: list[tuple[Skill, str, SkillReview]], - notes: list[str] | None) -> set[str]: - """The names that two different files among ``loaded`` share, with a note naming each path. +def _conflicting_names(listed: list[tuple[Skill, str]], notes: list[str] | None) -> set[str]: + """The names that two different files among ``listed`` share, with a note naming each path. Two skills with one name and different bytes cannot both stand for the card, so the name - leaves the field. The rest of the field stays. The names are the ``name-conflict`` findings - of the reviews, which ``catalog scan`` reports too. + leaves the field. The rest of the field stays. ``listed`` holds only the copies the flags + let into the field, so ``catalog scan`` names the same copies as conflicting. """ - conflicting = {skill.name for skill, _, review in loaded - if any(f.code == "name-conflict" for f in review.findings)} + digests: dict[str, set[str]] = {} + for skill, digest in listed: + digests.setdefault(skill.name, set()).add(digest) + conflicting = {name for name, found in digests.items() if len(found) > 1} if notes is not None: for name in sorted(conflicting): - paths = ", ".join(sorted(skill.path.as_posix() for skill, _, _ in loaded + paths = ", ".join(sorted(skill.path.as_posix() for skill, _ in listed if skill.name == name)) notes.append(f"skipped catalog name {name!r}: it has different content at " f"{paths}; dedupe or rename them to route them") diff --git a/dokimi/scripts/growth_baseline.json b/dokimi/scripts/growth_baseline.json index d751c4643..2031f1bf8 100644 --- a/dokimi/scripts/growth_baseline.json +++ b/dokimi/scripts/growth_baseline.json @@ -1,6 +1,6 @@ { "files": { - "dokimi/commands/novelty.py": {"lines": 1189, "rationale": "estimate-only projection and W1 routing cost output (#164) from main, plus the #172 routing field, --all-vs-all, tied/unscored caveat and blind-skill warning, --repeats refused under --all-vs-all; file split tracked separately"}, + "dokimi/commands/novelty.py": {"lines": 1187, "rationale": "estimate-only projection and W1 routing cost output (#164) from main, plus the #172 routing field, --all-vs-all, tied/unscored caveat and blind-skill warning, --repeats refused under --all-vs-all; file split tracked separately"}, "dokimi/harnesses/benchflow.py": {"lines": 869}, "dokimi/harnesses/docker_solver.py": {"lines": 899, "rationale": "pre-existing when the gate was introduced"}, "dokimi/llm.py": {"lines": 1093, "rationale": "grew on main in #180 (replay a stored retry) after #93 seeded this baseline; split it in its own PR"}, diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 931a01200..35c96f3e8 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -141,6 +141,16 @@ def test_same_name_different_content_is_listed_by_catalog_scan_as_a_warning(tmp_ assert all(s["findings"][-1]["code"] == "name-conflict" for s in report["skills"]) +def test_a_disabled_copy_of_a_name_is_not_a_name_conflict(tmp_path): + # the routing field lists only the enabled copy, so there is no conflict to warn about + _skill(tmp_path / "skills", "x", name="x", description="Use for x.") + _skill(tmp_path / "plugins", "x", name="x", description="Use for x, hidden.", + extra="disable-model-invocation: true\n") + reviews = review_skills(read_skill_manifest(tmp_path)) + assert all(f.code != "name-conflict" for r in reviews for f in r.findings) + assert [r.state for r in reviews] == ["ok", "ok"] + + def test_disable_model_invocation_is_reported_as_a_flag_not_a_finding(tmp_path): _skill(tmp_path / "skills", "hidden", name="hidden", extra="disable-model-invocation: true\n") diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 3ceb31078..a561dfa0e 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -255,9 +255,8 @@ def test_two_catalog_skills_with_one_name_and_different_content_are_skipped_with assert left_out == notes -def test_a_name_with_a_spec_error_copy_leaves_the_field_as_the_scan_warns(tmp_path): - # catalog scan warns on both copies of a name with different content, the spec-error one - # included. Routing must agree: the name leaves the field, not only when both copies pass. +def test_a_spec_error_copy_is_no_conflict_with_a_valid_copy_of_the_name(tmp_path): + # the spec-error copy is left out by default, so the valid one stays and nothing conflicts root = tmp_path / "catalog" _catalog_skill(root, "perf", "profile a slow binary with perf") _catalog_skill(root, "db", "x" * 1100) @@ -268,31 +267,82 @@ def test_a_name_with_a_spec_error_copy_leaves_the_field_as_the_scan_warns(tmp_pa ) notes: list[str] = [] names = [c.name for c in catalog_competitors(str(root), {"perf"}, notes)] + assert names == ["db"] + assert not [note for note in notes if "different content" in note] + assert str(root / "skills" / "db" / "SKILL.md") in " ".join(notes) # left out for its spec error + + +def test_include_invalid_makes_the_spec_error_copy_a_conflict_again(tmp_path): + root = tmp_path / "catalog" + _catalog_skill(root, "perf", "profile a slow binary with perf") + _catalog_skill(root, "db", "x" * 1100) + fork = root / "plugins" / "x" / "skills" / "db" + fork.mkdir(parents=True) + (fork / "SKILL.md").write_text( + "---\nname: db\ndescription: a valid card\n---\n\n# B\n", encoding="utf-8" + ) + notes: list[str] = [] + names = [c.name for c in catalog_competitors(str(root), {"perf"}, notes, include_invalid=True)] assert "db" not in names - assert len(notes) == 1 and "different content" in notes[0] - assert str(root / "skills" / "db" / "SKILL.md") in notes[0] - assert str(fork / "SKILL.md") in notes[0] + conflict = [note for note in notes if "different content" in note] + assert len(conflict) == 1 + assert str(root / "skills" / "db" / "SKILL.md") in conflict[0] + assert str(fork / "SKILL.md") in conflict[0] -def test_the_conflict_note_names_a_disabled_copy_of_the_name_too(tmp_path): +def test_an_explicit_spec_error_competitor_is_skipped_with_a_warning(tmp_path, capsys): + # a skill: competitor gets the same rule as a catalog skill: left out with a warning, not kept silently + long_ref = _skill(tmp_path, "long", "x" * 1100) + queries = tmp_path / "q.json" + queries.write_text(json.dumps(["profile a binary"]), encoding="utf-8") + argv = ["novelty", "routing", "--artifact", _skill(tmp_path, "a", "profile a binary"), + "--competitors", long_ref, _skill(tmp_path, "b", "tune a database"), + "--queries", str(queries), "--out", str(tmp_path / "r.json")] + left_out: list[str] = [] + field = _routing_field(_parse(*argv), {"a"}, left_out) + assert [c.name for c in field] == ["b"] + assert f"skipped competitor {long_ref}: spec error" in capsys.readouterr().err + assert any(long_ref in entry and "spec error" in entry for entry in left_out) + field = _routing_field(_parse(*argv, "--include-invalid"), {"a"}, []) + assert sorted(c.name for c in field) == ["b", "long"] + + +def _db_with_a_hidden_copy(tmp_path): catalog = tmp_path / "catalog" catalog.mkdir() _catalog_skill(catalog, "db", "tune a slow database query") - other = catalog / "plugins" / "b" / "skills" + other = catalog / "plugins" / "b" / "skills" / "db" other.mkdir(parents=True) - (other / "db").mkdir() - (other / "db" / "SKILL.md").write_text( + (other / "SKILL.md").write_text( "---\nname: db\ndescription: a different card\n---\n\n# B\n", encoding="utf-8") hidden = catalog / "plugins" / "c" / "skills" / "db" hidden.mkdir(parents=True) (hidden / "SKILL.md").write_text( "---\nname: db\ndescription: a hidden card\ndisable-model-invocation: true\n---\n\n# B\n", encoding="utf-8") + return catalog, other, hidden + + +def test_a_disabled_copy_of_a_name_is_no_conflict_with_the_enabled_copies(tmp_path): + # no harness lists the disabled copy, so the two listed copies are the conflict, and the + # disabled one is not named in it + catalog, other, hidden = _db_with_a_hidden_copy(tmp_path) notes: list[str] = [] assert catalog_competitors(str(catalog), set(), notes) == [] conflict = [note for note in notes if "different content" in note] assert len(conflict) == 1 - for path in (catalog / "skills" / "db", other / "db", hidden): + for path in (catalog / "skills" / "db", other): + assert str(path / "SKILL.md") in conflict[0] + assert str(hidden / "SKILL.md") not in conflict[0] + + +def test_include_disabled_brings_the_disabled_copy_back_into_the_conflict(tmp_path): + catalog, other, hidden = _db_with_a_hidden_copy(tmp_path) + notes: list[str] = [] + assert catalog_competitors(str(catalog), set(), notes, include_disabled=True) == [] + conflict = [note for note in notes if "different content" in note] + assert len(conflict) == 1 + for path in (catalog / "skills" / "db", other, hidden): assert str(path / "SKILL.md") in conflict[0] From a283eedb00afd113a00cb7ee5bdd7534a965eabf Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 1 Nov 2026 11:14:13 -0800 Subject: [PATCH 07/15] fix(routing): one verdict per skill: competitor, whatever the path spelling 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 --- ...20261031_120000_napetrov_rescan-catalog.md | 5 +-- dokimi/dokimi/novelty/routing.py | 16 ++++++---- dokimi/tests/test_novelty_routing_field.py | 18 +++++++++++ dokimi/tests/test_skill_repo_shapes.py | 32 +------------------ 4 files changed, 32 insertions(+), 39 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index f16857c83..ae0549ecd 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -9,9 +9,10 @@ - **`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. A name with two different copies, or a copy that is `disable-model-invocation`, also changes + keeps it. A name with two different copies also changes the field, so a routing result saved before this change is not comparable with one saved now for the - same `--catalog`: rerun it to compare. + same `--catalog`: rerun it to compare. With `--include-disabled`, a copy that is + `disable-model-invocation` counts as one of the copies. - **`novelty routing --catalog` skips a name that two different skills share, with a warning, instead of refusing the run** (exit 2). Every copy of that name leaves the field, the rest of it stays, and stderr names every path. diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 7eff0bbd3..23fe82de7 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -32,6 +32,7 @@ import json import math +import os import random import re from collections import Counter @@ -459,6 +460,9 @@ def competitor_left_out(ref: str, *, include_disabled: bool, include_invalid: bo path = skill_file(ref) if path is None: return None + # absolute first: a relative ``skill:SKILL.md`` has parent.parent == "." and would be read as a + # root skill (no directory-name check), so the same skill would get a different verdict by spelling + path = Path(os.path.abspath(path)) if not include_disabled and disables_model_invocation(path): return "disable-model-invocation (--include-disabled keeps it)" if include_invalid: @@ -526,9 +530,9 @@ def catalog_competitors(root: str, exclude: set[str], spec error (``catalog scan`` reports it as an error, e.g. a description over 1024 characters) is left out too, unless ``include_invalid``. A mirror, a byte-identical ``SKILL.md`` under another path (a plugin copy of the ``skills/`` tree), is left out too: - the copy nearest the catalog root stands for the card. Every copy of a name whose loaded - skills differ in content is left out, with the disabled and spec-error ones too, and - ``notes`` names every path of that name. + the copy nearest the catalog root stands for the card. 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. Raises: ValueError: ``root`` is not a directory, or its ``skills.yaml`` is invalid (with no @@ -554,9 +558,9 @@ def _eligible_catalog_skills(root: str, exclude: set[str], notes: list[str] | No include_invalid: bool = False) -> list[tuple[Skill, str]]: """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``. - 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. """ from dokimi.external.catalog import read_skill_manifest from dokimi.external.validate import review_skills diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index a561dfa0e..140996272 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -13,6 +13,7 @@ from dokimi.commands.novelty import _routing_field from dokimi.novelty.routing import ( catalog_competitors, + competitor_left_out, disables_model_invocation, ) @@ -307,6 +308,23 @@ def test_an_explicit_spec_error_competitor_is_skipped_with_a_warning(tmp_path, c assert sorted(c.name for c in field) == ["b", "long"] +def test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict(tmp_path, monkeypatch): + # ``skill:SKILL.md`` and ``skill:.`` (relative, from inside the skill) must get the verdict of + # the absolute reference: the directory name is checked for a skill that is not a catalog root + skill_dir = tmp_path / "checkout" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: db\ndescription: tune a database query\n---\n\n# B\n\n## A section\n", + encoding="utf-8") + absolute = competitor_left_out(f"skill:{skill_dir}/SKILL.md", + include_disabled=False, include_invalid=False) + assert absolute is not None and "not the directory name 'checkout'" in absolute + monkeypatch.chdir(skill_dir) + for ref in ("skill:SKILL.md", "skill:.", f"skill:{skill_dir}"): + assert competitor_left_out(ref, include_disabled=False, include_invalid=False) == absolute, ref + assert competitor_left_out("skill:SKILL.md", include_disabled=False, include_invalid=True) is None + + def _db_with_a_hidden_copy(tmp_path): catalog = tmp_path / "catalog" catalog.mkdir() diff --git a/dokimi/tests/test_skill_repo_shapes.py b/dokimi/tests/test_skill_repo_shapes.py index 0f01d3e0b..6b0364f04 100644 --- a/dokimi/tests/test_skill_repo_shapes.py +++ b/dokimi/tests/test_skill_repo_shapes.py @@ -20,9 +20,6 @@ from dokimi.novelty.client import LLMClient from tests.skill_repo_shapes import PROBE_QUERIES, SHAPES, build_shape, probe_artifact -FIXED_BY_CATALOG = "fixed by fix/rescan-catalog" -FIXED_BY_ROUTING = "fixed by fix/rescan-routing" - @dataclass(frozen=True) class Expect: @@ -54,17 +51,6 @@ def _ok(*field: str) -> Expect: "no_usable_headings": _ok(*_TWO), } -# Shape -> (the dimensions a bug still breaks on intel/main, the fix that will change them). -# Dimensions: scan, strict, routing, field. Only those dimensions are expected failures. -OPEN_BUGS = { - "drifted_mirror": (("routing",), FIXED_BY_CATALOG), - "same_name_distinct_dirs": (("routing",), FIXED_BY_CATALOG), - "zero_skill": (("scan", "strict"), FIXED_BY_CATALOG), - "description_too_long": (("field",), FIXED_BY_ROUTING), - "name_not_dir": (("field",), FIXED_BY_ROUTING), - "title_case_name": (("field",), FIXED_BY_ROUTING), -} - def _run(argv: list[str], capsys) -> tuple[int, str, str]: """Run one dokimi command in-process; (exit code, stdout, stderr). Crashes propagate.""" @@ -159,28 +145,12 @@ def _holds(shape: str, observed: dict) -> dict[str, bool]: @pytest.mark.parametrize("shape", sorted(SHAPES)) def test_every_shape_gets_its_expected_status(shape, shape_paths, tmp_path, capsys, fake_selector): - # Every dimension is strict except the one a bug in another PR still breaks (OPEN_BUGS). observed = _observe(shape, shape_paths, tmp_path, capsys) holds = _holds(shape, observed) - open_dims = OPEN_BUGS[shape][0] if shape in OPEN_BUGS else () - failed = [dim for dim, ok in holds.items() if not ok and dim not in open_dims] + failed = [dim for dim, ok in holds.items() if not ok] assert not failed, f"{shape}: {failed} off expectation; observed {observed}" -@pytest.mark.parametrize("shape", [ - pytest.param(shape, id=shape, - marks=pytest.mark.xfail(strict=True, raises=AssertionError, reason=reason)) - for shape, (_, reason) in sorted(OPEN_BUGS.items()) -]) -def test_open_bug_dimension_meets_its_expectation(shape, shape_paths, tmp_path, capsys, - fake_selector): - # Strict xfail on the open dimensions only: when that fix lands, this XPASSes and the mark comes off. - observed = _observe(shape, shape_paths, tmp_path, capsys) - holds = _holds(shape, observed) - for dim in OPEN_BUGS[shape][0]: - assert holds[dim], f"{shape}.{dim}: observed {observed}" - - def test_a_mirror_is_listed_by_scan_but_counted_once_in_the_field(shape_paths, tmp_path, capsys, fake_selector): root, artifact, queries = shape_paths From c3d32a173e3ca9e9011a7ade54d0724b00f5f174 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Sun, 1 Nov 2026 20:24:05 +0000 Subject: [PATCH 08/15] Note the skipped spec-error skill: competitor in the changelog Signed-off-by: Anton Volkov --- dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index ae0549ecd..6c1df570b 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -9,7 +9,7 @@ - **`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. A name with two different copies also changes + keeps it. A `--competitors skill:` with a spec error is skipped the same way. A name with two different copies also changes the field, so a routing result saved before this change is not comparable with one saved now for the same `--catalog`: rerun it to compare. With `--include-disabled`, a copy that is `disable-model-invocation` counts as one of the copies. From c936c71f49753b05f97f2b74125d5fcc95dbf358 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 1 Nov 2026 13:05:06 -0800 Subject: [PATCH 09/15] docs: name-conflict counts listed skills; the no-SKILL.md row uses --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 --- ...20261031_120000_napetrov_rescan-catalog.md | 13 +++++----- dokimi/docs/external-content.md | 26 +++++++++---------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index 6c1df570b..411f925e0 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -9,16 +9,17 @@ - **`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. A `--competitors skill:` with a spec error is skipped the same way. A name with two different copies also changes - the field, so a routing result saved before this change is not comparable with one saved now for the - same `--catalog`: rerun it to compare. With `--include-disabled`, a copy that is - `disable-model-invocation` counts as one of the copies. + keeps it. A `--competitors skill:` with a spec error is skipped the same way. A name with two + different copies also changes the field, so a routing result saved before this change is not + comparable with one saved now for the same `--catalog`: rerun it to compare. With + `--include-disabled`, a copy that is `disable-model-invocation` counts as one of the copies. - **`novelty routing --catalog` skips a name that two different skills share, with a warning, instead of refusing the run** (exit 2). Every copy of that name leaves the field, the rest of it stays, and stderr names every path. - **`catalog scan` reports a same-name skill with different content as a `name-conflict` warning** - naming the other path and that `novelty routing --catalog` leaves the name out of the field. Before, - each of the two skills was reported `ok`. It is a warning, not an error, so `--strict` does not fail on it. + (among listed skills: no `disable-model-invocation`, no spec error), naming the other path and that + `novelty routing --catalog` leaves the name out of the field. Before, each of the two skills was + reported `ok`. It is a warning, not an error, so `--strict` does not fail on it. ### Fixed diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index 5a299350a..b3a68ae48 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -120,18 +120,18 @@ with no directory is skipped, while a `planned` *skill* with no directory is an error — the skill reader has no notion of a not-yet-written entry. Either drop such a skill from `skills.yaml` or expect the non-zero exit. -A loaded skill is also checked against the Agent Skills spec. Its `name` must be -its directory name and kebab-case (at most 64 characters; a `SKILL.md` directly in the scanned root is not compared with the root directory's name), and its `description` -at most 1024 characters: a violation is an `error`, printed under the skill. It -exits 1 only with `--strict`; without it, the scan reports the error and still -exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to -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 -`dokimi novelty routing --catalog` skips that name rather than the whole catalog. Of identical copies, the one nearest the root (fewest -path parts, ties to path order) is the original, the copy the `skill-catalog:` arm -offers; each other copy is the `warn` and names it. Warnings never change -the exit code. `disable-model-invocation: true` is printed as a `note`, because -the model is never offered that skill. +A loaded skill is also checked against the Agent Skills spec. Its `name` must be its directory name +and kebab-case (at most 64 characters; a `SKILL.md` directly in the scanned root is not compared +with the root directory's name), and its `description` at most 1024 characters: a violation is an +`error`, printed under the skill. It exits 1 only with `--strict`; without it, the scan reports the +error and still exits 0. A `name` missing from the frontmatter, or a `SKILL.md` byte-identical to +another skill's, is a `warn`. So is a skill whose `name` another skill shares with different content +(`name-conflict`), when both are listed: neither is `disable-model-invocation` or has a spec error. +The finding names the other path, and `dokimi novelty routing --catalog` skips that name rather than +the whole catalog. Of identical copies, the one nearest the root (fewest path parts, ties to path +order) is the original, the copy the `skill-catalog:` arm offers; each other copy is the `warn` and +names it. Warnings never change the exit code. `disable-model-invocation: true` is printed as a +`note`, because the model is never offered that skill. Frontmatter that is not valid YAML (a bare colon in `description:`, say) stops the skill from loading, since the loader reads it as having no description. The @@ -396,7 +396,7 @@ Worth knowing before you read a suspiciously clean result: | Command | Root missing | Exit | |---|---|---| | `catalog scan --root ` | `--root is not a directory: ` | 1 | -| `catalog scan --root ` | `no SKILL.md found under ` | 1 | +| `catalog scan --root --kind skills` | `no SKILL.md found under ` | 1 | | `tasks run --tasks-root ` | `FileNotFoundError: Task directory not found: demo-widget` | 1 | | `tasks list --tasks-root ` | prints nothing | 0 | | `explore list --tasks-root ` | `--tasks-root is not a directory: ` | 2 | From 33d769260b2a70ad93cce26e28a0b00fe982d721 Mon Sep 17 00:00:00 2001 From: Anton Volkov Date: Sun, 1 Nov 2026 22:16:59 +0000 Subject: [PATCH 10/15] test(catalog): pin that a spec-error copy is no name conflict Signed-off-by: Anton Volkov --- dokimi/tests/test_catalog_validate.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dokimi/tests/test_catalog_validate.py b/dokimi/tests/test_catalog_validate.py index 35c96f3e8..fcb609884 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -151,6 +151,14 @@ def test_a_disabled_copy_of_a_name_is_not_a_name_conflict(tmp_path): assert [r.state for r in reviews] == ["ok", "ok"] +def test_a_spec_error_copy_of_a_name_is_not_a_name_conflict(tmp_path): + _skill(tmp_path / "skills", "x", name="x", description="Use for x.") + _skill(tmp_path / "plugins", "x", name="x", description="y" * 1100) + reviews = review_skills(read_skill_manifest(tmp_path)) + assert all(f.code != "name-conflict" for r in reviews for f in r.findings) + assert sorted(r.state for r in reviews) == ["error", "ok"] + + def test_disable_model_invocation_is_reported_as_a_flag_not_a_finding(tmp_path): _skill(tmp_path / "skills", "hidden", name="hidden", extra="disable-model-invocation: true\n") From 75b6f0dc9eada232cb807ea1531b839c5a0169e5 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 1 Nov 2026 20:25:33 -0800 Subject: [PATCH 11/15] fix(routing): a SKILL.md with no skills/ parent is a root skill for an explicit competitor's name check Signed-off-by: Nikolay Petrov --- dokimi/dokimi/novelty/routing.py | 5 ++++- dokimi/tests/test_novelty_routing_field.py | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/dokimi/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 23fe82de7..aeaf0ef30 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -468,8 +468,11 @@ def competitor_left_out(ref: str, *, include_disabled: bool, include_invalid: bo if include_invalid: return None 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 entry = SkillCatalogEntry(name=skill.name, status=None, path=path.parent, - source_root=path.parent.parent, skill=skill) + source_root=root, skill=skill) review = review_skills([entry])[0] if review is None or review.state != "error": return None diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 140996272..77fc61c22 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -310,7 +310,8 @@ def test_an_explicit_spec_error_competitor_is_skipped_with_a_warning(tmp_path, c def test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict(tmp_path, monkeypatch): # ``skill:SKILL.md`` and ``skill:.`` (relative, from inside the skill) must get the verdict of - # the absolute reference: the directory name is checked for a skill that is not a catalog root + # the absolute reference. A SKILL.md with no ``skills/`` parent is a root skill, as the catalog + # reads it, so the directory name ("checkout") does not decide whether the field keeps ``db``. skill_dir = tmp_path / "checkout" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text( @@ -318,13 +319,26 @@ def test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict(tmp_ encoding="utf-8") absolute = competitor_left_out(f"skill:{skill_dir}/SKILL.md", include_disabled=False, include_invalid=False) - assert absolute is not None and "not the directory name 'checkout'" in absolute + assert absolute is None monkeypatch.chdir(skill_dir) for ref in ("skill:SKILL.md", "skill:.", f"skill:{skill_dir}"): assert competitor_left_out(ref, include_disabled=False, include_invalid=False) == absolute, ref assert competitor_left_out("skill:SKILL.md", include_disabled=False, include_invalid=True) is None +@pytest.mark.parametrize("skill_dir", ["skills/checkout", "plugins/x/skills/checkout"]) +def test_a_skill_under_a_skills_directory_keeps_the_directory_name_check(tmp_path, skill_dir): + # the catalog's directory-name check applies to a skill in a skills/ tree (only a root skill is exempt) + path = tmp_path / skill_dir + path.mkdir(parents=True) + (path / "SKILL.md").write_text( + "---\nname: db\ndescription: tune a database query\n---\n\n# B\n\n## A section\n", + encoding="utf-8") + for ref in (f"skill:{path}/SKILL.md", f"skill:{path}"): + verdict = competitor_left_out(ref, include_disabled=False, include_invalid=False) + assert verdict is not None and "not the directory name 'checkout'" in verdict, ref + + def _db_with_a_hidden_copy(tmp_path): catalog = tmp_path / "catalog" catalog.mkdir() From c0bae6d80669fac3e5831beaea0a0522d38947ec Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Sun, 1 Nov 2026 21:39:24 -0800 Subject: [PATCH 12/15] test(routing): shorten the comment block to two lines Signed-off-by: Nikolay Petrov --- dokimi/tests/test_novelty_routing_field.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 77fc61c22..15ddba7fe 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -309,9 +309,8 @@ def test_an_explicit_spec_error_competitor_is_skipped_with_a_warning(tmp_path, c def test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict(tmp_path, monkeypatch): - # ``skill:SKILL.md`` and ``skill:.`` (relative, from inside the skill) must get the verdict of - # the absolute reference. A SKILL.md with no ``skills/`` parent is a root skill, as the catalog - # reads it, so the directory name ("checkout") does not decide whether the field keeps ``db``. + # A SKILL.md with no ``skills/`` parent is a root skill, as the catalog reads it, so its directory + # name ("checkout") must not decide the name check; ``skill:SKILL.md`` and ``skill:.`` match the absolute reference. skill_dir = tmp_path / "checkout" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text( From 6387fe6d2cb5e76f482b1d4e184d7c32d67d96d4 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 2 Nov 2026 08:11:18 -0800 Subject: [PATCH 13/15] test(routing): pin the whole command for a conflict note, and the relative 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 --- dokimi/tests/test_novelty_routing_field.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 15ddba7fe..8920ffa0b 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -254,6 +254,12 @@ def test_two_catalog_skills_with_one_name_and_different_content_are_skipped_with assert [c.name for c in field] == ["cache"] assert "WARNING: " + notes[0] in capsys.readouterr().err assert left_out == notes + # the whole command saves the field with the same note, and does not refuse the run + args.func(args) + saved = json.loads((tmp_path / "routing.json").read_text(encoding="utf-8")) + assert saved["status"] == "ok" + assert saved["field"]["competitors"] == ["cache"] + assert saved["field"]["left_out"] == notes def test_a_spec_error_copy_is_no_conflict_with_a_valid_copy_of_the_name(tmp_path): @@ -338,6 +344,21 @@ def test_a_skill_under_a_skills_directory_keeps_the_directory_name_check(tmp_pat assert verdict is not None and "not the directory name 'checkout'" in verdict, ref +def test_a_relative_reference_under_a_skills_directory_keeps_the_name_check(tmp_path, monkeypatch): + # from the skills/ directory a relative reference must get the same verdict as the absolute one: + # without the abspath, "checkout/SKILL.md" would read as a root skill and skip the name check + path = tmp_path / "skills" / "checkout" + path.mkdir(parents=True) + (path / "SKILL.md").write_text( + "---\nname: db\ndescription: tune a database query\n---\n\n# B\n\n## A section\n", + encoding="utf-8") + absolute = competitor_left_out(f"skill:{path}/SKILL.md", include_disabled=False, include_invalid=False) + assert absolute is not None and "not the directory name 'checkout'" in absolute + monkeypatch.chdir(path.parent) + for ref in ("skill:checkout/SKILL.md", "skill:checkout"): + assert competitor_left_out(ref, include_disabled=False, include_invalid=False) == absolute, ref + + def _db_with_a_hidden_copy(tmp_path): catalog = tmp_path / "catalog" catalog.mkdir() From 51ce87b4dfa555e0002dabe50abb311986a89c89 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Mon, 2 Nov 2026 09:34:39 -0800 Subject: [PATCH 14/15] docs(novelty): say which explicit skill: competitors are name-checked 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 --- dokimi/docs/skill-novelty.md | 6 ++++++ dokimi/tests/test_novelty_routing_field.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index 339f359b8..7180a25c8 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -614,6 +614,12 @@ not mean to route against, to route the name. `dokimi catalog scan` lists the sa name that is not kebab-case or not its directory name) is also left out, with a warning naming the error; pass `--include-invalid` to keep it, as a harness that refuses to list such a skill would not. A `skill:` competitor with a spec error is skipped with the same warning. +The directory-name check applies to a `skill:` competitor under a `skills/` directory +(`skills//SKILL.md`, `plugins/x/skills//SKILL.md`), as it does in a catalog. A `skill:` +competitor anywhere else is read as a root skill, and its name is not checked against its +directory: `tools/database/SKILL.md` named `db` is routed when passed explicitly, while a +`--catalog` scan of its tree leaves it out with a name error. Keep the directory name and the +`name` field equal to get the same field from both. A name that conflicts with another catalog skill leaves the field whatever the flag says. A `skill:` competitor that is not a `SKILL.md`, or a directory without one, is skipped with a warning. A shell glob must be expanded first, with the prefix on each path diff --git a/dokimi/tests/test_novelty_routing_field.py b/dokimi/tests/test_novelty_routing_field.py index 8920ffa0b..dd8cf7e09 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -344,6 +344,16 @@ def test_a_skill_under_a_skills_directory_keeps_the_directory_name_check(tmp_pat assert verdict is not None and "not the directory name 'checkout'" in verdict, ref +def test_an_explicit_skill_outside_a_skills_directory_is_not_name_checked(tmp_path): + # documented in skill-novelty.md: a skill: competitor with no skills/ parent is read as a root skill + path = tmp_path / "cat" / "tools" / "database" + path.mkdir(parents=True) + (path / "SKILL.md").write_text( + "---\nname: db\ndescription: tune a database query\n---\n\n# B\n\n## A section\n", + encoding="utf-8") + assert competitor_left_out(f"skill:{path}", include_disabled=False, include_invalid=False) is None + + def test_a_relative_reference_under_a_skills_directory_keeps_the_name_check(tmp_path, monkeypatch): # from the skills/ directory a relative reference must get the same verdict as the absolute one: # without the abspath, "checkout/SKILL.md" would read as a root skill and skip the name check From b8ac2a605aa9927b132ba73b5624353729032381 Mon Sep 17 00:00:00 2001 From: Alexander Rybkin Date: Tue, 3 Nov 2026 00:15:56 -0800 Subject: [PATCH 15/15] fix(novelty): name every reason a queried skill left the all-vs-all field 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 --- ...20261031_120000_napetrov_rescan-catalog.md | 3 ++- dokimi/dokimi/novelty/all_vs_all.py | 6 +++-- .../tests/test_novelty_routing_all_vs_all.py | 22 +++++++++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md index 411f925e0..30e11a71e 100644 --- a/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -15,7 +15,8 @@ `--include-disabled`, a copy that is `disable-model-invocation` counts as one of the copies. - **`novelty routing --catalog` skips a name that two different skills share, with a warning, instead of refusing the run** (exit 2). Every copy of that name leaves the field, the rest of it stays, and stderr - names every path. + names every path. Under `--all-vs-all`, a query file that names such a skill, or one left out + for a spec error, is still refused (exit 2), and the message names both reasons. - **`catalog scan` reports a same-name skill with different content as a `name-conflict` warning** (among listed skills: no `disable-model-invocation`, no spec error), naming the other path and that `novelty routing --catalog` leaves the name out of the field. Before, each of the two skills was diff --git a/dokimi/dokimi/novelty/all_vs_all.py b/dokimi/dokimi/novelty/all_vs_all.py index f26267c92..f3d339204 100644 --- a/dokimi/dokimi/novelty/all_vs_all.py +++ b/dokimi/dokimi/novelty/all_vs_all.py @@ -131,8 +131,10 @@ def _check_all_vs_all_input(names: list[str], queries_by_skill: dict[str, list[s if unknown: raise RoutingInputError("the queries name skills that are not in the field: " + ", ".join(f"`{n}`" for n in unknown) - + "; a skill that sets disable-model-invocation is left out " - "unless you pass --include-disabled") + + "; the field leaves out a skill that sets " + "disable-model-invocation (--include-disabled keeps it), one " + "with a spec error (--include-invalid keeps it) and every copy " + "of a name that two different skills share") if not queries_by_skill: raise RoutingInputError("no skill in the field has queries to route") diff --git a/dokimi/tests/test_novelty_routing_all_vs_all.py b/dokimi/tests/test_novelty_routing_all_vs_all.py index e13f0f540..07302ab9b 100644 --- a/dokimi/tests/test_novelty_routing_all_vs_all.py +++ b/dokimi/tests/test_novelty_routing_all_vs_all.py @@ -470,6 +470,28 @@ def test_an_unreadable_queries_file_is_a_message_not_a_traceback(tmp_path, make) args.func(args) +@pytest.mark.parametrize("left_out, says", [ + ("spec_error", "--include-invalid keeps it"), + ("name_conflict", "every copy of a name that two different skills share"), +]) +def test_queries_for_a_skill_the_field_left_out_are_refused_with_the_real_reason( + tmp_path, left_out, says): + root = tmp_path / "catalog" + _cat(root, {"cache": "cache slow lookups", "queue": "queue background jobs"}) + if left_out == "spec_error": + _catalog_skill(root, "db", "x" * 1100) + else: + _catalog_skill(root, "db", "tune a slow database query") + _catalog_skill(root / "plugins" / "p", "db", "migrate a database schema") + queries = _write_queries(tmp_path, {"cache": ["q"], "queue": ["q"], "db": ["q"]}) + args = _parse("novelty", "routing", "--all-vs-all", "--catalog", str(root), + "--include-disabled", "--queries-by-skill", str(queries), + "--out", str(tmp_path / "o.json")) + with pytest.raises(SystemExit, match="not in the field: `db`") as exc: + args.func(args) + assert says in str(exc.value) + + def _cat(root, names_descriptions): for name, description in names_descriptions.items(): _catalog_skill(root, name, description)