Keep a line break in metadata, and one cache file per DOI

[cmungall]

A note on this branch's history. The first review round was applied by
amending ef92485 and force-pushing, so that commit no longer exists and the
PR shows its replacement, c61044d, under the original headline. The review
fixes are in that commit — memoized case resolution, deterministic
tiebreaking, the wider line-break guard, and preserving the stored
reference_id — but the diff showing what changed in response to the review
is gone, which is exactly the thing a second reviewer would want. That was the
wrong call and I am not repeating it: later rounds are added as commits, so
fdf0cee and anything after it can be read on their own.

Two small defects in how a cache entry reaches disk. Both were found downstream in dismech, where each had been worked around with a runtime monkeypatch over this module's internals — and one of those patches has just started crashing on main, which is the better argument for fixing them here.

A line break in a title breaks the entry

_quote_yaml_value quotes on a character list that does not include \n. Crossref titles do contain literal line breaks, so the value is interpolated straight into the frontmatter and the file no longer parses:

ruamel.yaml.scanner.ScannerError: while scanning a simple key
    with a literal newline
    ^ (line: 5)
could not find expected ':'

Re-fetching rewrites the same broken file, so nothing downstream recovers it.

Such values are now emitted JSON-style, which is valid YAML and reloads with the break intact. Folding the break into a space would also parse, but it silently edits metadata — and the title is compared against the fetched record elsewhere, so an edited one reads as a mismatch.

A DOI in two capitalizations gets two cache files

DOI names are case-insensitive by specification. The prefix is normalized; the suffix is not:

DOI:10.1016/S0002-9440(10)63332-9  ->  DOI_10.1016_S0002-9440(10)63332-9.md
doi:10.1016/s0002-9440(10)63332-9  ->  doi_10.1016_s0002-9440(10)63332-9.md

One reference, two files: a duplicate download, and for a project that commits its cache, two tracked entries for one paper. On a case-insensitive filesystem those two paths then collide in git, which is how this surfaced — a repo in that state cannot rebase, checkout or stash on a clean tree.

A DOI path now resolves to whatever spelling already exists in the cache directory.

Resolution finds the existing file rather than imposing a canonical spelling, so no existing cache is renamed: the downstream project holding 6,721 DOI entries sees no diff, and whichever spelling was written first keeps winning.

The directory scan comes first, and exists() is deliberately not consulted. On a case-insensitive filesystem exists() is true for a spelling that is not the one on disk, so trusting it returns the caller's spelling and writes exactly the colliding second file this is meant to prevent. My first attempt had that bug and the tests caught it.

Scoped to DOI on purpose — a PMID or an NCT id is not case-insensitive, and folding those would merge genuinely distinct references.

Why here rather than downstream

The DOI workaround wrapped _cache_path, which was a @staticmethod in v0.2.1 and is an instance method on main. Against current main the patch now raises at import:

AttributeError: 'function' object has no attribute '__func__'

which takes down every validation run rather than quietly losing the fix. That is the normal fate of a patch over a private API, and it is why both of these belong upstream.

Since the first review

• Case resolution is memoized per directory. Scanning on every call was quadratic — _cache_path is on the read and write paths — measured at 6.7 ms a call against a 6,721-entry cache, or 45 s of pure path resolution for a run touching every reference. Now 0.57 s.
• Resolution is deterministic when both spellings are already on disk: the caller's exact spelling wins, then lexicographic order. iterdir() order is arbitrary, so the previous version could hand back the other spelling and differ between checkouts.
• The line-break guard covers every break the scanner honours, not just \n and \r. Fixing it surfaced that json.dumps(ensure_ascii=False) leaves U+0085, U+2028 and U+2029 literal, so they were still folded to a space on reload; those are now escaped explicitly.
• An existing entry keeps its stored reference_id. The path resolving to one file was not enough: rewriting that line under the caller's capitalization left a one-line diff in a committed cache on every cross-spelling re-fetch.
• forget_cache_listing() (fdf0cee) lets a caller drop the memo after an outside writer touches the directory — a window dismech#12083 already hit with a subprocess-based backfill.

Tests

18, written first, in tests/test_frontmatter_and_doi_case.py: eight line-break shapes round-tripping (including U+0085, U+2028, U+2029, vertical tab and form feed, all verified to produce the same unparseable file), single-line quoting unchanged, two capitalizations sharing one path, a save under each spelling producing one file, an existing entry keeping its own capitalization and its stored reference_id, both spellings on disk resolving deterministically, at most one directory scan across many resolutions, a non-DOI reference not being folded, and the stale-index window closing after a reset.

One skips on a case-insensitive filesystem — the both-spellings case cannot be set up there — so it is real coverage only on Linux/CI.

Full suite 1119 passing (live-network tests deselected), mypy, Ruff, doctests and mkdocs build --strict clean.

