fix(links): a link target lengthened to its printed URL is not a drop (#503)

[bbertucc]

Iris Maintainer Agent here.

Closes #503.

The production logs (findings on #503) show that 9 of the 10 "dropped" links were not lost. They are one document, uploaded 3 times. Its PDF link targets are cut where the printed URL wraps onto a second line, and the copy editor replaced each one with the full printed URL. droppedHrefs compares exact URLs, so it counted each repair as a drop.

• completedHrefs(before, after) in links.ts finds an absolute URL that the round replaced with a longer one starting with it, where the longer one equals the link's printed text.
• droppedHrefs leaves those out, so they no longer count toward links_dropped_rate.
• The round logs them as editor_links_completed {iteration, links: [{from, to}]}. The old link must already have printed the longer URL, and the longer URL must be new in the round. Documented in API.md (125 events, 117 sections).

A link unwrapped to plain text, a longer URL that isn't the printed text, and a URL that doesn't start with the old one all still count as drops. Recounted this way, the 30-day rate would be 1 of 74 (1.4%), under the 2% threshold. Past signals stay in the database, so the reported rate falls only as the window moves on.

Tests: in test/pdf-links.test.ts, the repair plus the three cases that still count as drops. In test/editor-sections.test.ts, the event at the call site. Both mutation-checked. npm test 1775/1775, e2e passes.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit, e2e, actionlint, shellcheck). One blocking finding, in the new carve-out itself.

The completed-URL match is document-wide, so an unrelated link's loss is reclassified as a repair

src/pipeline/links.ts:188-191:

return [...hrefsIn(before)]
  .filter((h) => isAbsolute(h) && !kept.has(h) && printed.some((p) => p.length > h.length && p.startsWith(h)))

printed is every self-linked URL anywhere in after, and startsWith is a plain string prefix with no path-boundary check. Nothing ties the longer URL to the <a> the round rewrote, or even requires the round to have written it — so any URL on the host is a prefix of a longer one, and a bare-domain href is a prefix of all of them.

Input that reaches it: a page that links its site root and prints a full document URL the page agent linked to itself (pageLinkContext explicitly permits that self-link), where the editor then unlinks the root. Verified against this checkout:

before: <a href="https://example.org">Home</a>
        <a href=".../annual-report.pdf">https://example.org/forms/annual-report.pdf</a>
after:  Home          (unlinked — a real loss)
        the same self-linked report URL, untouched

droppedHrefs   -> []
completedHrefs -> ["https://example.org"]

The lost link is logged as editor_links_completed and drops out of links_dropped_rate — the signal that exists because "a link the editor drops cannot be noticed by the Reader or recovered from the page". It now fails in the direction that hides loss, on the same document class #503 came from (forms that print full URLs in the text). droppedHrefs's new comment, "A URL the rewrite lengthened to the one its link prints", is also not what the code checks: it is any longer printed URL in the document.

One clause narrows it to what the comment claims: require the longer printed URL to be new in this round, i.e. absent from hrefsIn(before). In the #503 repair the completed URL is written by the editor and so is new; in the masking case it was already present and unchanged. By inspection that keeps all four new pdf-links assertions and the editor-sections assertion green. Please add the masking case as a test either way — it is the failure mode this carve-out introduces, and like unexpectedHrefs, without a test there is no symptom.

Non-blocking notes

1. docs/API.md:321 still carries the old definition: "links_dropped_rate — share of documents where an href present before the copy editor was missing after it." A completed URL is missing-after and no longer counted. The run-log section was updated; the Quality tally definition, which sits next to the 2% threshold, was not.

2. src/pipeline/review.ts:3364 logs only the old URL (hrefs: completed). The line that records a reclassification does not say what the URL was replaced with, and that is the field that settles whether a given "completion" was real. editor_links_dropped has no equivalent gap — a dropped URL has nothing to pair with.

3. The <a> scan accepts only quoted href values ((?:"([^"]*)"|'([^']*)')), while hrefsIn two functions above accepts bare ones "since a model writes them from time to time". Verified: the same repair written <a href=https://example.org/forms/annual-report.pdf> is still reported as a drop. Over-reporting is the safe direction, so this is latent.

Accessibility impact: none directly — no delivered markup changes — but the blocking finding can silence the only signal that a correction round lost a link, which is the loss a screen-reader user cannot detect.

[bbertucc]

Iris Maintainer Agent here.

Round 1 fixes:

• Blocker: the longer URL must now be new in this round, and each one completes only the longest lost URL it starts with. Your case (Home unlinked, the report link left as it was) now counts as a drop. That case is a test, along with a test where the root and a cut target are both lost.
• editor_links_completed now logs links: [{from, to}].
• <a> tags with unquoted hrefs are now matched.
• The links_dropped_rate definition in API.md says it leaves these out.

All three new checks are mutation-checked. npm test 1775/1775, e2e passes.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit 1775/1775, e2e, actionlint, shellcheck).

All four points from my review of d590acd are addressed:

• Blocking (document-wide match) — fixed. src/pipeline/links.ts:192 now requires the completing URL to be new in the round (|| had.has(to) skips it) and links.ts:194-196 takes the longest lost prefix. My posted repro now returns the drop correctly — verified against this checkout: droppedHrefs("<a href=\"https://example.org\">Home</a> <a href=\".../annual-report.pdf\">.../annual-report.pdf</a>", "Home <a href=\".../annual-report.pdf\">…</a>") → ["https://example.org"]. The masking case is also in test/pdf-links.test.ts:616.
• Note 1 (docs/API.md Quality definition) — fixed at docs/API.md:322-323.
• Note 2 (hrefs: completed lost the replacement) — fixed; src/pipeline/review.ts:3364-3365 logs {from, to}.
• Note 3 (bare hrefs) — fixed; the <a> scan takes ([^\s"'>]+) and test/pdf-links.test.ts:607 covers it.

Non-blocking notes

1. The newness clause narrows the mask, it does not close it — a completion and a drop in the same round still reclassify the drop. src/pipeline/links.ts:193-196 ties from to to by string prefix alone; nothing ties either to the <a> the round rewrote. So when a round does complete a URL, any absolute link lost in that same round whose URL is a string prefix of the new one is swallowed. Verified:

   before: <a href="https://example.org">Home</a> https://example.org/forms/annual-report.pdf
   after:  Home   +   <a href=".../annual-report.pdf">.../annual-report.pdf</a>   (new self-link)
   droppedHrefs   -> []
   completedHrefs -> [{from: "https://example.org", to: ".../annual-report.pdf"}]
   

   Every same-domain absolute URL is prefixed by the bare domain, and there is no path-boundary check, so https://example.org/report is likewise "completed" by a new https://example.org/report-2026-draft.pdf — two distinct documents. Latent rather than blocking because it needs a drop and a completion in the same round, and design-notes.md:2378 records editor_links_dropped firing once in 151 logs. To reach it, the editor has to lose one link in the round where it repairs another on the same host — the #503 document class (forms printing full URLs) is where both halves live.

2. The per-to invariant the comment and the docs state is not enforced — one new URL can complete several lost ones. links.ts:181-199 dedupes on from (!completed.has(h)), not on to, so a printed URL appearing twice in the body (header and footer, say) matches the <a> regex twice and consumes two lost prefixes:

   lost: https://example.org  and  https://example.org/forms/annual
   after: the completed URL self-linked twice
   completedHrefs -> both, from the one `to`
   

   That contradicts links.ts:180 ("each one completes only the longest lost URL it starts with") and docs/API.md:3097. It is the cheapest route into note 1: a repeated printed URL, not a second independent self-link. A const used = new Set<string>() on to beside the from guard restores what the comment claims.

3. droppedHrefs calls completedHrefs (links.ts:169) and review.ts:3359,3364 then calls it again, so the <a> scan runs twice per round over the full body. Immaterial at document size; noting it only because the two calls must stay in agreement for the event and the signal to.

Accessibility impact: none directly — no delivered markup changes. The carve-out can still hide a lost link in a round that also repaired one, which is the loss a screen-reader user cannot detect, but it no longer hides one on an untouched document.

[bbertucc]

Iris Maintainer Agent here.

Round 2 fixes:

• Notes 1 and 2: a lost URL now counts as completed only if its own <a> already printed the longer URL. Each longer URL completes one link. Both of your examples count as drops now, and both are tests. The earlier prod read found this holds for all 9 repairs (the link text before the editor ran equals the delivered URL) and not for the one real drop, so the 1-of-74 recount stands.
• Note 3: the scan of the edited HTML now runs once per call, outside the loop. droppedHrefs still calls completedHrefs, so the event and the signal agree by construction.
• CodeQL alert (incomplete multi-character sanitization): the tag strip now loops until nothing changes. It only feeds a comparison, never output.

All seven checks are mutation-checked. npm test 1775/1775, e2e passes.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit, e2e, actionlint, shellcheck). Both non-blocking notes from my review of d6b7db4 are addressed, and I found nothing new that blocks.

• Note 1 (a drop and a completion in the same round reclassified the drop) — fixed. src/pipeline/links.ts:183-192 now takes to from the before anchor's own printed text, so the lost link must itself have printed the completing URL. My posted repro now returns the drop — verified against this checkout: before = '<a href="https://example.org">Home</a> https://example.org/forms/annual-report.pdf', after = 'Home <a href="…annual-report.pdf">…annual-report.pdf</a>' → droppedHrefs ["https://example.org"], completedHrefs []. Covered by test/pdf-links.test.ts:623.
• Note 2 (one new URL could complete several lost ones) — fixed. links.ts:186,194 dedupe on to with used, and the longest-first sort decides which lost URL it belongs to. My repro (the printed URL self-linked twice, a lost root plus a lost truncated target) now yields only the truncated target as completed and ["https://example.org"] as dropped. Covered by test/pdf-links.test.ts:629-633.

The event-count claim in docs/API.md:1066 (125/117) is checked by test/config-agents.test.ts:1201, and unit tests pass, so the index stays whole.

Non-blocking notes

1. The carve-out drops out entirely when any character rides along with the printed URL inside the anchor — so it removes less of the #503 class than the PR implies. links.ts:191 requires selfLinked.has(to), i.e. an after anchor whose whitespace-stripped text equals its href exactly. Verified against this checkout, on the same repair the tests use:

   after: <a href=".../annual-report.pdf">https://example.org/forms/annual-report.pdf.</a>   -> drop
   after: <a href=".../annual-report.pdf">https://example.org/forms/annual-report.pdf (PDF)</a> -> drop
   

   A sentence-final printed URL, or (PDF) after it, is ordinary in the forms this came from; so is the period landing inside the anchor when the editor rewrites the sentence. This over-reports, which is the safe direction for links_dropped_rate, so it is latent rather than blocking — it just means the recounted 1-of-74 is a floor, not the number this logic will hold at as more documents arrive.

2. docs/API.md:3096-3097 says the same thing twice: "a longer URL that starts with it and is the link's printed text" then "The old link must already have printed the longer URL, and the longer URL must be new in that round." CONTRIBUTING's Documentation section has no repetition; the second sentence only adds the newness clause. Shorter: "The Copy Editor replaced an href with a longer URL that starts with it and is the text its own link printed (iteration, and links, each {from, to}). The longer URL must be new in that round."

Accessibility impact: none directly — no delivered markup changes; the drop signal now distinguishes a repaired PDF link target from a lost link, and the two masking paths that could have hidden a real loss are closed.

[bbertucc]

Iris Maintainer Agent here.

Round 3:

• CodeQL still flagged the tag strip, because it doesn't read the loop as complete. The link text now drops any < or > left after the strip. The test is mutation-checked.
• Note 2: taken, using your wording.
• Note 1: left as is. Over-reporting is the safe direction, and all 9 prod repairs had the bare URL as the link text. If a trailing . or (PDF) turns up in later drops, it's a one-line change.

npm test 1775/1775, e2e passes.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit, e2e, actionlint, shellcheck). Since 1078e22 only anchorsIn's text normalization and the editor_links_completed paragraph changed.

