Repository navigation
feat(catalog): check bundle links, descriptions and mirror drift - #321
Conversation
| 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.65% 94.66%
========================================
Files 331 331
Lines 42973 43286 +313
Branches 7287 7360 +73
========================================
+ Hits 40675 40975 +300
- Misses 1463 1471 +8
- Partials 835 840 +5
🚀 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.
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.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The commits since my last review are right: the use phrases in WHEN_RE end on a word boundary, a link's ?query is dropped before the file check, and the mirror-drift and near-duplicate messages name the other checkout. They add no bug or risk, and two nits are inline. The merge still waits on gaps from earlier rounds that are open at this head, listed below.
Still open from earlier rounds (reproduced at this head):
- @vlad-perevezentsev's three points still hold: paths outside the checkout are probed,
mirror-driftfires on unrelated same-named skills, and short descriptions sharing one word are near-duplicates. - @vlad-perevezentsev's suggestion to reuse
l0/markdown.pysplit()for link parsing waits for the author. @napetrov's choice between options A1, A2 and B in the summary comment is still open.
What I ran, on a Linux review host at 219e4cf:
make ci-local: 30 passed, 0 failed, 7 skipped (Docker, network and other-Python legs). The changed-line coverage gate passes.- Reproductions through
review_skills, with no model or network involved, for every point above and for the?querychange (a.md?x#ywith the file present gives no finding;a.md?xwith no file giveslink-missing). - A mutation run of 20 mutants of the new code against both link test files and
test_catalog_validate.py: 13 killed, 7 survived (the nits below cover the ones that matter).
Open bot points I confirm:
- I agree with exai-bot that a link inside an inline code span is still checked: a line with a code span holding guide gives
link-missing, and the double-backtick form gives the same. - I agree with exai-bot's repeat that inline code is still searched for links, and that an escaped link written with a backslash before the bracket also gives
link-missing. - I agree with exai-bot that percent-encoded destinations are looked up literally:
[g](references/guide%20notes.md)giveslink-missingwithreferences/guide notes.mdpresent. - I agree with exai-bot's repeat on percent-decoding:
[g](a%3Fb.md)witha?b.mdpresent giveslink-missing.
Checked, no issues
- Exit code: every new finding is
warn;--format jsonstill parses and carries the new codes. - Docs and rules: the changelog fragment and
external-content.mdmatch the codes and exemptions; every commit carries aSigned-off-by;validate.pyand the test files stay under the growth caps. ?query: the strip applies to link destinations and reference definitions only; code-span paths keep literal semantics.
Bot points I checked that no longer apply at this head (23)
- exai-bot's point on
tel:andurn:destinations does not apply:URI_SCHEME_REskips 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. - exai-bot's point on inline
<...>destinations with spaces does not apply:LINK_REhas an angle-bracket alternative that keeps spaces. - exai-bot's point on fence length and character does not apply:
_outside_fencescloses a block only at the same marker with at least the opening length. - exai-bot's point on a reference definition with its destination on the next line does not apply:
REF_DEF_REmatches over the whole body and allows one line break. - exai-bot's point on the
use forword boundary does not apply: every use phrase inWHEN_REnow ends on\b, soUse formatters to ...getsdescription-no-when. - exai-bot's point on parentheses in link destinations does not apply:
LINK_REkeeps one level of balanced parentheses, soguide(v2).mdis read whole. - exai-bot's point on single-quoted titles and a space before the closing parenthesis does not apply:
LINK_REaccepts all three title forms and surrounding whitespace. - Copilot's point on inline
<...>destinations with spaces does not apply: the angle-bracket alternative inLINK_REcovers it. - Copilot's point on
${CLAUDE_SKILL_DIR}/references/${VERSION}.mddoes not apply: the prefix is stripped first and the rest goes through the variable check. - exai-bot's repeat on single-quoted titles does not apply:
_TARGET_TITLEaccepts'...'. - exai-bot's point on dotted URI schemes does not apply:
_file_reftreatsz39.50r:example.org/databaseas a URI and reads a dotted scheme as a file only before a line or symbol. - exai-bot's point on
${CLAUDE_SKILL_DIR}/skipping the variable check does not apply: the prefix is stripped first. - exai-bot's point on ASCII-only description words does not apply:
_tokensreads letters in any script. - exai-bot's point on
run.py:42andrun.py:maindoes not apply:_file_refdrops the line or symbol suffix before resolving. - exai-bot's point on the
whenphrase whitelist does not apply:WHEN_REaccepts anywhenfollowed by a word. - exai-bot's point on inline
<...>destinations and parentheses does not apply: both are handled byLINK_RE, with tests intest_catalog_links.py. - exai-bot's repeat on a definition destination on the following line does not apply:
REF_DEF_REis matched over the whole body. - exai-bot's repeat on fence tracking by marker and length does not apply: a tilde line inside a backtick block no longer closes it.
- exai-bot's point on the
sub/..directory name does not apply:_dir_name_oftakes the lexical name, soskills/alpha/sub/..isalpha. - exai-bot's repeat on the trailing word boundary of
use fordoes not apply:Use formatting tools to normalize logs.andUse iframe helpers ...getdescription-no-whenagain, with tests. - exai-bot's point on
?queryin link destinations does not apply:_file_ref(link=True)drops the query, soreferences/guide.md?raw=1resolves and../../shared/guide.md?raw=1#topstill reportslink-escapes-skill. - exai-bot's point on counterpart labels across checkouts does not apply:
_counterpartappends the other source root across roots, and the tests assert it.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| # An inline link target: balanced parentheses (``guide(v2).md``) or ``<in angle brackets>`` with spaces; | ||
| # an optional title follows the target. | ||
| _TARGET_TITLE = r"""(?:\s+(?:"[^"]*"|'[^']*'|\([^()]*\)))?""" | ||
| LINK_RE = re.compile(rf"\]\(\s*(<[^<>\n]*>|(?:[^()\s]|\([^()\s]*\))+){_TARGET_TITLE}\s*\)") |
There was a problem hiding this comment.
Must-fix: This pattern handles only one level of parentheses inside a destination. The valid Markdown link [guide](references/a(b(c)).md) does not match at all, so a missing target produces no link-missing; likewise, an existing external target in [guide](../../shared/a(b(c)).md) produces no link-escapes-skill. Parse balanced destinations with support for nested parentheses and add regression cases for both missing and escaping targets.
There was a problem hiding this comment.
Fixed in 5e256dc. Link destinations are now balanced to any depth, so a(b(c)).md is checked as a missing or escaping target. Regression tests added for both.
There was a problem hiding this comment.
Decision A1 names split(), which reads one paren level, so a(b(c)).md needs a local any-depth scanner for inline links (01eb083); split() still reads reference definitions. Keep that scanner, or extend L0 split() to any depth for all callers?
| fence = "" | ||
| lines.append("") # a blank line keeps a definition above the block from reaching below it | ||
| continue | ||
| opened = FENCE_OPEN_RE.match(line) |
There was a problem hiding this comment.
Must-fix: Fence detection does not account for Markdown container prefixes. For example, the valid blockquoted fence > ```markdown\n> [example](missing.md)\n> ```\n is passed through unchanged, and LINK_RE then produces link-missing for the literal example. Fences nested in list items can have the same problem when their container indentation exceeds three spaces. This violates the promised exclusion of links inside code fences and creates false bundle warnings. Recognize fences in their blockquote/list context (preferably via Markdown block parsing), and add regression cases for both containers.
There was a problem hiding this comment.
Fixed in 5e256dc. Fence lines are now recognised after blockquote markers and under a list item at any indentation, so their example links are skipped. Regression tests for both containers.
| text = raw.strip() | ||
| if text.startswith("<") and text.endswith(">"): | ||
| text = text[1:-1].strip() |
There was a problem hiding this comment.
Must-fix: Markdown backslash escapes are passed unchanged to the filesystem. With an existing references/guide_notes.md, [notes](references/guide\_notes.md) is a valid Markdown link but produces link-missing; an existing ../../shared/guide_notes.md linked with the same escaped underscore is also reported as missing instead of link-escapes-skill. This affects both inline destinations and reference definitions. Decode Markdown punctuation escapes for link destinations (while retaining literal code-span semantics) and add tests for bundled and escaping targets.
There was a problem hiding this comment.
Fixed in 5e256dc. Link destinations and reference definitions decode backslash escapes of ASCII punctuation; code spans stay literal. Tests cover a bundled and an escaping target.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to f1968ff, which fix the two nits from my review:
c46fd31:dokimi/tests/test_catalog_validate.py:981: added the three suggested descriptions to the parametrize list.f1968ff:dokimi/tests/test_catalog_validate.py:682: the two existing mirror-drift assertions now requirediffers from <dir> (, so a root suffix on a same-checkout counterpart fails them.
make ci-local at f1968ff: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: @vlad-perevezentsev on link check reveals whether files outside the scanned checkout exist (link-escapes-skill vs link-missing); @vlad-perevezentsev on mirror-drift fires on unrelated skills sharing a directory name and hides their near-duplicate check; @vlad-perevezentsev on description-near-duplicate fires on short descriptions that share one word (50% overlap); exai-bot on links inside inline code spans and escaped links are still checked (link-missing); exai-bot on percent-encoded link destinations are looked up literally (link-missing); @vlad-perevezentsev on reuse l0/markdown.py split() for link parsing instead of the local regexes.
…nd fences in containers A destination with parentheses nested two levels deep was not seen as a link, so a missing or escaping target went unreported. A backslash escape of punctuation was checked literally (guide\_notes.md). A fence inside a blockquote or under a list item indented past three spaces was not seen as a code block, so the links in its example text were reported. Destinations are now scanned for balance; escapes are decoded for link destinations only, code spans stay literal. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| close = (DEEP_FENCE_CLOSE_RE if deep else FENCE_CLOSE_RE).match(rest) | ||
| if (close and in_quote == quoted and close.group(1)[0] == fence[0] | ||
| and len(close.group(1)) >= len(fence)): |
There was a problem hiding this comment.
Must-fix: A container fence must also end when its blockquote or list item ends, even without a closing marker. For example, > ```\n> example\n\n[real](missing.md)\n currently discards the real link because fence remains active through EOF, so no link-missing warning is emitted. Likewise, - item\n ```\n example\n\n[real](missing.md)\n suppresses the link after the list. Track quote depth and list-item indentation, end the fence when its container ends, and process the terminating line as outside the fence. Add regression tests for both cases; the new tests only cover explicit closing markers.
There was a problem hiding this comment.
Fixed in 46148b1. A quoted or list-item fence now ends at the first line that leaves its quote or item, so the links after it are checked.
Tests: test_a_blockquote_fence_ends_with_its_quote_when_it_has_no_closing_marker, test_a_list_item_fence_ends_with_its_item_when_it_has_no_closing_marker.
| deep = in_list | ||
| opened = (DEEP_FENCE_OPEN_RE if deep else FENCE_OPEN_RE).match(rest) |
There was a problem hiding this comment.
Must-fix: Determine whether the current fence belongs to the list before enabling deeply indented closing markers. in_list still reflects the preceding line when an unindented top-level fence opens. For example, - item\n\n```\n ```\n[example](missing.md)\n```\n[real](also-missing.md)\n incorrectly closes the top-level block at the four-space-indented backticks, reports missing.md from inside the code block, and suppresses the real also-missing.md link. A top-level closing fence may have at most three leading spaces; reset the list context for an outdented opener and add this regression case.
There was a problem hiding this comment.
Fixed in 46148b1. A fence at column 0 after a list item is outside the item, so its indented ``` is code and does not close it.
Test: test_a_top_level_fence_after_a_list_item_is_not_closed_by_an_indented_marker.
| prefix = QUOTE_PREFIX_RE.match(line) | ||
| return (line[prefix.end():], True) if prefix else (line, False) |
There was a problem hiding this comment.
Must-fix: Preserve the opener's blockquote depth rather than stripping every > on every line. For > ```\n> > ```\n> [example](missing.md)\n> ```\n[real](also-missing.md)\n, the second line is code containing > ````, not a closing fence. Here _unquoteremoves both markers and reports onlyTrue, so that line incorrectly closes the fence; the example link is checked and the actual closing fence reopens a block that hides the real link. Track the quote depth and remove only the container prefix belonging to the active fence; add a regression test asserting that only also-missing.md` is reported.
There was a problem hiding this comment.
Fixed in 46148b1. A quoted fence strips only its opener's ">" markers, so "> > " inside "> " is code.
Test: test_a_deeper_quote_marker_inside_a_quoted_fence_is_code_not_a_closing_marker.
A fence in a blockquote or under a list item stayed open to the end of the body when it had no closing marker, hiding the links after it. A deeper '>' inside a quoted fence was stripped and could close it, and a column-0 fence after a list item took the item's indentation rules. Track the opener's quote depth and item column, end the fence at the first line that leaves its container, and keep the closing rules to the fence's own column. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| rest, count = _unquote(line) | ||
| deep = bool(item) and _indent(rest) >= item |
There was a problem hiding this comment.
Must-fix: item survives changes in blockquote depth, so a fence outside a quoted list can inherit that list's indentation. With > - item\n>\n ```\n[example](missing.md)\n ```\n, the two-space top-level fence is incorrectly assigned col=2; its unindented code line then ends the fence and produces a false link-missing warning. Scope list state to its quote container (or reset it when that container ends) before deciding whether the opener is inside an item, and cover this transition in a regression test.
|
|
||
| def _indent(line: str) -> int: | ||
| """The number of leading spaces and tabs of ``line``.""" | ||
| return len(line) - len(line.lstrip(" \t")) |
There was a problem hiding this comment.
Must-fix: Markdown indentation uses columns, not character counts; tabs advance to four-column stops. For - item\n\t```\n\t[example](missing.md)\n\t```\n, the fence is inside the list item, but this returns 1 while item is 2. The opener consequently fails both fence patterns, and the code example produces a false link-missing warning. Calculate indentation and the list content column using Markdown tab expansion, and add a tab-indented list-fence regression test.
…with its quote A tab counted as one column, so a fence indented with a tab under a list item was not read as inside the item. A list item in a blockquote also kept its indent after the quote ended, so an indented fence after a blank line was read as belonging to the item. Indents now use tab stops every four columns, and an item drops its column when its blockquote ends. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| deep = bool(item) and _indent(rest) >= item | ||
| opened = (DEEP_FENCE_OPEN_RE if deep else FENCE_OPEN_RE).match(rest) |
There was a problem hiding this comment.
Must-fix: Only the most recent list item's content column is retained, so returning from a nested item loses the outer item's fence boundary. For - outer\n - inner\n\n ```\n example\n\n[real](missing.md)\n, item remains 4 when the two-space fence opens. It is therefore treated as a top-level fence (col=0) and consumes the real link through EOF, instead of ending when the outer list ends. Preserve the enclosing list-item indentation when outdenting and add a regression asserting link-missing for this input.
There was a problem hiding this comment.
Fixed in ff142a8: a fence ends with its innermost list item, so the outer item's indent is kept.
Regression: test_a_fence_under_an_inner_list_item_ends_with_the_outer_item.
| if count < item_depth: | ||
| item = 0 # the quote the item sits in has ended, so its indent no longer belongs to the item | ||
| deep = bool(item) and _indent(rest) >= item | ||
| opened = (DEEP_FENCE_OPEN_RE if deep else FENCE_OPEN_RE).match(rest) |
There was a problem hiding this comment.
Must-fix: Opening-fence detection runs before removing a list-item marker. With - ```markdown\n [example](missing.md)\n ```\n, the first line is not recognized as an opener, so the fenced example incorrectly produces link-missing; the closing marker is then mistaken for an opener and can suppress subsequent real links. Detect fences in the content following a list marker as well as on continuation lines, and test a list-item fence followed by a real link.
There was a problem hiding this comment.
Fixed in ff142a8: a list item's own line can open a fence (- ```markdown).
Regression: test_a_fence_opened_on_a_list_item_line_is_a_code_block.
| return None if depth or stop == start else stop | ||
|
|
||
|
|
||
| def _inline_destinations(line: str) -> list[str]: |
There was a problem hiding this comment.
Must-fix: Extracting inline links one line at a time silently skips valid multiline Markdown links. For example, See [guide](\nreferences/missing.md\n). renders a relative link, but none of its lines contains both the opening and closing syntax, so the scan reports no link-missing. The same layout hides an existing external target from link-escapes-skill. Parse inline links across the fence-filtered body, preserving block boundaries, and add regressions for missing and escaping multiline targets.
There was a problem hiding this comment.
Fixed in ff142a8: links are read per paragraph, so a destination can span lines.
Regressions: test_a_multiline_inline_link_is_checked (missing and escaping).
| real_dir = skill_dir.resolve() | ||
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys( | ||
| [m for line in lines for m in _inline_destinations(line)] |
There was a problem hiding this comment.
Must-fix: Inline code is passed directly to the link extractor. For a body containing Example: \guide`, this produces link-missing, although Markdown renders literal code, not a link. Mask code spans before extracting Markdown links, while retaining the original spans for the intentional ../` path checks. Add a regression covering a literal link example alongside a real link.
There was a problem hiding this comment.
Fixed in ff142a8: code spans are blanked before link extraction; the ../ span checks keep the original text.
Regression: test_a_link_inside_a_code_span_is_not_checked.
| lines = _outside_fences(skill.body) | ||
| links = dict.fromkeys( | ||
| [m for line in lines for m in _inline_destinations(line)] | ||
| + REF_DEF_RE.findall("\n".join(lines)) |
There was a problem hiding this comment.
Must-fix: _outside_fences retains blockquote markers, but REF_DEF_RE only accepts a label at the start of a line after up to three spaces. Thus > [notes][n]\n>\n> [n]: ../../shared/notes.md\n is a valid quoted reference link whose target is never checked; an existing external file gets no link-escapes-skill, and a missing target gets no link-missing. Normalize Markdown container prefixes before extracting reference definitions, preserving container boundaries for next-line destinations, and add quoted-reference regressions.
There was a problem hiding this comment.
Fixed in ff142a8: blockquote markers are removed before reference definitions are read.
Regression: test_a_reference_definition_in_a_blockquote_is_checked.
…d quoted definitions A fence ends with its innermost list item, an item's own line may open one, and links are read per paragraph with code spans blanked (an escaped backtick opens none). Blockquote markers no longer hide reference definitions. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| LOCATION_SUFFIX_RE = re.compile(r":(?:\d+(?:[-:]\d+)?|[A-Za-z_]\w*)$") | ||
| CODE_SPAN_RE = re.compile(r"`([^`\n]+)`") | ||
| # A code span of any length, across lines within a paragraph: its text is literal, so no link is read in it. | ||
| CODE_SPAN_MASK_RE = re.compile(r"(?<!\\)(`+)(?!`).*?(?<!`)(?<!\\)\1(?!`)", re.DOTALL) |
There was a problem hiding this comment.
Must-fix: backslashes are literal inside a Markdown code span, so they cannot escape its closing delimiter. For the body "Example: [example](missing.md)\\\n", this regex rejects the closing backtick, leaves the literal example unmasked, and emits link-missing. This is a remaining edge case of the inline-code exclusion fix. Remove the backslash restriction from the closing delimiter (while preserving escape handling outside spans), and add a regression with a literal trailing backslash and an adjacent real link.
There was a problem hiding this comment.
Fixed in 01eb083: a backslash no longer escapes a closing backtick, so the span closes there. Regression: the literal body, and a real link after it.
| rest, count = _unquote(line, depth) | ||
| ended = count < depth or bool(col and rest.strip() and _indent(rest) < col) | ||
| if not ended: | ||
| close = (DEEP_FENCE_CLOSE_RE if col else FENCE_CLOSE_RE).match(rest) |
There was a problem hiding this comment.
Must-fix: closing-fence indentation must be limited relative to the list item's content column, not unrestricted. With body "- item\n \n \n example\n ```\n\nreal\n", the six-space marker is code (four spaces beyond the item's column two), but DEEP_FENCE_CLOSE_RE closes the block there. The scan consequently reports missing.md from inside the code example. Strip the active container indentation and apply the normal zero-to-three-space fence rule; apply the same relative limit to continuation-line openers, which currently use unrestricted DEEP_FENCE_OPEN_RE as well. Add a regression asserting only `also-missing.md` is reported.
There was a problem hiding this comment.
Fixed in 01eb083: a closing marker indented past the opener's content column is content, for list items and continuation openers. Regression asserts only also-missing.md.
| text = text.split("#", 1)[0].strip() | ||
| if link: | ||
| text = text.split("?", 1)[0].strip() |
There was a problem hiding this comment.
Must-fix: fragment stripping also runs for literal code-span paths. With an existing shared/notes#draft.md and body Read \../../shared/notes#draft.md`., _file_ref(raw)returns../../shared/notes, so the scan silently misses path-escapes-skillunless that different path happens to exist. Unlike Markdown link destinations, code spans have no URL fragment semantics. Strip#fragmentonly whenlink=True, and add a regression with an existing external filename containing #`.
Suggestion:
| text = text.split("#", 1)[0].strip() | |
| if link: | |
| text = text.split("?", 1)[0].strip() | |
| if link: | |
| text = text.split("#", 1)[0].strip() | |
| text = text.split("?", 1)[0].strip() |
There was a problem hiding this comment.
Fixed in 01eb083: #fragment is stripped from link destinations only, so a code span path keeps it. Regression with an existing notes#draft.md.
| spans = dict.fromkeys( | ||
| s.strip() | ||
| for line in lines | ||
| for s in CODE_SPAN_RE.findall(line) | ||
| if s.strip().startswith("../") | ||
| ) |
There was a problem hiding this comment.
Must-fix: path spans are extracted one line at a time with a single-backtick regex, so valid Markdown code spans can bypass the escape check. With an existing shared/notes.md and a skill at skills/reach, body "Read \n../../shared/notes.md\n.\n" renders a code span naming that external file, but produces no path-escapes-skill. Multiple-backtick spans containing a literal backtick in the filename are also truncated. Extract complete code spans across each fence-filtered paragraph using matching delimiter runs, normalize their Markdown whitespace, then apply the ../ and existence checks. Add regressions for multiline spans and filenames containing backticks.
There was a problem hiding this comment.
Fixed in 01eb083: multiline and multi-backtick code spans are read whole, so their paths are checked. Regressions for both.
Inline links are read at any parenthesis depth and over lines, with their brackets matched by depth, and a link to a reference label reads its destination on the next line. Escaped brackets and backticks open nothing; a code span takes its text whole (matching backtick runs, over lines, a fragment or backslash kept literal). A destination's percent escapes name the decoded file, and single-quoted titles and a space before the closing parenthesis are read. A link outside the skill is classed by its written name, never probed. A fence closes only at its own marker, and a closing marker indented past the opening one is content. mirror-drift is only for copies with the same description, and a near-duplicate needs three shared words. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Maintainer decision: A1 (link parser reuses L0 |
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 1acb31c. The commits since f1968ff close the open points from earlier rounds: paths outside the checkout are classed by name and never read, mirror-drift needs a shared description, near-duplicates need three shared words, and the link parser now handles fences, code spans and destinations as CommonMark does. Three things are worth fixing before merge: two parser cases and one test gap, all inline. One doc nit.
What I ran, on a Linux review host at 1acb31c:
make ci-local: 28 passed, 2 failed, 7 skipped (Docker, network and other-Python legs). The two failures are the gitleaks download and the scan step that needs it, not the PR; a gitleaks scan of the PR's commits finds no leaks. The changed-line coverage gate passes.- Reproductions through
_bundle_findingsfor the code findings, with no model or network involved. - A mutation run of 70 mutants of the new parser code against
test_catalog_links.pyandtest_catalog_validate.py: 36 pass both files, most of them equivalent or message-only; the one that gates a probe is inline.
Checked, no issues
- Exit code: every new finding is
warn;--strictis unchanged. - Paths outside the checkout:
../../../../etc/passwdand a missing sibling both givelink-escapes-skillby their written name; nothing outside the checkout is stat-ed. - Robustness: NUL bytes, lone surrogates, 100 KB inputs of
](,[and backticks, and deep parentheses return in under 0.4 s with no exception. - Rules: every one of the 28 commits carries a
Signed-off-by;validate.pystays under its growth cap; the new tests write only undertmp_path.
Bot points I checked that no longer apply at this head (44)
- exai-bot's point on the
tel:andurn:URI destinations does not apply:URI_SCHEME_REskips them before any file lookup. - exai-bot's point on reference definitions and angle-bracket destinations does not apply: reference definitions are collected and angle brackets are unwrapped.
- exai-bot's point on inline angle-bracket destinations with spaces does not apply: the inline scanner keeps
<...>targets whole. - exai-bot's point on fence length and character does not apply: a fence closes only at the same marker with at least the opening length.
- exai-bot's point on a reference definition with its destination on the next line does not apply: definitions are read across one line break.
- exai-bot's point on the
use forword boundary does not apply: the use phrases inWHEN_REend on a word boundary. - exai-bot's point on parentheses in link destinations does not apply: the inline scanner balances parentheses to any depth.
- exai-bot's point on single-quoted titles and space before the closing parenthesis does not apply: all three title forms and padding are accepted.
- exai-bot's point on links in code spans and escaped links does not apply: code spans are blanked and escaped brackets are skipped before links are read.
- exai-bot's point on percent-encoded destinations does not apply: the destination is unquoted before the file lookup.
- Copilot's point on inline angle-bracket destinations with spaces does not apply: the inline scanner keeps
<...>targets whole. - Copilot's point on
${CLAUDE_SKILL_DIR}/references/${VERSION}.mddoes not apply: the prefix is stripped first and the rest goes through the variable check. - exai-bot's point on the repeat on single-quoted titles does not apply: the title forms are accepted.
- exai-bot's point on the repeat on percent-decoding does not apply: the destination is unquoted before the lookup.
- exai-bot's point on the repeat on code spans does not apply: code spans are blanked before links are read.
- exai-bot's point on dotted URI schemes does not apply:
z39.50r:example.org/databaseis read as a URI. - exai-bot's point on
${CLAUDE_SKILL_DIR}/skipping the variable check does not apply: the prefix is stripped first. - exai-bot's point on ASCII-only description words does not apply: description words are letters in any script.
- exai-bot's point on
run.py:42andrun.py:maindoes not apply: the line or symbol suffix is dropped before resolving. - exai-bot's point on the
whenphrase whitelist does not apply:WHEN_REaccepts anywhenclause. - exai-bot's point on inline destinations with
<...>and parentheses does not apply: both are handled by the inline scanner. - exai-bot's point on the repeat on a definition destination on the following line does not apply: definitions are matched across one line break.
- exai-bot's point on the repeat on fence tracking does not apply: a tilde line inside a backtick block no longer closes it.
- exai-bot's point on the
sub/..directory name does not apply: the lexical directory name is used. - exai-bot's point on the trailing word boundary of
use fordoes not apply:Use formatting tools ...getsdescription-no-whenagain. - exai-bot's point on
?queryin link destinations does not apply: the query is dropped before the file check. - exai-bot's point on counterpart labels across checkouts does not apply:
_counterpartnames the other source root. - exai-bot's point on nested parentheses in destinations does not apply: the scanner balances to any depth.
- exai-bot's point on fences inside blockquotes and list items does not apply: container prefixes are handled when a fence opens and closes.
- exai-bot's point on backslash escapes in destinations does not apply: ASCII punctuation escapes are decoded.
- exai-bot's point on a container fence that has no closing marker does not apply: the fence ends with its quote or list item.
- exai-bot's point on list context for a top-level fence does not apply: a fence at column 0 after a list item is outside the item.
- exai-bot's point on the quote depth of a fence does not apply: a quoted fence strips only its opener's markers.
- exai-bot's point on a list item inside a quote does not apply: a list item's indent ends with its quote.
- exai-bot's point on tab indentation does not apply: tabs count to the next four-column stop.
- exai-bot's point on returning from a nested list item does not apply: the outer item's indent is kept.
- exai-bot's point on a fence opened on a list item line does not apply: the item's own line can open a fence.
- exai-bot's point on multiline inline links does not apply: links are read per paragraph.
- exai-bot's point on inline code passed to the link extractor does not apply: code spans are blanked before extraction.
- exai-bot's point on reference definitions in a blockquote does not apply: quote markers are removed before definitions are read.
- exai-bot's point on a backslash before a closing backtick does not apply: a backslash no longer escapes the closing delimiter.
- exai-bot's point on closing-fence indentation inside a list item does not apply: a marker indented past the content column is content.
- exai-bot's point on
#fragmentin code-span paths does not apply: the fragment is stripped from link destinations only. - exai-bot's point on multiline and multi-backtick code spans does not apply: spans are read whole across a paragraph.
…e rules Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to fdd88b1, which fix the nit from my review:
fdd88b1:dokimi/docs/external-content.md:139: near-duplicate now says it shares three words, and mirror-drift says the description must match.
make ci-local at fdd88b1: 28 passed, 2 failed (host: gitleaks download and scan), 7 skipped.
GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.
Not approving yet. Still open: the two bugs and the risk from my review.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/external/validate.py # dokimi/tests/test_catalog_validate.py
Inline links come from the paragraph reader only; the L0 splitter reads reference definitions, so a link like [x](a\)b.md) is no longer cut at the backslash. A code span no longer runs across a list item or a heading. A code span naming a path outside the checkout is pinned as never read. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The lexical guard let a symlink in the checkout show, by finding code or the catalog-root note, whether a host file outside it exists. A target that resolves outside the skill is link-escapes-skill, and one that resolves outside the checkout is never stat-ed. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Every `](` scanned to the end of its paragraph while the depth only rose, so a 60 KB body of unclosed links took over a minute in catalog scan and routing. cmark caps the depth at 32 too; deeper is not read as a link. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Dropping _is_drift from the same-name skip, or the checkout guard on the catalog-root note, left the suite green. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Brings in intel#373, whose catalog eval tests now meet this branch's description checks. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…ption
With this branch's description-no-when check, 'Use to inspect {name} inputs.' added a second finding to test_empty_import_repo_alone_fails_the_import_check.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
…el#321) * feat(catalog): check bundle links, descriptions and mirror drift `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> * fix(catalog): a link target the OS refuses is a missing link, not a crash 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> * Address review: bundle links skip URIs and read reference and angle-bracket targets Thread: PRRT_kwDOVBg7g86yORfR Thread: PRRT_kwDOVBg7g86yORfU Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * Address review: parse reference definitions and <...> targets as CommonMark 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> * test(reference): pin the shared regex limit with a fake clock, not wall 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> * test(reference): keep the shared-limit comment to two lines The comment-block check allows two lines of reason per comment. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * Compare only skills that share a word in the near-duplicate pass Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * Pin the near-duplicate skips and the 0.5 overlap threshold in tests Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * docs(changelog): qualify the exit-code note for spec and load errors 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> * fix(catalog): read dotted URI schemes, template variables, root skills 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> * style(catalog): fit the URI scheme comment in two lines 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> * fix(catalog): read a line or symbol suffix as part of the file name 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> * test(catalog): pin range and multi-colon suffixes of missing file links Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * test(catalog): cover root skills of two checkouts in drift checks Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(catalog): read link targets, definitions and fences as CommonMark does - An inline target keeps balanced parentheses (guide(v2).md) and an angle-bracket target keeps its spaces (<my notes.md>). - A reference definition may have its destination on the line after its label. Definitions are matched over the body, and a fence leaves a blank line behind. - A fenced block closes only at the same marker, at least as long as the opener. - A backtick info string with a backtick is an inline code span, not a fence. - A skill named through a subdirectory (skills/alpha/sub/..) takes the name of the directory it resolves to lexically, both for name checks and mirror checks. Tests: tests/test_catalog_links.py (new; test_catalog_validate.py is at the length cap). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * Make the separator in REF_DEF_RE unambiguous Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * Cap the indent of a code fence at three spaces Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * Test link titles, padded targets and fence lookalikes Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(catalog): trigger phrases and link queries match whole words and paths A use phrase now needs its last word to end (use for, use if), so 'Use formatting tools' no longer passes as a when clause. A link destination's ?query is not part of the file it names. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): name the other checkout in mirror-drift and near-duplicate findings Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * Test the word boundaries of the remaining WHEN_RE phrases Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * Pin the same-checkout form of the mirror-drift counterpart Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(catalog): read link destinations of any nesting, unescape them, and fences in containers A destination with parentheses nested two levels deep was not seen as a link, so a missing or escaping target went unreported. A backslash escape of punctuation was checked literally (guide\_notes.md). A fence inside a blockquote or under a list item indented past three spaces was not seen as a code block, so the links in its example text were reported. Destinations are now scanned for balance; escapes are decoded for link destinations only, code spans stay literal. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): end a container fence with its quote or list item A fence in a blockquote or under a list item stayed open to the end of the body when it had no closing marker, hiding the links after it. A deeper '>' inside a quoted fence was stripped and could close it, and a column-0 fence after a list item took the item's indentation rules. Track the opener's quote depth and item column, end the fence at the first line that leaves its container, and keep the closing rules to the fence's own column. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): count tab indents as columns; end a list item's indent with its quote A tab counted as one column, so a fence indented with a tab under a list item was not read as inside the item. A list item in a blockquote also kept its indent after the quote ended, so an indented fence after a blank line was read as belonging to the item. Indents now use tab stops every four columns, and an item drops its column when its blockquote ends. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): read nested list fences, multiline links, code spans and quoted definitions A fence ends with its innermost list item, an item's own line may open one, and links are read per paragraph with code spans blanked (an escaped backtick opens none). Blockquote markers no longer hide reference definitions. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): check links, spans and fences as CommonMark does Inline links are read at any parenthesis depth and over lines, with their brackets matched by depth, and a link to a reference label reads its destination on the next line. Escaped brackets and backticks open nothing; a code span takes its text whole (matching backtick runs, over lines, a fragment or backslash kept literal). A destination's percent escapes name the decoded file, and single-quoted titles and a space before the closing parenthesis are read. A link outside the skill is classed by its written name, never probed. A fence closes only at its own marker, and a closing marker indented past the opening one is content. mirror-drift is only for copies with the same description, and a near-duplicate needs three shared words. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * docs(changelog): mirror-drift needs a shared description Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * docs(external-content): state the full mirror-drift and near-duplicate rules Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(catalog): read each inline link once, keep code spans inside a block Inline links come from the paragraph reader only; the L0 splitter reads reference definitions, so a link like [x](a\)b.md) is no longer cut at the backslash. A code span no longer runs across a list item or a heading. A code span naming a path outside the checkout is pinned as never read. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(catalog): class link targets by where they resolve, not only by name The lexical guard let a symlink in the checkout show, by finding code or the catalog-root note, whether a host file outside it exists. A target that resolves outside the skill is link-escapes-skill, and one that resolves outside the checkout is never stat-ed. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> * fix(catalog): stop reading a link destination past 32 nested parentheses Every `](` scanned to the end of its paragraph while the depth only rose, so a 60 KB body of unclosed links took over a minute in catalog scan and routing. cmark caps the depth at 32 too; deeper is not read as a link. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> * test(catalog): pin the same-name near-duplicate and the root-note guards Dropping _is_drift from the same-name skip, or the checkout guard on the catalog-root note, left the suite green. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> * test(catalog): give intel#373's import-check fixtures a 'when' description With this branch's description-no-when check, 'Use to inspect {name} inputs.' added a second finding to test_empty_import_repo_alone_fails_the_import_check. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> --------- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> Co-authored-by: Anton Volkov <antonwolfy@gmail.com> Co-authored-by: Alexander Rybkin <alexander.rybkin@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 only when two same-named skills also share a description; two unrelatedreview/skills with different descriptions get nomirror-drift(the description checks cover them).warnonly.Checklist
pytestpassesruff,mypy, and schema checks passdocs/external-content.md(catalog scan section); changelog fragment added🤖 Generated with Claude Code