[jimcadden]

Replace the lychee-only path with LINK_CHECKER=python|lychee (default python) and a stdlib link-check.py that emits lychee-compatible JSON. Egress-proxy refusals are classified as unchecked so they never open or auto-close issues. The link-health program now needs only python3 (plus gh/jq/bash) — no lychee install — so it runs on a basic agent image (rossoctl/repoman#3).

What's in the branch

• scripts/link-check.py — stdlib-only checker (urllib, concurrent.futures, html.parser, re, ipaddress, tomllib when available; Python 3.8+). Scanner arg parity, .lychee.toml subset (exclude/exclude_path/accept with a Python<3.11 fallback parser), per-host concurrency cap, Retry-After on 429, HEAD→GET fallback, redirect cap, real User-Agent.
• Scanner: LINK_CHECKER toggle (default python, lychee opt-in), shadow mode (LINK_HEALTH_SHADOW_OUT), unchecked split/reporting (count + top hosts in latest.json, history, dashboard), and no auto-close of previously-broken links that come back unchecked.
• extract-broken-links.sh: unchecked status token (mapped before the error catch-all).
• Tests: tests/test-link-check.py (39 unit tests: extraction goldens, code-block skipping, 429/Retry-After, HEAD→GET, proxy refusal → unchecked, TOML fallback, CLI smoke), tests/test-link-health-unchecked.sh (e2e: unchecked never opens/closes issues, crash containment, default-python path), fixtures under tests/fixtures/link-check/.

Hardening added on top of the original two commits

• Malformed URLs no longer crash the scan (urlsplit ValueError on e.g. http://[::1 ran unguarded in three places); they are reported as findings like lychee.
• 429 Retry-After and the HEAD→GET fallback are now effective under the scanner's accept list (the accept check previously short-circuited both). Behavioral change: a HEAD-403/GET-404 link is now correctly reported as broken instead of silently accepted. Naive HTTP-dates in Retry-After are treated as GMT, not local time.
• Python<3.11 TOML fallback fixed for the common multi-line array formatting, #-inside-values, and non-integer accept entries (warn+skip instead of crash).
• Extraction: parenthesized URLs (.../Foo_(bar)) are no longer truncated (Wikipedia-style false 404s), and links in nested lists / indented prose are no longer skipped as code blocks (CommonMark-approximate).
• Scanner robustness: checker stderr is kept and printed on failure, a non-zero python checker exit fails the repo (lychee still exempt — it exits 1 on mere broken links), and checker output is JSON-validated before parsing so a crash mid-write cannot abort the whole run.

Verification

• python3 tests/test-link-check.py — 39 tests OK.
• /bin/bash tests/test-link-health-unchecked.sh (bash 3.2) — all cases pass, including default-python and crash containment.
• Full link-health suite (extract-broken-links, dashboard, index-wiring, subset-carryforward, dryrun-baseline, weekly-report) green; shellcheck clean.
• Container simulation: full DRY_RUN scan of a real repo under env -i with a minimal PATH (no lychee present) — Link checker: python, repos scanned 1/0 failed, exit 0.
• In a Python container (where a runtime is available):

  docker run --rm -v $PWD:/w -w /w python:3.9-slim python3 tests/test-link-check.py   # pre-tomllib fallback path
  docker run --rm -v $PWD:/w -w /w python:3.12-slim python3 tests/test-link-check.py  # tomllib path

Assisted-By: Claude Code

[clawgenti]

Solid design (default-off flag, shadow mode, unchecked bucket) with thorough tests, but the macOS CI leg is red and the new tests never run in CI.

• must-fix scripts/link-health-scanner.sh:157 — the empty-array expansion "${CONFIG_ARGS[@]}" under set -u is fatal on the macOS runner's bash 3.2, so the scanner dies for any repo without .lychee.toml; test-link-health-dryrun-baseline.sh fails on macOS (ubuntu's bash 5 passes).
• must-fix tests/test-link-health-unchecked.sh:1 — this test and tests/test-link-check.py are missing from .github/workflows/tests.yml's explicit list (and the shellcheck step), so they silently never run in CI.
• suggestion tests/test-link-check.py:412 — the optional lychee-parity test fails on any host with lychee installed (reference-definition URLs).
• suggestion scripts/link-check.py:326 — with a proxy configured, any connection reset is classified "unchecked", including origin-side resets after the CONNECT tunnel is up.
• nit scripts/link-check.py:543 — --exclude-all-private filters the original host only; redirect targets are not re-checked.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

must-fix — this test (and tests/test-link-check.py) is not in .github/workflows/tests.yml's explicit test list, nor in the shellcheck step's list. The workflow's own comment warns: "A new test file must be added here (and to the relevant shellcheck step) or it silently never runs in CI." Right now neither new test executes on any CI leg. Notably, had this one run on macOS it would have failed non-vacuously and caught the bash 3.2 regression: case 1 asserts unchecked_count == 1 in a latest.json that the crashing scanner never writes.

[clawgenti]

suggestion — test_optional_lychee_extraction_compare fails on any host with lychee installed (verified locally with lychee 0.24.2): lychee does not extract unused reference-definition URLs like [ref]: https://extract.example/reference, while MD_REF_DEF_RE in link-check.py does, so missing is non-empty and the assertion trips. The test skips in CI (no lychee there), so it only bites shadow-mode hosts — exactly where the parity signal matters. Either drop reference definitions from the extractor for parity, exclude them from this comparison, or document the divergence as intentional.

[clawgenti]

nit — --exclude-all-private filters on the original URL's hostname only; urllib then follows redirects without re-validating, so a public URL that 302s to a private/loopback address is still fetched. Worth confirming whether lychee re-checks redirect targets against exclude_all_private and matching that behavior before LINK_CHECKER=python becomes the default.

[jimcadden]

Addressed the review in 7ad4a53.

• bash 3.2: both checker invocations use ${CONFIG_ARGS[@]+"${CONFIG_ARGS[@]}"}, so an empty array under set -u no longer aborts the macOS scan. Verified on bash 3.2.57.
• CI: tests/test-link-health-unchecked.sh is in the shellcheck list and the run loop. tests/test-link-check.py runs in the suite.
• Lychee parity: unused [label]: url definitions are dropped before the bare-URL pass. A used reference is still extracted.
• Proxy resets: only a Tunnel connection failed is an egress refusal. An origin Connection reset by peer behind a proxy stays a reported error.
• Private redirects: with --exclude-all-private, a redirect whose host is private or loopback is not fetched.

Locally: python3 tests/test-link-check.py, bash tests/test-link-health-unchecked.sh, and shellcheck on the scanner and the new shell test.

Assisted-By: Claude Code

[clawgenti]

All five prior findings are addressed and verified (bash 3.2-safe CONFIG_ARGS expansion, both new tests wired into CI, egress refusal now requires tunnel-failure text, redirect targets re-checked for private hosts, unused reference definitions dropped); CI is green on both legs and the full suite — including the lychee parity test, which runs on this host with lychee 0.24.2 — passes locally.

Findings:

• suggestion scripts/link-check.py:147 — 4-space-indented lines are always treated as code and blanked, but in CommonMark they are usually nested list items or lazy continuations whose URLs lychee extracts (details inline).

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Follow-up on 7ad4a53: the rebase wires both new test files into the shellcheck and test-suite lists and runs tests/test-link-check.py in CI; both legs are green and DCO is signed. Two extraction-heuristic gaps remain (one carried over, one new) plus a forward-compat note on the test loader.

Findings:

• suggestion scripts/link-check.py:147 — 4-space-indented lines are always blanked as code, but in CommonMark they are usually nested list items whose URLs lychee does extract (carried over from last round; verified against lychee 0.24.2).
• suggestion scripts/link-check.py:101 — BARE_URL_RE's lookbehind drops any URL starting immediately after (, so prose like "(https://example.com)" is never checked (lychee extracts it; verified).
• nit tests/test-link-check.py:29 — exec_module without sys.modules registration is the pattern that just broke #124's ubuntu leg on Python 3.14; worth fixing here too.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

nit — exec_module on a module that was never registered in sys.modules is the exact pattern that broke #124's ubuntu leg on Python 3.14 (sys.modules.get(cls.__module__) is None inside @dataclass processing, raising AttributeError). link-check.py has no dataclasses today so this passes, but adding sys.modules[spec.name] = mod before exec_module(mod) future-proofs the loader.

[jimcadden]

The design is sound and the hardening commits land well — malformed-URL containment, HEAD→GET under the accept list, Retry-After, the TOML fallback and the stderr/JSON guards are all real fixes, and I re-ran the suite green. Two must-fix issues remain, both in the new default path.

1. lychee accept ranges silently disable 2xx acceptance. lychee's own documented config example is accept = ["200..=204", "429"] (lychee.example.toml:87). normalize_accept drops any non-integer entry, so that config becomes {429} and — because _check_once only accepts codes listed in accept — every 200 link is reported broken. A comma-string scalar (accept = "200, 429", also documented) collapses to the empty set, which flags every link including 200s. Verified locally: check_one(<200 url>, accept={429}) → {"code": 200}. This is the new default checker, so any enrolled repo with a realistic .lychee.toml flips to "everything is broken" and files issues for all of it.

2. A failed repo now auto-closes that repo's issues. The new containment paths (continue at 176/183/192) drop a failed repo from the current findings, but nothing keeps its prior findings out of the diff, so every previously-broken link in that repo counts as "fixed" and its issue is closed — while repos_failed is merely reported. Reproduced with a seeded baseline and a truncated-JSON checker: Delta: +0 new, -1 fixed, Closed issue #9 for README.md:https://dead.example.net/x, and broken: [] in the new latest.json. On main the same input aborted the run (exit 5, nothing closed), so this is a behaviour regression introduced by "fail the repo, not the run". It also contradicts standing-orders/link-health.md: "Do not close issues without evidence the link is fixed". The unchecked path already solves exactly this; failed repos need the same carry-forward.

Suggestion (not blocking): the ( in BARE_URL_RE's lookbehind still means (https://example.com/x) in prose is never extracted (carryover from clawgenti's round 3, unchanged). In shadow mode that was a divergence; now that python is the default those links are simply never checked. Fix: skip only when the ( follows ], or dedupe against MD_INLINE_RE.

Also (not blocking): is_private_host does an uncached getaddrinfo per URL, serially in the main thread before the pool starts, and --exclude-all-private is always passed — a repo with N links pays N DNS lookups even when they share a host (functools.lru_cache would do); WARN lines from the checker (unsupported .lychee.toml keys, bad regexes, non-integer accepts) go to CHECK_STDERR, which is only printed on failure, so config problems are invisible in a healthy run; and standing-orders/link-health.md still describes "Lychee fails on a repo" with no mention of LINK_CHECKER default/opt-in, the unchecked bucket, or shadow mode — worth updating in this PR since it is the program's operational contract.

What I verified

• 8 commits, DCO 8/8 (Signed-off-by on all), CI 4/4 green at 0cf6277, PR title prefix conforms.
• python3 tests/test-link-check.py → 39 tests OK (1 skipped: the lychee parity test, lychee not installed here).
• tests/test-link-health-unchecked.sh 16 PASS; test-extract-broken-links.sh 26 PASS; test-link-health-dryrun-baseline.sh, test-link-health-subset-carryforward.sh, test-link-health-index-wiring.sh, test-weekly-report.sh, test-automation-health-dashboard.sh PASS.
• shellcheck --severity=warning (0.11.0) clean on the four changed shell files.
• Spot-checked behaviour, not just assertions: unchecked never opens/closes and is re-injected as recurring; shadow report is written before the DRY_RUN early-exit, so it cannot touch the baseline; malformed http://[::1 is reported as a finding instead of aborting; [a](…/Foo_(bar)) and nested-list prose extract correctly; unused reference definitions are dropped; inline/fenced/indented code are skipped.
• No .claude/ or .vscode/ paths anywhere in the diff.

Author: jimcadden (MEMBER — maintainer)
Areas reviewed: Python, shell, YAML/CI, docs
Agent/IDE config (.claude/.vscode): none
Commits: 8, all signed-off
CI status: passing (4/4)

Verdict: request changes on the two must-fix items above. Submitted as a COMMENT review because the PR author is the GitHub account this reviewer authenticates as (APPROVE/REQUEST_CHANGES self-reviews 422).

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[jimcadden]

must-fix — normalize_accept silently drops non-integer entries, but lychee's documented accept format uses ranges and comma strings: lychee.example.toml:87 ships accept = ["200..=204", "429"], and :88 accept = "200..=204, 429". With a config like that the set collapses to {429} (or set() for the comma-string scalar), and because _check_once only treats codes in accept as success, every 200 link is reported broken — on the new default checker that means a full-repo false-positive run and an issue per link. Verified locally on a 200 endpoint: check_one(url, accept={429}) -> {'code': 200, 'text': '200'}, accept=set() -> same. Fix: expand a..=b / a..b ranges, split comma-separated scalars, and if the normalized set ends up empty, fall back to {200, 204, 206} with a WARN instead of accepting nothing. Worth a unit test for both the range and the comma-string form.

[jimcadden]

must-fix — this (and the two above at :175 and :182) drops a failed repo from the current findings, but its prior findings stay in $PREV_BROKEN, so every previously-broken link in that repo diffs as fixed and its issue is auto-closed. Reproduced with a seeded baseline containing alice/a + https://dead.example.net/x and a checker that emits truncated JSON: Delta: +0 new, -1 fixed, Closed issue #9 for README.md:https://dead.example.net/x, and broken: [] in the freshly written latest.json — the baseline is wiped too, so the next run re-files everything as new. On main the same input aborted the whole scan (exit 5) with nothing closed, so this is a regression from "fail the repo, not the run", and it contradicts standing-orders/link-health.md ("Do not close issues without evidence the link is fixed"). The unchecked path already does the right thing; failed repos need the same treatment — collect the failed full_repo list and re-inject their $PREV_BROKEN records into broken.jsonl (or subtract their keys from the fixed set) before diff_against_previous. A test like case 2 but with a crashing checker and a seeded baseline would pin it.

[jimcadden]

suggestion — carryover from clawgenti's round 3, still open: the ( in the lookbehind means a URL that starts immediately after a parenthesis is never extracted, so prose like See (https://example.com/release) for details. yields [] (verified). lychee extracts it. In shadow mode this was only a parity divergence; now that python is the default those links are silently never checked. Suggest skipping only when the ( is preceded by ] (markdown-link target), or deduping against MD_INLINE_RE matches instead of using a lookbehind for (.

[jimcadden]

suggestion — is_private_host performs a getaddrinfo per URL, uncached, and this loop runs serially in the main thread before the ThreadPoolExecutor starts. The scanner always passes --exclude-all-private, so a repo with N links pays N DNS lookups even when they are all the same host, which is a visible cost now that this replaces a Rust implementation that caches. functools.lru_cache on is_private_host (or a dict memo keyed on the host) fixes it; it also caps the worst case, since a hanging resolver is not covered by --timeout.

[jimcadden]

suggestion — the checker's stderr is now captured (good) but only printed when the repo fails, so its WARN lines are invisible on a healthy run: unsupported .lychee.toml keys (load_lychee_toml), bad exclude regexes, and skipped non-integer accept entries all go there — exactly the diagnostics that explain a silent behaviour difference from lychee. Suggest surfacing them, e.g. if [ -s "$CHECK_STDERR" ]; then sed 's/^/ WARN: /' "$CHECK_STDERR" >&2; fi on the success path.

[jimcadden]

nit — carryover from round 3 and still unfixed: exec_module on a module that was never registered in sys.modules is the pattern that broke #124's ubuntu leg on Python 3.14 (sys.modules.get(cls.__module__) is None inside @dataclass). link-check.py has no dataclasses today, so this passes; sys.modules[spec.name] = mod before exec_module(mod) future-proofs the loader.

[jimcadden]

Review findings addressed in 3402934..31ebeca — full suite green locally (42 unit tests, 20 e2e assertions across 5 cases, extract-broken-links 26, dashboard/index-wiring/subset-carryforward/dryrun-baseline/weekly-report, shellcheck clean).

Must-fix

1. accept ranges silently disable 2xx acceptance — 3402934. normalize_accept/parse_accept now expand lychee's documented forms: ranges ("200..=204" inclusive, "200..204" Rust end-exclusive) and comma-string scalars ("200, 429"). A config accept that still expands to no codes keeps the scanner-level list with a warning instead of flagging every link. Verified: normalize_accept(["200..=204", "429"]) → {200..204, 429}; covered by unit tests including a load_lychee_toml round-trip of lychee's own example config.

2. A failed repo auto-closes its issues — e8bb618. Failed repos (non-zero exit / no output / invalid JSON) are now recorded, and their prior broken records are re-injected before the diff — the same rule the unchecked bucket already uses — so they diff as recurring, never fixed. E2e case 3b reproduces the review's exact scenario (seeded baseline + issue Fix: Improve scanner report quality and PR hygiene #9, truncated-JSON checker): no close, finding carried forward, delta.fixed == 0, repos_failed == 1.

Suggestions

3. Prose-parenthesized URLs never checked — f3428d4. The bare-URL lookbehind now blocks only ]( (the inline-markdown opener, owned by MD_INLINE_RE) instead of any (, so (https://example.com/x) extracts. Golden fixture extended.

4. Uncached getaddrinfo per URL — 1f27901. is_private_host is functools.lru_cache-decorated (maxsize 1024), deduping the serial main-thread DNS lookups under --exclude-all-private.

5. WARN lines invisible on healthy runs — 1f27901. The scanner now prints up to 10 WARN lines from checker stderr for every scanned repo, not just on failure.

6. Standing orders stale — 31ebeca. standing-orders/link-health.md now documents the LINK_CHECKER default/opt-in, the unchecked bucket and its no-close rule, shadow mode, the .lychee.toml subset (including accept ranges), and the failed-repo carry-forward escalation.

Also fixed in 3402934: the test loader now registers the module in sys.modules before exec_module (the Python 3.14 requirement from clawgenti's round-3 nit).

Assisted-By: Claude Code

[jimcadden]

All five findings confirmed and addressed — note the review was taken at 0cf6277, one commit before the previous round's fixes landed.

P1: Failed scans can auto-close existing issues — already fixed in e8bb618 (previous review round, same finding). Failed repos are recorded and their prior findings re-injected before the diff, so they count as recurring, never fixed. E2e case 3b is this exact repro — seeded baseline with issue #9, truncated-JSON checker, zero successful scans — and asserts: no gh issue close, finding still in broken[], delta.fixed == 0, repos_failed == 1.

P2: Shortcut reference links disappear — fixed in 3548860. MD_REF_SHORTCUT_RE ([label] not followed by (, [, or :) now marks labels as used in _drop_unused_ref_defs, so See [docs]. + [docs]: https://docs.test/guide extracts the URL. Verified by unit test + golden fixture.

P2: Markdown code stripping removes real HTML links — fixed in 3548860. Extraction is format-aware: .html/.htm files skip the markdown fence/indent/inline-code stripping entirely (a four-space indent is formatting in HTML); only <pre>/<code> hide URLs there. Unit tests cover both the HTML path (extracted) and the markdown path (still treated as code).

P2: URLs inside HTML code examples are checked — fixed in 3548860. <pre>/<code> regions are now blanked for every extraction pass (markdown, bare-URL, autolink, HTML parser), in both formats — previously only the HTML parser's href/src skip honored them, so <pre>https://…</pre> leaked into the bare-URL pass. Covered in test_code_blocks_skipped and the golden fixture.

P2: Older-Python TOML parsing changes exclusion regexes — fixed in ad75ef5. The fallback parser now decodes TOML basic-string escapes (simple escapes + \uXXXX/\UXXXXXXXX), matching tomllib; single-quoted literal strings stay raw; unknown escapes are kept literally. Verified parity on the reviewer's repro: exclude = ["foo\\.net"] yields the identical decoded value from _parse_toml_subset and tomllib.

Full suite green locally: 48 unit tests (up from 42), 20 e2e assertions across 5 cases, extract-broken-links 26, dashboard / index-wiring / subset-carryforward / dryrun-baseline / weekly-report all PASS, shellcheck clean.

Assisted-By: Claude Code

[jimcadden]

Both must-fix findings confirmed and fixed in 099a69d — reproduced first, then verified against the exact repros:

Empty strings crash the fallback parser — confirmed: _parse_toml_subset('accept = [""]') raised ValueError: int(''). The all-empty capture tuple was dispatched to the integer alternative by nonempty-content checks, so the crash happened inside the parser, before normalize_accept's warn-and-skip could run (my earlier test only exercised the tomllib path — a gap in that fix). Token type is now decided by which regex group matched (finditer + group(N) is not None), so "" is correctly an empty string. Regression runs explicitly through the fallback: test_fallback_parser_empty_string_entry asserts accept == [""], and test_normalize_accept_empty_fallback_entry_does_not_crash covers the end-to-end warn-and-skip.

Escaped quotes corrupt exclusion arrays — confirmed: exclude = ["foo\"bar", "baz"] returned ['foo\\', ', '] from the fallback vs ['foo"bar', 'baz'] from tomllib. Three fixes: the item pattern is escape-aware for basic strings ("((?:[^"\\]|\\.)*)"), and both _strip_toml_comment and _bracket_depth skip escaped characters inside double-quoted strings (literal '...' strings still have no escapes, per TOML). test_fallback_parser_escaped_quote_parity asserts byte-identical output with tomllib on this input; test_fallback_parser_escaped_quote_scanners pins the comment/bracket behavior around \".

Verified on both reviewer repros after the fix: accept = [""] → [""] (no crash), and the escaped-quote array matches tomllib exactly. Full suite green: 52 unit tests (4 new), e2e 20 assertions, extract-broken-links 26, dryrun-baseline / subset-carryforward / dashboard PASS.

Assisted-By: Claude Code