Add ignore-patterns, reuse the open issue, and fix the unreachable timeout path - #3
Conversation
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) <noreply@anthropic.com>
🔗 Link Check Results🚨 5 broken link(s) and 3 redirect(s) were found. Build Details:
Link Check Summary
Broken Linkstests/broken-links.html - 5 broken link(s): tests/broken-links.html - 3 redirect(s): AI-Powered Suggestionstests/broken-links.html - AI Suggestions: Next Steps:
📝 This comment was automatically generated by the AI-Powered Link Checker Action. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the action’s false-positive handling and issue/reporting workflow by adding URL-level exclusions (ignore-patterns), reusing an existing open “broken links” issue instead of creating duplicates, and fixing a previously-unreachable timeout-silencing path (including honoring status 0 in silent-codes for network failures).
Changes:
- Add
ignore-patterns(newline-separated regexes) to skip matching URLs entirely and exposeignored-count. - Reuse/update the newest open issue with matching
issue-title+broken-linkslabel (newissue-updatedoutput). - Fix network-failure handling by consolidating timeout/connection-error paths and making
silent-codesapply to status0; add/repair tests and update docs/changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
link_checker.py |
Adds ignore-pattern compilation/matching and consolidates network-failure handling so timeouts/connection errors can be silenced consistently (incl. status 0). |
action.yml |
Wires ignore-patterns through env→temp file→CLI arg; adds outputs and updates issue creation to optionally reuse an existing open issue. |
README.md |
Documents ignore-patterns, ignored-count, status 0 in silent-codes, and the recurring-issue reuse behavior. |
CHANGELOG.md |
Records new inputs/outputs and the network-failure + test-runner fixes. |
tests/test_modules.py |
Adds regression tests for ignore-patterns, status 0 silencing, and fixes the test runner to fail CI properly on failing tests. |
tests/test_bot_blocking.py |
Fixes import path so the bot-blocking test script can run under the current repo layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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) <noreply@anthropic.com>
🔗 Link Check Results🚨 5 broken link(s) and 3 redirect(s) were found. Build Details:
Link Check Summary
Broken Linkstests/broken-links.html - 5 broken link(s): tests/broken-links.html - 3 redirect(s): AI-Powered Suggestionstests/broken-links.html - AI Suggestions: Next Steps:
📝 This comment was automatically generated by the AI-Powered Link Checker Action. |
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-patternsA newline-separated list of regular expressions for URLs to skip entirely:
Matching URLs are never requested, so they can be reported as neither broken nor redirected — an explicit exemption rather than a suppressed finding. They are counted separately and exposed as a new
ignored-countoutput, so an exemption stays visible rather than silently shrinking the scanned set.Newline-separated rather than comma-separated so that regex quantifiers such as
{1,3}survive unsplit. Matched withre.search, so a bare domain works as a substring without anchoring, and a Sphinxlinkcheck_ignorelist can be pasted in unchanged. An invalid pattern is logged and skipped rather than failing the run. The value reaches the script through the environment and then a file rather than being interpolated into the shell, since it is multi-line and full of metacharacters.update-existing-issueDefaults to
true. Before creating an issue, the action now looks for the newest open issue with the same title and thebroken-linkslabel and refreshes its body, instead of opening a duplicate. A weekly cron on a persistent finding produces one issue rather than one per week. The issue body says so, so a reader is not surprised to find it changing under them. Set tofalseto restore the previous behaviour.This is a behaviour change for anyone already running
create-issue: 'true', and it is called out as such in the changelog.The timeout path could never be silenced
This is the part that was not in the original issue, and it is why adding
fred.stlouisfed.orgto any existing mechanism would not have helped.is_likely_bot_blocked()already exists to absorb exactly this class of false positive, but on a timeout it cannot fire, for two independent reasons.The
legitimate_domainsallowance was unreachable. It required the error stringConnection Error, while the timeout handler passestimeout. A listed domain was protected against one failure mode and reported broken on the other. Calling the function directly onmainas it stands today:https://github.com/xConnection ErrorTruehttps://github.com/xtimeoutFalsesilent_codeswas unreachable. It was only consulted on responses that carried a status code, so it could never apply to a timeout or a connection error.silent-codes: '0,403,503'looked like it should work and silently did nothing. Status0is now honoured there, with a README note that it suppresses every unreachable host andignore-patternsis the narrower tool.The three network-failure handlers are now a single helper, so they cannot drift apart again.
Verification
Reproduced the reported symptom against a fixture containing the two FRED URLs from QuantEcon/lecture-python-programming — both came back
Status: 0 (Timeout)and were reported broken, matching the weekly reports exactly. Withignore-patternsapplied, both are skipped and the third link in the fixture is still checked normally. Separately confirmed thatsilent-codes: '0,...'now suppresses an unreachable host and that omitting it still reports one.Nine new tests in
tests/test_modules.pycover pattern compilation and matching, bare-domain substring matching, invalid patterns, the empty default, timeout protection for both listed and unlisted domains, and status 0 insilent-codes.Two things found while testing
The test job could not fail.
tests/test_modules.pycalledunittest.main(argv=[''], exit=False)and never inspected the result, so the CI step exited 0 regardless. It now exits non-zero on failure — verified in both directions by injecting a failing assertion. Worth knowing that previous green runs of that job were not evidence of anything.tests/test_bot_blocking.pycould not run. Itssys.pathstill pointed at../../.github/actions/link-checkerfrom the pre-migration layout, so the import failed. One-line fix; it now runs and passes. It is still not invoked by CI — left as-is rather than widening this change.Also corrected the action's self-referencing links in issue bodies, PR comments and artifact footers, which pointed at
https://github.com/QuantEcon/meta/.github/actions/link-checkerand 404. Confined toaction.yml;examples.mdcarries the same stale reference in a dozen places and is better handled separately.Not included
The suggestion from my comment on #2 — having the action read
linkcheck_ignoredirectly from a project's_config.yml— is not implemented here. It would add a YAML dependency and a second configuration surface, and both routes need a workflow edit anyway. Worth doing as a follow-up if you want one exclusion list per repo rather than two that drift; happy to add it either here or separately.