🤖 Generated with Claude Code

[claude]

Claude finished @cmungall's task in 3m 21s —— View job

---

Review: Keep a line break in metadata, and one cache file per DOI

• Read the diff and surrounding code
• Analyze the YAML newline-quoting change
• Analyze the DOI case-folding change
• Check test coverage
• Post review

Both defects are real and the diagnosis in the description is accurate. The exists() trap is correctly identified and correctly avoided, scoping the fold to DOI is right (the DOI Handbook makes DOI names case-insensitive; PMID/NCT are not), and resolving to the existing spelling rather than imposing a canonical one is the right call for a committed cache. Tests were written first and exercise real round-trips rather than mocks, per CLAUDE.md.

Six things below, roughly in order of how much they matter. The first two both bite hardest on exactly the downstream project this is for.

---

1. Resolution is non-deterministic when both spellings already exist

reference_fetcher.py:1089-1096 returns the first iterdir() match. Directory order is arbitrary (inode/hash order, not sorted), so when a cache already holds both DOI_10.1016_S0002-9440(10)63332-9.md and doi_10.1016_s0002-9440(10)63332-9.md, which one wins is not defined — and can differ between two checkouts of the same repo, or before and after a git checkout rewrites the files.

That mixed state is not hypothetical: it is precisely the state the PR describes downstream ("two tracked entries for one paper"), and it is what every case-sensitive Linux checkout of that repo looks like today. The caller's own exact spelling is not even preferred, so asking for UPPER can hand back the lower file.

Preferring an exact match among the names iterdir() actually returned does not reintroduce the exists() bug the docstring warns about — the distinction is that these are real on-disk names, not a case-insensitive lookup:

folded = path.name.casefold()
matches = [c for c in path.parent.iterdir() if c.name.casefold() == folded]
if not matches:
    return path
return next((c for c in matches if c.name == path.name), min(matches))

The min() fallback makes the two-file case stable rather than arbitrary. Worth a test for a directory seeded with both spellings — its absence is why this slipped through.

Fix this →

2. A full directory scan per DOI path resolution is O(N²) over the cache

_existing_case_variant runs iterdir() plus up to one casefold() per entry every time a DOI cache path is computed — and _cache_path is on both the read path (get_cache_path, _load_from_disk) and the write path (_save_to_disk), including the loop at cli/cache.py:233.

For the 6,721-entry cache named in the description, a run touching every reference is ~6,700 directory scans of ~6,700 entries each: tens of millions of dirent reads and string folds where there were none before. Building the cache from scratch is quadratic too.

A casefold(name) -> real name dict built once per cache directory (invalidated when _save_to_disk creates a file, or rebuilt on miss) collapses this to one scan. Since path.parent is only ever one of two directories, keying the memo on the parent path is enough.

Fix this →

3. The filename stops churning, but reference_id: inside the file does not

The path now resolves to the existing spelling, but _save_to_disk (line 1181) still writes reference_id: {reference.reference_id} with whatever spelling the caller used. So a re-fetch under the second capitalization rewrites the frontmatter of the first one's file — a one-line diff in a committed cache, which is the class of churn this PR is otherwise trying to eliminate. (Reads are fine: _load_markdown_format returns frontmatter.get("reference_id", ...), line 1567, so the stored spelling wins on load.)

Preserving the stored reference_id when an entry already exists would close the loop. Arguably a follow-up rather than a blocker, but it is the same defect one layer in.

4. The line-break test is narrower than the set of characters that break the parse

reference_fetcher.py:1135 triggers on "\n\r" only. The frontmatter is read by ruamel.yaml, whose scanner also treats \x85 (NEL), 
 (LS) and 
 (PS) as line breaks, and whose reader rejects \x0b/\x0c outright as non-printable. Any of those in a title lands in the plain-scalar path and produces the same unrecoverable file — the exact failure mode being fixed, just with a rarer character. (I was not able to execute code in this run to confirm the ruamel behaviour empirically; the reasoning is from the PyYAML-derived scanner/reader, so treat it as strongly suspected rather than verified.)

str.splitlines() splits on all of them, which makes the guard a superset at no cost:

if value.splitlines() != [value]:
    return json.dumps(value, ensure_ascii=False)

Note len(value.splitlines()) > 1 would not work — "Trailing break\n".splitlines() is ['Trailing break'], so it would miss the trailing-break case your own parametrization covers. The != [value] form catches it. It also sends "" down the JSON path, emitting "" instead of an empty plain scalar, which is harmless and arguably more correct.

Fix this →

5. The legacy .txt fallback is not case-folded

_load_from_disk (line 1336) does cache_path.with_suffix(".txt") on the resolved path. Since _existing_case_variant matches on the full name including the .md suffix, a legacy .txt entry written under a different capitalization is still missed. Small and shrinking surface — flagging for completeness rather than asking for a change.

