Fetch only openly-licensed files, and never demote a cache entry - #85
Conversation
|
Claude finished @cmungall's task in 2m 56s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
This is a well-argued change and the diagnosis is convincing — an HTTP 200 reCAPTCHA page is genuinely indistinguishable downstream, and recording that as 1. The guard doesn't just refuse the delete — it serves the text the stale-HTML rule exists to withhold
preserved = self._preserve_cached_full_text(...)
if preserved is not None:
self._cache[normalized_reference_id] = preserved
return preserved # <- goes straight to validationThe docstring draws the distinction and then the code crosses it:
But Both goals are reachable — decide the write with the HTML bypass and the return without it: if force_refresh or not self.needs_full_text(fresh):
return None
cached = self._load_from_disk(id, allow_stale=True, allow_stale_html=True)
if cached is None or self.needs_full_text(cached):
return None
logger.warning(...)
# Serving is a separate question from deleting: withhold stale HTML here
# the same way _stale_fallback does, but still refuse the overwrite.
servable = self._load_from_disk(id, allow_stale=True)
return servable if servable is not None else freshThat needs the caller to distinguish "don't save" from "return this", e.g. returning 2.
|
26a5b2c to
b53f5a4
Compare
|
Thanks — finding 1 was a real defect, and the fix changed the shape of the guard rather than patching it. Also rebased onto #84, which landed while this was open and restructures the same method. Finding 1 — the guard served what the stale-HTML rule withholdsFixed, and you were right that the docstring described the intent while the code crossed it. The method now makes two decisions instead of one: # May it be overwritten? Stale HTML is readable for this question only.
cached = self._load_from_disk(id, allow_stale=True, allow_stale_html=True)
if cached is None or self.needs_full_text(cached):
return None
...
# May its text be served? Asked without the bypass, so stale HTML is withheld
# exactly as _stale_fallback withholds it.
servable = self._load_from_disk(id, allow_stale=True)
return FetchOutcome(content=servable or fresh, served_stale=True)So a pre-fix entry scraped from a landing page is now kept on disk and withheld from validation, which falls back to the freshly fetched abstract. The curator's file is not deleted, and its suspect text is not quoted back as though it came from the article.
Finding 2 —
|
|
Claude finished @cmungall's task in 4m 0s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
The head has moved since my last pass and the three things I raised are all addressed: the serve-vs-delete split is now explicit ( Four things on the new code. 1. The shrink rule can permanently block the migration it was added to protect
The code doesn't check that. So consider the population this release actually acts on: a pre-#62 That's the same shape as the bug being fixed, with the sign flipped: a type change from a page-scrape to a clean extraction is the case where a large shrink is expected. Either scope the ratio to same-type refreshes as the docstring says, or keep it cross-type but exempt a type the project considers better (XML over HTML). Whichever you pick, a test with cached 2. The licence rule lives in two providers; two others hand back
|
b53f5a4 to
b190701
Compare
|
Claude finished @cmungall's task in 4m 14s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All four findings from the last pass are closed. The CLI message now covers both branches without guessing which one ran ( One new finding on the exemption, then smaller notes. 1. The quality exemption is wider than the argument that justifies it, and the PDF route is the one it lets through
That argument holds for And
That is precisely the failure mode Two ways out, either fine: # (a) exempt only the upgrade that was actually argued for
FULL_TEXT_QUALITY = ("full_text_html", "full_text_xml")
# (b) keep the ranking, but let an exemption relax the bar rather than remove it
UPGRADE_SHRINK_RATIO = 0.2 # a cleaner extraction is shorter, not empty
ratio = UPGRADE_SHRINK_RATIO if cls._is_quality_upgrade(...) else SHRINK_PRESERVE_RATIO
return fresh_length < cached_length * ratio(b) keeps Related, and worth a second thought even if you keep the ranking: Smaller things
Tests
The gap is the I could not run the suite — the sandbox declined |
|
Finding 1 is real. You asked for it to be reproduced before acting, which was the right call — it reproduces exactly, and the consequence is worse than a refused update. And refused permanently, as you said: never written, so never stamped, so re-fetched and re-refused every run. That is the bug this guard exists to prevent, with the sign flipped, and it would have hit precisely the population this release acts on. 1. The shrink ruleTook the second of your two options rather than the first. Scoping the ratio to same-type refreshes would fix the reported case but leave clean XML replaceable by a fraction of it as HTML, which is the bot-page shape. FULL_TEXT_QUALITY = ("full_text_html", "full_text_pdf", "full_text_xml")A move up that order skips the length test; same-rank or a move down still applies it. Unranked types compare equal, so an unfamiliar type is never treated as an upgrade and the length test still governs it. Both directions are now covered, as you asked:
2. The licence rule's scopeYou read the intent correctly, and the docstring did read as project-wide policy. Recorded rather than patched: routing The rule now says it governs a file found by asking an OA index, where the index's own status is all that is known about the terms; an archive asked through its own API for a document it serves for machine retrieval is a stronger warrant. The asymmetry — the same green deposit declined from OpenAlex and admitted from PMC — is stated in 3. The surviving page fetchAgreed it isn't a bug, and agreed the doc made it look like one. 4. The CLI messageFixed — and fixing it surfaced a second one I had introduced. My own warning said The CLI message covers both paths rather than naming one:
That broke Two tests added: the preserved-entry message, and the exit-1-every-run-until- Smaller things
1087 passing, mypy, Ruff and |
b190701 to
0cac1a8
Compare
|
Claude finished @cmungall's task in 5m 11s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
The previous pass's finding is closed properly: Three things on the new head, one of which I think undoes part of rule 2 in practice. 1.
|
|
The PDF rung was a real gap, and the fix I made last round was wider than the argument I wrote for it. Reproduced before changing anything: 602 characters clear The exemptionTook option (b): relax the bar rather than remove it. UPGRADE_SHRINK_RATIO = 0.2 # a cleaner extraction is shorter, not emptyBoth repro cases now behave: the 602-character cover page is refused, and the 20k-page → 6k-XML upgrade still migrates. Option (a) would have fixed the reported case too, but it leaves Two tests on the rung that had none: The orderingYour second point is the better half of the finding, and I have written it into Smaller things
1089 passing, mypy, Ruff and One note on your closing caveat: you have now flagged three findings as traced-not-observed, and all three reproduced exactly as traced. I have added the reproduction to the test suite in each case rather than only fixing the code, so the next reader gets the failing case rather than the argument for it. |
0cac1a8 to
186f05e
Compare
|
Claude finished @cmungall's task in 4m 54s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All three of the previous pass's findings are closed, and closed properly rather than papered over. Three things on the new head. 1.
|
|
Finding 1 is right, and larger than the estimate. You asked for the value names to be confirmed against the APIs rather than taken from the review, so I pulled OpenAlex's whole licence vocabulary instead of spot-checking:
1. The licence allowlistTook your positive-allowlist form rather than naming the sentinels, since that way the exact spelling of def states_a_licence(licence):
value = (licence or "").strip().lower()
return value.startswith("cc-") or value in OPEN_LICENCES # cc0, public-domain, mitEvery CC variant qualifies: Nine new parametrized cases across the middle of the vocabulary — 2.
|
186f05e to
d4c9e3a
Compare
|
Claude finished @cmungall's task in 5m 43s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All three of the previous pass's findings are closed. Two findings on the ratio arithmetic, both in the part that changed last round. 1. The ratio is applied to the whole record, and the record starts with the abstract — which is most of what a cover-page refresh returns
content.content = f"{abstract}\n\n{text}" if abstract else textso both sides carry the abstract, and on the fresh side of a failed extraction the abstract is nearly all of it. The docstring says "less than this share of the cached text"; the code compares record lengths, and the difference is exactly the quantity that distinguishes a cover page from an article. At
A 1,400–2,000 character abstract is ordinary, and a 10k cached entry is an ordinary short paper. So for a medium-sized article the 0.2 floor is already spent on the abstract before the extraction is weighed at all, and The test does not see it because every test in the file builds 2.
|
|
Finding 1 is the best catch of the five rounds, because the loss is reached by following the tool's own advice. Reproduced: Now: The opt-out stays — an explicit refresh that finds less is a result the caller asked for — but it is no longer silent. 2. The spliced doc commentsMine, from inserting 3. Fail-closedAlso mine, from last round, and the sharper half of the finding is that the PR argues twice that an unknown status and an unknown licence are not grants and then resolves the unknown case permissively three lines from a routing decision that goes the other way. Switched to the allowlist form: PUBLIC_URL_ACCESS_TYPES = frozenset({None, "open", PUBLISHER_FREE_ACCESS})with a The vestigial parameter was hiding a bug
Smaller things
On the self-reinforcing loopYour first smaller note is the one I would most like to keep on the record, and I agree it is not this PR. The loop is real — a preserved entry is never stamped, so it is re-fetched every run, and the fetch storm is what produces the rate-limiting that produces the bot pages that cause the preserving. Your suggested shape (a metadata marker recording "a refresh was refused at extractor version N", distinct from the stamp) sounds right, and I would add that it also wants a way to distinguish "refused because the source was rate-limited" from "refused because this article genuinely has no better text", since those want different retry behaviour. Happy to open an issue for it if you would like, or leave it for whoever picks it up. 1107 passing, mypy, Ruff, 126 doctests and |
d4c9e3a to
5778d11
Compare
|
Claude finished @cmungall's task in 5m 30s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
Both findings from the last pass are closed, and the second one is closed better than I asked for: Three things on the new head — one in the arithmetic that replaced it, one in the relaxation that replaced the other, and one test that does not test what it is named for. 1. Subtracting the whole common prefix makes the verdict depend on where the text differs, not how much of it is lost
The degenerate case is reached whenever Concretely, a re-extraction that is byte-identical except that the extractor now trims a trailing section — an acknowledgements block, a final page of a PDF, a footer — is refused, and refused permanently by the never-written → never-stamped loop, with
2. The unverified-HTML relaxation keys on the cached entry alone, so it also relaxes the downgrades
|
|
Both findings are right, and finding 1 is worse than the estimate in the review. I checked the margin against this repo's own reference cache rather than reasoning about it: So the median abstract already defeats the floor on a 10k entry, and p75 defeats it at 15k. For an ordinary short paper the relaxed bar was entirely spent before the extraction was weighed at all. 1. The ratio now measures recovered text
Your diagnosis of why no test caught it was exact, and it caught two of mine in turn. My first version of the test passed the raw cache file text as 2.
|
5778d11 to
bc36540
Compare
|
Claude finished @cmungall's task in 4m 52s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
You took the closing observation from the last round seriously, and I think you were right to: The two rules either side of it are unchanged and still look right. Three things, all downstream of the deletion rather than in it. 1. The case the ratio existed for is now not merely unguarded but silentAccepting the cover-page overwrite is defensible, and the argument in the docstring is the one I'd make: a refusal is permanent (never written → never stamped → re-refused every run), an acceptance costs one entry, and a shorter extraction is usually a better one. The asymmetry is real. But the conclusion drawn from it is wider than the argument. "Don't refuse on size" does not imply "don't mention size". Today A warning has none of the properties that made the refusal harmful: it does not block the write, so the entry is still stamped, the migration still completes, if not self._refresh_loses_full_text(cached, fresh) and _is_much_shorter(cached, fresh):
logger.warning(
"Refresh of %s replaced the cached %s entry (%d characters) with %s. "
"Written as usual -- a shorter extraction is often a cleaner one -- "
"but check it if quoted excerpts stop verifying.",
...
)That keeps the decision where you've put it and restores the observability the ratio incidentally provided. Fix this → Related, and cheap: the docstring defers the real fix with "see the issue linked from the PR" ( 2.
|
|
All three findings are right, and I have taken them as evidence about the design rather than fixing them in place. I said last round that a seventh round of findings in the same spot would be a signal the ratio wants to be simpler rather than better tuned. That is what this is, so the shrink comparison is gone. Finding 3 is what settled it. What the ratio was actually buying
The rule alone refuses both cases that motivated the guard and allows all four migrations the ratio wrongly blocked. Its entire net contribution was the cover-page PDF, and the price was four rounds of defects — every one of which failed in the worse direction, because a refusal is never written, so never stamped, so re-refused on every later run. A guard meant to protect the cache pinned it permanently at its pre-migration content. Your finding 1 makes that concrete in a way I had not seen: with the whole-prefix subtraction, an extractor that merely trims a trailing section loses everything, with certainty, while the same loss mid-document sails through. I was about to add a fourth special case to a heuristic whose three existing special cases had each been wrong once. The change
All three of your findings are closed by deletion, along with rounds 4, 5 and 6's. The gap this leavesThe cover-page PDF is now allowed, and I would rather that be visible than implicit, so Your own review pointed at where it belongs: Smaller things
1105 passing (four fewer than last round, which is the eight deleted tests less the four added), mypy, Ruff, 126 doctests and |
bc36540 to
d1a2f40
Compare
|
Claude finished @cmungall's task in 3m 1s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
The deletion is the right call and it is complete: nothing in Finding 3 from last round is closed thoroughly: Finding 1 you implemented rather than deferred, which I think was right. Two things in the reporter it introduced. 1. The new report fires on the refused path too, and says the opposite of what happened
self._report_shrinking_refresh(normalized_reference_id, fresh) # line 427
cached = self._load_from_disk(..., allow_stale=True, allow_stale_html=True)
if cached is None or self.needs_full_text(cached):
return None
if not self._refresh_loses_full_text(cached, fresh):
return None
logger.warning("Refresh of %s returned %s; keeping the cached %s entry ...")
The first is false in every clause that matters: nothing was replaced, nothing was written, and the advice to check the new text describes text that does not exist. And this is not an edge case — it is the PR's motivating entry, on the path the whole guard exists for, since an abstract is essentially always under 20% of a cached body. Every preserved entry now emits a contradictory pair, every run.
The fix also collapses a second problem for free. The reporter re-loads and re-parses the cache file the guard is about to load two lines later, so every non-force fetch that reaches this method now costs two reads where it previously cost one — and this method runs on every reference the fetch actually goes to the source for, not only preserved ones. Taking 2. The report compares whole records, so the abstract defeats it at exactly the size where it mattersRound 7's finding 1 was that the ratio measured The consequence has flipped to the safe direction (a missed warning, not a permanent refusal), which is why this is a note on a new feature rather than a repeat of the finding. But it silences the reporter on the case it was added for. Using your own measurement from round 7 — 2,924 abstract-only entries, median 2,024 characters:
So for an ordinary short paper the cover-page overwrite is silent again, and the median abstract is most of the fresh side. Smaller things
TestsWhat remains is the set that matches the rule as written: the deletion claim over all three full-text types, the serve/keep split per type, both The two gaps map onto the findings: nothing asserts what is not logged on the refusal path (finding 1), and nothing builds a I could not run the suite — |
|
Finding 1 is a distinction I had collapsed, and you are right that it does not follow. "Don't refuse on size" and "don't mention size" are separate conclusions, and only the first one was supported by the evidence. What made refusing harmful was that it is permanent: never written, never stamped, re-refused on every later run. A warning has none of those properties — the write proceeds, the entry is stamped, the migration completes,
2 and 3 — cleanup debt from my own deletionBoth fair, and both the kind of thing that only shows up when someone reads the result rather than the diff. The The test file kept the shape of the ratio after the ratio had gone: three dead helpers, three empty section banners, two docstrings explaining failures in terms of a "strict ratio" and a "quality ranking" that no longer exist, and On the docstring's forward referenceAgreed — CIThe red tick on this run is not the branch. There is an irony in it that is also a small observation: a PR about transient upstream failures being recorded as facts has a suite that does the same thing to itself. A live test that fails on a 500 is reporting NCBI's availability, not the code's correctness, and it will keep doing so. 1091 passing with the live-network tests deselected, mypy, Ruff, 126 doctests and |
d1a2f40 to
0f7ed3a
Compare
|
Both findings are bugs in the report I added last round, and finding 1 is worse than the description. Reproduced before fixing: Two warnings, back to back, one saying the entry was replaced and the next saying it was kept. Not merely a message on the wrong path — a self-contradicting pair, which is worse than the silence the report was added to fix. It now fires inside the branch that returns without preserving, which is the only place a report of a write can be true. 2. The abstract, againReproduced too: a cover-page PDF carrying the abstract wrote in silence against a 10k cached entry — precisely the case the report exists for, at precisely the article size where it is most likely. This is the same defect as round 6's finding 1, reappearing in the code that replaced the thing it was found in. Worth naming, since the lesson is not "fix the arithmetic" but "this comparison keeps forgetting that
Both repros now behave: 1093 passing with live-network tests deselected, mypy, Ruff, 126 doctests and |
|
Claude finished @cmungall's task in 5m 22s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
Both of last round's findings are closed, and closed at the root rather than at the symptom. The two rules either side of the guard are unchanged and still look right. I have no correctness finding on the guard, the licence allowlist or the provider changes this round. What is left is about messages and docs — which is not a small category in a PR whose thesis is that a message misdescribing what happened is the defect. 1. The shrink report prints three numbers, and the two it shows the reader are not the two it decided on
cached_length = len(_text_after_abstract(cached.content)) # abstract removed
if len(_text_after_abstract(fresh.content)) >= cached_length * REPORT_SHRINK_RATIO:
return
logger.warning(
"... the cached %s entry (%d characters) with a much shorter one: %s ...",
cached.content_type, cached_length, _RefreshLoss(fresh, cached),
)but At that size it merely reads oddly. At the size the reporter was reinstated for it reads as wrong. Cached abstract 2,000 + body 3,000; refresh returns the abstract plus a 500-character cover sheet:
2,500 against 5,000 is half. A curator reading that cannot reconstruct why "much shorter" fired, and the natural conclusion — that the threshold is somewhere near 50% — is wrong. That branch of
2. The one warning a user will paste into a search box is the one the docs deny
The report is otherwise undocumented: it is the only user-visible output of this PR with no entry in the troubleshooting section, and it is specifically aimed at a curator whose quoted evidence stopped verifying — i.e. at someone who is already reading that page. One clause on the existing sentence (sizes are not compared to decide a refusal) plus two lines saying the warning exists and what to do with it would close it. Fix this → 3.
|
0f7ed3a to
72c2a3e
Compare
|
Claude finished @cmungall's task in 3m 52s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All three of last round's items are closed. The two rules either side of the guard are unchanged and still look right, and I have nothing further on the guard's arithmetic — it no longer has any. One finding a layer up from where the last several rounds have been, then two smaller ones. 1. A licensing decline is now recorded as
|
|
All three taken, and finding 1 was the one worth having: the message was not merely inconsistent, it was misleading about its own threshold. My test's captured output before the fix: 2,518 of 5,023 is half. A curator reading that would infer a threshold near 50%, which is wrong by a factor of two and a half, and the cached entry is quoted at two different sizes in one sentence. Landed on the convention you suggested, across all three call sites rather than just the one: every message now reports article text with the shared abstract subtracted, which is the quantity all of them decide on and the one at stake in each.
2. The docs denied their own warningFixed, and this was the one I would have been most annoyed to ship: a curator whose evidence stopped verifying searches the docs for the warning they just saw, and finds a passage explaining that sizes are not compared. The sentence is now qualified to the refusal, and the report has its own short section with a worked example and what to do about it — which is usually nothing, and occasionally re-fetch. 3. The CLI messageBoth clauses were two rounds stale. Reworded to the rule as it now stands, and Smaller things
1095 passing with live-network tests deselected, mypy, Ruff, 126 doctests and Ten rounds in, this is the first with no correctness finding, and the guard is now one rule plus one advisory line. Unless something surfaces here, I would merge on this one. |
72c2a3e to
992d02a
Compare
|
Claude finished @cmungall's task in 5m 21s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All three of the previous pass's findings are closed. The two rules either side of the guard are unchanged, and the guard itself is one line with its history in the docstring. I have nothing further on it. The new finding is in the flag fix — which I think is the right change, and stops one step short of the case that motivated the whole PR. 1. The
|
|
Finding 1 is the sharpest of the eleven rounds, and it lands squarely on this change's own argument. I had said I would merge on the last round; that was premature.
The consequence for the repository that prompted this is concrete. It has ~600 bronze entries; under the behaviour I was about to ship, every one would have been stamped "no full text available" and never looked at again, including the ones that convert to gold. The fixBoth cases now leave the flag unset, alongside The bronze case is easy — the location is in hand and gets declined at the Tests: a bronze-only DOI and a landing-page-only DOI are each cached What that broke, which is worth reportingThree of my own tests asserted I swept the rest — the remaining 2. The
|
Three changes, one policy: material a user may lawfully read is not automatically material a project may redistribute, and a checked-in reference cache is redistribution. That is the contract PR #61 drew for Zotero private libraries; OpenAlex and Unpaywall were not honouring it. Do not scrape landing pages. Both providers used `pdf_url or landing_url`, so a record with no file fell back to the article *page* and downloaded it. Hosts refuse this: PMC answers a rate-limited request with a reCAPTCHA interstitial served on an HTTP 200, so no status check downstream can tell it from article text. Measured on PMID:9177246, the same fetch returned real full text on 2 of 5 consecutive attempts; the other 3 cached the bot page's rejection as "no full text exists". Providers now return None when they have only a page. Bronze is not open. `oa_status: bronze` means free to read on the publisher's site under no open licence. New `access_type_for_oa_status` maps gold/diamond/ hybrid to "open" and everything else -- bronze, unrecognised, missing -- to "publisher_free". Nothing else was needed: `_enrich_with_full_text` already skips non-open locations. Unknown statuses fail closed, because an unknown licence is not a grant. A declined location is not an absence. full_text_attempted means "a clean run concluded no full text is available", and _maybe_retry_full_text never runs the chain again once it is set -- so folding a licence decline or a refused landing page into it would record a decision this project made as a fact about the article, which is this change's own thesis one layer up. Bronze-to-gold is a conversion publishers make, and a page-only DOI gains a repository PDF; neither would ever be looked at again. Both now record a `full_text_declined` reason instead of the absence flag. Recording rather than leaving nothing is the difference between retryable and retried: a decline never clears the way a transient error does, so an unset flag would re-walk the four-provider chain on every run for every such reference -- measured on one real corpus, 23,465 eligible entries and roughly thirteen hours of rate-limit sleep per run, aimed at the hosts whose rate limiting produces the interstitial this change defends against. The decline is re-examined when the entry is next re-fetched. The same laundering is fixed in PMC, which is the host in the original report: a 429 and a swallowed elink exception both returned None, which the chain read as a clean absence, so an article with a PMC body fetched during a rate-limit window was permanently recorded as having none. Both now raise into the existing had_error path; the undecidable 200-with-no-body case is unchanged. FullTextLocation gained a `declined` field so a provider can say it refused a candidate rather than returning a bare None, which the chain cannot tell from finding nothing; a declined location carries no url or text and is never fetched. The licence rule governs files found by asking an OA index. It deliberately does not govern the pmc and epmc_preprint providers, which ask an archive's own API for a document it serves for machine retrieval -- a stronger warrant than an index's summary of a third-party host, and routing them through it would decline most PMID full text for want of a licence field their API does not return. The same green deposit can be declined from OpenAlex and admitted from PMC; the asymmetry is recorded rather than left implicit. One page fetch also survives, in the pmc provider, and is safe for a reason the oa_url fallback never had: it returns text only from a div.article-body or div.tsec, which an interstitial does not carry. Green is decided by its licence, not by the status. The status names a repository rather than a permission -- a PMC author manuscript is free to read under a funder policy whose redistribution terms vary by publisher, which is the same shape of claim as bronze with a different host. A green location is open when it states a licence and not when it merely states its address. "States a licence" is an allowlist, not a non-empty field: OpenAlex's vocabulary includes other-oa (5.9M works) and publisher-specific-oa, and Unpaywall documents implied-oa, all of which are truthy strings naming the *absence* of a licence statement -- and a bare funder-policy repository deposit is exactly where they appear, so a truthiness test would reopen the route this rule closes. The source URL is recorded for an allowlist of access types (unset, open, publisher_free) rather than suppressed for a denylist, so an access type this version has not heard of stays suppressed -- the same fail-closed default this change applies to an unknown oa_status and an unknown licence. A non-open access type no longer discards the source URL. That rule was written for a private-library endpoint, where a localhost Zotero attachment address is session-specific and meaningless elsewhere; a publisher link is a stable public address, and what licensing keeps out of the public cache is the text rather than where it came from. A known miss is recorded in the docstring: a work's oa_status is its best across locations, so a work marked bronze may carry an openly-licensed repository copy this code never inspects. A refresh may improve an entry; it may never demote one. Without this the first change would itself destroy data: the #62 extractor stamp makes every pre-stamp entry stale, so the next run re-fetches, and with scraping removed those entries would come back abstract-only and overwrite themselves. `_stale_fallback` does not cover it -- that fires only when the source yields nothing, and here the abstract is something. `_preserve_cached_full_text` refuses a write that comes back with no full text at all, leaves the entry stale so a later run retries, and steps aside for `force_refresh`, which warns before it discards. The rule is about kind, not size. An earlier revision of this branch also refused a refresh whose text was a fraction of the cached length, to catch a PDF whose text layer is a publisher cover sheet. It caught that one case and mis-handled four others -- a scraped page replaced by a clean XML body, by a clean HTML body, a plain-text API body replaced by XML, and a re-extraction that merely trimmed a trailing section -- because a shorter extraction is usually a better one and a length comparison cannot tell those apart. Every one of those failed in the worse direction: a refused entry is never written, so it is never stamped, so every later run re-fetched and re-refused it. Judging whether text *is* an article belongs in the acceptance layer, alongside is_stub_notice, where a wrong answer costs one skipped fetch rather than a cache that can never migrate. The cover-page gap is recorded by a test rather than left implicit. Not refusing on size is not the same as not mentioning it. A refusal is permanent -- never written, never stamped, re-refused every run -- while a log line lets the write proceed and costs one line if it is wrong. So a refresh that keeps full text but returns a small fraction of the cached length is reported and written, which restores the observability the ratio provided incidentally without restoring the harm it caused. It is reported only on the branch that writes, since a report of a write is false anywhere else, and it measures the text after the abstract both records carry, since a median abstract alone clears a fifth of a short article. Every message quotes that same subtracted length, so the figures a reader sees are the figures the threshold compared. Keeping an entry and serving it are separate questions, and the method asks both. Whether it may be overwritten is asked with allow_stale_html=True, because a stale HTML entry is still somebody's data. Whether its text may be served as evidence is asked without that bypass, so _load_from_disk's refusal of stale HTML -- "it may be a repository landing page" -- still holds. A pre-fix entry scraped from a landing page is therefore kept on disk and withheld from validation, which falls back to the freshly fetched abstract. Built on #84: the guard returns a FetchOutcome, and both of its branches set served_stale. That command's message now covers both paths -- it could not be re-fetched, or the refresh was refused -- rather than naming only the first, which was untrue in the new branch. A preserved entry makes it exit 1 on every run until --force, which the docs now say. Verified end to end on a real cache entry: an 18,464-character `full_text_html` record survives a refresh that returns only an abstract, the file on disk is untouched, and the loss is a WARNING rather than a silent rewrite. Tests: 17 new across two files, written before the implementation. `test_locate_falls_back_to_oa_url` asserted the scraping this removes; it is inverted rather than deleted, so the file records why a landing page is not a located full text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
992d02a to
467a8d2
Compare
|
Claude finished @cmungall's task in 4m 0s —— View job Review: Fetch only openly-licensed files, and never demote a cache entry
All three of the previous pass's findings are closed. Three things, all in the interaction between the two new signals. 1. A decline and a transient error in the same run: the decline wins, so the transient failure stops being retried
if declined_on_policy:
content.full_text_declined = declined_on_policy
elif not had_error:
content.full_text_attempted = True
The combination is not exotic — it is the motivating article. Default chain is
The PMC body that was one un-rate-limited run away is now out of reach until a version bump. That is the defect this commit's other half exists to prevent, arriving through the half that was added alongside it. One line — 2. A decline reached through the retry path is computed and thrown away, so those entries re-walk the chain every run anyway
before = (content.content, content.content_type, content.full_text_attempted)
content = self._enrich_with_full_text(content)
after = (content.content, content.content_type, content.full_text_attempted)
if after != before:
self._save_by_access(content)
The population is specific but is the one this release creates. An entry written with Adding 3. A 404 from the PMC article page is now a transient failure, and a transient failure is retried forever
The consequence is the mirror of the bug being fixed, and it has the shape the PR spent several rounds eliminating elsewhere. The status code already distinguishes them: 429 and 5xx (and arguably 403, which PMC uses when blocking) are "ask me again"; 404 and 410 are "there is no page here" and should return Smaller things
Tests
Gaps map onto the findings: nothing runs a chain where one provider errors and another declines (finding 1), nothing reaches the chain through I could not run the suite — |
|
Finding 3 is the one that mattered, and it is a cost in my last change rather than a pre-existing one. I measured it against the corpus that prompted this work rather than reasoning about it: At four providers and a 0.5s opening sleep each, leaving the flag unset makes those 23,465 entries re-walk the chain on every run: roughly thirteen hours of sleep alone per validation, before the ~94,000 HTTP requests — aimed, as you say, at exactly the hosts whose rate limiting produces the interstitial this change exists to defend against. I would have shipped something that made that repository's validation unusable and fed the failure it fixes. Taken your remedy: the decline is recorded as 1. PMCReproduced before acting, per your note: A rate limit recorded as a fact about the article, in the host whose interstitial is this PR's evidence. You are right that it is pre-existing and right that this is the PR that should fix it, since it is the one that states the rule and builds the vocabulary for the exception. Took the cheap half exactly as scoped. A non-200 from 2.
|
1 participant
A cached reference could hold an article's full text one day and only its abstract the next, with nothing to say why. Three changes stop that, and stop the validator fetching text it has no licence to redistribute.
The trigger was a real dismech cache entry:
PMID_9177246.mdheld 18,464 characters of a 1997 PNAS paper, and a routine re-fetch rewrote it to a 1,199-character abstract. The quoted evidence in the knowledge base stopped validating. No error, no warning.What was happening
Three things compounding.
The fetch was scraping a web page. Both
OpenAlexProviderandUnpaywallProviderusedpdf_url or landing_url. When a record has no file, that falls back to the article page and downloads it. PMC refuses this — it answers a rate-limited request with a reCAPTCHA interstitial:served with HTTP 200, so nothing downstream can distinguish it from article text. Ten consecutive downloads of one URL returned
152089 152089 152089 21293 21294 21293 21293 21293 21292 21294bytes — the 21 KB responses are the bot page.The failure was recorded as a fact about the article. The bot page is correctly rejected, the abstract fetch succeeds on its own, and the record is written
content_type: abstract_onlywithfull_text_attempted: true— byte-identical to "this article has no full text."The refresh was automatic. The extractor stamp from #62 marks every pre-stamp entry stale, so the next run re-fetches it. Before #62 an entry already marked
full_text_htmlwas never re-fetched, so this flakiness only ever affected first fetches, where failing loses nothing. It now overwrites committed content.What changed
A landing page is not full text. Providers return a location only for a file (
pdf_url,url_for_pdf). With only a page, they returnNoneand the record staysabstract_only.Bronze open access is not redistributable.
oa_status: bronzemeans free to read on the publisher's site under no open licence. Newaccess_type_for_oa_statusmapsgold/green/diamond/hybridtoopenand everything else topublisher_free. No new gate was needed —_enrich_with_full_textalready skips non-open locations, which is the route #61 built for Zotero private-library material.access_typewas previously set by the Zotero provider alone, so everything the other providers returned defaulted to "open" and was saved as public.An unrecognised or missing status is deliberately not treated as open: an unknown licence is not a grant.
A refresh may improve an entry; it may never demote one.
_preserve_cached_full_textrefuses a write that loses full text. This is not optional alongside the first change — without it, removing scraping would itself destroy data, because every stale entry would re-fetch, come back abstract-only, and overwrite itself._stale_fallbackdoes not cover this case: it fires only when the source yields nothing, and here the abstract is something. The guard is narrow — only a loss is refused, the preserved entry stays stale so a later run retries, andforce_refreshoverrides it.Verified on the real entry
And a fresh fetch now declines rather than scrapes:
content_type: abstract_only | provider: None. No reCAPTCHA request is made.Behaviour changes to expect
abstract_only, and excerpts quoted from that text stop verifying. This is the intended fix, but it will look like a regression if you are not expecting it.One thing worth a second opinion
greenis currently treated as open, on the grounds that a repository deposit is author- or repository-licensed, and the no-scraping rule already catches the awkward case (green-via-landing-page is refused for being a page, before licence is considered). If onlygoldanddiamondshould count as redistributable, that is a one-line change toOPEN_LICENCE_OA_STATUSES.Tests
17 new across two files, written before the implementation.
test_open_access_policy.py— no landing pages from either provider, PDFs still returned, each open status maps toopen, bronze and unknown do not, and one test pinning the join: a bronze location never reaches_apply_full_text_location.test_no_cache_downgrade.py— a failed refresh keeps both the content and the file on disk; the entry stays un-stamped so migration retries; a successful refresh still rewrites; abstract→full-text upgrades are allowed; abstract→abstract is not a downgrade; no cached entry means nothing to preserve.test_locate_falls_back_to_oa_urlasserted the scraping this removes. It is inverted rather than deleted, so the file records why a landing page is not a located full text.Full suite, mypy, Ruff, doctests and
mkdocs build --strictall pass.🤖 Generated with Claude Code