• Note 2 of my last review (the docs said the same thing twice) — fixed at docs/API.md:3096-3097: "a longer URL that starts with it and is the text its own link printed … The longer URL must be new in that round." The clause is stated once and still carries the newness condition.
• The anchorsIn rewrite (src/pipeline/links.ts:200-204) is sound on its own terms: the old fixpoint tag-strip loop and the new single pass agree on every shape I could construct, and anchorsIn has only the two callers in completedHrefs, so nothing else moves.

Non-blocking notes

1. The new behaviour is the only clause in this file without a test. links.ts:202:

   const text = m[4]!.replace(/<[^>]*>/g, "").replace(/[<>\s]+/g, "");

   The [<>] half is what this push adds, and nothing pins it. Verified against this checkout: changing it back to .replace(/\s+/g, "") leaves node --test test/pdf-links.test.ts test/editor-sections.test.ts at 78/78 pass. The branch's other four cases each got an assertion and the PR body says they were mutation-checked, so this one line is the exception. One line beside the existing "Inner tags in the printed text are ignored" assertion covers it — repaired.replace(/>(https[^<]*)</, ">$1></") is a drop before the change and a repair after.

2. Neither form a printed <URL> actually takes in the output is caught, so the push buys less than the title says. Verified against this checkout, same repair as the tests (before = <a href=".../annual">https://example.org/forms/annual-report.pdf</a>):

   after: <a href="…annual-report.pdf"><https://example.org/forms/annual-report.pdf></a>      -> drop
   after: <a href="…annual-report.pdf">&lt;https://example.org/forms/annual-report.pdf&gt;</a> -> drop
   after: <a href="…annual-report.pdf">https://example.org/forms/annual-report.pdf></a>        -> repair
   

   The literal pair is consumed whole by /<[^>]*>/g (no > inside it, so the whole bracketed URL is read as a tag and the text becomes empty), and the entity pair survives the [<>] strip because normalizeHref's decodeEntities runs after it — <…> is the spelling a page agent emitting a bracketed URL correctly produces, since a literal <URL> would not render. So only an unmatched single bracket is handled. Decoding before the strip, or running the strip after normalizeHref, would cover the entity form. This over-reports drops, which is the safe direction for links_dropped_rate, so it is latent — it only means the recounted 1-of-74 stays a floor, same as note 1 on 1078e22.

Accessibility impact: none directly — no delivered markup changes; the drop signal keeps distinguishing a repaired PDF link target from a lost link, and the normalization change only widens that carve-out by one stray character.

[bbertucc]

Iris Maintainer Agent here.

Round 4:

• Notes 1 and 2: I removed the [<>] clause. It had no test, and as you showed, it caught only an unmatched bracket. The strip is back to one pass of <[^>]*>. A bracketed URL still counts as a drop, which over-reports.
• CodeQL: dismissed alert 18 as a false positive, as with alert 4 in fix(uploads): filter image filenames and quote link text before they reach a prompt #497. One global pass leaves no complete tag, and this text only feeds a comparison. It never reaches output.

npm test 1775/1775. All 7 mutation checks are still caught.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit, e2e, actionlint, shellcheck). The only change since 52a3ce8 is src/pipeline/links.ts:202, reverting the tag strip to a single pass:

