Skip to content

fix: treat off-site links as external in the docs link checker - #772

Open
Ectsang wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
Ectsang:fix/link-checker-off-site
Open

fix: treat off-site links as external in the docs link checker#772
Ectsang wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
Ectsang:fix/link-checker-off-site

Conversation

@Ectsang

@Ectsang Ectsang commented Aug 26, 2026

Copy link
Copy Markdown

Nothing on the current docs is broken by this. It is hardening plus the first tests for
this script.
I grepped for every shape below and found none of them in the docs today.

check_links.py decides whether a link is external by matching its scheme against
("http", "https"), plus a skip list for mailto:/tel:/javascript:/data:. Anything
else falls through and its path gets resolved against the local build, so a link pointing
somewhere else entirely is reported as a broken internal one:

Link in a page Reported as Should be
//example.com/x Target: local_preview/x (Not Found) skipped, another host
ftp://example.com/x Target: local_preview/x (Not Found) skipped, another host
vscode:extension/ms-python.python Target: local_preview/extension/… (Not Found) skipped, not a web page

This matters because the docs job treats any finding as fatal. A false positive blocks a
deploy on a link that is not ours to fix, and the reverse case ships a broken link to
readers.

Classifying on what the URL carries fixes all three: a host, or a scheme we do not
resolve, means off-site. A relative path that happens to contain a colon still parses with
no scheme and no host, so it keeps being resolved as before.

Two adjacent gaps in the same block, both one-liners:

  • The raw-Markdown guard from fix: preserve fragments in root spec links #759 compares a case-sensitive suffix, so page.MD is
    accepted while page.md is rejected.
  • path is percent-decoded but fragment is not, so target/#re%61l reports
    Anchor not found against an id of real.

Tests, and where they run

check_links.py had no tests. This adds scripts/test_check_links.py on the same harness
as test_validate_examples.py, covering off-site classification, resolution, the
raw-Markdown guard, exit codes, .linkignore precedence and DOCS_MODE=spec.

These fail on unpatched main:

off_site_skipped[//example.com/x]
off_site_skipped[//example.com/x.md]
off_site_skipped[ftp://example.com/x]
off_site_skipped[ws://example.com/socket]
off_site_skipped[vscode:extension/ms-python.python]
on_site_resolves[target/#re%61l]
on_site_failure_reported[target/page.MD]

.pre-commit-config.yaml gains a check-links-tests hook mirroring
validate-examples-tests, so the tests run rather than sit unused.

I did not add a CI step. Editing .github/workflows/docs.yml makes zizmor scan it, and it
reports seven unpinned-uses findings on actions/checkout@v5 and
tj-actions/changed-files@v47 that have nothing to do with this change. They only surface
when someone touches the file. Happy to send the CI step as a follow-up if you would like
it, but that decision looked like yours rather than mine to bundle in here.

The script reads sys.argv and the environment at module level, so the tests drive it as a
subprocess against a synthetic site rather than importing it. That exercises the same entry
point CI uses. Say the word if you would rather have an importable seam instead.

Verification

Built the site locally and diffed findings between this branch and main: 0 findings
either way, in both DOCS_MODE=root and DOCS_MODE=spec, running both versions as real
subprocesses over the same build.

pre-commit run --all-files passes. test_validate_examples.py still passes.

One removal to flag

The diff drops link = link[len(SITE_URL) - 1 :] after the site-prefix strip. That value
is never read again: the last read is the startswith(SITE_URL) test directly above it,
and everything downstream uses parsed.path or original_link. Behavior-neutral, but not
required by the fix, so I would rather name it than leave it for review to find.

check_links.py decides whether a link is external by matching its scheme
against ("http", "https") plus a short skip list. Anything else falls
through and its path is resolved against the local build, so an off-site
link is reported as a broken internal one:

  //example.com/x          -> "Not Found"   (protocol-relative)
  ftp://example.com/x      -> "Not Found"   (scheme we do not resolve)
  vscode:extension/x       -> "Not Found"   (scheme carrying no host)

Classify by what the URL carries instead: a host, or a scheme we do not
resolve, means off-site. A relative path containing a colon still parses
with no scheme and no host, so it keeps being resolved.

Two adjacent gaps in the same block:

- The raw-Markdown guard added in Universal-Commerce-Protocol#759 compares a case-sensitive suffix,
  so a link to page.MD is accepted while page.md is rejected.
- The path is percent-decoded but the fragment is not, so target/#re%61l
  reports "Anchor not found" against an id of "real".

Also drops the reassignment of `link` after the site-prefix strip. That
value is never read again -- the last read is the startswith() test above
it -- so the removal is behavior-neutral. Called out here rather than left
silent, since it is not required by the fix.

Adds scripts/test_check_links.py, the first tests for this script, on the
same harness as test_validate_examples.py, plus a check-links-tests hook
in .pre-commit-config.yaml mirroring validate-examples-tests.

No CI step is added. Editing .github/workflows/docs.yml makes zizmor scan
it, which surfaces pre-existing unpinned-uses findings unrelated to this
change. Left for a maintainer to decide on separately.

No behavior change on the current docs. Built the site locally and diffed
findings between this and main: 0 findings either way, in both DOCS_MODE
values. `pre-commit run --all-files` passes.
@Ectsang
Ectsang force-pushed the fix/link-checker-off-site branch from 26aad8e to b442f6b Compare August 26, 2026 07:18
@damaz91 damaz91 added status:needs-triage Signal that the PR is ready for human triage devops status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants