add check-project-links action by mshafer-NI · Pull Request #93 · ni/python-actions

mshafer-NI · 2026-09-02T15:26:02Z

What does this Pull Request accomplish?

Add a check-project-links action that validates that the links listed in pyproject.toml files do not return 404.

Why should this Pull Request be merged?

We want to make sure our documentation links are correct.

What testing has been done?

Added a test using to fake projects.

…hose domains

Copilot

🟡 Changes recommended

The new action and its tests have a few concrete security/robustness issues (notably eval usage, allowlist matching semantics, timeout handling, and doc/test determinism mismatches) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new composite GitHub Action (check-project-links) to scan pyproject.toml files for URLs and validate that trusted links don’t return 4xx responses, plus CI coverage and documentation updates to support using the action across NI Python projects.

Changes:

Added check-project-links composite action to discover URLs in pyproject.toml and validate them via curl in Docker, failing on 4xx.
Added action documentation and linked the new action from the repo root README.
Added a GitHub Actions workflow job to exercise the new action.

File summaries

File	Description
README.md	Documents the new `check-project-links` action in the top-level actions list.
check-project-links/README.md	Provides usage docs/inputs/behavior notes for the new action.
check-project-links/action.yml	Implements the composite action logic for link discovery, filtering, and validation in Docker.
.github/workflows/test_actions.yml	Adds a workflow job to test the new action behavior.

Review details

Suppressed comments (4)

.github/workflows/test_actions.yml:286

If switching the test URLs away from GitHub (e.g., to httpstat.us), the action will ignore them unless allowed-domains includes the new host. Setting it explicitly here makes the test intent clear and prevents accidental passes due to filtering.

      - name: Check valid project links
        uses: ./check-project-links
        with:
          project-directory: test-project
      - name: Create project with a missing link

check-project-links/README.md:44

The Behavior section says the action “Falls back to grep” and “Extracts every https://... URL”, but the implementation falls back to find for locating pyproject.toml files and initially matches both http and https URLs (then filters out http://). Tweaking these bullets would align docs with the current behavior.

- Uses `rg` to locate `pyproject.toml` files beneath the configured project directory when available.
- Falls back to `grep` with a warning if `rg` is not installed; consider adding a pre-step to install `rg` for faster runtime.
- Extracts every `https://...` URL that ends at the first whitespace character.

.github/workflows/test_actions.yml:299

This “expected to fail” case will be more deterministic if it targets a purpose-built 404 endpoint rather than relying on a specific GitHub repo path remaining 404 forever.

          Broken = "https://github.com/ni/project-that-does-not-exist"

.github/workflows/test_actions.yml:308

To ensure the failing-link test actually validates the httpstat.us URL (instead of filtering it out as untrusted), pass the same allowed-domains value here as in the passing case.

        uses: ./check-project-links
        with:
          project-directory: failing-project
      - name: Error if the previous step didn't fail

Files reviewed: 4/4 changed files
Comments generated: 7
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…ps://github.com/ni/python-actions into users/mshafer-ni/add_check_project_links_action

Copilot

🟡 Changes recommended

The current ripgrep URL extraction regex is incorrect and can prevent any links from being detected, which breaks the action’s core behavior and the expected failure test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

check-project-links/action.yml:90

host="${host%%:*}" breaks bracketed IPv6 literal hosts like https://[::1]/... (it becomes [), which prevents the subsequent localhost/IP-address filters from working as intended for IPv6 literals.

          host="${url#*://}"
          host="${host%%/*}"
          host="${host%%:*}"
          host="${host,,}"
          host="${host%.}"

check-project-links/action.yml:144