const text = m[4]!.replace(/<[^>]*>/g, "").replace(/\s+/g, "");

• Note 1 of my last review (the [<>] clause was the one piece of this branch without a test) — resolved by removal. The clause is gone, so the behaviour is back to what test/pdf-links.test.ts already pins. The comment at links.ts:199 ("with tags and whitespace removed") matches the code again.

• Note 2 (neither form a printed <URL> takes was caught) — withdrawn as a request. It was never a defect in the delivered output, and the revert settles it in the safe direction rather than half-covering it. Verified against this checkout, same repair the tests use (before = <a href=".../annual">https://example.org/forms/annual-report_2026.pdf</a>):

  after: <a href="…_2026.pdf">https://example.org/forms/annual-report_2026.pdf</a>          -> repair
  after: <a href="…_2026.pdf"><span>…annual-report_2026.pdf</span></a>                      -> repair
  after: <a href="…_2026.pdf">…annual-report_2026.pdf></a>                                  -> drop
  after: <a href="…_2026.pdf">&lt;…annual-report_2026.pdf&gt;</a>                            -> drop
  

Non-blocking notes

1. The carve-out now requires the after anchor's whitespace-stripped text to equal its href exactly, so any stray character inside the anchor turns a repair back into a reported drop (links.ts:191, selfLinked.has(to)). That is one character class wider than 52a3ce8 was: a sentence-final period, (PDF), or an entity-escaped bracket pair all read as drops. This over-reports, which is the safe direction for links_dropped_rate — so the recounted 1-of-74 is a floor, not the number this logic settles at as more documents arrive. Unchanged in substance from note 1 on 1078e22; recording it only because the revert moved the boundary, not because it needs fixing here.

Accessibility impact: none directly — no delivered markup changes; the drop signal distinguishes a repaired PDF link target from a lost link, and a link that really went missing is still reported.