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 new file mode 100644 index 000000000..30e11a71e --- /dev/null +++ b/dokimi/changelog.d/20261031_120000_napetrov_rescan-catalog.md @@ -0,0 +1,30 @@ +### 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`. A name that conflicts with another + catalog skill still leaves the 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. 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. 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 + reported `ok`. It is a warning, not an error, so `--strict` does not fail on it. + +### Fixed + +- **`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. 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/docs/external-content.md b/dokimi/docs/external-content.md index 4eb88f845..b3a68ae48 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -113,21 +113,25 @@ 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. -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`. 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 @@ -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 --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 | diff --git a/dokimi/docs/reference/cli-interface.json b/dokimi/docs/reference/cli-interface.json index 92f8caa80..0d1f194e2 100644 --- a/dokimi/docs/reference/cli-interface.json +++ b/dokimi/docs/reference/cli-interface.json @@ -8533,6 +8533,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..7180a25c8 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -605,8 +605,22 @@ 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. 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, 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. 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 (`skill:dir/a/SKILL.md skill:dir/b/SKILL.md`): an unexpanded `skill:dir/*` is refused as a missing @@ -673,7 +687,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`, `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 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..a6629967b 100644 --- a/dokimi/dokimi/commands/catalog.py +++ b/dokimi/dokimi/commands/catalog.py @@ -131,6 +131,15 @@ 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. + 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: print(json.dumps(report, indent=2)) if any_failure: @@ -335,7 +344,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..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,20 +317,19 @@ 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: 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: @@ -408,7 +406,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} @@ -855,16 +853,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 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 ff13c2c71..8344c7d8e 100644 --- a/dokimi/dokimi/external/validate.py +++ b/dokimi/dokimi/external/validate.py @@ -27,6 +27,10 @@ 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 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 check in :mod:`dokimi.harnesses.skill_treatment`). @@ -193,6 +197,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 +210,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 +228,31 @@ 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 listed skill that shares its name with a listed skill of different content. + + 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: + continue + review.findings.append( + Finding( + "warn", + "name-conflict", + f"name {name!r} is also used by {', '.join(others)}, with different content; " + "`novelty routing --catalog` leaves this name out of the field " + "until one is renamed or removed", + ) + ) 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/dokimi/novelty/routing.py b/dokimi/dokimi/novelty/routing.py index 021ba4475..aeaf0ef30 100644 --- a/dokimi/dokimi/novelty/routing.py +++ b/dokimi/dokimi/novelty/routing.py @@ -30,21 +30,25 @@ from __future__ import annotations -import hashlib import json import math +import os import random import re from collections import Counter 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". @@ -443,6 +447,39 @@ 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 + # 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: + 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=root, 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:"): @@ -484,27 +521,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. 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 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) keeper = _keepers_by_copy(eligible) candidates: list[Candidate] = [] for skill, digest in eligible: @@ -518,45 +557,74 @@ 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]]: - """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``.""" + include_disabled: bool, + include_invalid: bool = False) -> list[tuple[Skill, str]]: + """Each loadable, non-excluded catalog skill with the sha256 of its ``SKILL.md``. + + 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 - eligible: list[tuple[Skill, str]] = [] - for entry in read_skill_manifest(Path(root), notes): + entries = read_skill_manifest(Path(root), notes) + reviews = review_skills(entries) + 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 - 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 - eligible.append((skill, hashlib.sha256(skill.path.read_bytes()).hexdigest())) - return eligible + # 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)) + # 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 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, + 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 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.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 _refuse_conflicting_names(eligible: list[tuple[Skill, str]]) -> None: - """Two skills with one name and different bytes cannot both stand for the card. +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. - 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. 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. """ - copies_by_name: dict[str, list[tuple[str, 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" - ) + 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 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") + return conflicting def _keepers_by_copy(eligible: list[tuple[Skill, str]]) -> dict[tuple[str, str], Skill]: diff --git a/dokimi/scripts/growth_baseline.json b/dokimi/scripts/growth_baseline.json index b0b75b387..33bb9ba83 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/data/cli_flags_allowlist.txt b/dokimi/tests/data/cli_flags_allowlist.txt index 1350cae82..47f0d54c7 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..f3066343d 100644 --- a/dokimi/tests/test_catalog_command.py +++ b/dokimi/tests/test_catalog_command.py @@ -195,6 +195,48 @@ 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_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)]) + 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..fcb609884 100644 --- a/dokimi/tests/test_catalog_validate.py +++ b/dokimi/tests/test_catalog_validate.py @@ -114,7 +114,49 @@ 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_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_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): diff --git a/dokimi/tests/test_novelty_routing_all_vs_all.py b/dokimi/tests/test_novelty_routing_all_vs_all.py index f3076861b..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) @@ -596,7 +618,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 2de05770d..dd8cf7e09 100644 --- a/dokimi/tests/test_novelty_routing_field.py +++ b/dokimi/tests/test_novelty_routing_field.py @@ -10,9 +10,10 @@ import pytest from dokimi.cli import build_parser +from dokimi.commands.novelty import _routing_field from dokimi.novelty.routing import ( - RoutingInputError, catalog_competitors, + competitor_left_out, disables_model_invocation, ) @@ -213,22 +214,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 +247,165 @@ 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 + # 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): + # 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) + 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 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 + 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_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 test_the_spelling_of_an_explicit_competitor_does_not_change_its_verdict(tmp_path, monkeypatch): + # 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( + "---\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 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 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 + 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() + _catalog_skill(catalog, "db", "tune a slow database query") + other = catalog / "plugins" / "b" / "skills" / "db" + other.mkdir(parents=True) + (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): + 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] def test_two_differently_named_skills_with_identical_text_both_stay_in_the_field(tmp_path): @@ -276,13 +434,66 @@ 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] = [] + 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] + + +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_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" + 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"] 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