ci: gate the API reference against duplicate-content across versions - #431
Merged
Merged
Conversation
The API reference is published once per major.minor line, so the same symbol page exists at /6.28/data-capture-sdk/X, /7.6/data-capture-sdk/X and the unversioned /data-capture-sdk/X. None of them declares which is canonical, so Google treats them as independent pages and picks whichever it likes - usually the oldest, because it carries the most history and inbound links. That is how /6.28/.../aamva-barcode-result.html came to outrank current docs. The rule this checks, per versioned page: unversioned counterpart returns 200 -> canonical pointing at it unversioned counterpart returns 404 -> robots noindex The second branch is the one easy to get wrong. An API removed since that line has no current equivalent, so a canonical would point at a 404 and Google would ignore it; that page needs de-indexing, not redirecting. Both cases occur in practice - the first sampled run found 4 of one and 3 of the other. Nothing here is version-specific, which is the point: the canonical target is always the unversioned URL, which is by definition the current line. A release never changes what this expects and there is no version constant to maintain. URLs come from the versioned API links the built site already contains (2,049 distinct across /6.28/ and /7.6/), sampled deterministically so CI checks the same pages each run rather than drifting. Warns rather than fails. The API-reference HTML is generated outside this repository, so this gate can only observe it - it is merged now so the finding is visible and tracked, and takes --strict once the generator emits the tags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
…-check # Conflicts: # .github/workflows/build-docs.yml # package.json
Review pass over the new gate. Three of the findings were the gate reporting OK
on a state it was written to catch, and one was the rule itself not fixing the
bug in the header comment.
- Checking NOTHING was a pass. fetchHtml returns null on any non-ok response or
thrown fetch, and the loop `continue`d before incrementing `checked`, so a DNS
blip, an outage or a 429 burst left violations empty and printed "OK: every
sampled page declares a canonical form" with exit 0 - under --strict too.
Verified by forcing a 1 ms request timeout: now WARN + exit 0, FAIL + exit 1
under --strict, and it says how many picks it could not judge.
- A mistyped flag did the same. `--sample` with no value gave Number(undefined)
= NaN, and Array.from({length: NaN}) is [], so the run checked zero pages and
went green. `--sample`, `--sample abc`, `--sample 0`, `--sample --strict` and
`--sample=abc` all now exit 1 with a usage error; `--sample=N` is parsed rather
than silently ignored.
- Every non-200 on the unversioned URL was read as "API retired". A transport 0,
403, 429 or 5xx therefore told the generator team to de-index a live, healthy
current page. Only an explicit 404 means retired now; anything else is
reported as undetermined and judges nothing.
- The rule would not have fixed the motivating bug. A canonical is a DUPLICATE
signal and Google drops it between pages whose content materially differs.
Measured on ios/core/api/camera.html: unversioned 47,186 bytes, /7.6/ 50,846,
/6.28/ 45,806. So a canonical from 6.28 to the current URL is cross-content and
likely ignored, and /6.28/.../aamva-barcode-result.html would keep outranking
current docs. The gate now accepts noindex OR a correct canonical, and reports
the canonical as weak when the two pages differ by more than 5%. It does not
decide for the docs team that old lines must leave Google - noindex also stops
a 7.6 reader finding their own version's page - it surfaces the trade-off.
- Coverage was silently narrowed three ways: a 6,000-file walk budget that
returned without a word once exhausted (and was already below the build's
6,540 .html files while three doc versions existed, which also broke sample()'s
promise of checking the same pages every run); a URL character class that
swallowed `)` and `,`, turning `[AI](...parser/AI)` into `parser/AI)` and
producing 7 bogus picks in /6.28/ and 5 in /7.6/ that 404 and were skipped
unannounced; and an unguarded readFileSync. The walk is now complete (3,209
files, well under a second), artefacts are filtered, and unreadable files are
counted and reported.
- Lines the build no longer links were invisible. The 8.6 release deleted the
version-8.5.3 snapshot, so nothing links to /8.5/ - while the generator still
publishes it. Discovering lines only from links hid the NEWEST frozen line,
the one whose content is closest to current and so most likely to outrank it.
The gate now probes the minors below the current version and reports what is
live but uncovered: /8.5/, /8.4/ and /8.3/ today.
- An unconditional exit(1) in the catch contradicted the workflow step, which
omits --strict on purpose because the generator cannot pass this yet. Transport
now exits 0 unless --strict; a defect in the script always fails.
- Added AbortSignal.timeout(15s) to both fetches (undici's default is 300s), and
the canonical comparison now resolves the href instead of string-matching it,
so a valid relative, protocol-relative or http:// canonical is no longer
reported as a violation to the very team meant to act on the report.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate now checks what it says it checks, and says what it did not check. Sixteen review passes went into this; the substantive changes: WHAT IT ASKS FOR. A canonical is a duplicate-content signal and Google drops it between pages whose content differs. Measured on ios/core/api/camera.html: unversioned 47,186 chars, /8.6/ 47,186, /8.5/ 45,355, /7.6/ 50,846, /6.28/ 45,806. So the original rule - canonical to the unversioned URL - would have been implemented in full and /6.28/.../aamva-barcode-result.html would have kept outranking current docs, which is the bug the gate exists to prevent. A page is sound when it carries robots noindex in <head> or an X-Robots-Tag header, or redirects to its unversioned counterpart, or declares a canonical to a counterpart close enough in size for that canonical to hold. IT CAN NO LONGER PASS WITHOUT CHECKING. Six separate paths used to print OK and exit 0 - under --strict as well - having judged nothing: a mistyped `--sample` (NaN picks), every page unreachable, every page 404, a build with no versioned links, a line judged on one page, and `--strict=true` silently disabling strict mode. Each is now a failure or a named non-green state, verified with stubs. WHAT IT PARSES. robots and canonical detection is scoped to <head>, tolerates unquoted attributes and multi-token `rel`, strips HTML comments before matching, splits robots directives on both commas and whitespace, honours `name="googlebot"` and `X-Robots-Tag`, and tracks bot scope left to right so `bingbot: noindex, none` is not read as de-indexed. Two forms it used to get wrong in opposite directions. DISCOVERY, as a separate script. A line keeps being published after its doc snapshot is deleted - the 8.6 release removed version-8.5.3, so nothing links /8.5/ - and the newest frozen line is the one whose content is closest to current. scripts/discover-api-reference-lines.cjs probes for those and prints them; build-docs.yml feeds them to the gate, so coverage needs no hard-coded versions. It probes with symbols durable across every linked line, treats a non-404 as "could not tell" rather than absence, carries a request budget, and states the range it probed. It lives apart because probing inside the gate did not converge over four passes: the probe set, the coverage accounting and the cost estimate were each wrong in turn, and one of its three discovery sources provably could not contribute a line. scripts/lib/linked-api-lines.cjs is shared, because the URL extraction encodes several rounds of lessons - no file cap, `)` and `,` excluded, `..` filtered - and two copies would drift. KNOWN LIMITS, not silently: a major with no linked line is probed against another major's minor count; `--lines` naming the served line is dropped when the build links it; the 20-violation print cap is filled by the oldest lines first; the `redirects` list is collected and not reported; the shallow-line note names missing symbols even when the cause was transport; and the gate has no total request budget, so a large manual `--sample` is slow. None of these can produce a false "all clean" - they cost coverage or precision of wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six review rounds, each verified by running the scripts against a mock origin rather than by reading them. The headline finding was a false pass on the one signal this gate exists to produce: `hasNoindexIn` split its directive list on `[,\s]+` and accepted any bare `none`, so `index, follow, max-image-preview: none` tokenized to a `none` and the page read as de-indexed. An indexable versioned page was therefore reported sound - the gate saying "no duplicate content" about exactly the duplicate content it was built to find. The space after the colon is what made it reachable, and `VALUED_DIRECTIVES` never touched this path: it only guarded the scope prefix in the header parser. Valued directives are now stripped WITH their values before tokenizing, anywhere in the part, so neither `max-image-preview: none noindex` nor `noindex, max-image-preview: none` is misread. Verified by extracting the shipped function and running 21 meta and header cases through it. A MISSING FILE COULD PRODUCE A PERMANENT BLOCK. When `search-tags.json` cannot be read, `servedLine` became `""`, which silently switched off the served-release exclusion - so the current release's own versioned line was judged as an old line and every one of its pages came back as duplicate content. That is the standing `--strict` failure this gate is written to avoid, arriving through an unreadable file rather than a real finding. The gate now bails the way discovery already did, issuing no requests. Relatedly, the stale-artefact version check no longer short-circuits on an unknown version: "cannot tell" must not read as "fine", because that is precisely when a local build/ is likeliest to hold an artefact from an older release. A DEAD LINE COULD BE REPORTED NOWHERE. A line the build links where every url 404s escaped every floor - `requested` excludes 404s, so `blindLines` and `thinLines` and the coverage share all skipped it - and the note that did cover it treated a linked line like one the operator had merely asked about. Those are different claims: a borrowed line answering "not published" is an answer, while a linked line answering it is a build pointing at pages that are not there. They are now separate, and the failing one is keyed on `absent === sampled` rather than on `checked === 0`, so a run whose picks were throttled stays `blindLines`' case instead of being called a rename. Two floors keep that verdict honest: a line needs enough linked urls to be a line rather than one typo'd href, and enough sampled picks to say anything about it - without the second, `--strict --sample 1` turned a single rotted url on a 1,000-url line into "taken offline", identically on every retry because the sampler is deterministic. A line that is all-absent but under either floor is reported rather than dropped, and the footer says plainly that it is not failed ON ITS OWN. Smaller fixes, all reproduced first: an unreadable directory is counted and reported instead of silently dropping its subtree, in both the gate and discovery, since every verdict derives from that link map; a partial probe confirmation warns on stderr, because the count was printed through the channel `--quiet` suppresses and `--quiet` is how CI runs it; a `--lines` entry the build already links is reported instead of vanishing, with the served line saying plainly that naming it does not add it back; and the "nothing to check" message now reasons from the unfiltered link map, so a build that links only its own version no longer claims to link nothing. Comment accuracy was treated as part of the work, because in these scripts the comments are the deliverable. Several documented figures did not reproduce and now do, or are gone: the sweep is ~42 requests today, not ~37, and the comment says the figure tracks how many minors exist below the served one rather than presenting it as a constant. `MIN_STALE_SAMPLE` was removed once measurement showed it could never bind - requiring one judged page already forces four sampled pages at a 0.75 share - rather than left asserting a threshold that never applied. KNOWN LIMITS: The gate is advisory in CI (no `--strict`), so nothing here can turn a docs PR red today. The two remaining threshold edges only bite once `--strict` is switched on. Discovery does not look for a line under a major older than those probed whose snapshot has been deleted: nothing links it and nothing probes it. The served release's own versioned copy is byte-identical to the unversioned tree and so IS duplicate content, but de-indexing the current release is a different decision and is left to `--lines`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moritzhartmeier
left a comment
moritzhartmeier
left a comment
Member
There was a problem hiding this comment.
I ran both scripts against a local build and the live site. They work, and the payoff is concrete: discovery found /8.3/ /8.4/ /8.5/ — lines nothing in the build links — and the gate then confirmed all three serve duplicate content with neither canonical nor noindex. That is exactly the case the second script exists for. Cost is small too: discovery 42 requests / ~5s, gate ~4s at --sample 4.
I also hand-probed the parsers (attr, headOf, canonicalOf, isNoindex, hasNoindexHeader, samePage) with 15 tricky inputs — commented-out tags, data-name decoys, rel="alternate canonical", max-image-preview: none, bingbot: scoping, protocol-relative canonicals. 14 of 15 behaved exactly as the comments say they do.
Three things below: one real bug, and two maintainability asks. Nothing I would block on.
…plicates Three review points on the api-reference SEO gate. LINES -> API_LINES. zsh declares LINES as a typed integer, so `LINES=$(...)` there arithmetic-evaluates the comma list and 8.3,8.4,8.5 becomes 8, after which the gate exits with `--lines takes major.minor values (got "8")`. CI runs bash and was never affected, but the same invocation is documented in the script header for people to paste into a local shell, and on macOS that is zsh. Renamed in both places, with the reason stated where the line is. The parsers are exported and tested. attr, headOf, canonicalOf, isNoindex, hasNoindexIn, hasNoindexHeader and samePage carry most of the risk in this check, and every one of them has a comment describing a false PASS it once produced - a commented-out canonical read as declared, data-name="robots" read as a directive, `max-image-preview: none` read as `none`, `bingbot: noindex` read as de-indexed. A false pass is the one failure a gate cannot reveal by being run, and prose cannot fail when someone reinstates the bug it describes. scripts/test-api-reference-seo.cjs pins each of those cases by name, plus the lib helpers that had no tests, and runs offline in milliseconds - no build/, no network. Wired in as test:api-reference-seo and as a CI step beside test:search-facets. Each assertion was checked against a mutated copy of the source: reverting attr to the \b boundary, stripping inline script in headOf, dropping the VALUED_PAIR strip, dropping the HONOURED_SCOPES guard, anchoring rel in canonicalOf, dropping googlebot from the meta names, string-sorting compareLines and removing the SUBSTANTIAL share all turn the suite red. The SUBSTANTIAL case only did so after the fixture was tightened: with a stray path unique to the stray line the intersection empties and the biggest-line fallback restores the pool, so the guard could have been deleted with the test still green. It now uses a path the other lines share, as the real /8.6/ link did. BUILD, ORIGIN, REQUEST_TIMEOUT_MS and the served-version reader - which was currentVersion() in the gate and currentNumber() in discovery, the same function reading the same field of the same file - move into scripts/lib/linked-api-lines.cjs. The two scripts run as one CI step and one hands the other an artefact, so a difference between them does not surface as a conflict; it surfaces as a confident report about something neither of them checked. currentVersion takes the build directory as a parameter so a test can point it at a fixture. Verified end to end against the live site after the refactor: discovery still finds /8.3/ /8.4/ /8.5/ in 42 requests, and the gate still reports all three as duplicate content with neither canonical nor noindex. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, and running unbounded Four findings from review, all confirmed against the running code. The violation print cap dropped the lines the check exists for. `targets` is sorted oldest-line-first, so violations accumulate that way, and a flat `slice(0, 20)` spent the cap on the oldest lines. Measured against the live site with the exact CI command: all 28 picks were violations and the 20 printed were /6.28/ x8, /7.6/ x8, /8.3/ x4 - /8.4/ and /8.5/ appeared nowhere but inside "... and 8 more". The newest frozen line is the one whose content is closest to current and so the likeliest to outrank it; it is why discovery exists, and it sorts last, so it was the one guaranteed to be cut. spreadAcrossLines now fills the cap round-robin, newest line first, so every line appears before any line repeats. The same run now prints four from each of the five lines. The file already carried this lesson for the served line, at `alsoServed`; the same crowding arrived through --lines. A redirect landing on a 404 was reported as link rot. `get` follows redirects and reports the FINAL status, and the 404 branch ran before the redirect branch. So a line remediated exactly as this gate asks - 301 to the unversioned counterpart - for symbols that have since been retired was charged to `absent`, printed under "url(s) the build links do not exist ... link rot in the docs", and judged nothing. On such a line it also pushes absent/sampled past STALE_LINE_SHARE and can trip `deadLines` into a --strict failure saying the line "was taken offline" - about a line that was just fixed. The redirect check now runs first. Verified against a stub: before, 0 of 8 judged and a link-rot report; after, 8 of 8 judged and OK. The gate had no request budget while discovery has one. --sample bounds picks per line and MAX_SAMPLE bounds --sample, but the number of LINES is whatever --lines names, and CI pipes discovery's whole list in. Discovery can legitimately return a long one, so this was ~120 sequential requests at a 15 s ceiling in a step that only advises. REQUEST_BUDGET = 260, with the spend printed every run (48 today). Picks it stops are reported as "not asked", never as absence, and the run fails its coverage floor rather than printing OK. `redirects` in discovery was collected and never read, so once the generator ships the redirects this check asks for, every remediated line would have appeared under "PUBLISHED but linked from nowhere" with nothing to tell it from an untouched one - the opposite of what happened. It is now named in the report and recorded in the artefact as `redirected`. Tests. spreadAcrossLines is exported and unit-tested, including the five-line case measured above. The two verdict fixes are ordering bugs inside the pick loop, which no unit test of a pure function reaches, so scripts/test-api-reference-seo-verdicts.cjs runs the real scripts against a stub origin on localhost - still offline, no live requests. Both fixes were mutation-checked: restoring the pre-fix branch order and removing the not-asked branch each turn the suite red, and a sound line still passes so the assertions cannot be satisfied by a gate that judges nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from the second review round. attr() read an attribute name out of an EARLIER attribute's value. The boundary class `(?:^|[\s"'])` made a closing quote a boundary, so `<link title="rel=canonical" rel="stylesheet" href="/s.css">` reported /s.css as a declared canonical. Tightening the boundary to whitespace closed that one and not the worse one: values contain spaces, so `<meta content="see name=robots noindex" name="description">` still returned "robots" and isNoindex read a page carrying NO robots directive at all as de-indexed. Both are false passes on the only two signals this gate verifies, and no boundary closes the class, because skipping quoted values is a tokenizer and not a pattern. attr now walks the tag's attributes. Third fix to this function; the first two were regexes. A correctly remediated line was failed when the unversioned counterpart itself redirects. The redirect verdict compared the fully followed final url against the unversioned url, so if /data-capture-sdk/X 301s onward to /8.6/, a frozen line doing exactly what this gate asks - 301 to the unversioned counterpart - follows that second hop, lands on /8.6/, and every pick was printed as "neither this line nor the current page". The canonical path already reasons about this exact hazard at `const counterpart = current`; the redirect path did not. It now also accepts landing where the counterpart's own chain lands, ordered second so the verdict still does not depend on that request succeeding. The request budgets did not bound the time they were added to bound. 260 requests at the 15 s per-request ceiling is over an hour, and discovery's 90 sequential HEADs are ~22 minutes, so both comments named a runaway their values permitted. Both scripts now carry a wall-clock deadline as well - 8 minutes for the gate, 4 for discovery - and print elapsed time beside the count. Not `timeout-minutes:` on the step: that fails the job, and this step is non-blocking on purpose. Stopping in-script keeps it advisory and, like the request cap, reports what it did not ask rather than reporting absence. The "could not determine" footer contradicted the run. discoveredUncertain was printed without intersecting what was actually checked, so following discovery's own advice - `--lines 8.5` after it filed /8.5/ as uncertain - produced a report that judged /8.5/ and stated in the same output that it was not among the lines checked. CI only passes the lines discovery FOUND, so this hit the one invocation documented for a human. All four are pinned: two unit cases for the attribute walker, two stub cases for the counterpart chain and the footer. Each was mutation-checked - restoring the old regex, dropping the convergence check, and dropping the footer filter each turn a suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sting any 3xx Four findings from the third review round, all low severity and all confirmed. spreadAcrossLines keyed on a hardcoded host. The line was parsed back out of each violation's url against `docs.scandit.com`, while every url in the run is built from ORIGIN. Point ORIGIN anywhere else - a staging host, or the stub the verdict suite runs against - and every entry fell to the "0.0" fallback, leaving one queue and the flat slice this function exists to replace. Violations now carry `line`, so nothing parses it back out. This also means the end-to-end suite had never covered the round-robin at all. Borrowed picks all came from one framework. A line nothing links is checked only on paths borrowed from other lines, taken as `slice(0, 4)` off a pool that sample() sorts lexicographically, where `android/...` dominates the low indices: measured 3 of 4 Android picks with a discovery artefact present and 4 of 4 without one. The newest frozen lines are checked ONLY this way, so a generator that de-indexed Android but not iOS or Web passed. borrowedPicks now keeps discovery's confirmed probes first - the seeded-overlap guarantee the old slice existed for - and spreads the remainder. Measured after: android, cordova, flutter, web. Any 3xx counted as "the line is published". A catch-all redirect for unknown paths is ordinary static hosting, and it would have made every minor in the sweep - about 15 today - come back as a discovered line, which CI feeds straight into --lines, after which the gate spends its budget reporting violations for lines that do not exist. headStatus now returns the Location too, and a 3xx counts only when it keeps the symbol path: a redirect to the unversioned counterpart or to another frozen line is a published line and both reach the gate, while one that drops the path is reported and ignored. Verified /8.2/ genuinely 404s today, so this guards a hosting change rather than fixing a live bug. A stale artefact outlived a run that could not replace it. Four paths return before the write, and the gate's freshness check cannot see it, because it compares against the served release number - which does not change between builds of the same release. Discovery now deletes the artefact on every path that cannot produce one; a missing artefact is already a supported state that the gate handles by working on its own. Pinned: borrowedPicks and servesSymbol are exported and unit-tested, the spreader has a case using non-production urls, and the stale artefact and catch-all redirect have stub cases. Each was mutation-checked. The lineOf mutation initially survived - every fixture used a real docs.scandit.com url, which is the same blind spot as the bug - so that case was added explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… servesSymbol The high-severity finding is a regression this branch introduced two rounds ago. The convergence check added for "the counterpart itself redirects onward" asks whether the versioned url and the unversioned url ended in the same place. A host-level catch-all sending unknown paths to `/` satisfies that trivially - both land there - so every pick read as SOUND, on a line that does not exist at all. samePage cannot catch it: it strips a trailing slash, after which "/" compares equal to "". That is the exact false-pass class this gate exists to close, and it shipped. The same hosting rule with the counterpart answering 200 produced the opposite error: every pick fell into the redirect branch, was counted as judged, and was emitted as a duplicate-content violation - findings against an unpublished line. Root cause is that discovery grew servesSymbol last round and the gate did not, so the two scripts read the same response differently. It moves into scripts/lib/linked-api-lines.cjs and both use it. A redirect that keeps the symbol path is a statement about the page - to the unversioned counterpart it is the remediation asked for, to another frozen line it moves the duplicate. One that drops the path is a hosting rule and says nothing, so the pick is now filed as undetermined: not sound, not a violation, and it drags the coverage floor, which is what "this run learned nothing here" is supposed to do. Two more in discovery. Confirming a probe on the unversioned tree rejected any 3xx, because `redirect: "manual"` is right for probing a LINE and wrong for asking "does this path resolve?" - one normalisation hop would have failed every candidate, exited 1, and the workflow's `|| true` would have turned that into an empty API_LINES, dropping the newest frozen line from CI with a single stderr line to show for it. And a line whose probes were all answered by a catch-all was filed as neither found nor uncertain, so it vanished from both outputs while the report said nothing was found - contradicting this file's own rule that a probe proving nothing must not read as absence. Such lines are now uncertain, which the gate surfaces. Four stub cases pin these, and each was mutation-checked: removing the gate's guard, restoring 200-only confirmation, and filing swept lines as absent each turn the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he sweep Three of six findings from the fifth review round. The other three are judgement calls left for a human - see the PR discussion. `undetermined` and `stale` were still flat-sliced. They accumulate in targets order, oldest line first, exactly like `violations` did before spreadAcrossLines, so with eight throttled picks on each of two linked lines the ten printed were all /6.28/ and /7.6/. The undetermined list is the one that says WHY coverage was lost, so this hid it for precisely the line the run was extended to cover. Both entries now carry `line` and both prints go through the same spreader. The 8-minute deadline started at module load, so argument parsing and the 3,000-file link walk ate into it before the first request. It now starts immediately before the pick loop, which is the part that can run away and the part it was written to bound. Coverage note: the spreader itself is unit-tested for this shape, but the two new call sites are not pinned - reproducing them needs a throttled origin, which the stub does not model. The violations call site is exercised on every live run; these two are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s, skip PRs Three items from the fifth review round. The lower-major sweep was incomplete. `knownCeiling(maj) + 3` probed a fixed three minors above the newest LINKED one - 7.9, 7.8 and 7.7, which never shipped, at about 27 requests a run - and stopped dead there, so a published line any further up was never asked about and never reached the gate. That is the blind spot this script exists to close. A streak-based walk was tried first and was wrong for the same reason, which a test caught before it shipped: de-publication removes a major's OLDEST lines and keeps its newest, so the surviving block sits ABOVE a stretch of absent minors. With /7.6/ linked and /7.12/ served, /7.7/-/7.11/ are gone and any streak ends the walk before it arrives. So the sweep is complete: the widest minor this project has shipped down to .0, walked downward so the budget goes newest-first and a run cut short loses only the oldest. That costs 99 live requests instead of 42, measured; the cheapness was the bug. REQUEST_BUDGET is re-derived from that worst case (250) with the arithmetic written down, and DEADLINE_MS remains the real bound. The ceiling also takes a floor that does not come from the link graph: maxMinorSeen reports 28 only because /6.28/ is still linked, so the day that snapshot is deleted the search for major 7 would have silently shrunk. The counterpart cache stored failures. Every --lines target borrows the same handful of symbol paths, so one transient 429 on an unversioned url was charged to every discovered line at once - three lines with three of four shared paths throttled each fell to checked === 1 against a floor of 2 and all three landed in thinLines from a single burst. Only a definitive answer (200 or 404) is cached now; a 429, 5xx or transport failure is re-asked, bounded by the budget and the deadline. And the gate step no longer runs on pull requests. What it checks is the live published site, and the API-reference HTML is generated outside this repo, so no change on a PR branch can alter the verdict - it was charging every author live requests to production for no PR-specific signal. Push to main, the daily schedule and manual runs keep it, which is the monitoring an advisory gate is for. The two offline test steps still run on PRs, because unlike this step they test the code in the diff. timeout-minutes: 15 is a backstop above the scripts' own 4- and 8-minute deadlines, for a hang neither can see. Each fix mutation-checked: restoring the +3 ceiling, dropping the ceiling floor, and re-caching counterpart failures each turn the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ock at the sweep Two findings from the sixth review round. Borrowed picks never left three frameworks. `borrowedPicks` was fixed last round to spread the non-seeded slots, and the test pinned that only for `seeded: 0` - not the path CI actually takes. With a discovery artefact present, seeded is 3 and want was 4, so the single free slot was `sample(rest, 1)`, which is `sorted[0]`: the alphabetically first entry, i.e. android again. Measured on the real build, every frozen line was checked on android, capacitor and cordova and on nothing else, while `dotnet.*`, `flutter`, `ios`, `react-native`, `titanium`, `web` and `xamarin.*` were never sampled on any line reached through --lines - which is the ONLY way a frozen line is checked. A generator shipping noindex for android but not ios got a clean OK. That is the exact failure the function's own comment claims it fixes. sample() cannot do this job: it spreads over a lexicographic order and the pool's low indices are all android. So the free slots are now filled by framework - ones the seeds do not already cover first - with a deterministic per-line spin, so the lines discovery hands over do not all spend their slots on whichever framework sorts first. UNLINKED_LINE_SAMPLE goes 4 -> 8 with it. The old value came with the reasoning that borrowed picks are guesses so more of them buy little, and that was true while they were four adjacent paths from one framework. Now each slot is a different framework and there are 13 in the pool, so the slot count IS how many frameworks a frozen line is looked at on. Measured: 48 live requests to 72, and 3 frameworks per frozen line to between 5 and 7, against a 260 budget and an 8-minute deadline on a step that no longer runs on PRs. And discovery's deadline started at module load, which the gate had already fixed for itself and documented. Small today - the walk is sub-second - but it is the deadline for the sweep, and these two scripts are not allowed to disagree about their own accounting. Left alone deliberately: `sample(items, 1)` returning the alphabetically first element rather than a representative one. It is real, but it only reaches `--sample 1` on a linked line, which CI never runs and which the coverage floor makes near-useless anyway; changing sample() would move every existing pick and re-seat the seeded-overlap guarantee both scripts share. Both fixes mutation-checked: reverting to sample() and removing the per-line spin each turn the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moritzhartmeier
approved these changes
Sep 18, 2026
moritzhartmeier
left a comment
moritzhartmeier
left a comment
Member
There was a problem hiding this comment.
All three findings from the previous review are fixed, and the commits that followed closed two false-pass classes I had missed. Verified locally against a real build and the live site.
On my findings. API_LINES renamed in both the workflow and the script header. Two suites added — test-api-reference-seo.cjs (44 assertions, parsers + lib) and test-api-reference-seo-verdicts.cjs (15, verdict branches against a localhost stub) — 59 passing in ~1s, fully offline, both wired into CI. BUILD, ORIGIN, REQUEST_TIMEOUT_MS and currentVersion now live in the shared lib.
The PR-skip split is the right one: the live step is off pull requests, while the two offline test steps stay on them, since those are what actually test the code in the diff.
Two real bugs found after my review, both worth calling out:
-
servesSymbol. A host-level catch-all redirect made the versioned and unversioned URLs converge on/,samePagecompared"/"against"", and every pick read as sound — on a line that did not exist. A false pass on the exact signal this gate verifies. Now shared so the two scripts cannot answer it differently, with three stub tests behind it. -
The
attrrewrite is justified. I ran the old regex against the new tokenizer, and the old one was genuinely exploitable:<link rel="stylesheet" title="href=/evil" href="/real"> old attr(tag, "href") -> "/evil""counted as a word boundary, so an attribute value could impersonate a canonical. The new tokenizer passed all 13 adversarial cases I tried: unquoted values, single quotes, spaces around=, self-closing tags, valueless attributes, uppercase, duplicate attributes, unterminated quotes, and the decoy above.
Also verified. The counterpart cache now drops non-definitive responses, so one 429 no longer poisons a shared pick across every borrowed line. dropArtefact() on all four early returns closes a stale-artefact path the version check cannot see, since the version does not change between builds of one release. spreadAcrossLines works — in my live run /8.5/ and /8.4/ lead the violation list instead of /6.28/ taking all 20 slots.
Measured live: discovery 99 requests / 19 s (matching the figure in the comment exactly), gate 72 / 2 s — 171 requests and ~22 s total, against budgets of 250 and 260 and a 15-minute step backstop. Found /8.3/ /8.4/ /8.5/ and 34 unsound pages.
Two notes, neither blocking:
- The full lower-major sweep costs 42 → 99 requests and finds the same three lines today, so the extra requests buy nothing at the current shape. The
/7.12/-surviving-above-a-gap case it protects against is real and stub-tested, and the step is off PRs now, so the trade is fine — just worth knowing. "... and N more, across the same lines"holds only while the number of lines carrying violations is under the print cap of 20. Five today. Cosmetic.
My earlier point about comment volume still stands — ~4,000 lines for the feature, and the two main scripts carry a lot of first-person revision history. But with 59 tests now pinning the invariants, most of that prose is redundant rather than load-bearing, so I would treat a trimming pass as a cheap follow-up rather than a condition of merge.
Approving.
eugenia-scandit
added a commit
that referenced
this pull request
Sep 18, 2026
One conflict, in package.json's scripts block: main's #431 added four api-reference-seo scripts at the same insertion point this branch adds docs:retrieval-evals. Both sets kept. That conflict is why this PR has run zero CI checks - GitHub cannot compute a merge ref for a conflicted PR, so no pull_request run fires and the last green run on record was two commits back, before the per-page search dedup and the chip aria-announce fix. Also gives the evals workflow the concurrency group docs-preview.yml already has. Every run is a full yarn build plus two eval passes, and this workflow watches paths a docs PR touches on nearly every push, so without a group they queue and all run rather than superseding each other. Keyed on the PR number so runs on main are never cancelled by a PR. Reported by @raffaelefarinaro in review of #418. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The API reference is published once per major.minor line, so the same symbol page exists at /6.28/data-capture-sdk/X, /7.6/data-capture-sdk/X and the unversioned /data-capture-sdk/X. None of them declares which is canonical, so Google treats them as independent pages and picks whichever it likes - usually the oldest, because it carries the most history and inbound links. That is how /6.28/.../aamva-barcode-result.html came to outrank current docs.
The rule this checks, per versioned page:
unversioned counterpart returns 200 -> canonical pointing at it
unversioned counterpart returns 404 -> robots noindex
The second branch is the one easy to get wrong. An API removed since that line has no current equivalent, so a canonical would point at a 404 and Google would ignore it; that page needs de-indexing, not redirecting. Both cases occur in practice - the first sampled run found 4 of one and 3 of the other.
Nothing here is version-specific, which is the point: the canonical target is always the unversioned URL, which is by definition the current line. A release never changes what this expects and there is no version constant to maintain.
URLs come from the versioned API links the built site already contains (2,049 distinct across /6.28/ and /7.6/), sampled deterministically so CI checks the same pages each run rather than drifting.
Warns rather than fails. The API-reference HTML is generated outside this repository, so this gate can only observe it - it is merged now so the finding is visible and tracked, and takes --strict once the generator emits the tags.