Same IPv6 host parsing issue as above: host="${host%%:*}" will truncate bracketed IPv6 literals to [ and the IP/loopback drop logic won’t apply.

          host="${url#*://}"
          host="${host%%/*}"
          host="${host%%:*}"
          host="${host,,}"
          host="${host%.}"

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

bkeryan · 2026-09-02T16:49:38Z

+          rg_available=1
+        else
+          rg_available=0
+          echo "::warning title=Check Project Links Warning::rg is not available; falling back to grep. Consider adding a pre-step to install rg for faster runtime."


I love ripgrep but this seems like premature optimization. If the filesystem search is slow, then the problem is likely that it's searching large directories like .git and .venv, which should be excluded.

bkeryan · 2026-09-02T17:12:34Z

+              fi
+            fi
+          fi
+        done < <("${manifest_list_cmd[@]}") | sort -u > "$manifest_links"


I think parsing the toml would be more reliable than grepping for URLs that end in a double quote.

import pathlib import tomllib def get_urls(toml): urls = [] if isinstance(toml, dict): for v in toml.values(): urls.extend(get_urls(v)) elif isinstance(toml, list): for v in toml: urls.extend(get_urls(v)) elif isinstance(toml, str) and (toml.startswith("http://") or toml.startswith("https://")): urls.append(toml) return urls pyproject = tomllib.loads(pathlib.Path("pyproject.toml").read_text()) print(get_urls(pyproject))

If we were validating links in markdown and rst, it would be a different story, but I would want to use a link checker tool for those.

bkeryan · 2026-09-02T17:16:27Z

+          : > "$link_file"
+        fi
+
+        # Explicitly drop localhost and any IP-based hostname before validation.


Why is this important? Why would a pyproject.toml contain this kind of link?

Also, if you're going to ignore anything that isn't in on the trusted domains list, then you will drop these anyway.

I'm pretty sure after filtering to httpS only, that this is a no-op. However, was inclined to include it so that copilot couldn't flag any security issues.

bkeryan · 2026-09-02T17:18:28Z

+          if printf '%s\n' "$url" | grep -q 'http://'; then
+            echo "::warning title=Check Project Links Warning::Found insecure HTTP link in $url. Ignoring link."
+            continue
+          fi
+
+          host="${url#*://}"
+          host="${host%%/*}"
+          host="${host%%:*}"
+          host="${host,,}"
+          host="${host%.}"


I do not like the idea of parsing URLs in bash.

bkeryan · 2026-09-02T17:22:59Z

What is the required order for adding readthedocs support to a project?

Create RTD project

Add RTD project URL to pyproject.toml

Do you need to publish a (pre-)release? I guess not.

I'm not exactly sure what this looks like...

I guess if we say all links must be valid at CI time..., then it would be:
publish 1.0 (with docs on rtd, without link to them)
update source to include links to rtd
publish 1.0.1

(or similar)..., which is admittedly not great...

Another option:
Set this to not scan rtd
publish (including to rtd) with a link that's not valid in source until the publish happens
Update code for default allowed-domains

It's probably fine as long as you can bring up the RTD site before publishing the package to PyPI.

…ps://github.com/ni/python-actions into users/mshafer-ni/add_check_project_links_action

bkeryan · 2026-09-03T00:26:26Z

+def _main() -> int:
+    args = _parse_args()
+    project_directory = args.project_directory
+    safe_project_directory = os.path.realpath(project_directory, strict=True)


No pathlib? Where is @mshafer-NI and what have you done with him? 😅

bkeryan · 2026-09-03T00:36:11Z

+                with open(manifest_path, "r", encoding="utf-8") as manifest_file:
+                    for line in manifest_file:
+                        line = line.strip()
+                        prefix, comment = line.split("#", maxsplit=1) if "#" in line else (line, "")
+                        if comment.strip():
+                            comment = comment.strip()
+                            if comment.startswith("https://"):
+                                host = urlsplit(
+                                    comment
+                                ).hostname  # handles extra at the end just fine
+                                if host and _is_allowed(host, allowed):
+                                    results.add(comment)


nit: extract a function

bkeryan · 2026-09-03T00:38:56Z

+        if [ ! -s "$link_file" ]; then
+          echo "No trusted project links found under $PROJECT_DIRECTORY."
+          exit 0
+        fi
+
+        echo "Found $(wc -l < "$link_file") unique trusted links:"
+        cat "$link_file"
+
+        if [ ! -s "$link_file" ]; then
+          echo "No trusted project links found."
+          exit 0
+        fi
+
+        echo "Found $(wc -l < "$link_file") unique trusted links:"
+        cat "$link_file"


This is duplicated.

bkeryan · 2026-09-03T00:50:42Z

+    project_directory = args.project_directory
+    safe_project_directory = os.path.realpath(project_directory, strict=True)
+    safe_project_directory = _safe_under(os.getcwd(), safe_project_directory)
+    allowed_domains = args.allowed_domains
+    safe_output_path = _safe_under("/tmp", os.path.realpath(args.output_path, strict=True))


I get that you want to avoid following untrusted symlinks, but I think the action should be able to trust that project_directory points to the project directory. All of our other actions do that.

I also think that requiring the output path to be under /tmp is a bad assumption. $TMPDIR might point to /var/tmp/$USER or something. $RUNNER_TEMP might point somewhere else entirely. Also, requiring /tmp makes it hard to test the script on Windows.

Checking that the pyproject.toml files are under the project directory seems reasonable, though.

bkeryan · 2026-09-03T00:56:39Z

+        link_file="$(mktemp)"
+        cleanup() {
+          rm -f "$link_file"
+        }
+        trap cleanup EXIT
+        chmod 0644 "$link_file"


runner.temp points to a temp directory that is cleared after each job. I think it should already be in the environment as $RUNNER_TEMP.

I would expect the runner to have a reasonable umask like 002 or 022 (see https://github.com/orgs/community/discussions/40876 ), so chmod should not be necessary.

Suggested change

link_file="$(mktemp)"

cleanup() {

rm -f "$link_file"

}

trap cleanup EXIT

chmod 0644 "$link_file"

link_file="$(mktemp -p \"$RUNNER_TEMP\")"

mshafer-NI added 8 commits September 2, 2026 09:49

copilot: first draft

e3c6568

copilot: setup tests

ab0adce

fix zizmor issues

705a350

use @v0 in doc and add disclaimer

30bef4f

don't care about the full link for http warning

6116fb7

also handle quote ended links

fd83fa7

fix copilot suggested issues

44028ac

add trusted domains input and limit to only checking https links at t…

882ba28

…hose domains

mshafer-NI requested a review from bkeryan as a code owner September 2, 2026 15:26

mshafer-NI added 2 commits September 2, 2026 10:32

add checks for rg and docker commands

c04646d

whitespace

e3d97a1

mshafer-NI requested a lite review from Copilot September 2, 2026 15:33

Copilot started reviewing on behalf of mshafer-NI September 2, 2026 15:34 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

mshafer-NI changed the title ~~Users/mshafer ni/add check project links action~~ add check project links action Sep 2, 2026

mshafer-NI changed the title ~~add check project links action~~ add check-project-links action Sep 2, 2026

mshafer-NI and others added 3 commits September 2, 2026 10:43

Apply batched suggestions from code review

35617bd

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Merge branch 'users/mshafer-ni/add_check_project_links_action' of htt…

37f96fb

…ps://github.com/ni/python-actions into users/mshafer-ni/add_check_project_links_action

explicitly drop loopback and IP based hosts

c7b1ff9

mshafer-NI requested a lite review from Copilot September 2, 2026 15:52

Copilot started reviewing on behalf of mshafer-NI September 2, 2026 15:52 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread check-project-links/action.yml Outdated

Comment thread check-project-links/README.md Outdated

Apply batched suggestions from code review

bd73f6a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

bkeryan requested changes Sep 2, 2026

View reviewed changes

bkeryan reviewed Sep 2, 2026

View reviewed changes

mshafer-NI added 5 commits September 2, 2026 16:24

use existing env vars directly

8943bd4

move it to a py file

b61454c

Merge branch 'users/mshafer-ni/add_check_project_links_action' of htt…

0e3d049

…ps://github.com/ni/python-actions into users/mshafer-ni/add_check_project_links_action

update doc

a772e33

switch to python impl

1ef1d05

mshafer-NI added 5 commits September 2, 2026 17:28

all private

4c19544

fix docstring

d9ead73

format

d37c307

also not a public module

ad3ece3

set the file as world read so the docker user can read it

bcbd82f

bkeryan requested changes Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

mshafer-NI commented Sep 2, 2026

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants