From f54bbb7f44082002947983df5cfb39a59c9038d1 Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Thu, 3 Sep 2026 09:00:34 -0700 Subject: [PATCH] fix(validate): a dead link in an imported body warns instead of failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `main` is red on Validate, and every open pull request inherits it: FAIL skills/vllm-xpu-run/SKILL.md: link https://docs.vllm.ai/en/latest/getting_started/xpu-installation.html is gone (HTTP 404) The link really is dead — docs.vllm.ai dropped the `.html` URL scheme, so it now redirects to a path that 404s. But `skills/vllm-xpu-run/` is an import pinned to intel/gpu-ai-skills, so editing the URL here would only move the failure one step down the job, to `sync_external.py --check`, which byte-compares the copy against its pin. There is no edit to this repository that both satisfies the link check and keeps the import intact. Check 8 already resolves exactly this tension. A file a skill ships but never mentions fails when the skill was written here and warns when it was imported, because "the only way to satisfy it would be to edit another team's body, and an edited import no longer matches the text their measurements describe". A dead link in an imported body is the same shape of finding, so it gets the same treatment, and the warning says where the repair has to land: WARN skills/vllm-xpu-run/SKILL.md: link ... is gone (HTTP 404) (imported: upstream's body is kept as it is — fix it upstream, then move external-commit) What still fails: a dead link in a body this repository wrote, and a pinned commit that has gone missing. The second one shares the mechanism but not the ownership — `external-commit` is our claim about upstream, not upstream's text, so a SHA rewritten out of history is our defect to fix and stays an error. link_targets() now returns a third element saying which origin a URL came from, rather than check_links() re-deriving it, because the pinned-commit target is synthesised from .source.json and would otherwise be indistinguishable from a URL found in the body it sits next to. MAINTAINERS.md documents the route the warning asks for, and says the thing worth saying out loud: a warning that outlives a release means the pin is the wrong pin. Verified locally: the gate passes with the warning, and both error paths were checked directly — a dead link attributed to a body written here still errors, and a rewritten external-commit still errors. The stale docs.vllm.ai URLs themselves are fixed in intel/gpu-ai-skills#12; the pin move lands here once that merges. Signed-off-by: Nikolay Petrov --- .github/workflows/validate.yml | 6 ++- MAINTAINERS.md | 9 +++++ tools/validate_skills.py | 70 +++++++++++++++++++++++++--------- 3 files changed, 65 insertions(+), 20 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index b6d689d..e8dd242 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -48,8 +48,10 @@ jobs: # Its own step because it is the one check that depends on someone else's # server. A 404 or 410 fails it — that is a pointer an agent would follow - # into nothing. A timeout, a 5xx, or rate limiting only warns, so an outage - # somewhere on the internet cannot hold a contributor's pull request. + # into nothing — unless the link came from an imported body, where it warns + # instead, because the repair has to land upstream and arrive through a moved + # pin. A timeout, a 5xx, or rate limiting only warns, so an outage somewhere on + # the internet cannot hold a contributor's pull request. - name: Link check run: python3 tools/validate_skills.py --check-links diff --git a/MAINTAINERS.md b/MAINTAINERS.md index b1e2c28..4d1c15f 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -203,6 +203,15 @@ written, and that is what the pin records. Keep the imported text as it was — it means existing measurements of the original no longer describe the file here. The one edit the generator does make is to repository-relative command paths, which would otherwise point at upstream's layout and resolve to nothing once the skill is installed. + +That is also why two checks report an imported body as a warning where they would fail a +skill written here: an unmentioned file, and a link that has gone 404. Both name a real +defect and neither can be fixed in this repository — editing the body breaks the +byte-compare in `sync_external.py --check`, which is the thing that proves the copy is +still what was reviewed. The route is a pull request upstream, then move +`external-commit` and re-run `--write`. A warning that outlives a release is worth +raising with the upstream maintainer rather than living with; if upstream will not take +the fix, the pin is the wrong pin. Every file it touches is listed in `.source.json` under `modified-files` and carries a one-line notice saying so, and the validator fails if either is missing. diff --git a/tools/validate_skills.py b/tools/validate_skills.py index 55212c1..f7f6166 100644 --- a/tools/validate_skills.py +++ b/tools/validate_skills.py @@ -63,7 +63,10 @@ resolves the http(s) links in each SKILL.md, and for an imported skill the upstream repository and its pinned commit, so a moved or deleted target is caught before a reader follows a dead reference. It is opt-in and a separate CI step because it is -the one check whose result depends on someone else's server being up. +the one check whose result depends on someone else's server being up. A dead link in +an imported body warns rather than fails, for the same reason 8 does: the fix belongs +upstream, and an edited import no longer matches its pin. The pinned commit itself +still fails, because that one is this repository's own claim rather than upstream's. Needs Python 3.11 or newer for tomllib. Nothing else outside the standard library. """ @@ -546,8 +549,8 @@ def extract_urls(body: str) -> list[str]: return urls -def link_targets(body: str, source: dict | None) -> list[tuple[str, str]]: - """(url, what it is) pairs to probe for one skill. +def link_targets(body: str, source: dict | None) -> list[tuple[str, str, bool]]: + """(url, what it is, came from an imported body) triples to probe for one skill. For an imported skill the pinned commit is probed as well as the repository, from .source.json rather than from the body: the body is upstream's own text and says @@ -556,15 +559,20 @@ def link_targets(body: str, source: dict | None) -> list[tuple[str, str]]: provenance leads nowhere, while a reachable repository with an unreachable commit means the SHA was rewritten out of history, so nothing can be re-verified against what we shipped. + + The third element separates those two origins. A link in an imported body is + upstream's, and the pin is ours, so a dead one is a different kind of finding + even though both are dead URLs. """ - targets = [(url, "link") for url in extract_urls(body)] + imported = source is not None + targets = [(url, "link", imported) for url in extract_urls(body)] if not source: return targets repo = str(source.get("repo", "")).rstrip("/").removesuffix(".git") commit = str(source.get("commit", "")) if UPSTREAM_URL_RE.fullmatch(repo) and COMMIT_RE.match(commit): - targets.append((f"{repo}/commit/{commit}", "pinned commit")) + targets.append((f"{repo}/commit/{commit}", "pinned commit", False)) return targets @@ -597,24 +605,46 @@ def probe_url(url: str) -> tuple[str, str]: return "unknown", "no method accepted" -def check_links(targets: list[tuple[str, str, str]], report: Report) -> None: - """Probe every collected link. targets are (where, url, kind) triples.""" +def check_links( + targets: list[tuple[str, str, str, bool]], report: Report +) -> None: + """Probe every collected link. + + targets are (where, url, kind, from an imported body) quadruples. A dead link + warns rather than fails when it came from an imported body, matching check 8: the + body is upstream's, editing it here would break the byte-compare against the pin + that tools/sync_external.py --check enforces, and the repair has to land upstream + and arrive through a moved pin. The warning names that so it is actionable rather + than noise. Everything this repository wrote — its own skills, and the pin itself + — still fails. + """ if not targets: return - unique = sorted({url for _, url, _ in targets}) + unique = sorted({url for _, url, _, _ in targets}) with ThreadPoolExecutor(max_workers=LINK_WORKERS) as pool: results = dict(zip(unique, pool.map(probe_url, unique))) - for where, url, kind in targets: + for where, url, kind, upstream_body in targets: verdict, detail = results[url] if verdict == "dead": - report.error(f"{where}: {kind} {url} is gone ({detail})") + note = f"{where}: {kind} {url} is gone ({detail})" + if upstream_body: + report.warn( + f"{note} (imported: upstream's body is kept as it is — fix it " + "upstream, then move external-commit)" + ) + else: + report.error(note) elif verdict == "unknown": report.warn(f"{where}: {kind} {url} was not reachable ({detail})") checked = len(unique) - dead = sum(1 for verdict, _ in results.values() if verdict == "dead") - print(f"link check: {checked} URL(s), {dead} gone") + dead = {url for url, (verdict, _) in results.items() if verdict == "dead"} + ours = {url for _, url, _, imported in targets if url in dead and not imported} + summary = f"link check: {checked} URL(s), {len(dead)} gone" + if dead - ours: + summary += f", {len(dead - ours)} of them only in imported bodies (warned)" + print(summary) def check_length(text: str, skill_dir: Path, report: Report) -> None: @@ -1353,7 +1383,7 @@ def main() -> int: action="store_true", help="also resolve the http(s) links in each SKILL.md, and an imported skill's " "upstream repository and pinned commit. Needs network; a 404 fails, a timeout " - "warns.", + "warns, and a 404 in an imported body warns because the fix belongs upstream.", ) args = parser.parse_args() @@ -1367,7 +1397,7 @@ def main() -> int: descriptions: dict[str, str] = {} fronts: dict[str, dict] = {} sources: dict[str, dict | None] = {} - links: list[tuple[str, str, str]] = [] + links: list[tuple[str, str, str, bool]] = [] for skill_dir in skill_dirs: sources[skill_dir.name] = read_source(skill_dir, report) skill_md = skill_dir / "SKILL.md" @@ -1388,17 +1418,21 @@ def main() -> int: if args.check_links: where = f"skills/{skill_dir.name}/SKILL.md" links += [ - (where, url, kind) - for url, kind in link_targets(body, sources[skill_dir.name]) + (where, url, kind, upstream_body) + for url, kind, upstream_body in link_targets( + body, sources[skill_dir.name] + ) ] # references/ as well as SKILL.md. The guide asks for "the links in the # references table", and for a skill of any size those links are one # indirection away, in the file the table points at — official-sources.md - # is nothing but links. + # is nothing but links. On an imported skill these files are upstream's + # too: sync_external.py vendors the whole directory, not just SKILL.md. + upstream_body = sources[skill_dir.name] is not None for path in sorted((skill_dir / "references").glob("*.md")): rel = path.relative_to(REPO_ROOT).as_posix() links += [ - (rel, url, "link") + (rel, url, "link", upstream_body) for url in extract_urls(path.read_text(encoding="utf-8")) ]