Report a Retired Badge Written Inline, Not Only One Defined - #637
Conversation
readme_shield_findings scanned reference definitions alone for a retired badge service, so a repo writing it as an inline image rendered the dead badge and reported nothing. The comment above the loop claimed it was scanned "wherever it sits", and the selftest case was named "reported wherever it sits" while testing the reference form only, so the claim and its evidence agreed with each other and not with the code. shield_endpoints already resolves both forms, and its own docstring records this exact lesson for every other shield. This is the one site that had not taken it: the link-naming loops read definitions because their rule is about definitions, and the two shield-presence sites already call shield_endpoints. A definition is still reported where nothing renders it, since a retired service left in the reference block is removed with the badge rather than after it, and a definition rendered by an image is one finding rather than two. Measured against all 22 cataloged READMEs: shield findings stay at 7 and no verdict moves, so no false positive. All three repos carrying the badge use the reference form, so this closes a blind spot rather than catching a live miss. The new cases were A/B tested against the pre-fix function: the inline case goes 0 -> 1 and the fenced sample stays 0. Raised by Copilot as a suppressed finding on promotion pull request #635. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the README shield auditing logic so deprecated/retired badge services are detected when written as inline images, not only when present as reference-style link definitions. This closes a blind spot in readme_shield_findings and expands the selftest suite to cover the newly supported cases.
Changes:
- Use
shield_endpoints()to scan the unfenced document for inline image URLs that matchdeprecatedShields. - Keep reporting deprecated reference definitions while avoiding double-reporting when the same URL is also rendered.
- Add selftest cases covering inline deprecated badges, definition+render deduping, and fenced samples remaining ignored.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two findings from round 1, both accepted. The finding text claimed every matching definition "renders" the retired badge, while the comment directly above it said a definition is reported even where nothing renders it. The two contradicted each other, and the message was the wrong one: telling a maintainer a badge renders sends them looking for it on a page that does not show it. The wording now follows which of the three shapes it is, and the selftest asserts the wording rather than only the count, since a count cannot tell one wording from another. unfenced_text was computed twice back to back. Once now, which is the cheaper read and also the safer one: the definitions and the rendered images can no longer be taken from two different views of the document. Measured against all 22 cataloged READMEs: shield findings stay at 7 and every live retired-badge finding keeps the word "renders", because all three repos carrying the badge do render it. The new wording fires only on the unrendered case, A/B tested against the pre-fix function, which says "renders" there and is wrong to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
spec/audit.py:1754
- The new wording logic has an important third shape: a deprecated badge URL can be rendered in the README, but not via the specific
[ref]definition being reported (e.g., the same URL is rendered inline while the reference definition is unused). Add a selftest case that covers this branch and asserts the updated wording (for example, "rendered elsewhere").
("a retired badge written inline is reported", conformant.replace("[![Last Commit][b]][x]", "[![Last Commit][b]][x]\\\n"), {}, 1),
("a retired badge defined and rendered is one finding, not two", conformant.replace("[![Last Commit][b]][x]", "[![Last Commit][b]][x]\\\n![Last Build][last-build-shield]").replace("[license-shield]: https://img.shields.io/github/license/o/r\n", "[license-shield]: https://img.shields.io/github/license/o/r\n[last-build-shield]: https://byob.yarr.is/o/r/lastbuild\n"), {}, 1),
("a retired badge shown as a fenced sample is markup", conformant.replace("## Overview", "```md\n\n```\n\n## Overview"), {}, 0),
spec/audit.py:805
- The deprecated-shield finding decides between "renders" vs "defines" by checking whether the definition URL appears anywhere in
rendered. If the badge is rendered via an inline image (or via a different reference name) but this particular[ref]definition is unused, the message will incorrectly claim that[ref]"renders" the badge. It should only say[ref]renders when that reference name is actually used as an image reference; otherwise prefer wording like "rendered elsewhere" to avoid attributing the render to the unused definition.
This issue also appears on line 1752 of the same file.
for ref, url in sorted(defs.items()):
if dep["match"] in url:
defined.add(url)
verb = f"renders {dep['label']}" if url in rendered else f"defines {dep['label']} and nothing renders it"
findings.append(("LETTER", f"readme: `[{ref}]` {verb}, which is retired - {dep['reason']} (spec/readme-structure.md)"))
Round 2 found the fourth shape, and it was a mis-attribution rather than a wording gap: the same endpoint rendered inline leaves a reference definition unused, so testing whether the URL appears anywhere in the rendered set credited that render to a reference nothing uses. The message then told a maintainer that `[ref]` renders the badge when deleting `[ref]` would leave the badge on the page. Attribution is now by reference name, from the image-reference uses, and the URL decides only between the two unused cases: rendered elsewhere, or rendered nowhere at all. Measured against all 22 cataloged READMEs: shield findings stay at 7 and no verdict or wording moves, since no repo carries the shape. A/B on a fixture that does: the previous code says "renders" and is wrong to, where this says "defines it and it is rendered elsewhere". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering both suppressed comments from round 2. They are one finding stated at two altitudes, and the code-level one is the sharper of the two, so it is quoted first.
Both accepted, fixed in Attribution is now by reference name, taken from the image-reference uses, and the URL decides only between the two unused cases. Four shapes, and the suggested wording is adopted:
The selftest case you asked for is there and asserts the wording rather than the count, since a count cannot tell these four apart. A/B on that fixture: the round-1 code says "renders" and is wrong to, where this says "defines it and it is rendered elsewhere". Measured across all 22 cataloged READMEs: shield findings stay at 7, and no verdict and no wording moves, because no repo carries the shape. So this, like both pipe fixes in #636, closes a blind spot rather than correcting a live report. Worth noting for the record: this is the third round in a row where the defect was a claim wider than the code behind it — the comment that said "wherever it sits" over a definitions-only scan, the message that said "renders" over an unrendered definition, and now an attribution that said |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
spec/audit.py:804
shield_endpoints()returns a list, but this code uses it for repeated membership checks (url in rendered) and set operations. Converting it once to a set improves clarity and avoids repeated O(n) scans. While changing that, consider naming the URL sets explicitly (rendered_urls,defined_urls) to avoid confusion with reference names.
rendered = shield_endpoints(unfenced, defs)
used_refs = {m.group(1) for m in _MD_IMAGE_REF.finditer(unfenced)}
for dep in model.get("deprecatedShields", []):
defined = set()
for ref, url in sorted(defs.items()):
if dep["match"] in url:
Round 3 clarity finding, accepted. shield_endpoints returns a list and every use here is a membership test or a set difference, so it is converted once. The names now say which namespace each set holds, rendered_urls and defined_urls against used_refs, since this block reasons about reference names and URLs a line apart and the bug the previous round fixed was exactly a URL standing in for a reference name. No behavior change: the selftest passes unchanged and the corpus reports the same 7 shield findings with no set moving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Answering the round-3 suppressed comment on
Accepted in full, fixed in No behavior change, which is asserted rather than assumed: the selftest passes unchanged, and the corpus run reports the same 7 shield findings across all 22 READMEs with no repo's finding set moving. |
…ate (#635) Promotion of develop at 2082547, six squashes since the last one. Closes #633. #631 routes the README by reader and documents the GH_WRITE_GUARD_ALLOW grant where a denied cross-owner write puts the reader. #632 moves readme.sections from intent to letter with four checks beside it, backed by spec/readme-sections.json and spec/third-party-tools.json, and settles the tagline rule. #634 adds repo_gate.py --check eol-coverage, reading the line-ending pins against the tree rather than only against .editorconfig. #636 and #637 repair two readers Copilot found on this pull request, both defects develop already carried: a tool row required both outer table pipes that GitHub's Markdown makes optional, and a retired badge written as an inline image was invisible to a scan that read reference definitions alone. #638 turns the host contract's presence check into a version gate, and retires two gh workarounds that were artifacts of a stale distribution package, re-tested on an upgraded host rather than inferred. Four carried files owe a downstream re-vendor and none is recorded in the TODO.md entry yet. GOVERNANCE.md Repository Details is verbatim, so the audit reports it, and it propagates a rule: the About description is the tagline alone, and Docker Hub receives it from the About panel rather than from the README. CODESTYLE.md item 4 and .gitattributes are intent, so nothing reports them, and the second couples to the new gate through the forward-declared mark. .github/copilot-instructions.md is intent and propagates a correction rather than a refresh, so a repo left on the old copy is wrong rather than merely stale.
Raised by Copilot as a suppressed finding on promotion pull request #635, against
developcode rather than anything that promotion introduces. It is the same shape as #636: a comment claiming more coverage than the code delivers.The gap
readme_shield_findingsscanned reference definitions alone for a retired badge service:So a repo writing the badge as an inline image renders the dead badge and reports nothing. Measured against the pre-fix function:
What made it hard to see
The comment above the loop said the service is "scanned across the whole document rather than per section, since a dead badge is wrong wherever it sits", and the selftest case was named "a retired badge service is reported wherever it sits" while constructing only the reference form. The claim, the case name and the case body all agreed with each other, and none of them agreed with the code. That is the pattern worth naming: the evidence was shaped to the claim rather than to the behavior.
The sweep, not the instance
shield_endpointsalready resolves![alt][ref]andalike, and its docstring records this exact lesson: "Reading references alone made an inline shield invisible rather than wrong." So the fix is to take the reading that already existed rather than to write a second one.Every sibling site was enumerated before fixing, and this was the only one that had not taken it:
readme_link_findingsnaming and grouping loops (4)shield_endpointsdeprecatedShields(1)A definition is still reported where nothing renders it, since a retired service left in the reference block is removed with the badge rather than after it, and a definition that is rendered produces one finding rather than two.
Measured
All 22 cataloged READMEs, pre against post, in both directions: shield findings stay at 7 and no verdict moves, so no false positive. All three repos carrying the badge (ESPHome-NonRoot, KiCadLibrary, MediaTools) write it as a reference definition, so this closes a blind spot rather than catching a live miss. The
TODO.mdentry for deleting those three badges is unaffected and its count is unchanged.Verification
Four selftest cases: the inline badge reported, a definition-plus-render counted once, a fenced sample still markup, and the original reference case unchanged. Each was A/B tested against the pre-fix function rather than reasoned about, which is what showed the inline case going
0 -> 1and the fenced one correctly staying0.spec/audit.py --selftest,spec/validate.py,prose_lint.py --diff developand editorconfig-checker all clean.spec/audit.pystays LF.🤖 Generated with Claude Code