Skip to content

fix(scripts): correctly resolve internal absolute root site URLs to root index.html in check_links.py - #709

Open
yappermoar-boop wants to merge 3 commits into
Universal-Commerce-Protocol:mainfrom
yappermoar-boop:fix-check-links-absolute-root-url-resolution
Open

fix(scripts): correctly resolve internal absolute root site URLs to root index.html in check_links.py#709
yappermoar-boop wants to merge 3 commits into
Universal-Commerce-Protocol:mainfrom
yappermoar-boop:fix-check-links-absolute-root-url-resolution

Conversation

@yappermoar-boop

Copy link
Copy Markdown

Resend because google cla ,Updated check_links.py to correctly recognize internal absolute root site URLs (e.g. https://ucp.dev or https://ucp.dev/#anchor) and resolve them to ROOT_DIR/index.html instead of falling back to file_path. Also handles unslashed site URL matches.

@google-cla

google-cla Bot commented Aug 11, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Aug 11, 2026
@damaz91

damaz91 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Hi @yappermoar-boop,

Thank you for this contribution! Fixing the absolute root URL resolution in the link checker is a great improvement.

I have a couple of suggestions to make the internal link detection and resolution logic more robust:

1. Robust Internal Link Detection

The current check link.startswith(site_url_no_slash) can be fragile. For example, it would incorrectly classify an external link containing userinfo like https://ucp.dev@evil.example/x.json as internal because it starts with https://ucp.dev.

We can make this more robust by parsing the URL and comparing the hostnames directly:

      parsed = urlparse(link)
      is_absolute_internal = False
      if parsed.scheme and parsed.scheme in ("http", "https"):
        parsed_site = urlparse(SITE_URL)
        if parsed.hostname == parsed_site.hostname:
          path = parsed.path if parsed.path else "/"
          site_path = parsed_site.path
          site_path_no_slash = site_path.rstrip("/")
          if (path == site_path_no_slash) or path.startswith(site_path):
            is_absolute_internal = True
            
            # Normalize link path to be relative to ROOT_DIR
            if path.startswith(site_path):
              rel_link_path = "/" + path[len(site_path):]
            else: # path == site_path_no_slash
              rel_link_path = "/"
            
            query_part = f"?{parsed.query}" if parsed.query else ""
            fragment_part = f"#{parsed.fragment}" if parsed.fragment else ""
            link = rel_link_path + query_part + fragment_part
            parsed = urlparse(link)
        
        if not is_absolute_internal:
          continue  # External link

2. Redundant Code Cleanup

With the normalization logic, internal absolute links will always have a path of at least /. This means path_part is never empty for these links, and the not path_part block is only entered for relative anchor links (where is_absolute_internal is always False).

We can revert this line back to its original state to keep it clean:

      # Resolve Target File
      if not path_part:
        target_file = file_path

The root URL resolution (when path is /) will still be correctly handled by the elif path_part.startswith("/") branch.

What do you think?

Thanks again for the PR!

@yappermoar-boop

Copy link
Copy Markdown
Author

Hi @damaz91 ,

Thank you for the thoughtful review and detailed feedback!

I have updated the PR with your suggestions:

  1. Implemented hostname-based comparison using urlparse to prevent edge-case external links (like userinfo format) from being misclassified.
  2. Normalized the path resolution so internal absolute links start at /, allowing us to keep the if not path_part target assignment clean and concise.

All tests pass cleanly. Thanks again!

@damaz91

damaz91 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Thanks! LGTM - this PR actually uncovered some broken links - fixing those in #710

@damaz91
damaz91 requested a review from carolinerg1 August 11, 2026 12:38
@damaz91

damaz91 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Let's wait for #710 to merge first, then we can go ahead with this

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.

3 participants