From a5b91fc9472f20175df9a6c343d2bbd79c5ecb8c Mon Sep 17 00:00:00 2001 From: Nikolay Petrov Date: Thu, 3 Sep 2026 09:00:34 -0700 Subject: [PATCH 1/2] 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 660e3e1..88d7044 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")) ] From b4c1e17c0a44ee03eca3d82f1ea6decab7d0c931 Mon Sep 17 00:00:00 2001 From: Rybkin Date: Thu, 3 Sep 2026 13:06:37 -0700 Subject: [PATCH 2/2] docs: state the imported-body dead-link warning in README and MAINTAINERS The behaviour change landed in validate_skills.py, validate.yml and the generator paragraph of MAINTAINERS.md, but three places still described the old policy: - README's list of what blocks a merge said a 404 or 410 fails, full stop. That is now false for 23 of the 33 skills in the catalog, and the README is where a contributor reads the merge bar. - README's "Reported but not blocking" line omitted the new warning. - MAINTAINERS' Level 1 section said dead links are reported without blocking, which understates it the other way: a dead link in a skill written here still fails. Also moves the modified-files sentence back beside the generator paragraph it belongs to. Inserted where it was, with no blank line before the new paragraph, rendered Markdown joined it to the warnings paragraph and "it" lost its antecedent. Signed-off-by: Rybkin --- MAINTAINERS.md | 9 +++++---- README.md | 8 +++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 4d1c15f..2b25df1 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -55,8 +55,9 @@ commit still resolves. `python3 tools/validate_skills.py`. Keyless, offline, runs on every pull request including from a fork, and is the only level that can block a merge. What it enforces is -in the README; what it reports without blocking is dead links and the coverage gaps -between what a suite claims and what it implements. +in the README; what it reports without blocking is a dead link in an imported body — a +dead link in a skill written here fails — and the coverage gaps between what a suite +claims and what it implements. Two things it deliberately does not do. It does not check that `SKILL.md` carries no measured numbers — that rule is enforced by review, because a validator cannot tell a @@ -203,6 +204,8 @@ 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. +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. 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 @@ -212,8 +215,6 @@ 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. ## CI diff --git a/README.md b/README.md index f1fa5df..695c815 100644 --- a/README.md +++ b/README.md @@ -214,11 +214,13 @@ Blocking, keyless, and runnable on a fork: still byte-for-byte the pinned upstream commit - `npx … install` writes every skill in the catalog, and `verify` accepts each one and rejects an installed copy that was altered -- a link that answers 404 or 410 — a pointer an agent would follow into nothing. A timeout, - a 5xx or rate limiting only warns, so an outage elsewhere cannot hold up a pull request +- a link that answers 404 or 410 — a pointer an agent would follow into nothing; a warning + rather than a failure in an imported body, for the same reason as the mentions check, and + because the repair has to land upstream and arrive here through a moved pin. A timeout, a + 5xx or rate limiting only warns, so an outage elsewhere cannot hold up a pull request Reported but not blocking: the coverage gaps between what a suite claims and what it -implements. +implements, and a dead link in a body this repository copied rather than wrote. Everything else — eval cases, hardware measurements, the differential, discoverability — is described in [MAINTAINERS.md](MAINTAINERS.md) and gates promotion, not merging.