fix: third review round — the release pipeline, the rate limiter, the audit log, and a fetch that ate your tags

[sotashimozono]

Third review round on the 0.8.13 release. Lands on next, so it becomes part
of #580.

The first round's Rust reviewer and comment auditor both died on a session
limit before reporting, and async/concurrency was never covered by hand
either. So the bulk of this release — the 79 commits accumulated on next
since 0.8.12 — had never had a completed Rust review. This round pointed six
agents at exactly that. Every finding below is from code no earlier round had
reached.

---

The release could have published a wrapper with no binaries

.github/workflows/release-plz.yml — the npm publish loop reads its package
list through a pipeline ending in sort -u, which exits 0 on empty input.
set -euo pipefail therefore protects nothing: a grep that matches nothing
(a reformat of stage-npm.sh's MAP block, a delimiter change) yields an empty
word list, the loop runs zero times, and the step falls through to publish the
wrapper alone — green. npm install doiget-cli would then succeed while
every platform optionalDependency fails to resolve.

The doiget-* glob this replaced failed loudly on no-match. Trading that for
silence in the step that performs the irreversible publish is the wrong
direction — and posture-lint.yml guards the identical grep for the
identical reason one file over, citing this exact failure mode. The guard had
been applied to the check and not to the publish.

Now refuses an empty list and a count that is not 4. Probed all three cases.

A fetch silently ate the user's tags

merge_metadata protected oa_status and license under ADR-0056 and left
tags, collections and annotation to the incoming side — which for a fetch
is always empty, because all three orchestrator construction sites hard-code
Vec::new() / None.

doiget tag 10.1234/x --add priority   -> tags = ["priority"]
doiget fetch 10.1234/x                -> tags = []      (no warning, no log row)

Same loss the adjacent fix closed, on the fields where it is the user's own
data rather than a re-derivable reading.

The policy is an explicit UserFields parameter rather than
preserve-when-empty, because tag --remove and annotate --clear legitimately
mean the empty value — collapsing the two would trade one silent data problem
for another. Store gains write_user_authored for those four call sites;
write keeps the fetch semantics. Both directions are tested.

The MCP server was not pacing arXiv, and could break its own audit log

build_fetch_context() constructed a fresh RateLimiter and a fresh
ProvenanceLog inside every one of thirteen tool handlers.

RateLimiter's pacing state is instance-local, so each call got an empty
per_source_next and no memory of the last: arXiv's 3 s spacing, which
docs/LEGAL.md treats as an obligation rather than politeness, was not
enforced between MCP calls at all. The type calls itself "process-wide";
nothing made it so.

ProvenanceLog::open documents the same contract in its own words — the
session_id "MUST be a 26-char ULID generated once per process". Opening per
call broke that thirteen times over, and worse: open seeds
(next_seq, last_hash) by reading the file, so two overlapping calls both read
the same state and both append rows claiming the same ts_seq with a
prev_hash that does not match the row before them — which is what
audit-log --verify reports as a broken chain.

Server now owns one of each. RateLimiter::new, Ulid::generate and
ProvenanceLog::open each appear exactly once. Asserted by pointer identity,
because that is the property; injecting a per-call limiter makes the test fail.

Six deterministic failures were advertised as retriable

FetchError::Http(_) => NetworkError was a wildcard over all eight HttpError
variants, and NetworkError's disposition is retry_after. An allowlist
refusal, an http:// downgrade, a size cap, a wrong content type, an
unregistered source key and a malformed header cannot change on a retry. The
mapping every surface routes through was giving the advice ADR-0055 exists
to stop giving; the DenialContext impl 100 lines below already matched all
eight variants.

One existing assertion changed with it: UnknownSource had been pinned to
NetworkError, which recorded the wildcard rather than a decision. It is a
wiring fault, and it is the error the TDM reproduction in #462 actually hit —
part of why that read as a transport problem.

Accidental semver surface, and the dead code it was hiding

resolver_cache's ten functions and openalex::describe_locations were pub
with no caller outside doiget-core and no entry in docs/PUBLIC_API.md —
accidental commitments under the 0.x guarantee, including the on-disk cache
layout they encode. This cycle had doubled that surface by adding the
_with_options half.

Narrowing to pub(crate) is what revealed production calls none of the five
plain wrappers. read and write have no caller anywhere, not even a test,
and are deleted; the rest are #[cfg(test)], which is what they are. pub had
been keeping the dead-code lint quiet.

Advisories

• resolver_cache wrote with a plain std::fs::write; a racing reader saw a
  half-written file and degraded to a miss. atomic_write already existed one
  module over.
• safekey_from_metadata_filename mints a Safekey from a directory listing
  on the argument that the filesystem only holds guarded names — true, and a
  claim about the world rather than something the type enforces. debug_assert
  catches a future write path that skips the guard.

---

Verification

Local, x86_64-pc-windows-gnu:

• --features oa-only — 927 passed, 0 failed, 1 ignored
• --features oa-only,metadata,tdm-aps,tdm-elsevier,tdm-springer,tdm-ieee — 1044 passed, 0 failed
• cargo clippy -D warnings across all three feature sets — clean
• cargo fmt --all — clean

Each guard was proven by breaking it: a per-call rate limiter fails the sharing
test, and the publish guard was probed against an empty list, a reformatted MAP
and a dropped platform.

Still open

Not fixed here, and worth a decision before the tag: Store::read/write do
synchronous filesystem I/O — including a lock poll with std::thread::sleep up
to a 5 s budget — directly inside async fns, with no spawn_blocking. Under
the CLI's batch JoinSet or a long-running MCP server that blocks a tokio
worker rather than yielding. Pre-existing, newly exercised by call sites this
release added.

[codecov]

Codecov Report

❌ Patch coverage is 90.20619% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/doiget-core/src/store/fs_store.rs	89.36%	10 Missing ⚠️
crates/doiget-mcp/src/lib.rs	92.00%	4 Missing ⚠️
crates/doiget-cli/src/commands/tag.rs	0.00%	2 Missing ⚠️
crates/doiget-core/src/resolver_cache.rs	77.77%	2 Missing ⚠️
crates/doiget-core/src/source.rs	97.43%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!