From b077d9a3b8e0905927227b0cc78c9a55226a9c2c Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 3 Aug 2026 14:13:16 +1000 Subject: [PATCH 1/2] Add ignore-patterns, reuse the open issue, and fix the timeout path Implements both halves of #2, plus the underlying bug that made the existing false-positive handling unreachable for the case that prompted the issue. ignore-patterns --------------- A newline-separated list of regular expressions for URLs to skip. Matching URLs are never requested, so they can be reported as neither broken nor redirected. Newline- rather than comma-separated so regex quantifiers such as {1,3} survive unsplit; the value is passed through the environment to a file rather than interpolated into the shell, since it is multi-line and full of metacharacters. Invalid patterns are logged and skipped instead of failing the run. Skipped links are counted separately and exposed as the ignored-count output. update-existing-issue --------------------- Defaults to true. Before creating an issue the action now looks for the newest open issue with the same title and the broken-links label, and refreshes its body instead of opening a duplicate. A weekly cron on a persistent finding produces one issue rather than one per week. Setting it to false restores the previous behaviour. Timeout path fixes ------------------ is_likely_bot_blocked() already existed to absorb this class of false positive, but could not fire on a timeout: - The legitimate_domains allowance required the error string 'Connection Error', while the timeout handler passes 'timeout'. A listed domain was protected against one failure mode and reported broken on the other. Verified before the change: github.com with a connection error returned True, with a timeout returned False. - silent_codes was only consulted on responses that carried a status code, so it could never apply to a timeout or connection error. Status 0 is now honourable in silent-codes. The three network-failure handlers are now one helper, so they cannot drift apart again. Also in this change ------------------- - tests/test_modules.py called unittest.main(exit=False) without inspecting the result, so the CI test job passed even when tests failed. It now exits non-zero. Verified both directions. - tests/test_bot_blocking.py imported through a path left over from the QuantEcon/meta layout, so it could not run at all. - The action's self-referencing links in issue bodies, PR comments and artifacts still pointed at the pre-migration QuantEcon/meta path. Corrected in action.yml only; examples.md has the same stale references throughout and is left for a separate change. Nine new tests cover pattern compilation and matching, invalid and empty pattern handling, timeout protection for listed and unlisted domains, and status 0 in silent-codes. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 14 +++++ README.md | 35 ++++++++++- action.yml | 122 +++++++++++++++++++++++++++++-------- link_checker.py | 119 ++++++++++++++++++++++++++---------- tests/test_bot_blocking.py | 2 +- tests/test_modules.py | 69 +++++++++++++++++++-- 6 files changed, 297 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4312745..147f696 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- `ignore-patterns` input: newline-separated regular expressions for URLs to skip entirely. Matching URLs are never requested, so they can be reported as neither broken nor redirected, and are counted separately via the new `ignored-count` output +- `update-existing-issue` input (default `true`): reuse the newest open issue with the same title and `broken-links` label, refreshing its body, instead of opening a duplicate on every run. New `issue-updated` output reports which path was taken +- Status `0` — a request that failed before the server answered — can now be listed in `silent-codes` + +### Fixed +- The `legitimate_domains` allowance in bot-blocking detection required the error string `Connection Error`, but the timeout handler passes `timeout`. A listed domain was therefore protected against connection errors and reported broken on timeouts. Both are now treated alike +- `silent-codes` was only consulted on responses that returned a status code, so it could never apply to timeouts or connection errors +- The test runner called `unittest.main(exit=False)` without inspecting the result, so the CI test job reported success even when tests failed +- Corrected the action's self-referencing links in issue bodies, PR comments and artifacts, which still pointed at the pre-migration `QuantEcon/meta` path + +### Changed +- **Behaviour change:** with `create-issue: 'true'`, a recurring finding now refreshes one open issue rather than opening a new one per run. Set `update-existing-issue: 'false'` to restore the previous behaviour + +### Previously unreleased - Initial release of the AI-Powered Link Checker action - Smart link validation with configurable timeouts - AI-powered suggestions for broken and redirected links diff --git a/README.md b/README.md index 07cd451..eeeda70 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,26 @@ jobs: The action includes intelligent logic to reduce false positives for legitimate sites: +### Ignore Patterns + +Some hosts throttle or block datacenter IP ranges outright, so they time out from a CI runner while working perfectly for readers. The automatic detection below catches many of these, but it cannot know which hosts a particular project depends on. Use `ignore-patterns` to declare them: + +```yaml +- uses: QuantEcon/action-link-checker@main + with: + html-path: '_site' + ignore-patterns: | + https://fred\.stlouisfed\.org/.* + # blank lines and comments are ignored + linkedin\.com +``` + +Patterns are Python regular expressions, one per line — newline-separated rather than comma-separated so that quantifiers such as `{1,3}` survive intact. Each is matched against the full URL with `re.search`, so a bare domain works as a substring without anchoring. + +Ignored URLs are never requested at all. They therefore cannot be reported as broken *or* as redirects, and they are counted separately in the report and exposed as the `ignored-count` output. An invalid pattern is logged and skipped rather than failing the run. + +If a project already maintains a Sphinx `linkcheck_ignore` list, those patterns can be pasted here directly — the syntax is the same. + ### Bot Blocking Detection - **Major Sites**: Automatically detects common sites that block automated requests (Netflix, Amazon, Facebook, etc.) - **Encoding Issues**: Identifies encoding errors that often indicate bot protection @@ -202,6 +222,13 @@ Common codes to consider: - `503`: Service Unavailable (temporary outages) - `429`: Too Many Requests (rate limiting) - `502`: Bad Gateway (temporary server issues) +- `0`: No response at all — the request failed before the server answered (timeout, DNS failure, connection refused). Silencing `0` suppresses every unreachable host, so prefer `ignore-patterns` when only specific hosts are affected. + +### Recurring Reports + +When `create-issue` is enabled on a schedule, the action reuses the newest open issue carrying the same `issue-title` and the `broken-links` label, refreshing its body with the latest run instead of opening another issue. A weekly cron on a persistent finding therefore produces one issue, not one per week. + +Close the issue once the links are fixed; if the finding recurs afterwards, a fresh issue is opened. To restore the previous behaviour of always opening a new issue, set `update-existing-issue: 'false'`. ### Performance Tuning @@ -287,11 +314,13 @@ permissions: |-------|-------------|----------|---------| | `html-path` | Path to HTML files directory | No | `./_build/html` | | `mode` | Scan mode: `full` or `changed` | No | `full` | -| `silent-codes` | HTTP codes to silently report | No | `403,503` | +| `silent-codes` | HTTP codes to silently report (`0` = no response) | No | `403,503` | +| `ignore-patterns` | Regex patterns for URLs to skip entirely, one per line | No | *(none)* | | `fail-on-broken` | Fail workflow on broken links | No | `true` | | `ai-suggestions` | Enable AI-powered suggestions | No | `true` | | `create-issue` | Create GitHub issue for broken links | No | `false` | | `issue-title` | Title for created issues | No | `Broken Links Found in Documentation` | +| `update-existing-issue` | Refresh the open issue with this title instead of opening a duplicate | No | `true` | | `create-artifact` | Create workflow artifact | No | `false` | | `artifact-name` | Name for workflow artifact | No | `link-check-report` | | `notify` | Users to assign to created issue | No | `` | @@ -305,9 +334,11 @@ permissions: | `broken-links-found` | Whether broken links were found | | `broken-link-count` | Number of broken links | | `redirect-count` | Number of redirects found | +| `ignored-count` | Number of links skipped by `ignore-patterns` | | `link-details` | Detailed broken link information | | `ai-suggestions` | AI-powered improvement suggestions | -| `issue-url` | URL of created GitHub issue | +| `issue-url` | URL of the created or updated GitHub issue | +| `issue-updated` | Whether an existing issue was reused rather than a new one opened | | `artifact-path` | Path to created artifact file | ## Best Practices diff --git a/action.yml b/action.yml index 425a348..173ef8f 100644 --- a/action.yml +++ b/action.yml @@ -12,9 +12,13 @@ inputs: required: false default: 'full' silent-codes: - description: 'HTTP status codes to silently report without failing (comma-separated)' + description: 'HTTP status codes to silently report without failing (comma-separated). Status 0 covers requests that failed before the server answered (timeouts, connection errors)' required: false default: '403,503' + ignore-patterns: + description: 'Regex patterns for URLs to skip entirely, one per line. Matched against the full URL, so a bare domain works as a substring. Ignored URLs are never requested and cannot be reported as broken or redirected' + required: false + default: '' fail-on-broken: description: 'Whether to fail the workflow if broken links are found' required: false @@ -31,6 +35,10 @@ inputs: description: 'Title for the GitHub issue when broken links are found' required: false default: 'Broken Links Found in Documentation' + update-existing-issue: + description: 'Reuse the newest open issue with the same title instead of opening a duplicate. Set to false to restore the previous behaviour of opening a new issue on every run' + required: false + default: 'true' create-artifact: description: 'Whether to create a workflow artifact with the link report' required: false @@ -62,6 +70,9 @@ outputs: redirect-count: description: 'Number of redirects found' value: ${{ steps.check.outputs.redirect-count }} + ignored-count: + description: 'Number of links skipped because they matched ignore-patterns' + value: ${{ steps.check.outputs.ignored-count }} link-details: description: 'Details of broken links and suggestions' value: ${{ steps.check.outputs.link-details }} @@ -69,8 +80,11 @@ outputs: description: 'AI-powered suggestions for link improvements' value: ${{ steps.check.outputs.ai-suggestions }} issue-url: - description: 'URL of the created GitHub issue (if create-issue is enabled)' + description: 'URL of the created or updated GitHub issue (if create-issue is enabled)' value: ${{ steps.create-issue.outputs.issue-url }} + issue-updated: + description: 'Whether an existing issue was reused rather than a new one opened (true/false)' + value: ${{ steps.create-issue.outputs.issue-updated }} artifact-path: description: 'Path to the created artifact file (if create-artifact is enabled)' value: ${{ steps.create-artifact.outputs.artifact-path }} @@ -86,10 +100,14 @@ runs: - name: Check links and generate AI suggestions id: check shell: bash + env: + # Passed through the environment rather than interpolated into the + # script: the value is multi-line and contains regex metacharacters. + IGNORE_PATTERNS: ${{ inputs.ignore-patterns }} run: | # Get the action directory ACTION_DIR="${{ github.action_path }}" - + # Parse inputs HTML_PATH="${{ inputs.html-path }}" MODE="${{ inputs.mode }}" @@ -98,15 +116,25 @@ runs: AI_SUGGESTIONS="${{ inputs.ai-suggestions }}" TIMEOUT="${{ inputs.timeout }}" MAX_REDIRECTS="${{ inputs.max-redirects }}" - + echo "Scanning HTML files in: $HTML_PATH" echo "Mode: $MODE" echo "Silent codes: $SILENT_CODES" echo "AI suggestions enabled: $AI_SUGGESTIONS" - + + # Write ignore patterns to a file so the checker never has to parse + # them out of an argument list + IGNORE_FILE="$(mktemp)" + printf '%s\n' "$IGNORE_PATTERNS" > "$IGNORE_FILE" + IGNORE_COUNT=$(grep -cve '^[[:space:]]*$' -e '^[[:space:]]*#' "$IGNORE_FILE" || true) + if [ "$IGNORE_COUNT" -gt 0 ]; then + echo "Ignore patterns in effect: $IGNORE_COUNT" + fi + # Initialize counters TOTAL_BROKEN=0 TOTAL_REDIRECTS=0 + TOTAL_IGNORED=0 BROKEN_LINKS_FOUND="false" LINK_DETAILS="" AI_SUGGESTIONS_OUTPUT="" @@ -155,6 +183,7 @@ runs: --timeout "$TIMEOUT" \ --max-redirects "$MAX_REDIRECTS" \ --silent-codes "$SILENT_CODES" \ + --ignore-patterns-file "$IGNORE_FILE" \ $AI_FLAG 2>/tmp/stderr.log) if [ $? -ne 0 ] || [ -z "$result_json" ]; then @@ -166,11 +195,13 @@ runs: # Parse results and update counters broken_count=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(len(data['broken_results']))") redirect_count=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(len(data['redirect_results']))") + ignored_count=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(len(data.get('ignored_results', [])))") total_links=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(data['total_links'])") - + TOTAL_BROKEN=$((TOTAL_BROKEN + broken_count)) TOTAL_REDIRECTS=$((TOTAL_REDIRECTS + redirect_count)) - + TOTAL_IGNORED=$((TOTAL_IGNORED + ignored_count)) + if [ "$broken_count" -gt 0 ] || [ "$redirect_count" -gt 0 ]; then BROKEN_LINKS_FOUND="true" @@ -194,13 +225,16 @@ runs: fi fi - echo " Found $total_links total links, $broken_count broken, $redirect_count redirected" + echo " Found $total_links total links, $broken_count broken, $redirect_count redirected, $ignored_count ignored" done - + + rm -f "$IGNORE_FILE" + # Set outputs echo "broken-links-found=$BROKEN_LINKS_FOUND" >> $GITHUB_OUTPUT echo "broken-link-count=$TOTAL_BROKEN" >> $GITHUB_OUTPUT echo "redirect-count=$TOTAL_REDIRECTS" >> $GITHUB_OUTPUT + echo "ignored-count=$TOTAL_IGNORED" >> $GITHUB_OUTPUT echo "link-details<> $GITHUB_OUTPUT echo -e "$LINK_DETAILS" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT @@ -211,7 +245,11 @@ runs: # Create detailed report for artifacts/issues DETAILED_REPORT="## Link Check Summary\n\n" DETAILED_REPORT="$DETAILED_REPORT- **Total broken links**: $TOTAL_BROKEN\n" - DETAILED_REPORT="$DETAILED_REPORT- **Total redirects found**: $TOTAL_REDIRECTS\n\n" + DETAILED_REPORT="$DETAILED_REPORT- **Total redirects found**: $TOTAL_REDIRECTS\n" + if [ "$TOTAL_IGNORED" -gt 0 ]; then + DETAILED_REPORT="$DETAILED_REPORT- **Links skipped by ignore-patterns**: $TOTAL_IGNORED\n" + fi + DETAILED_REPORT="$DETAILED_REPORT\n" if [ "$TOTAL_BROKEN" -gt 0 ]; then DETAILED_REPORT="$DETAILED_REPORT## Broken Links\n$LINK_DETAILS\n\n" @@ -266,7 +304,7 @@ runs: '3. Consider applying AI suggestions for better alternatives', '4. Push the changes to update this PR', '', - '📝 *This comment was automatically generated by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/meta/.github/actions/link-checker).*' + '📝 *This comment was automatically generated by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker).*' ].join('\n'); try { @@ -314,7 +352,7 @@ runs: echo "" echo "---" echo "" - echo "Generated by [AI-Powered Link Checker Action](https://github.com/QuantEcon/meta/.github/actions/link-checker)" + echo "Generated by [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker)" } > "$ARTIFACT_FILE" echo "artifact-path=$ARTIFACT_FILE" >> $GITHUB_OUTPUT @@ -339,7 +377,8 @@ runs: const detailedReport = ${{ toJSON(steps.check.outputs.detailed-report) }}; const title = '${{ inputs.issue-title }}'; const notify = '${{ inputs.notify }}'; - + const updateExisting = '${{ inputs.update-existing-issue }}' === 'true'; + const body = [ '# Link Check Report', '', @@ -365,25 +404,58 @@ runs: '3. Consider applying AI suggestions for better alternatives', '4. Re-run the link check to verify fixes', '', - '**Note:** This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/meta/.github/actions/link-checker).', + '**Note:** This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker). While it stays open, later runs refresh this body in place rather than opening a new issue, so the report above always reflects the most recent run.', '', 'Please close this issue once all broken links have been addressed.' ].join('\n'); try { - const response = await github.rest.issues.create({ - owner: context.repo.owner, - repo: context.repo.repo, - title: title, - body: body, - labels: ['bug', 'documentation', 'broken-links'] - }); - + // Reuse the newest open issue with this exact title rather than + // opening a duplicate. A weekly cron on a recurring finding would + // otherwise accumulate one issue per run, which buries the real + // findings among the repeats. + let existing = null; + if (updateExisting) { + const open = await github.paginate(github.rest.issues.listForRepo, { + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + labels: 'broken-links', + per_page: 100 + }); + existing = open + .filter(issue => !issue.pull_request && issue.title === title) + .sort((a, b) => b.number - a.number)[0] || null; + } + + let response; + if (existing) { + response = await github.rest.issues.update({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: existing.number, + body: body + }); + console.log('Updated existing issue #' + existing.number + + ' instead of opening a duplicate'); + } else { + response = await github.rest.issues.create({ + owner: context.repo.owner, + repo: context.repo.repo, + title: title, + body: body, + labels: ['bug', 'documentation', 'broken-links'] + }); + } + const issueUrl = response.data.html_url; const issueNumber = response.data.number; - console.log('Created issue: ' + issueUrl); + if (!existing) { + console.log('Created issue: ' + issueUrl); + } core.setOutput('issue-url', issueUrl); - + core.setOutput('issue-updated', existing ? 'true' : 'false'); + // Assign users to the issue if notify parameter is provided if (notify && notify.trim()) { try { @@ -427,7 +499,7 @@ runs: '', `For detailed analysis and AI-powered suggestions, please check ${issueUrl}`, '', - 'Note: This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/meta/.github/actions/link-checker).' + 'Note: This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker).' ].join('\n'); try { diff --git a/link_checker.py b/link_checker.py index b8b7b3b..006c15d 100644 --- a/link_checker.py +++ b/link_checker.py @@ -36,10 +36,16 @@ def is_likely_bot_blocked(url, response_content=None, status_code=None, error=No if indicator in url.lower(): return True - # Check if it's a legitimate domain that might be blocked by network restrictions - for domain in legitimate_domains: - if domain in url.lower() and error and 'Connection Error' in str(error): - return True + # Check if it's a legitimate domain that might be blocked by network + # restrictions. A timeout and a connection error are two symptoms of the + # same cause -- a host that will not answer a datacenter IP -- so both are + # treated alike here. Matching only 'Connection Error' meant a listed + # domain was protected against one and reported broken on the other. + network_errors = ('connection error', 'timeout') + if error and any(err in str(error).lower() for err in network_errors): + for domain in legitimate_domains: + if domain in url.lower(): + return True # Check for encoding issues which often indicate bot blocking if error and 'encoding' in str(error).lower(): @@ -52,6 +58,39 @@ def is_likely_bot_blocked(url, response_content=None, status_code=None, error=No return False +def compile_ignore_patterns(raw_patterns): + """Compile ignore patterns, skipping blanks, comments and invalid regexes""" + compiled = [] + for pattern in raw_patterns: + pattern = pattern.strip() + if not pattern or pattern.startswith('#'): + continue + try: + compiled.append(re.compile(pattern)) + except re.error as e: + print(f"Warning: skipping invalid ignore pattern {pattern!r}: {e}", + file=sys.stderr) + return compiled + +def is_ignored(url, ignore_patterns): + """Check whether a URL matches any ignore pattern""" + return any(pattern.search(url) for pattern in ignore_patterns) + +def network_failure_result(url, error, silent_codes): + """Build a result for a request that never returned a status code. + + Status 0 is what this checker reports when a request fails before the + server answers. Honouring 0 in silent-codes lets a project silence + unreachable hosts without also silencing genuine 404s. + """ + likely_blocked = is_likely_bot_blocked(url, error=error) + silent = likely_blocked or 0 in silent_codes + return { + 'url': url, 'status_code': 0, 'final_url': url, + 'redirect_count': 0, 'redirected': False, 'broken': not silent, + 'silent': silent, 'error': error, 'likely_bot_blocked': likely_blocked + } + def check_link(url, timeout, max_redirects, silent_codes): """Check a single link and return status info""" # Use a more browser-like user agent to reduce blocking @@ -104,20 +143,10 @@ def check_link(url, timeout, max_redirects, silent_codes): except requests.exceptions.Timeout: # Check if timeout on a likely legitimate site - likely_blocked = is_likely_bot_blocked(url, error='timeout') - return { - 'url': url, 'status_code': 0, 'final_url': url, - 'redirect_count': 0, 'redirected': False, 'broken': not likely_blocked, - 'silent': likely_blocked, 'error': 'Timeout', 'likely_bot_blocked': likely_blocked - } + return network_failure_result(url, 'Timeout', silent_codes) except requests.exceptions.ConnectionError as e: - # Check if connection error on a likely legitimate site - likely_blocked = is_likely_bot_blocked(url, error='Connection Error') - return { - 'url': url, 'status_code': 0, 'final_url': url, - 'redirect_count': 0, 'redirected': False, 'broken': not likely_blocked, - 'silent': likely_blocked, 'error': 'Connection Error', 'likely_bot_blocked': likely_blocked - } + # Check if connection error on a likely legitimate site + return network_failure_result(url, 'Connection Error', silent_codes) except UnicodeDecodeError as e: # Encoding issues often indicate bot blocking return { @@ -127,12 +156,7 @@ def check_link(url, timeout, max_redirects, silent_codes): } except Exception as e: # Check if the error suggests bot blocking - likely_blocked = is_likely_bot_blocked(url, error=str(e)) - return { - 'url': url, 'status_code': 0, 'final_url': url, - 'redirect_count': 0, 'redirected': False, 'broken': not likely_blocked, - 'silent': likely_blocked, 'error': str(e), 'likely_bot_blocked': likely_blocked - } + return network_failure_result(url, str(e), silent_codes) def extract_links_from_html(file_path): """Extract all external links from HTML file""" @@ -265,32 +289,58 @@ def main(): parser.add_argument('--max-redirects', type=int, default=5, help='Maximum redirects') parser.add_argument('--silent-codes', default='403,503', help='Silent status codes') parser.add_argument('--ai-suggestions', action='store_true', help='Enable AI suggestions') - + parser.add_argument('--ignore-patterns', default='', + help='Regex patterns for URLs to skip entirely, one per line') + parser.add_argument('--ignore-patterns-file', default='', + help='Path to a file of regex patterns to skip, one per line') + args = parser.parse_args() - + silent_codes = [int(x.strip()) for x in args.silent_codes.split(',') if x.strip()] - + + # Patterns are newline-separated rather than comma-separated so that regex + # quantifiers such as {1,3} survive unsplit. + raw_ignore_patterns = args.ignore_patterns.splitlines() + if args.ignore_patterns_file: + try: + with open(args.ignore_patterns_file, 'r', encoding='utf-8') as f: + raw_ignore_patterns.extend(f.read().splitlines()) + except OSError as e: + print(f"Warning: could not read ignore patterns file " + f"{args.ignore_patterns_file!r}: {e}", file=sys.stderr) + ignore_patterns = compile_ignore_patterns(raw_ignore_patterns) + # Extract links links = extract_links_from_html(args.file_path) if not links: print(json.dumps({ - 'broken_results': [], 'redirect_results': [], + 'broken_results': [], 'redirect_results': [], 'ignored_results': [], 'ai_suggestions': [], 'total_links': 0 })) return - + broken_results = [] redirect_results = [] - + ignored_results = [] + print(f"Checking {len(links)} links in {args.file_path} (timeout: {args.timeout}s)...", file=sys.stderr) - + # Check each link for i, link_info in enumerate(links): url = link_info['url'] + + # Ignored URLs are never requested, so they can be neither broken nor + # redirected -- an explicit exemption, not a suppressed finding. + if is_ignored(url, ignore_patterns): + ignored_results.append({ + 'url': url, 'file': args.file_path, 'text': link_info['text'] + }) + continue + result = check_link(url, args.timeout, args.max_redirects, silent_codes) result['file'] = args.file_path result['text'] = link_info['text'] - + if result['broken'] and not result['silent']: broken_results.append(result) elif result['redirected']: @@ -305,10 +355,15 @@ def main(): if args.ai_suggestions: ai_suggestions = generate_ai_suggestions(broken_results, redirect_results) + if ignored_results: + print(f"Skipped {len(ignored_results)} link(s) matching ignore patterns", + file=sys.stderr) + # Output results print(json.dumps({ 'broken_results': broken_results, - 'redirect_results': redirect_results, + 'redirect_results': redirect_results, + 'ignored_results': ignored_results, 'ai_suggestions': ai_suggestions, 'total_links': len(links) })) diff --git a/tests/test_bot_blocking.py b/tests/test_bot_blocking.py index 77ea968..53a698b 100644 --- a/tests/test_bot_blocking.py +++ b/tests/test_bot_blocking.py @@ -4,7 +4,7 @@ """ import sys import os -sys.path.insert(0, os.path.join(os.path.dirname(__file__), '../../.github/actions/link-checker')) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) from link_checker import is_likely_bot_blocked diff --git a/tests/test_modules.py b/tests/test_modules.py index 7fbce1d..6b4f5f4 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -27,6 +27,62 @@ def test_module_imports(self): self.assertTrue(hasattr(link_checker, 'main')) self.assertTrue(hasattr(format_results, 'main')) + def test_ignore_patterns_compile_and_match(self): + """Ignore patterns skip matching URLs and leave others alone""" + patterns = link_checker.compile_ignore_patterns([ + 'https://fred\\.stlouisfed\\.org/.*', + '', # blank lines are skipped + ' # a comment', # comments are skipped + ]) + self.assertEqual(len(patterns), 1) + self.assertTrue(link_checker.is_ignored('https://fred.stlouisfed.org/', patterns)) + self.assertTrue(link_checker.is_ignored('https://fred.stlouisfed.org/series/UNRATE', patterns)) + self.assertFalse(link_checker.is_ignored('https://example.com/', patterns)) + + def test_ignore_patterns_bare_domain_matches_as_substring(self): + """A bare domain works without regex anchoring""" + patterns = link_checker.compile_ignore_patterns(['fred.stlouisfed.org']) + self.assertTrue(link_checker.is_ignored('https://fred.stlouisfed.org/series/UNRATE', patterns)) + + def test_invalid_ignore_pattern_is_skipped_not_fatal(self): + """An unparseable pattern is dropped rather than crashing the run""" + patterns = link_checker.compile_ignore_patterns(['[unclosed', 'example\\.com']) + self.assertEqual(len(patterns), 1) + self.assertTrue(link_checker.is_ignored('https://example.com/', patterns)) + + def test_empty_ignore_patterns_ignore_nothing(self): + """The default of no patterns must not skip any URL""" + patterns = link_checker.compile_ignore_patterns([]) + self.assertFalse(link_checker.is_ignored('https://example.com/', patterns)) + + def test_legitimate_domain_protected_on_timeout(self): + """A listed domain is protected on timeout, not only connection error + + Regression test: the legitimate_domains branch previously required the + literal string 'Connection Error', so it could never fire on a timeout. + """ + for error in ('Connection Error', 'timeout'): + with self.subTest(error=error): + self.assertTrue( + link_checker.is_likely_bot_blocked('https://github.com/x', error=error)) + + def test_unlisted_domain_not_protected_on_timeout(self): + """Widening the branch must not silence unknown hosts""" + self.assertFalse( + link_checker.is_likely_bot_blocked('https://unknown-domain.example/', error='timeout')) + + def test_status_zero_can_be_silenced_via_silent_codes(self): + """silent-codes reaches the network-failure path, where 0 is reported""" + loud = link_checker.network_failure_result( + 'https://unknown-domain.example/', 'Timeout', [403, 503]) + self.assertTrue(loud['broken']) + self.assertFalse(loud['silent']) + + quiet = link_checker.network_failure_result( + 'https://unknown-domain.example/', 'Timeout', [0, 403, 503]) + self.assertFalse(quiet['broken']) + self.assertTrue(quiet['silent']) + def test_link_checker_with_test_files(self): """Test link checker with actual test HTML files""" test_dir = Path(__file__).parent @@ -48,10 +104,15 @@ def test_link_checker_with_test_files(self): def main(): print("🧪 Running Link Checker Tests") print("=" * 40) - - # Run basic module tests - unittest.main(argv=[''], exit=False, verbosity=2) - + + # Run basic module tests. exit=False keeps the trailing message, so the + # result has to be inspected explicitly or CI would pass on a failure. + result = unittest.main(argv=[''], exit=False, verbosity=2).result + + if not result.wasSuccessful(): + print("\n❌ Tests failed") + sys.exit(1) + print("\n✅ All tests completed successfully!") if __name__ == "__main__": From 1b1943a1b357a1254636ca0e9e7ec7dbeea2bf72 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 3 Aug 2026 15:36:32 +1000 Subject: [PATCH 2/2] Address review: fail-quiet paths, invisible diagnostics, test coverage Follow-up on the review of this branch. Six defects, all of which made the action quieter than it should be about problems it had found. silent-codes '0' silenced too much. network_failure_result is also the return path for the catch-all `except Exception`, so a malformed href, a redirect loop or a bug in the checker all reported status 0 and were suppressed alongside the unreachable hosts the option is meant for. Status 0 silencing and the legitimate_domains allowance are now gated on which handler caught the failure rather than on sniffing 'timeout' out of an arbitrary exception message -- a misconfigured `timeout: '0'` makes urllib3 raise a ValueError whose message says 'timeout', which would otherwise have silenced every link on a listed domain. ChunkedEncodingError gets its own handler so that a response broken mid-stream stays silenceable; it is neither a Timeout nor a ConnectionError but it is still a transport failure. compile_ignore_patterns caught only re.error, but re.compile raises OverflowError on an oversized repetition count and RecursionError on deep nesting. Either aborted the whole step, contradicting the README's promise that an invalid pattern is skipped rather than fatal. None of the checker's diagnostics could reach a job log. stderr went to a file that was only displayed inside `if [ $? -ne 0 ]`, which is dead code under the composite shell's `set -e` -- the assignment aborts the step first. The status is now captured explicitly, stderr is always surfaced, and a crash fails loudly instead of continuing to a summary that would report the remaining files as clean. The temp file is removed by an EXIT trap, so the HTML-path-missing exit no longer leaks it. GITHUB_OUTPUT heredocs use a per-run delimiter, since link text containing a line reading exactly EOF truncated the output file. The generated issue body no longer promises to refresh itself in place when update-existing-issue is false. tests/test_bot_blocking.py had no assertions -- it printed PASS/FAIL and exited 0 either way -- and CI never ran it. Rewritten as unittest and wired in. test_modules.py grows to 17 tests covering the exception widening, the network_failure gate, malformed links staying loud, and the --ignore-patterns-file path end to end. CI now asserts ignored-count and broken-link-count in both directions against a generated fixture; previously the plumbing had no assertion at all and crossing the two counters left every check green. Also folds the CHANGELOG's non-standard headings into the existing 1.0.0 section, corrects the status 0 and Sphinx linkcheck_ignore wording, and repoints the nine stale QuantEcon/meta references in examples.md that the previous commit left behind. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 63 ++++++++++++++++- CHANGELOG.md | 38 +++++----- README.md | 12 ++-- action.yml | 56 ++++++++++----- examples.md | 18 ++--- link_checker.py | 59 ++++++++++++---- tests/README.md | 12 ++-- tests/test_bot_blocking.py | 130 +++++++++++++++++----------------- tests/test_modules.py | 140 +++++++++++++++++++++++++++++++++++-- 9 files changed, 388 insertions(+), 140 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 634da6d..fc9960a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,6 +36,11 @@ jobs: cd tests python test_modules.py + - name: Run bot-blocking tests + run: | + cd tests + python test_bot_blocking.py + test-action: runs-on: ubuntu-latest name: Test Link Checker Action @@ -66,13 +71,67 @@ jobs: fail-on-broken: 'true' - name: Test with silent codes for broken links - continue-on-error: true + continue-on-error: true uses: ./ with: html-path: 'tests/broken-links.html' fail-on-broken: 'true' silent-codes: '404,500' # Treat these as silent - + + # The ignore-patterns fixture is built here rather than kept in tests/ so + # that the counts below are exact and the other jobs keep scanning the + # same files they always have. Every host is under .invalid, which is + # reserved and never resolves, so the ignored case makes no requests at + # all and the un-ignored case fails fast. + - name: Create ignore-patterns fixture + run: | + mkdir -p ci-fixtures + cat > ci-fixtures/ignore-patterns.html <<'HTML' + + FRED + UNRATE + Blocked + + HTML + + - name: Test ignore-patterns skips matching links + id: test-ignored + uses: ./ + with: + html-path: 'ci-fixtures' + fail-on-broken: 'true' + ai-suggestions: 'false' + timeout: 10 + ignore-patterns: | + https://fred\.stlouisfed\.org\.invalid/.* + # comments and blank lines are skipped + + blocked\.example\.invalid + + - name: Test ignore-patterns off reports the same links + id: test-not-ignored + uses: ./ + with: + html-path: 'ci-fixtures' + fail-on-broken: 'false' + ai-suggestions: 'false' + timeout: 10 + + - name: Verify ignore-patterns counts + run: | + assert() { + if [ "$2" != "$3" ]; then + echo "::error::$1: expected '$3', got '$2'" + exit 1 + fi + echo "ok: $1 = $2" + } + assert "ignored-count with patterns" "${{ steps.test-ignored.outputs.ignored-count }}" "3" + assert "broken-link-count with patterns" "${{ steps.test-ignored.outputs.broken-link-count }}" "0" + assert "broken-links-found with patterns" "${{ steps.test-ignored.outputs.broken-links-found }}" "false" + assert "ignored-count without patterns" "${{ steps.test-not-ignored.outputs.ignored-count }}" "0" + assert "broken-link-count without patterns" "${{ steps.test-not-ignored.outputs.broken-link-count }}" "3" + test-empty-directory: runs-on: ubuntu-latest name: Test Empty Directory diff --git a/CHANGELOG.md b/CHANGELOG.md index 147f696..0ad5933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,19 +10,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - `ignore-patterns` input: newline-separated regular expressions for URLs to skip entirely. Matching URLs are never requested, so they can be reported as neither broken nor redirected, and are counted separately via the new `ignored-count` output - `update-existing-issue` input (default `true`): reuse the newest open issue with the same title and `broken-links` label, refreshing its body, instead of opening a duplicate on every run. New `issue-updated` output reports which path was taken -- Status `0` — a request that failed before the server answered — can now be listed in `silent-codes` +- Status `0` — a request that never completed — can now be listed in `silent-codes`. It applies to transport failures only (timeout, connection failure, a response broken mid-stream), not to malformed links or redirect loops + +### Changed +- **Behaviour change:** with `create-issue: 'true'`, a recurring finding now refreshes one open issue rather than opening a new one per run. Set `update-existing-issue: 'false'` to restore the previous behaviour +- A crash in `link_checker.py` now fails the action with the checker's stderr in the job log, instead of aborting the step with no diagnostic +- `$GITHUB_OUTPUT` heredocs use a per-run delimiter, so scanned link text containing a line reading `EOF` can no longer truncate an output ### Fixed -- The `legitimate_domains` allowance in bot-blocking detection required the error string `Connection Error`, but the timeout handler passes `timeout`. A listed domain was therefore protected against connection errors and reported broken on timeouts. Both are now treated alike +- The `legitimate_domains` allowance in bot-blocking detection required the error string `Connection Error`, but the timeout handler passes `timeout`. A listed domain was therefore protected against connection errors and reported broken on timeouts. Both are now treated alike, keyed on which handler caught the failure rather than on the error text - `silent-codes` was only consulted on responses that returned a status code, so it could never apply to timeouts or connection errors +- `compile_ignore_patterns` caught only `re.error`, so a pattern raising `OverflowError` (an oversized repetition count) or `RecursionError` aborted the whole run instead of being skipped as documented +- The checker's stderr was written to a file that was never displayed, so its warnings — including a skipped ignore pattern — could not reach the job log - The test runner called `unittest.main(exit=False)` without inspecting the result, so the CI test job reported success even when tests failed -- Corrected the action's self-referencing links in issue bodies, PR comments and artifacts, which still pointed at the pre-migration `QuantEcon/meta` path +- `tests/test_bot_blocking.py` used a pre-migration `sys.path`, so it could not import the module under test; it now asserts rather than printing, and CI runs it +- Corrected the action's self-referencing links in issue bodies, PR comments, artifacts and `examples.md`, which still pointed at the pre-migration `QuantEcon/meta` path +- The generated issue body claimed later runs would refresh it in place even when `update-existing-issue` was `false` +- The temporary ignore-patterns file is removed via an `EXIT` trap, so it is not left behind when the step exits early -### Changed -- **Behaviour change:** with `create-issue: 'true'`, a recurring finding now refreshes one open issue rather than opening a new one per run. Set `update-existing-issue: 'false'` to restore the previous behaviour +## [1.0.0] - 2025-10-01 -### Previously unreleased -- Initial release of the AI-Powered Link Checker action +### Added +- Initial stable release migrated from QuantEcon/meta repository - Smart link validation with configurable timeouts - AI-powered suggestions for broken and redirected links - Bot-blocking detection and handling @@ -31,21 +40,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - GitHub issue creation with detailed reports - Workflow artifact generation with link analysis - MyST Markdown and Jupyter Book compatibility - -### Features -- Enhanced robustness compared to traditional link checkers -- Respectful rate limiting and improved timeout handling - Redirect detection and improvement suggestions - Comprehensive JSON output with detailed link information - Integration with GitHub Issues API for automated reporting -- Performance optimizations for large documentation sites - -## [1.0.0] - 2025-10-01 - -### Added -- Initial stable release migrated from QuantEcon/meta repository +- Respectful rate limiting and improved timeout handling - Full compatibility with existing workflows -- Enhanced documentation and examples +- Enhanced documentation and examples - Comprehensive test suite with Python module testing - GitHub Marketplace listing -- Python requirements management with requirements.txt \ No newline at end of file +- Python requirements management with requirements.txt diff --git a/README.md b/README.md index eeeda70..72bde66 100644 --- a/README.md +++ b/README.md @@ -128,7 +128,7 @@ The action includes intelligent logic to reduce false positives for legitimate s Some hosts throttle or block datacenter IP ranges outright, so they time out from a CI runner while working perfectly for readers. The automatic detection below catches many of these, but it cannot know which hosts a particular project depends on. Use `ignore-patterns` to declare them: ```yaml -- uses: QuantEcon/action-link-checker@main +- uses: QuantEcon/action-link-checker@v1 with: html-path: '_site' ignore-patterns: | @@ -139,9 +139,9 @@ Some hosts throttle or block datacenter IP ranges outright, so they time out fro Patterns are Python regular expressions, one per line — newline-separated rather than comma-separated so that quantifiers such as `{1,3}` survive intact. Each is matched against the full URL with `re.search`, so a bare domain works as a substring without anchoring. -Ignored URLs are never requested at all. They therefore cannot be reported as broken *or* as redirects, and they are counted separately in the report and exposed as the `ignored-count` output. An invalid pattern is logged and skipped rather than failing the run. +Ignored URLs are never requested at all. They therefore cannot be reported as broken *or* as redirects, and they are counted separately in the report and exposed as the `ignored-count` output. An invalid pattern is logged to the job log and skipped rather than failing the run. -If a project already maintains a Sphinx `linkcheck_ignore` list, those patterns can be pasted here directly — the syntax is the same. +If a project already maintains a Sphinx `linkcheck_ignore` list, those patterns can be pasted here directly. Both take Python regular expressions, so the syntax carries over unchanged; note that Sphinx anchors its patterns at the start of the URL while this action matches anywhere in it, so a pattern here may match slightly more than the same pattern does under Sphinx. ### Bot Blocking Detection - **Major Sites**: Automatically detects common sites that block automated requests (Netflix, Amazon, Facebook, etc.) @@ -222,7 +222,7 @@ Common codes to consider: - `503`: Service Unavailable (temporary outages) - `429`: Too Many Requests (rate limiting) - `502`: Bad Gateway (temporary server issues) -- `0`: No response at all — the request failed before the server answered (timeout, DNS failure, connection refused). Silencing `0` suppresses every unreachable host, so prefer `ignore-patterns` when only specific hosts are affected. +- `0`: The request never completed — it timed out, the connection failed (DNS failure, connection refused), or the server broke the response mid-stream. Silencing `0` suppresses every unreachable host, so prefer `ignore-patterns` when only specific hosts are affected. It applies to transport failures only: a malformed link such as `https://` and a redirect loop also report status `0`, and those stay reported however `silent-codes` is set, since they are the project's own to fix. ### Recurring Reports @@ -230,6 +230,10 @@ When `create-issue` is enabled on a schedule, the action reuses the newest open Close the issue once the links are fixed; if the finding recurs afterwards, a fresh issue is opened. To restore the previous behaviour of always opening a new issue, set `update-existing-issue: 'false'`. +Two things follow from matching on the title and the label. Give each workflow its own `issue-title` if a repository runs more than one link check, or they will overwrite each other's report. And leave the `broken-links` label in place — if it is removed from the tracking issue, later runs stop finding it and start opening duplicates again. + +Note that GitHub does not send notifications for an edit to an issue body, so a refreshed report is quiet by design. Watch the scheduled workflow itself if you want to be told about every run. + ### Performance Tuning ```yaml diff --git a/action.yml b/action.yml index 173ef8f..afbe7d0 100644 --- a/action.yml +++ b/action.yml @@ -123,8 +123,11 @@ runs: echo "AI suggestions enabled: $AI_SUGGESTIONS" # Write ignore patterns to a file so the checker never has to parse - # them out of an argument list + # them out of an argument list. The trap is installed with the file + # so that cleanup survives the early exits below and any set -e abort. IGNORE_FILE="$(mktemp)" + STDERR_LOG="$(mktemp)" + trap 'rm -f "$IGNORE_FILE" "$STDERR_LOG"' EXIT printf '%s\n' "$IGNORE_PATTERNS" > "$IGNORE_FILE" IGNORE_COUNT=$(grep -cve '^[[:space:]]*$' -e '^[[:space:]]*#' "$IGNORE_FILE" || true) if [ "$IGNORE_COUNT" -gt 0 ]; then @@ -178,20 +181,31 @@ runs: AI_FLAG="--ai-suggestions" fi - # Run link checker and capture JSON output + # Run link checker and capture JSON output. The exit status is + # captured explicitly: this step runs under `set -e`, where a bare + # assignment aborts before any `if [ $? -ne 0 ]` guard could run. + rc=0 result_json=$(python3 "$ACTION_DIR/link_checker.py" "$file" \ --timeout "$TIMEOUT" \ --max-redirects "$MAX_REDIRECTS" \ --silent-codes "$SILENT_CODES" \ --ignore-patterns-file "$IGNORE_FILE" \ - $AI_FLAG 2>/tmp/stderr.log) - - if [ $? -ne 0 ] || [ -z "$result_json" ]; then - echo "Warning: Failed to process $file" - cat /tmp/stderr.log >&2 - continue + $AI_FLAG 2>"$STDERR_LOG") || rc=$? + + # The checker reports progress and warnings -- a skipped ignore + # pattern, an unreadable file -- on stderr. Surface them, or they + # never reach the job log. + if [ -s "$STDERR_LOG" ]; then + cat "$STDERR_LOG" >&2 fi - + + # A crashed checker is a hard failure, not a file to skip: carrying + # on would report the remaining files as clean and exit green. + if [ "$rc" -ne 0 ] || [ -z "$result_json" ]; then + echo "::error::link_checker.py failed on $file (exit $rc)" + exit 1 + fi + # Parse results and update counters broken_count=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(len(data['broken_results']))") redirect_count=$(echo "$result_json" | python3 -c "import json, sys; data=json.load(sys.stdin); print(len(data['redirect_results']))") @@ -228,20 +242,24 @@ runs: echo " Found $total_links total links, $broken_count broken, $redirect_count redirected, $ignored_count ignored" done - rm -f "$IGNORE_FILE" + # Multi-line outputs carry scanned link text verbatim. A fixed EOF + # delimiter is terminated early by an anchor whose text contains a + # line reading exactly EOF, which corrupts the whole output file, so + # the delimiter is generated per run instead. + DELIM="ghadelim_${RANDOM}${RANDOM}${RANDOM}" # Set outputs echo "broken-links-found=$BROKEN_LINKS_FOUND" >> $GITHUB_OUTPUT echo "broken-link-count=$TOTAL_BROKEN" >> $GITHUB_OUTPUT echo "redirect-count=$TOTAL_REDIRECTS" >> $GITHUB_OUTPUT echo "ignored-count=$TOTAL_IGNORED" >> $GITHUB_OUTPUT - echo "link-details<> $GITHUB_OUTPUT + echo "link-details<<$DELIM" >> $GITHUB_OUTPUT echo -e "$LINK_DETAILS" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT - echo "ai-suggestions<> $GITHUB_OUTPUT + echo "$DELIM" >> $GITHUB_OUTPUT + echo "ai-suggestions<<$DELIM" >> $GITHUB_OUTPUT echo -e "$AI_SUGGESTIONS_OUTPUT" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT - + echo "$DELIM" >> $GITHUB_OUTPUT + # Create detailed report for artifacts/issues DETAILED_REPORT="## Link Check Summary\n\n" DETAILED_REPORT="$DETAILED_REPORT- **Total broken links**: $TOTAL_BROKEN\n" @@ -259,9 +277,9 @@ runs: DETAILED_REPORT="$DETAILED_REPORT## AI-Powered Suggestions\n$AI_SUGGESTIONS_OUTPUT\n\n" fi - echo "detailed-report<> $GITHUB_OUTPUT + echo "detailed-report<<$DELIM" >> $GITHUB_OUTPUT echo -e "$DETAILED_REPORT" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + echo "$DELIM" >> $GITHUB_OUTPUT # Summary if [ "$BROKEN_LINKS_FOUND" = "true" ]; then @@ -404,7 +422,9 @@ runs: '3. Consider applying AI suggestions for better alternatives', '4. Re-run the link check to verify fixes', '', - '**Note:** This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker). While it stays open, later runs refresh this body in place rather than opening a new issue, so the report above always reflects the most recent run.', + '**Note:** This issue was automatically created by the [AI-Powered Link Checker Action](https://github.com/QuantEcon/action-link-checker). ' + (updateExisting + ? 'While it stays open, later runs refresh this body in place rather than opening a new issue, so the report above always reflects the most recent run.' + : 'This workflow sets `update-existing-issue: false`, so each run opens a new issue rather than refreshing this one.'), '', 'Please close this issue once all broken links have been addressed.' ].join('\n'); diff --git a/examples.md b/examples.md index 7a56d33..b021b90 100644 --- a/examples.md +++ b/examples.md @@ -30,7 +30,7 @@ jobs: ref: gh-pages - name: AI-Powered Link Check - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: '.' mode: 'full' @@ -83,7 +83,7 @@ jobs: jupyter-book build lectures/ - name: Check links in changed files - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: './lectures/_build/html' mode: 'changed' # Only check files changed in this PR @@ -132,7 +132,7 @@ jobs: jupyter-book build . - name: AI-Powered Link Check - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: './_build/html' mode: ${{ github.event_name == 'pull_request' && 'changed' || 'full' }} @@ -174,7 +174,7 @@ jobs: ref: ${{ matrix.project.ref }} - name: Link Check - ${{ matrix.project.name }} - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: '.' fail-on-broken: 'false' @@ -190,7 +190,7 @@ For projects with many external links or slow-responding sites: ```yaml - name: Patient Link Checker - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: './_build/html' timeout: '60' # 60 seconds per link @@ -208,7 +208,7 @@ For debugging and development of documentation: ```yaml - name: Development Link Check - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: './_build/html' fail-on-broken: 'false' # Don't fail during development @@ -248,7 +248,7 @@ jobs: fail-on-warning: 'true' - name: Check for broken links - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: './_build/html' mode: 'changed' @@ -276,7 +276,7 @@ jobs: ref: gh-pages - name: Silent Link Check - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: '.' fail-on-broken: 'false' # Never fail @@ -309,7 +309,7 @@ jobs: ### After (using AI-powered link checker): ```yaml - name: AI-Powered Link Checker - uses: QuantEcon/meta/.github/actions/link-checker@main + uses: QuantEcon/action-link-checker@v1 with: html-path: '.' fail-on-broken: 'false' diff --git a/link_checker.py b/link_checker.py index 006c15d..b6dd245 100644 --- a/link_checker.py +++ b/link_checker.py @@ -17,8 +17,16 @@ def is_external_link(url): """Check if URL is external (starts with http/https)""" return url.startswith(('http://', 'https://')) -def is_likely_bot_blocked(url, response_content=None, status_code=None, error=None): - """Detect if a site is likely blocking automated requests rather than being truly broken""" +def is_likely_bot_blocked(url, response_content=None, status_code=None, error=None, + network_failure=False): + """Detect if a site is likely blocking automated requests rather than being truly broken + + ``network_failure`` says the request never reached the server -- a + timeout or a connection error. Only the caller knows that, so it is + passed in rather than sniffed out of the error string: an arbitrary + exception message can contain the word 'timeout' without the request + having failed in transit. + """ domain_indicators = [ 'netflix.com', 'amazon.com', 'facebook.com', 'twitter.com', 'instagram.com', 'youtube.com', 'linkedin.com', 'pinterest.com', 'reddit.com', 'wikipedia.org' @@ -39,10 +47,11 @@ def is_likely_bot_blocked(url, response_content=None, status_code=None, error=No # Check if it's a legitimate domain that might be blocked by network # restrictions. A timeout and a connection error are two symptoms of the # same cause -- a host that will not answer a datacenter IP -- so both are - # treated alike here. Matching only 'Connection Error' meant a listed - # domain was protected against one and reported broken on the other. - network_errors = ('connection error', 'timeout') - if error and any(err in str(error).lower() for err in network_errors): + # treated alike here. Keying this on the error string meant a listed + # domain was protected against 'Connection Error' and reported broken on + # a timeout; keying it on which handler caught the exception protects + # both without also matching an unrelated message that says 'timeout'. + if network_failure: for domain in legitimate_domains: if domain in url.lower(): return True @@ -67,24 +76,34 @@ def compile_ignore_patterns(raw_patterns): continue try: compiled.append(re.compile(pattern)) - except re.error as e: - print(f"Warning: skipping invalid ignore pattern {pattern!r}: {e}", - file=sys.stderr) + except Exception as e: + # Deliberately broader than re.error: re.compile also raises + # OverflowError on an oversized repetition count and + # RecursionError on deep nesting, and a pattern the user typed + # must never be able to abort the run. + print(f"Warning: skipping invalid ignore pattern {pattern!r}: " + f"{type(e).__name__}: {e}", file=sys.stderr) return compiled def is_ignored(url, ignore_patterns): """Check whether a URL matches any ignore pattern""" return any(pattern.search(url) for pattern in ignore_patterns) -def network_failure_result(url, error, silent_codes): +def network_failure_result(url, error, silent_codes, network_failure=False): """Build a result for a request that never returned a status code. Status 0 is what this checker reports when a request fails before the server answers. Honouring 0 in silent-codes lets a project silence unreachable hosts without also silencing genuine 404s. + + ``network_failure`` is set only by the timeout and connection-error + handlers. A malformed href, a redirect loop and a bug in this script all + surface as status 0 too, and silencing an unreachable host must not + silence those as well -- they are the project's own mistakes to fix. """ - likely_blocked = is_likely_bot_blocked(url, error=error) - silent = likely_blocked or 0 in silent_codes + likely_blocked = is_likely_bot_blocked(url, error=error, + network_failure=network_failure) + silent = likely_blocked or (network_failure and 0 in silent_codes) return { 'url': url, 'status_code': 0, 'final_url': url, 'redirect_count': 0, 'redirected': False, 'broken': not silent, @@ -143,10 +162,18 @@ def check_link(url, timeout, max_redirects, silent_codes): except requests.exceptions.Timeout: # Check if timeout on a likely legitimate site - return network_failure_result(url, 'Timeout', silent_codes) + return network_failure_result(url, 'Timeout', silent_codes, + network_failure=True) except requests.exceptions.ConnectionError as e: # Check if connection error on a likely legitimate site - return network_failure_result(url, 'Connection Error', silent_codes) + return network_failure_result(url, 'Connection Error', silent_codes, + network_failure=True) + except requests.exceptions.ChunkedEncodingError as e: + # The server answered and then broke the body mid-stream. Neither a + # Timeout nor a ConnectionError, but a transport failure all the + # same, and not something the project can fix by editing the link. + return network_failure_result(url, f'Connection broken: {e}', silent_codes, + network_failure=True) except UnicodeDecodeError as e: # Encoding issues often indicate bot blocking return { @@ -155,7 +182,9 @@ def check_link(url, timeout, max_redirects, silent_codes): 'silent': True, 'error': f'Encoding issue: {str(e)}', 'likely_bot_blocked': True } except Exception as e: - # Check if the error suggests bot blocking + # Not a recognised transport failure: a malformed href, a redirect + # loop or a bug here. Reported loudly regardless of silent-codes, + # since the project can fix it. return network_failure_result(url, str(e), silent_codes) def extract_links_from_html(file_path): diff --git a/tests/README.md b/tests/README.md index 8f7adba..9710ec1 100644 --- a/tests/README.md +++ b/tests/README.md @@ -22,16 +22,16 @@ This directory contains test HTML files and scripts used to validate the link-ch - Tests timeout handling and retry logic ### test_bot_blocking.py -- Python script to test bot-blocking detection functionality -- Tests the action's ability to handle sites that block automated requests +- Unit tests for `is_likely_bot_blocked`, the detection that keeps a site which blocks automated requests from being reported broken +- Covers the major-site list, encoding and status-code signals, and the legitimate-domain allowance on transport failures ### test_modules.py - Unit tests for the Python modules (link_checker.py, format_results.py) -- Tests module imports and basic functionality +- Tests module imports, ignore-pattern compilation and matching, the network-failure result, and the `--ignore-patterns-file` path end to end ## Usage in CI -These files are automatically used by the GitHub Actions CI workflow to test: +The HTML fixtures are scanned by the `test-action` job, and both Python test files are run by the `test-python` job. Between them they cover: - Link validation accuracy - Error detection and reporting - Timeout and retry handling @@ -39,6 +39,8 @@ These files are automatically used by the GitHub Actions CI workflow to test: - AI-powered suggestions - Module functionality +The `ignore-patterns` assertions in CI use a fixture generated by the workflow rather than one kept here, so that the expected counts stay exact as fixtures are added to this directory. + ## Running Tests Locally You can test the action locally using these files: @@ -54,7 +56,7 @@ You can test the action locally using these files: ./action.yml --html-path tests --silent-codes "404,500" --fail-on-broken true # Run Python module tests -cd tests && python test_modules.py +cd tests && python test_modules.py && python test_bot_blocking.py ``` ## Test Requirements diff --git a/tests/test_bot_blocking.py b/tests/test_bot_blocking.py index 53a698b..5aa65dd 100644 --- a/tests/test_bot_blocking.py +++ b/tests/test_bot_blocking.py @@ -1,75 +1,79 @@ #!/usr/bin/env python3 """ -Test script to simulate bot blocking scenarios +Tests for the bot-blocking detection logic in link_checker.py """ import sys import os +import unittest + sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) from link_checker import is_likely_bot_blocked -def test_bot_blocking_detection(): - """Test the bot blocking detection logic""" - - # Test major site domains that commonly block bots - test_cases = [ - ("https://www.netflix.com/", True, "Netflix should be detected as likely bot-blocked"), - ("https://code.tutsplus.com/tutorial/something", False, "Tutsplus should not be automatically flagged"), - ("https://www.amazon.com/", True, "Amazon should be detected as likely bot-blocked"), - ("https://example.com/", False, "Example.com should not be flagged"), - ("https://github.com/user/repo", False, "GitHub should not be flagged as bot-blocked"), - ("https://www.wikipedia.org/wiki/Test", True, "Wikipedia should be detected as likely bot-blocked"), - ] - - print("Testing bot blocking detection logic:") - print("-" * 50) - - for url, expected, description in test_cases: - result = is_likely_bot_blocked(url) - status = "✅ PASS" if result == expected else "❌ FAIL" - print(f"{status}: {description}") - print(f" URL: {url}") - print(f" Expected: {expected}, Got: {result}") - print() - - # Test encoding error detection - print("Testing encoding error detection:") - print("-" * 50) - - encoding_cases = [ - ("https://www.netflix.com/", None, None, "encoding issue", True, "Encoding error should be detected"), - ("https://example.com/", None, None, "timeout", False, "Regular timeout should not be flagged"), - ("https://example.com/", None, 429, None, True, "Rate limiting should be detected"), - ("https://example.com/", None, 503, None, True, "Service unavailable should be detected"), - ] - - for url, content, status_code, error, expected, description in encoding_cases: - result = is_likely_bot_blocked(url, content, status_code, error) - status = "✅ PASS" if result == expected else "❌ FAIL" - print(f"{status}: {description}") - print(f" URL: {url}, Status: {status_code}, Error: {error}") - print(f" Expected: {expected}, Got: {result}") - print() - # Test legitimate domains with connection errors (simulating network restrictions) - print("Testing legitimate domain protection:") - print("-" * 50) - - legitimate_cases = [ - ("https://www.python.org/", None, None, "Connection Error", True, "Python.org with connection error should be protected"), - ("https://jupyter.org/", None, None, "Connection Error", True, "Jupyter.org with connection error should be protected"), - ("https://docs.python.org/3/", None, None, "Connection Error", True, "Python docs with connection error should be protected"), - ("https://github.com/user/repo", None, None, "Connection Error", True, "GitHub with connection error should be protected"), - ("https://unknown-domain.com/", None, None, "Connection Error", False, "Unknown domain with connection error should not be protected"), - ] - - for url, content, status_code, error, expected, description in legitimate_cases: - result = is_likely_bot_blocked(url, content, status_code, error) - status = "✅ PASS" if result == expected else "❌ FAIL" - print(f"{status}: {description}") - print(f" URL: {url}, Error: {error}") - print(f" Expected: {expected}, Got: {result}") - print() +class TestBotBlockingDetection(unittest.TestCase): + + def test_major_sites_that_block_bots(self): + """Sites known to block automated requests are never reported broken""" + cases = [ + ("https://www.netflix.com/", True), + ("https://www.amazon.com/", True), + ("https://www.wikipedia.org/wiki/Test", True), + ("https://code.tutsplus.com/tutorial/something", False), + ("https://example.com/", False), + ("https://github.com/user/repo", False), + ] + for url, expected in cases: + with self.subTest(url=url): + self.assertEqual(is_likely_bot_blocked(url), expected) + + def test_error_and_status_code_signals(self): + """Encoding errors and rate-limit style status codes are absorbed""" + cases = [ + ("https://www.netflix.com/", None, "encoding issue", True), + ("https://example.com/", None, "timeout", False), + ("https://example.com/", 429, None, True), + ("https://example.com/", 503, None, True), + ] + for url, status_code, error, expected in cases: + with self.subTest(url=url, status_code=status_code, error=error): + self.assertEqual( + is_likely_bot_blocked(url, None, status_code, error), expected) + + def test_legitimate_domains_on_transport_failure(self): + """A listed domain is protected when the request never reached the server + + Both a timeout and a connection error are symptoms of a host that will + not answer a datacenter IP, so both are treated alike. An unlisted + domain stays reported. + """ + cases = [ + ("https://www.python.org/", True), + ("https://jupyter.org/", True), + ("https://docs.python.org/3/", True), + ("https://github.com/user/repo", True), + ("https://unknown-domain.com/", False), + ] + for error in ("Connection Error", "Timeout"): + for url, expected in cases: + with self.subTest(url=url, error=error): + self.assertEqual( + is_likely_bot_blocked(url, None, None, error, + network_failure=True), + expected) + + def test_legitimate_domains_not_protected_without_transport_failure(self): + """The allowance is keyed on the handler, not on the error text + + Any exception reaching the catch-all handler can carry the word + 'timeout' in its message without the request having failed in transit. + """ + for url in ("https://www.python.org/", "https://github.com/user/repo"): + with self.subTest(url=url): + self.assertFalse( + is_likely_bot_blocked(url, None, None, + "ValueError: connect timeout to 0")) + if __name__ == "__main__": - test_bot_blocking_detection() \ No newline at end of file + unittest.main(verbosity=2) diff --git a/tests/test_modules.py b/tests/test_modules.py index 6b4f5f4..505d1b2 100644 --- a/tests/test_modules.py +++ b/tests/test_modules.py @@ -5,6 +5,8 @@ import sys import os +import json +import subprocess import unittest import tempfile from pathlib import Path @@ -55,34 +57,162 @@ def test_empty_ignore_patterns_ignore_nothing(self): patterns = link_checker.compile_ignore_patterns([]) self.assertFalse(link_checker.is_ignored('https://example.com/', patterns)) + def test_pattern_raising_non_re_error_is_skipped(self): + """re.compile raises more than re.error, and none of it may be fatal + + An oversized repetition count raises OverflowError, which is not a + subclass of re.error, so a narrower except aborted the whole run on a + pattern a user could plausibly type. + """ + patterns = link_checker.compile_ignore_patterns( + ['a{1,4294967296}', 'example\\.com']) + self.assertEqual(len(patterns), 1) + self.assertTrue(link_checker.is_ignored('https://example.com/', patterns)) + def test_legitimate_domain_protected_on_timeout(self): """A listed domain is protected on timeout, not only connection error Regression test: the legitimate_domains branch previously required the literal string 'Connection Error', so it could never fire on a timeout. """ - for error in ('Connection Error', 'timeout'): + for error in ('Connection Error', 'Timeout'): with self.subTest(error=error): self.assertTrue( - link_checker.is_likely_bot_blocked('https://github.com/x', error=error)) + link_checker.is_likely_bot_blocked( + 'https://github.com/x', error=error, network_failure=True)) def test_unlisted_domain_not_protected_on_timeout(self): """Widening the branch must not silence unknown hosts""" self.assertFalse( - link_checker.is_likely_bot_blocked('https://unknown-domain.example/', error='timeout')) + link_checker.is_likely_bot_blocked( + 'https://unknown-domain.example/', error='Timeout', network_failure=True)) + + def test_legitimate_domain_not_protected_by_error_text_alone(self): + """The allowance keys on the handler, not on the word 'timeout' + + A misconfigured timeout makes urllib3 raise a ValueError whose message + contains 'timeout'. Sniffing the error string would silence every link + on a listed domain on what is really a configuration error. + """ + self.assertFalse( + link_checker.is_likely_bot_blocked( + 'https://github.com/x', error='ValueError: connect timeout to 0')) def test_status_zero_can_be_silenced_via_silent_codes(self): """silent-codes reaches the network-failure path, where 0 is reported""" loud = link_checker.network_failure_result( - 'https://unknown-domain.example/', 'Timeout', [403, 503]) + 'https://unknown-domain.example/', 'Timeout', [403, 503], + network_failure=True) self.assertTrue(loud['broken']) self.assertFalse(loud['silent']) quiet = link_checker.network_failure_result( - 'https://unknown-domain.example/', 'Timeout', [0, 403, 503]) + 'https://unknown-domain.example/', 'Timeout', [0, 403, 503], + network_failure=True) self.assertFalse(quiet['broken']) self.assertTrue(quiet['silent']) + def test_status_zero_silencing_does_not_cover_malformed_links(self): + """A bad href is the project's own to fix, so silent-codes must not hide it + + Malformed URLs and redirect loops reach the catch-all handler and also + report status 0. Silencing unreachable hosts must not silence these. + """ + result = link_checker.network_failure_result( + 'https://', "Invalid URL 'https://': No host supplied", [0, 403, 503]) + self.assertTrue(result['broken']) + self.assertFalse(result['silent']) + + def test_connection_broken_mid_response_counts_as_transport_failure(self): + """A body that stops mid-stream is silenceable like a timeout + + ChunkedEncodingError is neither a Timeout nor a ConnectionError, so it + needs its own handler or it falls through to the catch-all and stays + loud however silent-codes is set. + """ + import requests + from unittest import mock + + exc = requests.exceptions.ChunkedEncodingError('Connection broken') + with mock.patch.object(requests.Session, 'get', side_effect=exc): + quiet = link_checker.check_link( + 'https://unknown-domain.example/', 5, 5, [0, 403, 503]) + loud = link_checker.check_link( + 'https://unknown-domain.example/', 5, 5, [403, 503]) + + self.assertEqual(quiet['status_code'], 0) + self.assertTrue(quiet['silent']) + self.assertFalse(quiet['broken']) + self.assertTrue(loud['broken']) + self.assertFalse(loud['silent']) + + def test_malformed_link_stays_loud_via_check_link(self): + """The catch-all path is not silenceable, end to end through check_link""" + result = link_checker.check_link('https://', 5, 5, [0, 403, 503]) + self.assertEqual(result['status_code'], 0) + self.assertTrue(result['broken']) + self.assertFalse(result['silent']) + + def test_bot_blocked_domains_still_silent_without_network_failure(self): + """The domain_indicators list is unaffected by the network_failure gate""" + self.assertTrue( + link_checker.is_likely_bot_blocked('https://netflix.com/title/1', error='boom')) + + def test_ignore_patterns_file_end_to_end(self): + """--ignore-patterns-file skips matching URLs and reports them separately + + Every URL in the fixture is ignored, so this runs the whole script + without making a single request -- the point being that an ignored URL + is never requested at all. + """ + script = Path(__file__).parent.parent / 'link_checker.py' + with tempfile.TemporaryDirectory() as tmp: + html = Path(tmp) / 'page.html' + html.write_text( + '' + 'FRED' + 'UNRATE' + 'Other' + '', encoding='utf-8') + + patterns = Path(tmp) / 'patterns.txt' + patterns.write_text( + 'https://fred\\.stlouisfed\\.org/.*\n' + '\n' + '# a comment\n' + 'blocked\\.example\n', encoding='utf-8') + + proc = subprocess.run( + [sys.executable, str(script), str(html), + '--ignore-patterns-file', str(patterns)], + capture_output=True, text=True, timeout=60) + + self.assertEqual(proc.returncode, 0, proc.stderr) + data = json.loads(proc.stdout) + + self.assertEqual(len(data['ignored_results']), 3) + self.assertEqual(data['broken_results'], []) + self.assertEqual(data['redirect_results'], []) + self.assertEqual(data['total_links'], 3) + self.assertIn('url', data['ignored_results'][0]) + self.assertIn('file', data['ignored_results'][0]) + self.assertIn('text', data['ignored_results'][0]) + + def test_ignore_patterns_file_missing_is_not_fatal(self): + """An unreadable patterns file warns and checks nothing away""" + script = Path(__file__).parent.parent / 'link_checker.py' + with tempfile.TemporaryDirectory() as tmp: + html = Path(tmp) / 'page.html' + html.write_text('no links', encoding='utf-8') + + proc = subprocess.run( + [sys.executable, str(script), str(html), + '--ignore-patterns-file', str(Path(tmp) / 'does-not-exist.txt')], + capture_output=True, text=True, timeout=60) + + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertEqual(json.loads(proc.stdout)['total_links'], 0) + def test_link_checker_with_test_files(self): """Test link checker with actual test HTML files""" test_dir = Path(__file__).parent