Repository navigation
Conversation
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| if not text or "://" in text or text.startswith(("mailto:", "/")): | ||
| return None |
There was a problem hiding this comment.
Must-fix: This URL test misclassifies valid URI destinations without :// as local files. For example, [call](tel:+15551234567) and [source](urn:isbn:0451450523) generate link-missing warnings although neither refers to a bundled file. This contradicts the documented URL exemption. Recognize URI schemes (including case-insensitive schemes such as MAILTO:) before filesystem resolution, and extend the URL-exemption test with these destinations.
There was a problem hiding this comment.
Fixed in 9e85776. A URI destination (tel:, urn:, mailto: in any letter case, and other schemes with no dot) is no longer read as a file, so [call](tel:+15551234567) and [source](urn:isbn:0451450523) no longer give link-missing. Regression test: test_uri_scheme_destinations_are_not_files.
| skill_dir = skill.path.parent | ||
| real_dir = skill_dir.resolve() | ||
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys(m for line in lines for m in LINK_RE.findall(line)) |
There was a problem hiding this comment.
Must-fix: Valid relative Markdown links can bypass the bundle checks. With a missing references/missing.md, either [guide][g]\n\n[g]: references/missing.md or [guide](<references/missing.md>) produces no link-missing finding: reference links are never collected, and angle-bracket destinations are discarded by _file_ref. The same forms also bypass escape detection. Resolve reference definitions and unwrap Markdown destination delimiters before applying the file checks, and add regression cases for both forms.
There was a problem hiding this comment.
Fixed in 9e85776 and 482056f. Reference definitions ([label]: target) are now collected and checked like inline links, and an angle-bracket destination (<...>) is unwrapped before the # split, so [guide](<references/missing.md>) and [guide][g] with [g]: references/missing.md both give link-missing, and an escape from the skill directory is reported too. Footnotes and fenced code are not checked. Regression tests cover these forms, and prose lines that only look like definitions ([Note]: read this first) are not read as one.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
==========================================
- Coverage 94.06% 94.05% -0.01%
==========================================
Files 297 297
Lines 38018 38143 +125
Branches 6424 6457 +33
==========================================
+ Hits 35760 35874 +114
- Misses 1451 1458 +7
- Partials 807 811 +4
🚀 New features to boost your workflow:
|
| should must may might if else only other such via per about over under more most very | ||
| just""".split() | ||
| ) | ||
| LINK_RE = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") |
There was a problem hiding this comment.
Must-fix: Inline angle-bracket destinations containing spaces still bypass both bundle checks. For example, [notes](<../../shared/my notes.md>) produces no match, even when that external file exists; [notes](<references/missing notes.md>) likewise produces no missing-link warning. Reference definitions already support this valid Markdown form, but [^)\s]+ excludes it here. Add a separate <...> destination alternative and regression tests for inline spaced destinations (both missing and escaping).
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [n](<references/missing notes.md>) and [n](<../../shared/my notes.md>) both give no finding, because the inline LINK_RE still excludes spaces. Not fixed in this round: this is a code change outside the approved scope, so it is held for the user's decision (see the summary comment on this PR).
| if line.lstrip().startswith(("```", "~~~")): | ||
| fenced = not fenced |
There was a problem hiding this comment.
Must-fix: Track the opening fence character and length instead of toggling on every marker. A valid four-backtick block containing a three-backtick example closes suppression prematurely, so [example](missing.md) inside the block incorrectly gets link-missing. Similarly, a ~~~ line inside a backtick block can cause real links after its actual closing fence to be skipped. Only a matching marker of at least the opening length should close the block; add regression cases for nested shorter fences and mismatched marker types.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: an ~~~ line inside a ```` fence closes it early, so [after](missing.md) after the real closing fence gives no finding; and a four-backtick fence holding a ``` example gives `link-missing` on the example. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys( | ||
| [m for line in lines for m in LINK_RE.findall(line)] | ||
| + [m for line in lines for m in REF_DEF_RE.findall(line)] |
There was a problem hiding this comment.
Must-fix: Valid multiline reference definitions still bypass the bundle checks. For example, See [guide][g].\n\n[g]:\n references/missing.md\n is valid Markdown, but neither line matches REF_DEF_RE, so no link-missing warning is emitted. Using ../../shared/notes.md as the destination likewise bypasses escape detection. Parse definitions across their permitted continuation line (without joining across fenced blocks), and add regression tests for missing and escaping destinations.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: a reference link whose destination is on the next line, [g]: then references/missing.md, gives no finding. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
`catalog scan` reviews each loaded skill beyond the spec frontmatter: - a relative Markdown link to a file the skill does not have is a warn (link-missing), with a note when the file exists from the catalog root instead. Example links such as [Title](URL) are common, so this does not fail --strict. A link or ../ code span that reaches outside the skill directory is a warn (link-escapes-skill, path-escapes-skill). Code fences, URLs, anchors and code spans naming no file are not checked. - a description with no "when to use" clause is a warn (description-no-when); one whose words overlap another skill's at a Jaccard similarity of at least 0.5 is a warn on both (description-near-duplicate). - two skills in different paths with the same directory name and different SKILL.md text are a warn on both (mirror-drift), naming the other copies. Skills with disable-model-invocation: true are exempt from the description checks. Warnings never change the exit code. The review reads the scan's own entries, which keep byte-identical copies (discover_skills counts a mirror once), so a mirror is still seen here. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…rash A relative link whose path has a component over 255 bytes made Path.exists() raise OSError on Python 3.11 to 3.13 (3.14 returns False), so one such skill aborted catalog scan with a traceback and listed no skill. Treat a target the OS refuses as missing: link-missing warn, as for any absent file. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…racket targets Thread: PRRT_kwDOVBg7g86yORfR Thread: PRRT_kwDOVBg7g86yORfU Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…onMark does A reference definition needs an end of line after its destination and an optional title, so prose such as '[Note]: read this first' is not one. The <...> unwrap runs before the #-anchor split, so an anchored angle-bracket target is checked. Thread: PRRT_kwDOVBg7g86yORfR Thread: PRRT_kwDOVBg7g86yORfU Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
482056f to
cae82c0
Compare
|
|
||
| # A "when to use" clause: the model chooses a skill from its description alone. | ||
| WHEN_RE = re.compile( | ||
| r"\b(?:when to use|when to delegate|use when|use this when|use for|use this skill|" |
There was a problem hiding this comment.
Must-fix: The trigger alternatives lack trailing word boundaries. For example, Use formatters to standardize Python source. matches use for inside Use formatters, so it receives no description-no-when warning despite having no usage-trigger clause. Similar prefix matches occur with use if in Use iframe .... Require word boundaries after the trigger words and add regression tests for these non-trigger prefixes.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: Use formatters to standardize Python source. gets no description-no-when, because the use for alternative matches the prefix of Use formatters. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| should must may might if else only other such via per about over under more most very | ||
| just""".split() | ||
| ) | ||
| LINK_RE = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") |
There was a problem hiding this comment.
Must-fix: This pattern stops at the first closing parenthesis, including parentheses belonging to a valid destination. [guide](references/guide(v2).md) is extracted as references/guide(v2, producing link-missing even when guide(v2).md exists. External destinations with balanced parentheses similarly lose their escape finding. Parse balanced and escaped destination parentheses rather than treating every ')' as the link terminator, and add existing bundled-file and external-file regression cases.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [guide](references/guide(v2).md) gives link-missing although the file exists, because LINK_RE stops at the first ). Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| should must may might if else only other such via per about over under more most very | ||
| just""".split() | ||
| ) | ||
| LINK_RE = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") |
There was a problem hiding this comment.
Must-fix: Valid inline links with single-quoted titles or whitespace before the closing delimiter are silently skipped. For example, [guide](references/missing.md 'Guide') and [guide](references/missing.md ) produce no match, so neither reports link-missing; the corresponding links to an existing ../../shared/notes.md also bypass escape detection. Support Markdown's title delimiters and permitted destination/title whitespace, and add missing-target and escaping-target regressions for these forms.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [guide](references/missing.md 'Guide') and [guide](references/missing.md ) both give no finding. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| real_dir = skill_dir.resolve() | ||
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys( | ||
| [m for line in lines for m in LINK_RE.findall(line)] |
There was a problem hiding this comment.
Must-fix: Link extraction also runs inside inline code and on escaped Markdown. For example, a body containing Format: \guide`or[guide](missing.md)` gets a link-missing warning even though neither contains a rendered Markdown link. Mask inline-code regions and honor Markdown escapes before extracting links, while retaining the separate ../ code-span check. Add regression tests showing these literals stay clean and an actual adjacent link is still checked.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: a link inside an inline code span (a line reading Format: then a code span holding guide) and an escaped link written with a backslash before the bracket both give link-missing. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| for raw in links: | ||
| text = _file_ref(raw) |
There was a problem hiding this comment.
Must-fix: Markdown destinations are URLs, but the filesystem lookup uses their percent-encoded spelling. With an existing references/guide notes.md, [guide](references/guide%20notes.md) incorrectly produces link-missing; an existing ../../shared/my notes.md linked with %20 likewise loses its link-escapes-skill classification. Decode local Markdown destinations before filesystem resolution and catalog-root lookup, for both inline links and reference definitions. Keep literal code-span paths separate so percent characters in those are not decoded. Add bundled-file and escaping-target regressions.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [guide](references/guide%20notes.md) gives link-missing although references/guide notes.md exists. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at cae82c0. The checks are useful and stay warnings, so --strict is not affected. But the scan now reports whether files outside the checkout exist, and mirror-drift fires on unrelated skills that share a directory name. Most of the parsing gaps can be closed by reusing repos/l0/markdown.py split().
Agreed with @exai-bot: inline code, fence markers, parentheses in targets, single-quoted titles, <...> targets with spaces, %20, two-line reference definitions, and the WHEN_RE word boundary. I reproduced each one. Its first two threads (URI schemes, reference and <...> targets) are fixed.
What I ran, at cae82c0:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (Docker, network and other-Python legs).- A repro on
review_skills/_bundle_findings: links to/etc/passwdand to a missing/etcfile, two unrelatedreviewskills in different plugins, two short descriptions, and the eight bot cases, each run against this PR's parser and againstl0/markdown.split().
Checked, no issues
- Exit code: the new findings are all
warn, andcatalog scan --strictstill fails only onerror(covered by a test). - Docs and changelog:
external-content.md, the module docstring and the fragment (Added) describe the same codes and exemptions.
| target = _resolved(skill_dir / text) | ||
| if target is not None and _exists(target): | ||
| if not target.is_relative_to(real_dir): | ||
| findings.append( | ||
| Finding( | ||
| "warn", | ||
| "link-escapes-skill", | ||
| f"link {raw!r} points outside the skill directory; it breaks when " | ||
| "the skill is installed alone", | ||
| ) | ||
| ) | ||
| continue | ||
| from_root = "" | ||
| if _exists(source_root / text): |
There was a problem hiding this comment.
The link check tells whether files outside the scanned checkout exist.
A catalog SKILL.md with [a](../../../../../../../../etc/passwd) [b](../../../../../../../../etc/nonexist) gives link-escapes-skill for the first and link-missing for the second, so the scan report shows which host paths exist. catalog._within says a scan must not "read or report on files outside the checkout the caller pointed us at". The from-root probe at line 316 (source_root / text with ../) and the ../ code-span check at line 329 do the same.
Suggestion: resolve the target and check _within(target, source_root) before any exists(). A target outside the skill directory is link-escapes-skill whether or not it exists (it breaks when the skill is installed alone either way), and outside the root it should not be probed at all.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [a](../../../../../../../../etc/passwd) gives link-escapes-skill and [b](../../../../../../../../etc/nonexist) gives link-missing, so the message reveals which host path exists. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| def _mirror_drift(loaded: list[tuple[SkillCatalogEntry, Skill, SkillReview]]) -> None: | ||
| """``mirror-drift`` on each skill that shares its directory name with a differing copy.""" | ||
| by_dir_name: dict[str, list[tuple[SkillCatalogEntry, Skill, SkillReview]]] = {} | ||
| for item in loaded: | ||
| by_dir_name.setdefault(item[1].path.parent.name, []).append(item) |
There was a problem hiding this comment.
mirror-drift groups skills only by directory name, so unrelated skills that share a common name are reported as drifted copies.
plugins/git/skills/review ("Use when reviewing a git diff before commit.") and plugins/sql/skills/review ("Use when checking SQL query plans for slow joins.") each get mirror-drift ... the copies may have drifted apart. Plugins namespace their skills, so names like review, setup or test repeat across plugins. _near_duplicates then skips such pairs ("already a mirror-drift"), so a real description overlap between them is hidden too. Also, #328 adds name-conflict for the same pairs (same name, different bytes), so once both are in, one pair gets two warnings that say different things.
Suggestion: call it drift only when the copies are still close (for example the same description, or a high text similarity), and keep the near-duplicate check for the rest; and agree with #328 which of the two warnings a same-name pair gets.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: two review skills under plugins/git and plugins/sql each get mirror-drift, and _near_duplicates skips the pair. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| def _outside_fences(body: str) -> list[str]: | ||
| """The lines of ``body`` that are not inside a fenced code block.""" | ||
| lines: list[str] = [] | ||
| fenced = False | ||
| for line in body.splitlines(): | ||
| if line.lstrip().startswith(("```", "~~~")): | ||
| fenced = not fenced | ||
| elif not fenced: | ||
| lines.append(line) | ||
| return lines | ||
|
|
There was a problem hiding this comment.
The new link parsing repeats dokimi/repos/l0/markdown.py split(), less accurately, and most of the open bot threads come from that.
split() already matches the fence marker and its length, drops inline code spans before it looks for links, and reads balanced parentheses, all three title forms and <...> targets with spaces. I ran both on the cases in the open threads: inline code `[g](missing.md)`, a fence holding a ```` example, `~~~` inside a ```` ```` fence, guide(v2).md, a `'T'` title and ``: this PR gets all six wrong and `split()` gets all six right. `%20` (needs `urllib.parse.unquote`) and a definition split over two lines are wrong in both.
Suggestion: build the link and span lists from split(skill.body).links / .spans and drop _outside_fences, LINK_RE, REF_DEF_RE and CODE_SPAN_RE; then unquote the target in _file_ref. That closes the threads on lines 91, 241 and 289 in one place.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: the six cases in this thread each fail at 8c07f95. I ran the existing dokimi/repos/l0/markdown.py split() on the same six, and it gets all six right, so reusing it is a real option (%20 still needs urllib.parse). Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| similarity = _jaccard(tokens[i], tokens[j]) | ||
| if similarity >= NEAR_DUPLICATE_JACCARD: |
There was a problem hiding this comment.
On short descriptions one shared word is enough for description-near-duplicate.
Use for deploys. and Use when deploys fail. leave the tokens {deploys} and {deploys, fail}, Jaccard 0.5, so both skills warn that "the model may pick either".
Suggestion: also require a minimum number of shared tokens (3, say) before the Jaccard test, and add a test with two short descriptions.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: Use for deploys. and Use when deploys fail. both get description-near-duplicate. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
…ll time The shared-limit test compared elapsed time against 1.5x REGEX_TIMEOUT and flaked on the loaded runner. A count of searches alone did not pin the shared budget: a per-pattern budget left the same count. A fake clock now charges each search 0.6x the limit, so the second pattern can only finish with a fresh budget, which the shared deadline does not give it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Inline destinations containing spaces bypass missing-file and escape checks, while skill-directory-prefixed links with unresolved variables incorrectly produce missing-file warnings.
2 open issues
What changed in this PR
3 new findings.
This PR extends catalog scan with warning-only checks for skill bundles, descriptions, and drifted mirrors, building on the existing spec validation without changing exit-code behavior.
Changes:
- Detects missing links and paths that reach outside a skill’s directory.
- Warns about descriptions without usage triggers, overlapping descriptions, and drifted mirrors.
- Adds validation tests and documentation, and makes the regex-budget test deterministic.
| File | Description |
|---|---|
dokimi/tests/test_reference_checks.py |
Replaces timing-sensitive regex-budget testing with a fake clock. |
dokimi/tests/test_catalog_validate.py |
Covers bundle, description, mirror, and warning-only behavior. |
dokimi/dokimi/external/validate.py |
Implements the new catalog checks. |
dokimi/docs/external-content.md |
Documents warnings and description-check exemptions. |
dokimi/changelog.d/20261030_170000_napetrov_catalog-bundle-drift-checks.md |
Adds release notes for the expanded validation. |
Review effort: Balanced
| should must may might if else only other such via per about over under more most very | ||
| just""".split() | ||
| ) | ||
| LINK_RE = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") |
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: an inline destination with a space, [n](<my notes.md>), gives no finding. This is the same finding as the open thread on the inline angle-bracket destination. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| if text.startswith(SKILL_DIR_VAR): | ||
| return text[len(SKILL_DIR_VAR) :] or None | ||
| if any(c in text for c in "${}<>*"): | ||
| return None | ||
| return text |
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: [guide](${CLAUDE_SKILL_DIR}/references/${VERSION}.md) gives link-missing for a template with an unresolved variable, because _file_ref returns the path before the variable check. This is a false positive. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
|
Re-checked the open threads at 8c07f95. Every one of the 12 unanswered findings reproduces by probe (output in each thread reply). None is declined. None is fixed in this round, because only the test fix is approved for this round, and each fix below is a code change to the link checker or the description checks. Decision needed from the author:
Reply A1, A2, B or another option and the round goes ahead. Nothing else is pushed until then. |
| should must may might if else only other such via per about over under more most very | ||
| just""".split() | ||
| ) | ||
| LINK_RE = re.compile(r"\]\(([^)\s]+)(?:\s+\"[^\"]*\")?\)") |
There was a problem hiding this comment.
Must-fix: The optional inline-link title accepts only double quotes, although Markdown also permits single-quoted titles. For example, [notes](../../shared/notes.md 'Notes') does not match this regex, so an existing external file receives no link-escapes-skill warning; [guide](references/missing.md 'Guide') likewise receives no link-missing. Accept the valid title delimiters and add regression tests for both missing and escaping destinations.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: the same finding as the open thread on single-quoted titles, [guide](references/missing.md 'Guide'), which gives no finding. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| return text[len(SKILL_DIR_VAR) :] or None | ||
| if any(c in text for c in "${}<>*"): | ||
| return None | ||
| return text |
There was a problem hiding this comment.
Must-fix: Relative link destinations are URI paths, but this returns their percent escapes unchanged. With an existing references/my notes.md, [notes](references/my%20notes.md) incorrectly produces link-missing; an existing ../../shared/my notes.md linked as ../../shared/my%20notes.md likewise produces link-missing instead of link-escapes-skill. Decode the URL path after removing its fragment and filtering non-file destinations, before filesystem lookup. Apply this to inline and reference-style links and add encoded-space/encoded-# regression tests; literal code-span paths should retain filesystem semantics.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: the same finding as the open thread on percent-decoding, references/guide%20notes.md, which gives link-missing although the file exists. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
| real_dir = skill_dir.resolve() | ||
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys( | ||
| [m for line in lines for m in LINK_RE.findall(line)] |
There was a problem hiding this comment.
Must-fix: Inline code is still included in link extraction. For example, a body containing Example: guide`` (with the link text enclosed in backticks) produces link-missing, although it is literal code rather than a Markdown link. Multiple-backtick spans have the same problem. Exclude inline-code spans from the text used for link matching, while retaining the separate check for actual `../` path spans, and add regression tests for both delimiter lengths.
There was a problem hiding this comment.
Re-checked at 8c07f95. Confirmed by probe: the same finding as the open thread on code spans, a link inside an inline code span, which gives link-missing. Not fixed in this round: it is a code change outside the approved scope, so it is held for the author's decision (see the summary comment on this PR).
The comment-block check allows two lines of reason per comment. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at ae8cad9. The merge waits on the parser and check gaps listed below: every one is confirmed, and the author's own re-check reproduces them. The new checks are all warn, so exit codes and --strict are unchanged. One new risk inline, two nits.
I agree with @vlad-perevezentsev on the probe of paths outside the checkout (link-escapes-skill vs link-missing reveals which files outside the checkout exist), mirror-drift firing on unrelated same-named skills, reusing l0/markdown.py split() for link parsing, and description-near-duplicate on short descriptions. The open decision on the author's summary comment (options A1, A2, B) also stays open.
What I ran, on a Linux review host at ae8cad9:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, network and other-Python legs). Changed-line coverage gate passes.- Reproductions through
catalog scanfor the code findings, with no model or network involved. - A mutation run of 54 mutants of
validate.pyagainst both test files: 27 pass the suite. The ones that decide a warning are the near-duplicate threshold and the two pair skips (inline), the/and template exemptions in_file_ref, the footnote and indent limits inREF_DEF_RE, andValueErrorhandling for a NUL byte in a link.
Open bot points I confirm:
- I agree with exai-bot and Copilot that the link parser misses or misreports these forms, all reproduced at this head: an inline
<...>destination with spaces, fence length and character, a reference definition with its destination on the next line, balanced parentheses such asguide(v2).md, single-quoted titles and space before the closing), links in code spans and escaped links, percent-encoded destinations, and${CLAUDE_SKILL_DIR}/references/${VERSION}.md, which giveslink-missinginstead of the unresolved-variable check. -
- I agree with exai-bot that
WHEN_REhas no word boundary afteruse for, soUse formatters to ...passes as a trigger.
- I agree with exai-bot that
Bot points I checked that no longer apply at this head (2)
- exai-bot's point on
tel:andurn:destinations does not apply:URI_SCHEME_REnow skips them before any file lookup. - exai-bot's point on reference definitions and
<...>destinations without spaces does not apply:REF_DEF_REcollects definitions and_file_refunwraps the brackets.
| """``mirror-drift`` on each skill that shares its directory name with a differing copy.""" | ||
| by_dir_name: dict[str, list[tuple[SkillCatalogEntry, Skill, SkillReview]]] = {} | ||
| for item in loaded: | ||
| by_dir_name.setdefault(item[1].path.parent.name, []).append(item) |
There was a problem hiding this comment.
A SKILL.md at the scanned root is grouped by the checkout's directory name, so it triggers mirror-drift against an unrelated skill, and the message names the other copy ..
Repro with catalog scan --root tools --kind skills: tools/SKILL.md (name root-skill) and tools/skills/tools/SKILL.md (name tools). Output:
root-skill ... warn mirror-drift: SKILL.md differs from skills/tools (the same skill name in another path); ...
tools ... warn mirror-drift: SKILL.md differs from . (the same skill name in another path); ...
_frontmatter_findings already skips the name/directory comparison for this case (at_root), because the directory name of a root skill is the checkout's name. _mirror_drift groups on the same name without that exemption. This is separate from the same-name-in-different-plugins point already raised.
Suggestion: leave a skill whose directory is the entry's source_root out of by_dir_name, as at_root does at line 435.
There was a problem hiding this comment.
Fixed in 3b270ea. A root SKILL.md is left out of the same-directory-name grouping for mirror-drift and the near-duplicate skip, as at_root already does for the name check. The red test test_root_skill_and_a_nested_skill_named_like_the_checkout_are_not_mirror_drift failed on e1afc3c with mirror-drift on both.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| a `warn` on both (`mirror-drift`), naming the other copies. Links inside code fences, URLs, | ||
| anchors, and code spans that name no file are not checked. Skills with | ||
| `disable-model-invocation: true` are exempt from the description checks. Warnings never | ||
| change the exit code; only `--strict` makes an `error` fail. |
There was a problem hiding this comment.
Must-fix: This exit-code statement also includes load errors, which fail without --strict. For example, a declared skill whose directory is missing is reported as error and exits 1 even in a non-strict scan (_scan_skills fails whenever skill.loaded is false). Qualify this as a spec error and preserve the distinction already documented in docs/external-content.md.
Suggestion:
| change the exit code; only `--strict` makes an `error` fail. | |
| change the exit code; spec errors fail only with `--strict`, while load errors always fail. |
There was a problem hiding this comment.
Fixed in e4dc404. The note now says a spec error fails only with --strict, and a load error always fails. That matches docs/external-content.md.
| r"""(?:[ \t]+(?:"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*$""" | ||
| ) | ||
| # A scheme has no dot, so ``README.md:42`` and ``run.py:main`` stay paths to check. | ||
| URI_SCHEME_RE = re.compile(r"[A-Za-z][A-Za-z0-9+\-]*:") |
There was a problem hiding this comment.
Must-fix: URI schemes can contain dots (RFC 3986), so this classifies valid URI links as missing bundle files. For example, [record](z39.50r:example.org/database) produces link-missing even though z39.50r is a registered URI scheme and URI destinations are explicitly exempt. Recognize the full URI-scheme grammar, handling the intended run.py:42 file-reference exception separately, and extend the URI destination test with a dotted scheme.
There was a problem hiding this comment.
Fixed in 3b270ea. A dotted scheme is a URI unless what follows the colon is a line number or a bare symbol, so z39.50r:example.org/database is no longer a file. run.py:42 and run.py:main are still checked. A dotted scheme followed by a bare word, such as z39.50r:foo, is still read as a file. Red test: test_dotted_uri_scheme_destinations_are_not_files.
| if text.startswith(SKILL_DIR_VAR): | ||
| return text[len(SKILL_DIR_VAR) :] or None |
There was a problem hiding this comment.
Must-fix: Returning immediately after stripping ${CLAUDE_SKILL_DIR}/ bypasses the template-variable exclusion below. For example, [run](${CLAUDE_SKILL_DIR}/scripts/${SCRIPT_NAME}.py) produces link-missing for a literal ${SCRIPT_NAME}.py, although _file_ref explicitly exempts templates containing other variables. Strip the supported prefix, then apply the remaining-variable checks to the resulting path before returning it. Add a regression test for this mixed-variable destination.
There was a problem hiding this comment.
|
|
||
| def _tokens(description: str) -> frozenset[str]: | ||
| """The lowercase words of three or more letters in ``description``, less the stop words.""" | ||
| words = re.findall(r"[a-z]{3,}", description.lower()) |
There was a problem hiding this comment.
Must-fix: ASCII-only tokenization silently disables similarity detection for supported non-ASCII descriptions. For two differently named skills with description Use when проверка исходного кода необходима., all Cyrillic words are discarded and use/when are stop words, so both token sets are empty and neither skill gets description-near-duplicate despite identical descriptions. The loader accepts UTF-8 descriptions without a language restriction. Tokenize Unicode letters and add a regression using distinct skill names with this shared description.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to e1afc3c, which fix the two nits from my review:
3f40580:dokimi/dokimi/external/validate.py:393:_near_duplicatesbuilds a word-to-skill index once and compares only pairs that share a word.e1afc3c:dokimi/tests/test_catalog_validate.py:642: new tests pin the identical-pair and same-name skips and the threshold with pairs at 0.5 and 24/49.
make ci-local at e1afc3c: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the risk from my review; @vlad-perevezentsev on link check probes and reports which paths outside the scanned checkout exist (link-escapes-skill vs link-missing); @vlad-perevezentsev on mirror-drift fires on unrelated skills that share a directory name and hides their near-duplicate check; @vlad-perevezentsev on link parsing repeats l0/markdown.py split() less accurately; reuse it; @vlad-perevezentsev on description-near-duplicate fires on short descriptions that share one word; exai-bot on link parser misses or misreports inline <...> with spaces, fence length/char, two-line reference definitions, balanced parentheses, single-quoted titles, code spans and escapes, percent-encoding; exai-bot on WHEN_RE lacks a trailing word boundary, so 'Use formatters ...' counts as a trigger; copilot-pull-request-reviewer on inline <...> destination with spaces is skipped; ${CLAUDE_SKILL_DIR}/x/${VAR}.md gives a false link-missing.
Load errors fail without --strict, so the note now says which errors fail only under --strict. Thread: PRRT_kwDOVBg7g86ybI3b Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…s and non-ASCII words
- A dotted RFC 3986 scheme (z39.50r:example.org/database) is a URI, not a
file. A dotted scheme followed by a line number or symbol (run.py:42,
run.py:main) is still a file reference.
- A ${CLAUDE_SKILL_DIR}/ path that also holds another variable is not
checked as a file; the prefix no longer skips the variable check.
- The root SKILL.md is named by the checkout's directory, so it is not
grouped with a same-named skill for mirror-drift or the near-duplicate
skip.
- Description words are any letters, not only ASCII, so a non-English
description overlaps other skills.
Thread: PRRT_kwDOVBg7g86ybCa1
Thread: PRRT_kwDOVBg7g86ybI3p
Thread: PRRT_kwDOVBg7g86ybI3s
Thread: PRRT_kwDOVBg7g86ybI3y
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if scheme and not ("." in scheme.group() and FILE_SUFFIX_RE.fullmatch(text[scheme.end() :])): | ||
| return None |
There was a problem hiding this comment.
Must-fix: Recognized file-location suffixes are retained in the filesystem path. With an existing bundled run.py, [entry](run.py:42) and [entry](run.py:main) both produce link-missing, because the lookup checks files literally named run.py:42 and run.py:main. The same issue affects ../../shared/run.py:42 links and ../ code spans, hiding the escape warning for existing external files. Normalize recognized line/symbol suffixes before resolving the target (including paths with directory components), and add existing-target regression cases; the new tests cover only absent targets.
There was a problem hiding this comment.
Fixed in 99bbe67. A line or symbol suffix (:42, :10-20, :main) is dropped before the target is resolved, for links and ../ code spans, including paths with directories. An existing run.py is no longer link-missing, and ../../shared/run.py:42 is now reported as link-escapes-skill (or path-escapes-skill for the span).
Tests: existing-target cases (4 bodies) and escape cases with a suffix, red before the fix.
The Comment blocks check fails a three-line block of comments. Fixes the Static checks failure on 3b270ea. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| r"whenever|when the user|activate\b|should be used when|when building|" | ||
| r"when (?:you|the user) )", |
There was a problem hiding this comment.
Must-fix: Ordinary usage conditions are missed by this phrase whitelist. For example, Run database migrations when deploying a new release. contains a clear condition for using the skill, but none of these alternatives matches it, so the scan incorrectly emits description-no-when. The same happens with Diagnose failures when debugging production incidents. Recognize general “when …” clauses rather than only selected subjects/verbs, and add regression tests for these descriptions.
There was a problem hiding this comment.
Fixed in 99bbe67. The description check now accepts any "when" clause (when followed by a word), not a fixed list of subjects and verbs, so "Run database migrations when deploying a new release." and "Diagnose failures when debugging production incidents." are no longer description-no-when.
Tests: three parametrized descriptions, red before the fix. The "when to use" / "use when" messages and the docs now say "when" clause.
A link or code span like run.py:42 or run.py:main named a file that does not exist, so an existing bundled file was reported as link-missing and an escape through ../ was hidden. The suffix is now dropped before the target is resolved. A description is checked for any "when" clause, not only a fixed list of phrases, so "Run migrations when deploying" is not a warning. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Summary
catalog scannow reviews each loaded skill's bundle and description, and finds drifted mirrors. Every new check is awarn, so the exit code does not change, and--strictstill fails only on spec errors.Builds on #163 (spec review,
--strict,--format json) and #167 (discovery dedupe), both merged. The validator reads the scan's own entries, which keep byte-identical copies, so a mirror is still seen even thoughdiscover_skillscounts it once.Evidence
Demo catalog with a broken link, a description without a trigger, and a byte-identical mirror (
catalog scan --root <root> --kind skills, this branch): trimmed where marked...:The new tests are in
tests/test_catalog_validate.py. Against the intel/mainvalidate.py, 11 of them fail (the W9 checks are absent there); on this branch:Local checks on this branch:
Merge Danger
Door: two-way. No stored format, no new flag;
--format jsongains new finding codes. Reverting restores the earlier findings.Blast Radius: low. Every new finding is a
warn, so exit codes and--strictare unchanged for these checks.Ramifications:
[Title](URL)(not failing, but printed), descriptions without a trigger, or mirrors that have drifted now shows them.[Title](URL)is deliberately not an error.mirror-driftfires on both when two unrelated skills share a directory name with different text (for example tworeview/skills). Whether this is noisy on real catalogs is a product question; the changelog says it is intended.warnonly.Checklist
pytestpassesruff,mypy, and schema checks passdocs/external-content.md(catalog scan section); changelog fragment added🤖 Generated with Claude Code