fix(link-check): count broken links from the errors section only - #2080
fix(link-check): count broken links from the errors section only#2080JOhnsonKC201 wants to merge 2 commits into
Conversation
The summary step scanned the whole lychee report for lines shaped `* [<status>] <url>`, which is the wrong set on both ends. Checked against last night's sweep (run 31995703306), the count was wrong on five of the nine sites. Over-counting: the report also has a "Redirects per input" section whose entries have that identical shape. Every followed redirect was counted as a broken link. TinyTorch reported 27 broken when lychee found 1, and 26 of those were `[200]` redirect entries. Four were `[403]`, a status this workflow passes `--accept 200,403` for, so links lychee was explicitly told to treat as fine were still reported broken. Under-counting: the status is not always numeric. A request that never produced an HTTP response is reported as `[ERROR]` or `[TIMEOUT]`, which covers TLS failures, DNS failures, timeouts and missing local files. The `[[:digit:]]+` class could not match those, so they were dropped from both the count and the triage sample. When all of a site's failures were that kind the count parsed to 0 and the "broken but unknown" guard rewrote it to `?`, which is why Book and Slides show `?` in the tracker issue and their URLs are never listed. Book was hiding two TLS failures and Slides a missing slide file plus a timeout. Replaces the two whole-file greps with one awk pass scoped to the "Errors per input" section that accepts any bracketed status. Also strips the markdown autolink delimiters, so the tracker lists `https://example.com` instead of `<https://example.com>`. Verified against the archived reports from all nine sweeps in run 31995703306. The new count now equals lychee's own errors + timeouts tally on every site: Book 2, Labs 0, Kits 0, MLSys-im 0, Slides 2, Instructors 35, TinyTorch 1, Site 1, StaffML 0.
|
Hi @JOhnsonKC201 ! I Found two correctness bugs in the awk logic, verified against lychee's actual markdown formatter output (checked against 1. URL extraction leaves trailing junk (line 196)The regex only strips a trailing Reproduced with a line copied straight from lychee's own test fixtures: After the current awk transforms, instead of a clean URL. This corrupts every entry in the Fix: strip the 2. Timeouts are never actually counted (line 195)The awk stops collecting at the next That contradicts what the PR is trying to fix: a report whose only failures are timeouts (like the Fix: the section-scoping needs to treat "Errors per input" and "Timeouts per input" as two sections to collect from, instead of stopping at the first Given #2, I'd also double check the "matches lychee's Errors + Timeouts tally" claim in the manual testing notes. As written, the script can't produce that total for any report where timeouts are present. please fix these suggested changes and feel free to ping me later on to review those changes too ! |
…span Addresses both bugs @Shashank-Tripathi-07 found in review. Both are real and both are reproducible against lychee's own test fixtures. Timeouts were never counted. lychee writes each outcome as its own top-level section, so "## Timeouts per input" is a sibling of "## Errors per input" and not nested inside it. Bounding the scan at the first "## " after Errors turned collection off the moment the Timeouts heading appeared, so a report whose only failures are timeouts produced a count of 0, which the guard below then rewrote to "?". That is the same silent under-count this step was added to fix. The scan now starts on either heading and still stops at Redirects, Ignored and Suggestions, none of which are failures. URL extraction left trailing junk. lychee appends the source position as "(at line:col)" whenever a link carries a span, so the URL is not last on the line and stripping only a trailing ">" left entries like "https://example.com/foo> (at 1:1)" in the triage list. The span is now removed before the autolink delimiters. Verified two ways. Against a fixture in the current report shape, taken from lychee's own formatter tests, the old parser emitted one corrupted URL and dropped the timeout entirely, while the new one emits both URLs clean and still ignores the redirect and ignored entries. Replaying the nine archived reports from nightly run 31995703306 shows no regression: every site still equals lychee's own errors plus timeouts tally. Worth recording for whoever touches this next: lychee-action v2.8.0 currently defaults to lycheeVersion v0.23.0, whose output has no spans and lists timeouts inside "Errors per input", which is why the nightly tracker has been correct so far. The parser was right for exactly one pinned binary, and a version bump would have broken it quietly.
|
Thanks @Shashank-Tripathi-07, both of these are real and both are fixed in 52f6666. Reading the formatter rather than just my own output was the right call, and it caught something I would not have. TimeoutsConfirmed. The scan now starts on either heading and still stops at the three that are not failures: /^##[[:space:]]+(Errors|Timeouts)[[:space:]]+per input[[:space:]]*$/ { collect = 1; next }
/^##[[:space:]]/ { collect = 0 }The span suffixConfirmed too. On the tally claimYou were right to flag it, and it is worth pinning down precisely because it decides how urgent this is.
So the errors-plus-timeouts claim held for the pinned binary. It just held by accident. Being correct against exactly one version is fragile, and bumping VerificationTwo ways. Against a fixture in the current report shape, built from the formatter's own tests:
One corrupted URL and a dropped timeout, against two clean URLs, with the Redirects and Ignored entries still correctly excluded. Then a regression pass replaying all nine archived reports from run 31995703306. No change: every site still equals lychee's own errors plus timeouts tally.
I also updated the comment block above the parser to list all five sections and say which are failures, so the next person does not have to rediscover that Timeouts is a sibling. Ready for another look whenever you have time. |
Shashank-Tripathi-07
left a comment
There was a problem hiding this comment.
Both bugs from my earlier review are fixed correctly.
Timeouts are collected now: Errors and Timeouts are both treated as start-of-collection headings, since they're sibling sections in lychee's output, not nested. A report where every failure is a timeout no longer parses to 0 and gets rewritten to ? by the guard below.
URL extraction is clean: the (at line:col) span is stripped before the autolink delimiters, so entries that carry a source position no longer end up with > (at 12:3) left on the end.
I tested this against a fixture covering all five section types (Errors, Timeouts, Redirects, Ignored, Suggestions), mixing entries with and without a source-position span, a local file:// entry, and a redirect that's a status this workflow accepts. The parser collected exactly the Errors and Timeouts entries, 5 of 5, with clean URLs, and correctly skipped everything else. I also ran a Timeouts-only fixture with no Errors section in the file at all, which is the exact case the old code under-counted, and it collected correctly.
Good call flagging in the commit message that this only works for the currently pinned lychee version's output shape. Worth a comment in the workflow or a version pin note as a follow-up, but not a blocker for this PR.
Approving.
Land four more contributor PRs, each audited by running it rather than reading it, with the defects found repaired here: #2015 bare 'tito package reset' now actually resets #2023 conftest validates all 20 module exports #2080 link-check counts from the failure sections only #2092 Jupyter server reuse + start/resume desync recovery Three of the four had a real defect that only surfaced under execution: #2015 --force before the SUBCOMMAND token was silently dropped by the subparser default, then blocked on input() (EOFError in CI) #2023 the module-20 registry path never resolved, so every pytest run on a fully-exported tree printed a false 'not exported' warning, and two of the PR's own tests asserted the wrong value #2092 the pid check returned True for any live process on macOS, so a recycled pid would permanently block launching Jupyter #2080 needed no repair: its three claims check out against real lychee 0.23.0 output.
|
Thanks @JOhnsonKC201. Integrated into dev unchanged, CI green. I went and checked this against real lychee output rather than taking the reasoning on faith, and all three of your points hold. The section headings are exactly The old expression was worse than the PR describes. On a report with one Same number, wrong item, which is exactly the kind of bug that survives review. Add a second redirect and the old count is simply wrong. Nothing needed changing. Thanks for the thorough comments in the awk too, they made this quick to verify. @all-contributors please add @JOhnsonKC201 for code, tool |
|
I couldn't determine which project(s) to add the contributor to. 🤔 Your comment: @all-contributors please add @JOhnsonKC201 for code, tool This repo has multiple projects. Specify one or more explicitly, e.g.:
How project detection works:
|
|
Filing this under book: @all-contributors please add @JOhnsonKC201 for code, tool in book |
|
I've added @JOhnsonKC201 as a contributor to book! 🎉 Recognized for: code, tool The contributor list has been updated in:
We love recognizing our contributors! ❤️ |
|
Thanks @profvjreddi, and for checking it against real output rather than the write-up. The one-error-one-redirect case is sharper than anything I had: same count, wrong item, which is exactly what a diff-only review misses. |
|
No worries. More importantly, I appreciate you taking the time to issue a PR and contribute. Absolutely. This is one of my favorite things about doing all this stuff: I get to interact with so many of you through the sort of real work. |
Summary
The nightly link-rot summary counts broken links by scanning the whole lychee report for lines shaped
* [<status>] <url>. That matches the wrong set in both directions, so the counts in #1810 are wrong on five of the nine sites. This scopes the scan to the report's "Errors per input" section and accepts non-numeric statuses.Area
Changes
Measured against last night's sweep, run 31995703306:
???Two separate causes, same line of code.
Over-counting. The report also carries a
## Redirects per inputsection whose entries have the identical* [<status>] <url>shape, so every followed redirect was counted as broken. TinyTorch is the clearest case: lychee found 1 error, the tracker said 27, and 26 of those were redirect entries likeFour of them were
[403], which is a status this workflow already passes--accept 200,403for. Links lychee was explicitly told to accept were still being reported broken.Under-counting. The status is not always numeric. A request that never produced an HTTP response is reported as
[ERROR]or[TIMEOUT], covering TLS failures, DNS failures, timeouts and missing local files.[[:digit:]]+cannot match those, so they were dropped from both the count and the triage sample. When every failure on a site was that kind, the count parsed to 0 and the "broken but unknown" guard rewrote it to?. That is why Book, MLSys·im and Slides show?in #1810 with no URL list, and it means the two failures that are hardest to diagnose from a URL alone were the ones being hidden:The fix replaces the two whole-file greps with one awk pass scoped to the errors section, matching any bracketed status. It also strips lychee's markdown autolink delimiters, so the tracker lists
https://example.comrather than<https://example.com>as it does today.The step's contract is unchanged: same three outputs, same
?fallback when the report is missing, same behaviour underfail_on_broken.Testing
quarto render)pytest tests/)tito module test NNfor affected module(s)Pulled the archived lychee report out of the job logs for all nine sweeps in run 31995703306 and replayed both the old and new parser over them. The new count equals lychee's own
Errors+Timeoutstally on all nine; the old one matched on four. Timeouts are counted because lychee tallies them separately in the summary table but lists them in the same "Errors per input" section, and a timeout is a link worth triaging.Nothing outside this repo is needed to reproduce it: the reports are in the logs of that run.
Related Issues
Related to #1810 (the counts and the missing URL lists in that tracker come from this step).