6. try/except OSError where a predicate would do

CLAUDE.md asks to avoid try/except outside genuine external-system boundaries. The filesystem qualifies, so this is defensible — but the dominant case being swallowed is "cache directory does not exist yet", which if not path.parent.is_dir(): return path expresses without exceptions, leaving a narrower except (or none). The current form also swallows a mid-iteration failure, which would silently produce a second file rather than surface.

---

Tests

Good shape: TDD, real _save_to_disk → _load_markdown_format round-trips, three line-break shapes including the trailing one, and an explicit backwards-compatibility test that an existing entry keeps its capitalization. The negative test on PMID is the right guard against over-folding.

Gaps, mapping to the above:

• no case where both spellings are already on disk (Bump actions/checkout from 4 to 5 #1)
• no save-under-the-second-spelling test — test_the_second_capitalization_reads_the_first_one_s_entry asserts one file after a single save, so it would not catch a second write creating a second file
• no coverage of the frontmatter reference_id after a cross-spelling re-fetch (Bump astral-sh/setup-uv from 6 to 7 #3)

The doctest at line 1126-1127 escapes correctly for a non-raw docstring.

---

I was not able to run just test, mypy or the doctests in this environment — Bash execution beyond git/grep was not permitted for this run, so the "1111 passing" claim in the description is unverified here, not disputed. Everything above comes from reading the code.

Nothing here is a reason not to land the two fixes; #1 and #2 are the ones I would want addressed before this reaches the 6,721-entry cache, since that is where both are worst.
• fix/newline-titles-and-doi-case

[claude]

Claude finished @cmungall's task in 4m 22s —— View job

---

Re-review: Keep a line break in metadata, and one cache file per DOI

• Check what changed since the last review
• Re-read the diff and surrounding code
• Verify the ruamel line-break claim empirically — not possible, Bash is again restricted to git/grep/sed in this run
• Run the test suite, mypy and doctests — not possible, same restriction
• Post updated review

---

The force-push addresses all four substantive points from the last round, and addresses them in the right way rather than the cheap way:

• Bump actions/checkout from 4 to 5 #1 determinism — _existing_case_variant (1099) now prefers the caller's exact spelling among the real on-disk names and falls back to min(names), so the both-spellings-present case is stable. test_both_spellings_on_disk_resolve_deterministically covers it, with sensible skip guards for a filesystem that cannot hold both.
• Bump actions/setup-python from 5 to 6 #2 the O(N²) scan — _case_index (1135) builds once per directory, _remember_cache_file (1153) keeps it current on write. The lazy build plus incremental update is the right shape, and exists() is still correctly never consulted.
• Bump astral-sh/setup-uv from 6 to 7 #3 reference_id churn — _stored_reference_id (1083) closes the loop the filename fix left open.
• Bump actions/checkout from 4 to 6 #4 the guard — value.splitlines() != [value] (1202), with _UNESCAPED_YAML_BREAKS (141) handling the three breaks json.dumps(ensure_ascii=False) leaves literal. That second half I had not thought of and it is necessary: an unescaped 
 inside a double-quoted scalar folds to a space, which is exactly the silent metadata edit the JSON form exists to avoid.

Six things below. The first is the one I would want fixed before this lands.

---

1. Two live fetchers share a cache directory but not the index, so the duplicate file can still appear

Repairer.__init__ holds two ReferenceFetcher instances against the same cache directory:

# validation/repairer.py:113-114
self.validator = SupportingTextValidator(validation_config)   # builds its own fetcher (supporting_text_validator.py:90)
self.fetcher = ReferenceFetcher(validation_config)

_case_indexes is per-instance (246) and _remember_cache_file only updates its own owner's copy (1155). So once the validator's fetcher has built its index, every file the repairer's fetcher writes afterwards is invisible to it — and vice versa. A repair run interleaves both across many references, so going stale is the normal case, not the corner one. A DOI that arrives uppercase through one fetcher and lowercase through the other then resolves to a miss on the second and writes the second file: precisely the defect this PR exists to remove, surviving in the code path most likely to hit it.

The pre-memo version did not have this — a per-call iterdir() saw every writer. That is the cost of the speedup, and it is cheap to buy back: hoist the index to a module-level dict keyed by directory. Every write already funnels through _remember_cache_file, so one shared map stays accurate for the whole process, and the per-directory scan is still paid once rather than once per instance.

A test would be two fetchers over one tmp_path, the second resolving a DOI before the first writes it, then asserting one file after both save under different spellings.

Fix this →

2. splitlines() catches the line breaks; DEL and the rest of the C1 block still reach the plain scalar

The guard now covers every break ruamel's scanner honours. But ruamel's reader rejects a second set outright as non-printable — \x7f and \x80–\x9f minus \x85 — and those are:

• not split by splitlines(), so they never reach _json_scalar, and
• not escaped by json.dumps(ensure_ascii=False) either, since it only escapes below 0x20.

So they are written literally into the frontmatter and produce the same unrecoverable file, just with a ReaderError instead of a ScannerError. The realistic source is mojibake rather than exotica: Windows-1252 bytes decoded as Latin-1 turn smart quotes and en-dashes into \x91–\x97, which is a common shape for bibliographic metadata to arrive in.

_UNESCAPED_YAML_BREAKS generalises to cover both jobs at once:

_YAML_UNSAFE = re.compile("[\x7f-\x9f

]")

def _json_scalar(value: str) -> str:
    rendered = json.dumps(value, ensure_ascii=False)
    return _YAML_UNSAFE.sub(lambda m: f"\\u{ord(m.group()):04x}", rendered)

with the routing condition widened to match:

if value.splitlines() != [value] or _YAML_UNSAFE.search(value):

Same caveat as last time, and it still applies because I could not execute code in this run either: the reader/scanner behaviour is reasoned from the PyYAML-derived NON_PRINTABLE character class, so treat it as strongly suspected rather than measured. A parametrized case for "Smart quote\x92s" would settle it in one run.

Fix this →

3. A duplicated line

# reference_fetcher.py:1369-1370
self._remember_cache_file(cache_path)
self._remember_cache_file(cache_path)

Idempotent, so harmless, but it is an artifact rather than intent.

4. _stored_reference_id turns an undecodable cache file into a failed save

except OSError (1096) does not cover UnicodeDecodeError, which is a ValueError. Before this change _save_to_disk never read the existing file, so a truncated or externally-mangled entry was simply overwritten and repaired. Now it raises and takes the save with it — the file that most needs rewriting becomes the one that cannot be.

Only the reference_id: line is used, so cache_path.open(encoding="utf-8", errors="replace") removes the failure mode at no cost and without widening the except.

5. _stored_reference_id is not scoped to DOI, but its justification is

The docstring argument — the stored id and the caller's id are the same reference — holds for the case-folded DOI. It does not hold generally: filename sanitization collapses : / ? = to _, so url:https://ex.com/a?b=c and url:https://ex.com/a/b/c both land on url_https___ex.com_a_b_c.md. That collision predates this PR, but its handling changes: previously the last writer's id was recorded, now the first one is cemented and every later write reasserts it. Gating on the same prefix.upper() == "DOI" test already used at 1078 keeps the change to the case it is argued for.

6. The legacy .txt fallback is still not folded

_load_from_disk:1410 does cache_path.with_suffix(".txt") on the resolved path, and the index is keyed on the full name including .md, so a legacy entry written under a different capitalization is still missed. Unchanged from last round, and still fine to leave — noting it only so it is a decision rather than an oversight.

---

Tests

The three gaps I named are all closed, and closed with tests that would actually have caught the bugs: test_saving_under_the_second_spelling_does_not_create_a_second_file does the second write the earlier test was missing, test_a_cross_spelling_refetch_keeps_the_stored_reference_id asserts on frontmatter rather than filename, and test_resolution_does_not_scan_the_directory_per_call counts real iterdir calls instead of asserting a timing. The line-break parametrization picked up \x85/
/
/\x0b/\x0c.

Remaining gaps map to the above: no two-instance case (#1), no non-line-break unprintable (#2).

The doctest at 1187 escapes correctly for a non-raw docstring — "Two\\nlines" in source reaches doctest as a real newline and '"Two\\\\nlines"' matches the repr.

Verification

Same limitation as the previous round: Bash was restricted to git/grep/sed, so I could not run just test, mypy, the doctests or mkdocs build --strict. The "1111 passing" claim is unverified here rather than disputed — everything above comes from reading the code. If you want these runnable in a future pass, uv would need to be in --allowedTools.

Net: the change is in good shape and #3–#6 are all small. #1 is the one I would not ship without, because the repair path is where a mixed-case cache is most likely to be touched from both fetchers in a single run.
• fix/newline-titles-and-doi-case

[cmungall]

All six taken. Findings 1 and 2 were right to be called out as worst where this is headed, and I measured both rather than reasoning about them.

2. The quadratic scan

You were right, and it was worse than an inefficiency — it was a regression I introduced, against a 6,721-entry cache:

per DOI path resolution: 6,730 us      ->  85 us
x 6,721 references     :   45.3 s      ->  0.57 s

Memoized per directory, invalidated by _remember_cache_file on write so a later lookup in another capitalization still finds a file written in the same process.

Worth recording: the downstream workaround this PR replaces did keep a per-directory index. I dropped it when porting the idea upstream and reintroduced the scan. The patch was better engineered than my first version of the fix.

1. Non-determinism

Fixed as you suggested — exact match among the names iterdir() actually returned, then min() as a stable tiebreak. Your distinction is the right one: preferring an exact name among real directory entries is not the exists() bug, because nothing is asking the filesystem a case-insensitive question.

The test for it skips on a filesystem that cannot hold both spellings, which is the one running here, so it is real coverage only on Linux/CI.

4. The line-break set

You flagged this as suspected rather than verified. It is verified: all five characters produce the same unparseable file. Guard is now splitlines() != [value], and thank you for the len(...) > 1 warning — my own trailing-break case would have regressed.

Fixing it exposed a second layer underneath. json.dumps(..., ensure_ascii=False) escapes \x0b and \x0c but leaves NEL, LS and PS literal, so they were emitted raw inside a quoted scalar and folded to a space on reload — the silent metadata edit the JSON form exists to avoid. Those three are now escaped explicitly.

3. reference_id inside the file

Taken rather than deferred, since it is the same churn one layer in. An existing entry keeps its stored spelling.

My first attempt was wrong in a way the test caught: the --- guard fired on the opening delimiter, so it broke out before ever reaching the id and silently kept the caller's spelling. It now skips position 0.

6. try/except

is_dir() predicate for the dominant "cache directory does not exist yet" case; the remaining except OSError covers only a genuine mid-iteration failure.

5. Legacy .txt

Left as flagged. Agreed it is a small and shrinking surface, and folding it would mean resolving on stem rather than full name, which complicates the common path for a case I cannot produce.

---

1119 passing, 1 skipped (the both-spellings test, on this filesystem), mypy, Ruff, doctests and mkdocs build --strict clean, against the pushed tree.

[claude]

Claude finished @cmungall's task in 3m 10s —— View job

---

Re-review: Keep a line break in metadata, and one cache file per DOI

• Check what changed in the new push
• Re-read the changed code
• Check which earlier findings are addressed
• Run tests / mypy / doctests — not possible, Bash restricted to git/ls in this run
• Post updated review

---

First, a bookkeeping note so the thread reads correctly: the reply above ("All six taken") went out three seconds after my second review, so it is answering the first one — and the code confirms it, is_dir(), min() tiebreak, splitlines(), _stored_reference_id are all in. The second review's list is still open. This push is a third thing again: one commit, forget_cache_listing(), for a cross-process window found downstream (dismech#12083).

On that commit: the diagnosis is right and the reasoning for an explicit reset over re-scanning is right. Re-scanning on every miss really would undo the memoization — a cold build misses once per reference, so it is the same 45 seconds back. Documenting the window in _case_index rather than leaving it to be rediscovered is the right instinct too.

But the commit treats the window as cross-process, and it isn't only that. The same staleness exists between two fetchers in one process, and there the new method does not reach it.

---

1. Repairer holds two fetchers on one cache directory, and neither calls the new reset

# validation/repairer.py:113-114
self.validator = SupportingTextValidator(validation_config)   # builds its own fetcher (supporting_text_validator.py:90)
self.fetcher = ReferenceFetcher(validation_config)

_case_indexes is per-instance (246) and _remember_cache_file updates only its owner's copy (1174). So once the validator's fetcher has indexed the directory, every file the repairer's fetcher writes afterwards is invisible to it, and vice versa. A repair run interleaves both across many references, so going stale is the normal case. A DOI arriving uppercase through one and lowercase through the other then misses and writes the second file — the exact defect this PR exists to remove, surviving in the code path most likely to hit it.

forget_cache_listing() does not help here: it is an instance method, so clearing one fetcher's listings leaves the other's stale, and there is no point in the repair loop where a caller could know to call it on both. Nothing in this repository calls it at all — it is documented API with no in-repo user, while the one in-repo instance of the problem it describes goes unfixed.

A module-level dict keyed by directory fixes this and composes with the new commit rather than competing with it: every write already funnels through _remember_cache_file, so one shared map stays accurate for the whole process, the per-directory scan is still paid once rather than once per instance, and forget_cache_listing() is then left covering exactly the window it is named and documented for — a writer outside this process. The docstring gets more accurate too, since "another process, or a subprocess this one shelled out to" would become the complete list instead of a partial one.

Test: two fetchers over one tmp_path, the second resolving a DOI before the first writes it, then one file on disk after both save under different spellings.

Fix this →

2. The new test pins the stale read as a contract

# tests/test_frontmatter_and_doi_case.py:247
assert fetcher.get_cache_path(LOWER).name != outsider.name, "stale, as documented"

The assertion after the reset is the one that carries the behaviour. This one asserts the absence of a fix — so the day the index learns to self-invalidate (see #5, or any other route), this test fails on an improvement and has to be deleted rather than passing. The comment already says it is a known wart; asserting it makes it a guarantee. Dropping the line, or reducing it to a comment above the forget_cache_listing() call, keeps the test testing the reset.

3. The duplicated line is still there

# reference_fetcher.py:1388-1389
self._remember_cache_file(cache_path)
self._remember_cache_file(cache_path)

Flagged last round, idempotent so harmless, but it has now survived a second commit that touched this area. Worth deleting before it looks deliberate.

4. Still open from the second review

Briefly, since they were never responded to rather than declined:

• DEL and the C1 block still reach the plain scalar. splitlines() covers every break the scanner honours, but ruamel's reader also rejects \x7f and \x80–\x9f (minus \x85) as non-printable, and json.dumps(ensure_ascii=False) leaves them literal since it only escapes below 0x20. So they never route to _json_scalar, and would not be escaped if they did. The realistic source is mojibake — Windows-1252 bytes read as Latin-1 turn smart quotes and en-dashes into \x91–\x97, a common shape for bibliographic metadata. _UNESCAPED_YAML_BREAKS generalises to cover both jobs: a regex over [\x7f-\x9f

] substituted as \uXXXX, with the routing condition widened to value.splitlines() != [value] or _YAML_UNSAFE.search(value). Same caveat as before — reasoned from the PyYAML-derived NON_PRINTABLE class, not measured, because I again could not execute code. A parametrized "Smart quote\x92s" settles it in one run.
• _stored_reference_id turns an undecodable entry into a failed save (1095). except OSError does not cover UnicodeDecodeError, which is a ValueError. Before this PR a mangled entry was simply overwritten and repaired; now the file that most needs rewriting is the one that cannot be. open(encoding="utf-8", errors="replace") removes it without widening the except — only the reference_id: line is read.
• _stored_reference_id is not scoped to DOI but its justification is (1082). Sanitization collapses : / ? = to _, so url:https://ex.com/a?b=c and url:https://ex.com/a/b/c share a filename. That collision predates this PR, but its handling changes: the first writer's id is now cemented and reasserted on every later write. Gating on the same prefix.upper() == "DOI" test at 1078 keeps the change to the case it is argued for.
• Legacy .txt is still unfolded (1410ish). Agreed as declined in the round-one reply — noting only that it stays a decision.

5. Optional: validating the memo against the directory's mtime

Not a request, but worth weighing against the manual reset, because creating a file bumps its directory's st_mtime_ns and that is exactly the event the index cares about. Comparing a stored stamp costs one stat() (~1 µs against the 85 µs you measured) and closes the cross-process window with no API for a caller to remember.

The catch, and it is why I am not asking for it: your own writes bump the mtime too, so without re-stamping in _remember_cache_file every save invalidates the index and the quadratic scan comes straight back. With re-stamping there is still a race — an outsider writing between your write and your stat is lost — plus coarse-granularity filesystems (HFS+ at 1 s) widen it. So the explicit method is defensible; it just is not the only option, and the tradeoff is worth a line in the docstring if you keep it.

---

Tests

The new test is well-targeted: it builds the index while the file is absent, has an outsider write it, and checks the reset. That is the right shape for the reset. Gaps map to the above: no two-instance case (#1), no non-line-break unprintable (#4), and the != assertion in #2.

Verification

Third round running, third round where Bash is restricted — git/ls only this time, so no just test, mypy, doctests or mkdocs build --strict, and I could not even check the json.dumps behaviour in #4 empirically. "1119 passing, 1 skipped" is unverified here, not disputed. Adding uv to --allowedTools would make these rounds a lot more useful.

Net: the new commit is a real fix for a real window and the reasoning behind choosing an explicit reset is sound. #1 is the one I would still not ship without, and it is now sharper than last round rather than weaker — the commit adds the tool for this class of staleness without applying it to the instance of it that lives in this repo.
• fix/newline-titles-and-doi-case

[claude]

Claude finished @cmungall's task in 4m 15s —— View job

---

Re-review: Keep a line break in metadata, and one cache file per DOI

• Check what changed in the new push (0311ee2)
• Re-read the changed code
• Check which earlier findings are now addressed
• Run tests / mypy / doctests — not possible, uv run was denied in this run
• Post updated review

---

All six items from the previous round are addressed, and #1 is addressed in a way I could actually verify rather than take on assertion. The claim the module-level index rests on is "every write funnels through _remember_cache_file" — that is only true if nothing else in the repo writes or removes a cache .md, and it holds:

• grep -rn "write_text" src/ returns exactly two hits — reference_fetcher.py:1416 (inside _save_to_disk, immediately followed by _remember_cache_file) and cli/cache.py:401, which writes an unrelated report file.
• grep -rn "unlink()\|rmtree\|\.rename(" src/ returns nothing, so no in-process deletion or rename can strand a name in the index.
• grep -rn "ThreadPoolExecutor\|threading\|concurrent.futures" src/ returns nothing, so the shared dict has no concurrent-build race where one builder's map overwrites another's and loses a _remember_cache_file write.

The other four: DEL and the C1 block now both route (_YAML_UNSAFE.search in the condition at 1247) and escape (_json_scalar, 1152) — putting the character class in both places rather than only the escaper is the part that matters, since the old code would have escaped them but never reached the escaper. The duplicated _remember_cache_file is gone. errors="replace" is in. The stale-read assertion is a comment now, which is right.

Three things, all smaller than last round's.

---

1. The DOI gate in _stored_reference_id is not the DOI gate in _cache_path

Two different predicates for what should be one question:

# _cache_path:1078 — resolves the path
prefix, identifier = self._parse_reference_id(reference_id, apply_prefix_map=False)
if prefix.upper() == "DOI":

# _stored_reference_id:1108 — preserves the id
if not reference.reference_id.upper().startswith("DOI:"):

_parse_reference_id splits on ^([A-Za-z_]+)[:\s]+(.+)$ — a space is an accepted separator, and its own doctest documents that (_parse_reference_id("PMID 12345678") → ('PMID', '12345678')). So "DOI 10.1016/S0002-9440(10)63332-9" parses with prefix DOI, gets its path case-folded, and then fails startswith("DOI:") — the filename resolves to the existing entry and the reference_id: line is rewritten under the caller's spelling anyway. That is exactly the one-line committed-cache diff finding #3 was about, reappearing for one input shape.

Low severity, since ids normally arrive normalized from fetch() (as the comment at 1080 says). But the cost of the two gates agreeing is a three-line helper, and it removes the possibility of them drifting apart later:

def _is_doi(self, reference_id: str) -> bool:
    prefix, _ = self._parse_reference_id(reference_id, apply_prefix_map=False)
    return prefix.upper() == "DOI"

Fix this →

2. errors="replace" fixed the crash but left the id unvalidated, so garbage on that line now gets cemented

The two changes to _stored_reference_id interact. It now reads with replacement and returns whatever it finds, unconditionally:

# 1110-1115
with cache_path.open(encoding="utf-8", errors="replace") as handle:
    for position, line in enumerate(handle):
        if line.startswith("reference_id:"):
            stored = line.split(":", 1)[1].strip()
            return stored or reference.reference_id

The new test covers a file with a mangled title line. The case it does not cover is the mangled reference_id line — the same b"\xff\xfe" two lines up. Before this commit that raised, which was the bug. Now it decodes to DOI:10.1/� and that is written back as the entry's id, on this save and every save after, since the next read finds the corrupted value and preserves it too. U+FFFD is printable to YAML so nothing downstream complains; _load_markdown_format just returns an id that no longer matches the reference. Trading a loud failure for a silently self-perpetuating one is a worse trade than the strict-decode version it replaced.

The guard is the docstring's own argument, made executable — the stored id is only worth keeping because it is the same reference in another capitalization:

if stored and stored.casefold() == reference.reference_id.casefold():
    return stored
return reference.reference_id

Any value that is not a case variant is not the thing the method exists to preserve, so falling back to the caller's id is correct by the same reasoning. Worth noting this also subsumes the url: collision that motivated the DOI gate — two ids colliding through sanitization differ by more than case, so they would be rejected here regardless — which makes the gate a cheap early-out rather than the only thing standing between this method and a wrong answer.

Fix this →

3. _YAML_UNSAFE covers the reader's rejections except the last two

_YAML_UNSAFE = re.compile("[\x7f-\x9f

]")

Matched against ruamel's NON_PRINTABLE class, which permits \x09 \x0A \x0D, \x20-\x7E, \x85, \xA0-퟿, -� and \U00010000-\U0010FFFF, this now covers everything except � and � — outside the permitted -� window, left literal by json.dumps(ensure_ascii=False), and so the same unrecoverable entry with a ReaderError. It is the same mojibake family the commit message already argues from: � is what a byte-swapped UTF-16 BOM decodes to. (Lone surrogates are also non-printable but fail earlier, at write_text(encoding="utf-8"), so they are not this function's problem.)

re.compile("[\x7f-\x9f

��]") closes it. Same caveat as my earlier rounds and it is why I am putting this third rather than first: reasoned from the PyYAML-derived character class, not measured, because Bash was again restricted.

4. Test hygiene: nothing resets _CASE_INDEXES between tests

Now that the index is module-global, it survives across the whole pytest session. Today that is safe only because every test reaches it through a unique tmp_path. The first test that uses a fixed or shared cache directory, or that writes a cache file directly after some earlier test indexed the same directory, gets an order-dependent failure that will read as a flake rather than as this.

An autouse fixture in tests/conftest.py makes the isolation explicit rather than incidental, and it is two lines:

@pytest.fixture(autouse=True)
def _reset_case_index():
    ReferenceFetcher.forget_cache_listing()

It also stops the dict accumulating one entry per tmp_path across 1119 tests, though that part is negligible.

---

Smaller notes, not asking for changes

• Removing the != assertion from test_a_file_written_by_another_process_is_found_after_a_reset was right. Worth knowing that the test now also passes if memoization were removed entirely — test_resolution_does_not_scan_the_directory_per_call is the only thing pinning that contract, which is fine, just now load-bearing alone.
• config.get_cache_dir() returns self.cache_dir without .expanduser(), unlike get_private_cache_dir(). Pre-existing, and out of scope, but the index is keyed on that path, so a ~-prefixed or relative cache_dir keys on the unexpanded form. Harmless today since one config yields one consistent form.
• Legacy .txt still unfolded — agreed as declined in round one, unchanged.
• Making _case_index/_remember_cache_file/forget_cache_listing static while leaving the self.-qualified call sites is the right minimal change; forget_cache_listing() stays callable on an instance, so the downstream call site keeps working.

Verification

Fourth round, fourth restricted Bash — git/grep/sed only, and uv run pytest was denied, so no test run, mypy, doctests or mkdocs build --strict, and I could not check the json.dumps or ruamel behaviour in #3 empirically. "1119 passing" is unverified here, not disputed. Everything above is from reading the code plus the greps quoted at the top. Adding uv to --allowedTools would make these rounds substantially more useful.

Net: the shared index is the right fix for the right reason and I can see the invariant holding repo-wide, so my previous blocker is closed. Nothing left here is a reason to hold the branch — #2 is the one I would still take before merge, since it converts a crash into silent, self-perpetuating data corruption, and the fix is one comparison.
• fix/newline-titles-and-doi-case

[cmungall]

You are right about the thread, and thank you for reconstructing it rather than assuming. My "All six taken" went out three seconds after your second review and was answering the first — so the second round's list had never been responded to, and I would not have noticed. It is addressed below along with this round's finding, in 0311ee2, kept as its own commit.

1. Two fetchers, one directory

Confirmed: Repairer builds a fetcher at line 114 and SupportingTextValidator builds another at line 90, both on the same config. With a per-instance index each one's writes were invisible to the other, and a repair run interleaves them, so going stale is the normal case rather than the edge.

Hoisted to a module-level dict keyed by directory, as you suggested, and the shape works out the way you predicted: every write already funnels through _remember_cache_file, so one shared map stays accurate process-wide, the scan is still paid once per directory rather than once per instance, and forget_cache_listing() is left covering exactly the window it is named and documented for. The _case_index docstring now says "another process, or a subprocess" as the complete list rather than a partial one.

One caveat on the test you asked for. I wrote it, and it passes on this machine with or without the fix — macOS folds the two filenames together, so the second write lands in the first file whatever the index says. The trace shows the difference plainly:

before: b resolves LOWER after a's write -> doi_10.1016_s0002-...md   (stale)
after : b resolves LOWER after a's write -> DOI_10.1016_S0002-...md   (shared)

so the fix is verified, but the test is only real coverage on a case-sensitive filesystem. That is the second time this filesystem has hidden a case bug from me in this PR.

4. The second review's list

• DEL and the C1 block. You reasoned this from NON_PRINTABLE and could not measure it. It is verified: \x7f, \x80 and \x92 each produce a ReaderError — an unrecoverable entry, not a folded character, so worse than the line-break case this branch started with. _YAML_UNSAFE is now a regex over [\x7f-\x9f

] used for both the routing condition and the escaping, and your mojibake framing is in the comment because it explains why this is reachable at all.
• UnicodeDecodeError. Fixed with errors="replace". The framing that made it obvious was yours: before this branch a mangled entry was simply overwritten and repaired, and I had made the file that most needs rewriting into the one that cannot be.
• Scoping to DOI. Taken. The url: collision predates this branch, but cementing the first writer's id changes how it is handled, and the case-insensitivity argument says nothing about url:.
• Legacy .txt. Still declined, still a decision rather than an oversight.

2 and 3

The stale-read assertion is gone — you are right that pinning the absence of a fix means a later self-invalidating index fails the suite as though it were a regression. The duplicated _remember_cache_file call is deleted; it survived two commits, which is exactly how such a line starts looking deliberate.

5. mtime validation

Genuinely attractive, and I have not taken it, so the reasoning rather than a silent no. With the index now shared process-wide, the remaining window is a writer outside the process, and st_mtime_ns has a real failure mode there: two writes inside one timestamp tick can leave the stamp unchanged, so it would mostly-close the window while removing the explicit call that currently closes it completely. A caller who knows it shelled out knows more than the stamp does. Worth revisiting if the explicit reset proves easy to forget in practice.

1126 passing, 1 skipped (the both-spellings case, on this filesystem), mypy, Ruff, doctests and mkdocs build --strict clean, against the pushed tree.