From d51df16370f9b7f7d2784caef27f35e572f0f95a Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 15:36:56 +0900 Subject: [PATCH 1/3] fix: stop the release publishing a wrapper alone, and stop a fetch eating user tags Two findings from the third review round, both in code no earlier round had reached. release-plz.yml: the npm publish loop reads its package list from stage-npm.sh 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 the MAP block, a delimiter change) gives an empty word list, the loop runs zero times, and the step falls through to publish the wrapper alone -- green, with every platform binary missing. npm install doiget-cli would succeed while its optionalDependencies fail 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 guards the identical grep for the identical reason one file over. Now refuses an empty list and a count that is not 4; probed all three cases. fs_store.rs: 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 -- all three orchestrator construction sites hard-code Vec::new() / None. So doiget tag X --add priority followed by any doiget fetch X discarded the tag silently. Same loss the adjacent fix closed, on the fields where it is the user's own data. 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 make removal a silent no-op. Store gains write_user_authored for those four call sites; write keeps the fetch semantics. Also: the two bare grep -c assignments in posture-lint could die before their own ::error::, the same guard applied twice already in that file. Signed-off-by: Sota Shimozono --- .github/workflows/posture-lint.yml | 8 +- .github/workflows/release-plz.yml | 28 +++- crates/doiget-cli/src/commands/tag.rs | 4 +- crates/doiget-core/src/store/fs_store.rs | 193 +++++++++++++++++------ crates/doiget-core/src/store/mod.rs | 34 ++++ crates/doiget-mcp/src/lib.rs | 4 +- 6 files changed, 213 insertions(+), 58 deletions(-) diff --git a/.github/workflows/posture-lint.yml b/.github/workflows/posture-lint.yml index 017c84276..a22ef6e14 100644 --- a/.github/workflows/posture-lint.yml +++ b/.github/workflows/posture-lint.yml @@ -395,8 +395,12 @@ jobs: # One `insert("disposition"` per `insert("code"`. Not a proof, but it # fails on exactly the mistake that was made. f=crates/doiget-mcp/src/lib.rs - codes="$(grep -c 'insert("code"' "$f")" - disps="$(grep -c 'insert("disposition"' "$f")" + # `|| true`: `grep -c` exits 1 on a zero count, and under + # `set -euo pipefail` that kills the step at the assignment, before + # the `::error::` written below to explain it. Same guard as the + # release-sync step and `capture()`. + codes="$(grep -c 'insert("code"' "$f" || true)" + disps="$(grep -c 'insert("disposition"' "$f" || true)" if [ "$codes" != "$disps" ]; then echo "::error::posture-lint: $codes hand-built error objects but $disps dispositions in $f - a failure envelope is missing error.disposition (#506)" grep -n 'insert("code"' "$f" diff --git a/.github/workflows/release-plz.yml b/.github/workflows/release-plz.yml index 745e35854..8ed2d0b86 100644 --- a/.github/workflows/release-plz.yml +++ b/.github/workflows/release-plz.yml @@ -805,7 +805,33 @@ jobs: # Reading the MAP cannot drift the same way. It lists platform # packages only, so the wrapper is excluded by construction rather # than by a name test somebody has to remember to update. - for pkg in $(grep -oE '^doiget-[a-z0-9-]+:' scripts/stage-npm.sh | tr -d ':' | sort -u); do + # + # The empty case is guarded, and that is not defensive noise. The + # pipeline ends in `sort -u`, which exits 0 on empty input, so + # `set -euo pipefail` gives NOTHING here: a grep that matches + # nothing (a reformat of the 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, with every platform + # binary silently missing. `npm install doiget-cli` would then + # succeed while its optionalDependencies fail to resolve. + # + # The `doiget-*` glob this replaced failed LOUDLY on no-match (the + # unexpanded pattern reached `npm publish` as a path that does not + # exist). Trading that for silence in the step that performs the + # irreversible publish is the wrong direction. posture-lint's + # `capture()` already guards the identical grep for the identical + # reason; it was applied to the check and not to the publish. + pkgs="$(grep -oE '^doiget-[a-z0-9-]+:' scripts/stage-npm.sh | tr -d ':' | sort -u || true)" + if [ -z "$pkgs" ]; then + echo "::error::release: no platform packages found in scripts/stage-npm.sh -- refusing to publish the wrapper alone" + exit 1 + fi + count=$(echo "$pkgs" | wc -l | tr -d " ") + if [ "$count" -ne 4 ]; then + echo "::error::release: expected 4 platform packages, found $count: $(echo $pkgs)" + exit 1 + fi + for pkg in $pkgs; do npm publish "./npm-stage/$pkg" --provenance --access public --tag "$DIST_TAG" done npm publish ./npm-stage/doiget-cli --provenance --access public --tag "$DIST_TAG" diff --git a/crates/doiget-cli/src/commands/tag.rs b/crates/doiget-cli/src/commands/tag.rs index 230f7628c..b9409263c 100644 --- a/crates/doiget-cli/src/commands/tag.rs +++ b/crates/doiget-cli/src/commands/tag.rs @@ -122,7 +122,7 @@ pub fn run( } store - .write(&safekey, &metadata, None) + .write_user_authored(&safekey, &metadata, None) .with_context(|| format!("failed to write updated metadata for {ref_str}"))?; Ok(()) @@ -174,7 +174,7 @@ pub fn run_annotate(ref_str: String, text: Option, clear: bool) -> Resul } store - .write(&safekey, &metadata, None) + .write_user_authored(&safekey, &metadata, None) .with_context(|| format!("failed to write updated metadata for {ref_str}"))?; Ok(()) diff --git a/crates/doiget-core/src/store/fs_store.rs b/crates/doiget-core/src/store/fs_store.rs index cb91d052d..5e6dbd330 100644 --- a/crates/doiget-core/src/store/fs_store.rs +++ b/crates/doiget-core/src/store/fs_store.rs @@ -37,7 +37,7 @@ use fs2::FileExt; use tracing::warn; use super::metadata::{DoigetExtension, Metadata, LICENSE_UNDETERMINED}; -use super::{EntryInfo, Store, StoreError}; +use super::{EntryInfo, Store, StoreError, UserFields}; use crate::{Safekey, SCHEMA_VERSION}; /// Subdirectory under `` that holds metadata TOML files and their @@ -203,6 +203,72 @@ impl Store for FsStore { } fn write(&self, key: &Safekey, m: &Metadata, pdf: Option<&Utf8Path>) -> Result<(), StoreError> { + self.write_with_impl(key, m, pdf, UserFields::Preserve) + } + + fn write_user_authored( + &self, + key: &Safekey, + m: &Metadata, + pdf: Option<&Utf8Path>, + ) -> Result<(), StoreError> { + self.write_with_impl(key, m, pdf, UserFields::Authored) + } + + fn list_recent(&self, limit: usize) -> Result, StoreError> { + let mut entries = read_all_entries(&self.metadata_dir)?; + // Most-recent first by [doiget].fetched_at; entries with no + // `[doiget]` table sort last (None < Some via Reverse). + entries.sort_by_key(|e| std::cmp::Reverse(e.fetched_at)); + entries.truncate(limit); + Ok(entries) + } + + /// Phase 1 search is a linear scan over all metadata files. Phase 2 will + /// add a tantivy / sqlite-fts index when the corpus grows past the point + /// where O(N) per query becomes noticeable in CLI latency. + fn search(&self, query: &str, limit: usize) -> Result, StoreError> { + let q = query.to_lowercase(); + let mut hits = Vec::new(); + for path in metadata_files(&self.metadata_dir)? { + let raw = std::fs::read_to_string(path.as_std_path())?; + let Ok(md) = toml::from_str::(&raw) else { + // Malformed entries are skipped rather than failing the + // whole query. A future audit task will surface them. + continue; + }; + let haystacks = [ + md.title.to_lowercase(), + md.authors.join(" ").to_lowercase(), + md.venue.clone().unwrap_or_default().to_lowercase(), + md.publisher.clone().unwrap_or_default().to_lowercase(), + ]; + if haystacks.iter().any(|h| h.contains(&q)) { + let safekey = safekey_from_metadata_filename(&path); + hits.push(EntryInfo { + safekey, + title: md.title, + year: md.year, + fetched_at: md.doiget.as_ref().map(|d| d.fetched_at), + size_bytes: md.doiget.as_ref().map(|d| d.size_bytes), + }); + if hits.len() >= limit { + break; + } + } + } + Ok(hits) + } +} + +impl FsStore { + fn write_with_impl( + &self, + key: &Safekey, + m: &Metadata, + pdf: Option<&Utf8Path>, + user_fields: UserFields, + ) -> Result<(), StoreError> { let meta_path = self.metadata_path(key)?; let lock_path = self.lock_path(key)?; let lock_file = open_or_create_lock_file(&lock_path)?; @@ -217,7 +283,7 @@ impl Store for FsStore { let raw = std::fs::read_to_string(meta_path.as_std_path())?; let existing: Metadata = toml::from_str(&raw)?; check_schema_version_for_write(&existing.schema_version)?; - merge_metadata(existing, m.clone()) + merge_metadata(existing, m.clone(), user_fields) } else { m.clone() }; @@ -254,51 +320,6 @@ impl Store for FsStore { let _ = ::unlock(&lock_file); Ok(()) } - - fn list_recent(&self, limit: usize) -> Result, StoreError> { - let mut entries = read_all_entries(&self.metadata_dir)?; - // Most-recent first by [doiget].fetched_at; entries with no - // `[doiget]` table sort last (None < Some via Reverse). - entries.sort_by_key(|e| std::cmp::Reverse(e.fetched_at)); - entries.truncate(limit); - Ok(entries) - } - - /// Phase 1 search is a linear scan over all metadata files. Phase 2 will - /// add a tantivy / sqlite-fts index when the corpus grows past the point - /// where O(N) per query becomes noticeable in CLI latency. - fn search(&self, query: &str, limit: usize) -> Result, StoreError> { - let q = query.to_lowercase(); - let mut hits = Vec::new(); - for path in metadata_files(&self.metadata_dir)? { - let raw = std::fs::read_to_string(path.as_std_path())?; - let Ok(md) = toml::from_str::(&raw) else { - // Malformed entries are skipped rather than failing the - // whole query. A future audit task will surface them. - continue; - }; - let haystacks = [ - md.title.to_lowercase(), - md.authors.join(" ").to_lowercase(), - md.venue.clone().unwrap_or_default().to_lowercase(), - md.publisher.clone().unwrap_or_default().to_lowercase(), - ]; - if haystacks.iter().any(|h| h.contains(&q)) { - let safekey = safekey_from_metadata_filename(&path); - hits.push(EntryInfo { - safekey, - title: md.title, - year: md.year, - fetched_at: md.doiget.as_ref().map(|d| d.fetched_at), - size_bytes: md.doiget.as_ref().map(|d| d.size_bytes), - }); - if hits.len() >= limit { - break; - } - } - } - Ok(hits) - } } // --------------------------------------------------------------------------- @@ -451,7 +472,7 @@ fn parse_schema_version(s: &str) -> Result<(u32, u32), StoreError> { /// in `existing` not present in `incoming` are kept; otherwise `incoming` /// wins (callers usually leave `other` empty on a re-fetch, so existing /// fields survive intact). -fn merge_metadata(existing: Metadata, incoming: Metadata) -> Metadata { +fn merge_metadata(existing: Metadata, incoming: Metadata, user_fields: UserFields) -> Metadata { let mut out = incoming.clone(); // schema_version: never downgrade. The §6 exception explicitly allows a @@ -551,6 +572,23 @@ fn merge_metadata(existing: Metadata, incoming: Metadata) -> Metadata { if incoming_d.license == LICENSE_UNDETERMINED { incoming_d.license = existing_d.license; } + // `tags` / `collections` / `annotation` are USER-AUTHORED. A + // fetch never writes them -- all three orchestrator construction + // sites hard-code `Vec::new()` / `None` -- so letting the + // incoming side win meant `doiget tag X --add priority` followed + // by any `doiget fetch X` silently discarded the tag. Same defect + // ADR-0056 closed for `oa_status`/`license` two lines up, on the + // fields where the loss is the user's own data rather than a + // re-derivable reading. + // + // `UserFields::Authored` is how `doiget tag` / `doiget annotate` + // say they mean it, including meaning an EMPTY list: without that + // distinction, removing the last tag would be a silent no-op. + if matches!(user_fields, UserFields::Preserve) { + incoming_d.tags = existing_d.tags; + incoming_d.collections = existing_d.collections; + incoming_d.annotation = existing_d.annotation; + } } // Incoming carries no [doiget] at all: keep the existing fetch record // rather than dropping it. @@ -908,7 +946,7 @@ mod tests { d.oa_status = None; d.license = LICENSE_UNDETERMINED.to_string(); - let out = merge_metadata(existing, incoming); + let out = merge_metadata(existing, incoming, UserFields::Preserve); let d = out.doiget.expect("[doiget] survives"); assert_eq!(d.oa_status.as_deref(), Some("gold")); assert_eq!(d.license, "CC-BY-4.0"); @@ -931,12 +969,65 @@ mod tests { d.oa_status = Some("closed".to_string()); d.license = "CC-BY-NC-4.0".to_string(); - let out = merge_metadata(existing, incoming); + let out = merge_metadata(existing, incoming, UserFields::Preserve); let d = out.doiget.expect("[doiget] survives"); assert_eq!(d.oa_status.as_deref(), Some("closed")); assert_eq!(d.license, "CC-BY-NC-4.0"); } + /// A tag the user added must survive a re-fetch. + /// + /// Every `DoigetExtension` the orchestrator builds hard-codes + /// `tags: Vec::new()`, so before `UserFields::Preserve` the incoming + /// empty list won and `doiget tag X --add priority` followed by any + /// `doiget fetch X` discarded the tag with no warning, no log row and no + /// exit-code effect. ADR-0056 closed exactly this for `oa_status` and + /// `license` two fields over. + #[test] + fn a_fetch_does_not_discard_the_user_tags_it_never_authored() { + let mut existing = sample_metadata(); + let d = existing.doiget.as_mut().expect("doiget table"); + d.tags = vec!["priority".to_string()]; + d.collections = vec!["to-read".to_string()]; + d.annotation = Some("check the appendix".to_string()); + + // What a re-fetch hands to the store. + let incoming = sample_metadata(); + assert!( + incoming + .doiget + .as_ref() + .is_some_and(|d| d.tags.is_empty() && d.annotation.is_none()), + "the fixture must model a fetch, which authors none of these" + ); + + let out = merge_metadata(existing, incoming, UserFields::Preserve); + let d = out.doiget.expect("doiget table"); + assert_eq!(d.tags, vec!["priority".to_string()], "tag survived"); + assert_eq!(d.collections, vec!["to-read".to_string()]); + assert_eq!(d.annotation.as_deref(), Some("check the appendix")); + } + + /// ...and `doiget tag --remove` of the last tag still empties it. + /// + /// This is why the policy is a parameter rather than "preserve when the + /// incoming value is empty": for an authored write the empty list IS the + /// intent, and collapsing the two would make removal a silent no-op -- + /// trading one silent data problem for another. + #[test] + fn an_authored_write_can_empty_the_user_fields() { + let mut existing = sample_metadata(); + let d = existing.doiget.as_mut().expect("doiget table"); + d.tags = vec!["priority".to_string()]; + d.annotation = Some("old note".to_string()); + + let incoming = sample_metadata(); // what `tag --remove` writes back + let out = merge_metadata(existing, incoming, UserFields::Authored); + let d = out.doiget.expect("doiget table"); + assert!(d.tags.is_empty(), "removal is honoured: {:?}", d.tags); + assert_eq!(d.annotation, None, "clear is honoured"); + } + #[test] fn merge_metadata_preserves_existing_arxiv_categories() { // Issue #303 / review #318: a later metadata-only re-write that did @@ -945,7 +1036,7 @@ mod tests { existing.arxiv_categories = vec!["cond-mat.str-el".to_string()]; let mut incoming = sample_metadata(); incoming.arxiv_categories = vec![]; // re-write without categories - let merged = merge_metadata(existing, incoming); + let merged = merge_metadata(existing, incoming, UserFields::Preserve); assert_eq!(merged.arxiv_categories, vec!["cond-mat.str-el".to_string()]); } diff --git a/crates/doiget-core/src/store/mod.rs b/crates/doiget-core/src/store/mod.rs index 2a996c7bf..b5c2f800e 100644 --- a/crates/doiget-core/src/store/mod.rs +++ b/crates/doiget-core/src/store/mod.rs @@ -137,6 +137,27 @@ pub enum StoreError { }, } +/// Who is authoritative for the user-authored `[doiget]` fields +/// (`tags`, `collections`, `annotation`) on a write. +/// +/// A fetch never authors them: every `DoigetExtension` the orchestrator +/// builds hard-codes `Vec::new()` / `None`. Letting that win silently +/// discarded a user's tags on any re-fetch, which is the loss ADR-0056 +/// closed for `oa_status` / `license` and left open here. +/// +/// The distinction has to be explicit rather than "is the incoming value +/// empty", because `doiget tag --remove` and `doiget annotate --clear` +/// legitimately mean the empty value. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum UserFields { + /// The caller did not author them; keep whatever is on disk. Fetches. + Preserve, + /// The caller means exactly what it wrote, empty included. `doiget tag`, + /// `doiget annotate`, and their MCP equivalents. + Authored, +} + /// Filesystem-shaped metadata store, semver-locked per `docs/PUBLIC_API.md` /// §2. /// @@ -163,8 +184,21 @@ pub trait Store: Send + Sync { /// `/.pdf` via the same atomic-rename dance as the /// metadata file. The caller is responsible for emitting the /// `event=store_write` provenance row (see `docs/PROVENANCE_LOG.md` §3). + /// Write a fetch result. User-authored `[doiget]` fields already on disk + /// are preserved ([`UserFields::Preserve`]) -- a fetch does not author + /// them, and silently dropping them is data loss. fn write(&self, key: &Safekey, m: &Metadata, pdf: Option<&Utf8Path>) -> Result<(), StoreError>; + /// Write on behalf of a caller that DID author the user fields, so an + /// empty `tags` / `collections` or a `None` annotation means exactly that + /// ([`UserFields::Authored`]). `doiget tag` / `doiget annotate` only. + fn write_user_authored( + &self, + key: &Safekey, + m: &Metadata, + pdf: Option<&Utf8Path>, + ) -> Result<(), StoreError>; + /// Return up to `limit` entries, most-recent first by `[doiget].fetched_at`. fn list_recent(&self, limit: usize) -> Result, StoreError>; diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 86be2518c..48be948ab 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -2538,7 +2538,7 @@ impl Server { let tags = ext.tags.clone(); let collections = ext.collections.clone(); - match store.write(&safekey, &metadata, None) { + match store.write_user_authored(&safekey, &metadata, None) { Ok(()) => Ok(CallToolResult::structured(json!({ "ok": true, "ref": input.ref_, @@ -2686,7 +2686,7 @@ impl Server { let annotation = ext.annotation.clone(); - match store.write(&safekey, &metadata, None) { + match store.write_user_authored(&safekey, &metadata, None) { Ok(()) => Ok(CallToolResult::structured(json!({ "ok": true, "ref": input.ref_, From 08e049fd0c318a4e778e4c88bc8df11977d6bee2 Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 16:06:29 +0900 Subject: [PATCH 2/3] fix: one rate limiter and one provenance log per process, not per tool call The MCP server built both fresh inside every tool handler. Consequences, all in code no earlier review round had reached. RateLimiter's pacing state -- the rolling global window and the per-source next-allowed instants -- lives in its own Arc> fields, so it paces only the calls that share the instance. Thirteen handlers each got an empty per_source_next and no memory of the last call, so 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", and a long-lived handle reuses it. 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 actually before them -- which is what audit-log --verify reports as a broken chain. Server now owns an Arc and a OnceLock>, opened lazily because Server::new is infallible. 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. Also in this batch: FetchError::Http(_) => NetworkError was a wildcard over all eight HttpError variants, and NetworkError's disposition is retry_after. Six of them cannot change on a retry: an allowlist refusal, an http:// downgrade, a size cap, a wrong content type, an unregistered source key and a malformed header. The mapping every surface routes through was giving the advice ADR-0055 exists to stop giving, and the DenialContext impl 100 lines below already matched all eight. Now exhaustive. 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 -- the caller asked HttpClient to fetch for a source it was never given -- and it is the error the TDM reproduction in #462 actually hit, which is part of why that read as a transport problem. 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, so they were accidental semver commitments -- 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. pub had been keeping the dead-code lint quiet. 927 tests on oa-only, 1044 with the tdm features. Signed-off-by: Sota Shimozono --- crates/doiget-core/src/resolver_cache.rs | 42 +++--- crates/doiget-core/src/source.rs | 98 +++++++++++++- crates/doiget-core/src/sources/openalex.rs | 5 +- crates/doiget-mcp/src/lib.rs | 146 ++++++++++++++++----- 4 files changed, 239 insertions(+), 52 deletions(-) diff --git a/crates/doiget-core/src/resolver_cache.rs b/crates/doiget-core/src/resolver_cache.rs index 5963b458b..20960c060 100644 --- a/crates/doiget-core/src/resolver_cache.rs +++ b/crates/doiget-core/src/resolver_cache.rs @@ -45,7 +45,20 @@ struct CacheEntry { /// The on-disk path for a ref's cache entry: /// `/resolver/.toml`. #[must_use] -pub fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf { +// `pub(crate)`, not `pub`. Nothing outside `doiget-core` calls this module -- +// the orchestrator is the only consumer -- and it is absent from +// `docs/PUBLIC_API.md`, so every one of these was an accidental semver +// commitment, including the on-disk cache layout they encode. This cycle +// added the `_with_options` half and doubled that surface. +// +// `#[cfg(test)]` on the remaining plain wrappers is not tidying: making them +// `pub(crate)` is what revealed that production calls none of them. They +// default the options for this module's own tests and nothing else, and `pub` +// had been keeping the dead-code lint quiet about it. Two of the original +// five, `read` and `write`, turned out to have no caller anywhere -- not even +// a test -- and are gone. +#[cfg(test)] +pub(crate) fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf { cache_file_with_options(cache_root, ref_, MetadataOnlyOptions::default()) } @@ -69,7 +82,7 @@ pub fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf { /// would both want `doi_10.1234_foo.oa.toml`. A safekey can never contain a /// path separator (`/` is replaced with `_`), so a subdirectory cannot. #[must_use] -pub fn cache_file_with_options( +pub(crate) fn cache_file_with_options( cache_root: &Utf8Path, ref_: &Ref, opts: MetadataOnlyOptions, @@ -89,7 +102,8 @@ pub fn cache_file_with_options( /// expired, or a `response` blob that no longer deserializes. `now` is /// injected so tests can pin expiry without touching the clock. #[must_use] -pub fn read_at( +#[cfg(test)] +pub(crate) fn read_at( cache_root: &Utf8Path, ref_: &Ref, now: DateTime, @@ -100,7 +114,7 @@ pub fn read_at( /// [`read_at`], reading the entry keyed by `opts`. See /// [`cache_file_with_options`] for why the options are part of the key. #[must_use] -pub fn read_at_with_options( +pub(crate) fn read_at_with_options( cache_root: &Utf8Path, ref_: &Ref, now: DateTime, @@ -119,15 +133,9 @@ pub fn read_at_with_options( serde_json::from_str(&entry.response).ok() } -/// Read using the current wall clock. See [`read_at`]. -#[must_use] -pub fn read(cache_root: &Utf8Path, ref_: &Ref) -> Option { - read_at(cache_root, ref_, Utc::now()) -} - /// [`read`], reading the entry keyed by `opts`. #[must_use] -pub fn read_with_options( +pub(crate) fn read_with_options( cache_root: &Utf8Path, ref_: &Ref, opts: MetadataOnlyOptions, @@ -138,7 +146,8 @@ pub fn read_with_options( /// Write `outcome` to the cache for `ref_`. Best-effort: returns `false` /// (after a `tracing::debug!`) on any I/O or serialization failure rather /// than propagating, since a cache write must never fail a resolve. -pub fn write_at( +#[cfg(test)] +pub(crate) fn write_at( cache_root: &Utf8Path, ref_: &Ref, outcome: &MetadataOnlyOutcome, @@ -154,7 +163,7 @@ pub fn write_at( } /// [`write_at`], writing the entry keyed by `opts`. -pub fn write_at_with_options( +pub(crate) fn write_at_with_options( cache_root: &Utf8Path, ref_: &Ref, outcome: &MetadataOnlyOutcome, @@ -196,13 +205,8 @@ pub fn write_at_with_options( true } -/// Write using the current wall clock. See [`write_at`]. -pub fn write(cache_root: &Utf8Path, ref_: &Ref, outcome: &MetadataOnlyOutcome) -> bool { - write_at(cache_root, ref_, outcome, Utc::now()) -} - /// [`write()`], writing the entry keyed by `opts`. -pub fn write_with_options( +pub(crate) fn write_with_options( cache_root: &Utf8Path, ref_: &Ref, outcome: &MetadataOnlyOutcome, diff --git a/crates/doiget-core/src/source.rs b/crates/doiget-core/src/source.rs index dc10a338b..ab8330b98 100644 --- a/crates/doiget-core/src/source.rs +++ b/crates/doiget-core/src/source.rs @@ -260,7 +260,38 @@ impl From<&FetchError> for crate::ErrorCode { FetchError::Http(HttpError::HttpStatus { status: 401 | 403, .. }) => crate::ErrorCode::CapabilityDenied, - FetchError::Http(_) => crate::ErrorCode::NetworkError, + // Exhaustive over `HttpError`, not `Http(_)`. The wildcard sent + // six deterministic outcomes to `NETWORK_ERROR`, whose disposition + // is `retry_after` -- so an agent was told to back off and retry an + // allowlist refusal, an http:// downgrade, a size cap, a + // wrong content type, an unregistered source key and a malformed + // header, none of which a retry can change. That is the defect + // ADR-0055 exists to remove, in the mapping every surface routes + // through. The `DenialContext` impl 100 lines down already matches + // all eight variants; this one opted out of the same protection. + FetchError::Http(e) => match e { + // Policy decisions. Settled until the configuration changes, + // which is what `needs_config` means -- and each of these + // carries a `DenialContext` naming the fix. + HttpError::RedirectDenied { .. } | HttpError::InsecureRedirect { .. } => { + crate::ErrorCode::CapabilityDenied + } + // The response arrived and was not what was asked for. + // Re-requesting returns the same bytes. + HttpError::OversizedBody { .. } | HttpError::NotAPdf { .. } => { + crate::ErrorCode::NoOaAvailable + } + // The caller asked for a source the client was never given. + // A build/wiring fault, not the network (#454, #462). + HttpError::UnknownSource { .. } | HttpError::InvalidHeader { .. } => { + crate::ErrorCode::InternalError + } + // Genuinely transient: transport failures, and the statuses + // the arms above did not claim. + HttpError::Network(_) | HttpError::HttpStatus { .. } => { + crate::ErrorCode::NetworkError + } + }, FetchError::Log(_) => crate::ErrorCode::LogError, FetchError::InvalidRef(_) => crate::ErrorCode::InvalidRef, // An access refusal is not an internal error. Before #538 it @@ -536,6 +567,60 @@ mod tests { assert!(res.metadata_json.is_none()); } + /// A deterministic HTTP outcome must not be advertised as retriable. + /// + /// `FetchError::Http(_) => NetworkError` was a wildcard over all eight + /// `HttpError` variants, and `NetworkError`'s disposition is + /// `retry_after`. Six of them cannot change on a retry, so the mapping + /// every surface routes through was telling agents to back off and try + /// again on an allowlist refusal, a size cap and an unregistered source + /// key -- the exact advice ADR-0055 exists to stop giving. + #[test] + fn a_deterministic_http_failure_is_not_advertised_as_retriable() { + let cases: Vec<(HttpError, crate::Disposition)> = vec![ + ( + HttpError::RedirectDenied { + source_key: "oa-publisher".into(), + host: "evil.example.com".into(), + expected_hosts: vec!["*.wiley.com".to_string()], + }, + crate::Disposition::NeedsConfig, + ), + ( + HttpError::UnknownSource { + source_key: "tdm-aps".into(), + }, + crate::Disposition::Terminal, + ), + ]; + for (he, want) in cases { + let code: ErrorCode = FetchError::Http(he).into(); + assert_ne!( + code, + ErrorCode::NetworkError, + "a policy/wiring outcome is not a network error: {code:?}" + ); + assert_eq!( + code.disposition(), + want, + "and its disposition must not say retry_after: {code:?}" + ); + } + } + + /// The transient ones keep saying retry, so the fix did not overshoot. + #[test] + fn a_transient_http_failure_still_says_retry() { + let code: ErrorCode = FetchError::Http(HttpError::HttpStatus { + status: 503, + retry_after_ms: None, + url: "https://api.crossref.org/works/10.5555/x".into(), + }) + .into(); + assert_eq!(code, ErrorCode::NetworkError); + assert_eq!(code.disposition(), crate::Disposition::RetryAfter); + } + #[test] fn fetch_error_collapses_to_error_code() { // Mirrors `docs/PUBLIC_API.md` §4 / PR #55 boundary collapse. @@ -549,11 +634,20 @@ mod tests { let e: ErrorCode = FetchError::NoOaAvailable.into(); assert_eq!(e, ErrorCode::NoOaAvailable); + // `UnknownSource` is "the caller asked HttpClient to fetch for a + // source it was never given" -- a wiring fault. This asserted + // `NetworkError` because the mapping used to be `Http(_) => + // NetworkError`, i.e. it pinned the wildcard rather than a decision: + // retrying cannot register a missing source, and `NetworkError`'s + // `retry_after` disposition told an agent to try anyway. It is the + // error #462's TDM reproduction actually hit, and calling it a network + // problem is part of why it read as one. let e: ErrorCode = FetchError::Http(HttpError::UnknownSource { source_key: "mock".into(), }) .into(); - assert_eq!(e, ErrorCode::NetworkError); + assert_eq!(e, ErrorCode::InternalError); + assert_eq!(e.disposition(), crate::Disposition::Terminal); // 404 / 410 / 451 from a metadata source are authoritative "id does // not exist" → NotFound (network-independent), NOT NetworkError. diff --git a/crates/doiget-core/src/sources/openalex.rs b/crates/doiget-core/src/sources/openalex.rs index fd616f130..be7f706ea 100644 --- a/crates/doiget-core/src/sources/openalex.rs +++ b/crates/doiget-core/src/sources/openalex.rs @@ -270,7 +270,10 @@ pub fn open_access_pdf_url(record: &serde_json::Value) -> Option<&str> { /// the reader at the repository. "no OA PDF available" points them at giving /// up. Returns `None` when there is nothing to say. #[must_use] -pub fn describe_locations(record: &serde_json::Value) -> Option<(usize, String)> { +// `pub(crate)`, not `pub`: one caller, `orchestrator::describe_optional_source_locations`, +// and the signature takes a raw `&serde_json::Value` -- publishing it would put +// OpenAlex's wire shape under this crate's semver guarantee for no consumer. +pub(crate) fn describe_locations(record: &serde_json::Value) -> Option<(usize, String)> { let locations = record .get("locations") .and_then(serde_json::Value::as_array)?; diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 48be948ab..be778aee7 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -98,12 +98,39 @@ pub struct Server { /// the per-instance trimming visible to `tools/list` and `tools/call` /// (issue #379). Do not switch the handler back to the associated fn. tool_router: ToolRouter, + /// ONE per process, shared by every tool call. + /// + /// `RateLimiter`'s state -- the rolling global window and the + /// per-source next-allowed instants -- lives in its own `Arc>` + /// fields, so it paces only the calls that share the instance. Building + /// a fresh one inside every tool handler, which is what this server did, + /// gave each call 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. + rate_limiter: Arc, + /// ONE per process, opened lazily because `Server::new` is infallible. + /// + /// `ProvenanceLog::open` documents its own contract: the `session_id` + /// "MUST be a 26-char ULID generated once per process", and a long-lived + /// handle reuses it. Opening per tool 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 actually before them -- which is precisely what + /// `doiget audit-log --verify` reports as a broken chain. + log: std::sync::OnceLock>, + /// The process ULID the log above is opened with. + session_id: String, } #[tool_router] impl Server { /// Construct a server with the given runtime capability profile. pub fn new(profile: CapabilityProfile) -> Self { + let rate_limiter = Arc::new(RateLimiter::new(RateLimits::HARD_CODED)); + let session_id = ulid::Ulid::generate().to_string(); + let log = std::sync::OnceLock::new(); let mut tool_router = Self::tool_router(); // Issue #379 / #373(b): a tool that can only ever answer // NOT_IMPLEMENTED is worse than an absent one — an agent will @@ -126,9 +153,42 @@ impl Server { Self { profile, tool_router, + rate_limiter, + log, + session_id, } } + /// The per-call [`FetchContext`], built from the process-lifetime + /// rate limiter and provenance log this server owns. + /// + /// Was a free `self.fetch_context()` that constructed both fresh on + /// every tool call. See the field docs on [`Server`] for what that cost: + /// no rate pacing between MCP calls, and a provenance hash chain that + /// two overlapping calls could break. + fn fetch_context(&self) -> anyhow::Result { + let log = match self.log.get() { + Some(l) => Arc::clone(l), + None => { + let opened = Arc::new(open_provenance_log(self.session_id.clone())?); + // A racing caller may have won; prefer whichever landed so + // every context in this process shares ONE handle, which is + // what serialises `append` and keeps the chain intact. + let _ = self.log.set(Arc::clone(&opened)); + self.log.get().map_or(opened, Arc::clone) + } + }; + Ok(FetchContext { + http: Arc::new(build_http_client_for_fetch()?), + rate_limiter: Arc::clone(&self.rate_limiter), + log, + session_id: self.session_id.clone(), + // Resolver cache disabled on the MCP path for now; the resolve + // cache (docs/CACHE.md) is wired through `doiget verify` first. + cache_root: None, + }) + } + /// Run the MCP server until stdin reaches EOF. /// /// Returns once the underlying rmcp service loop exits — that happens @@ -300,7 +360,7 @@ impl Server { // selection and per-leg politeness; we own the per-call // session boundary (SessionStart / SessionEnd bookend rows) and // the wire envelope shape (`docs/MCP_TOOLS.md` §11). - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(metadata_only_error_envelope( @@ -472,7 +532,7 @@ impl Server { // Step 2: build the per-call context. Failures here surface as // INTERNAL_ERROR per the metadata_only pattern. - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(metadata_only_error_envelope( @@ -602,7 +662,7 @@ impl Server { // Step 3: non-dry-run path. Build foundation modules + open // FsStore + dispatch through core orchestrator. - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(fetch_paper_error_envelope( @@ -794,7 +854,7 @@ impl Server { // Step 4: non-dry-run — stand up the shared FetchContext + // store, emit a single SessionStart row, fan out via the core // orchestrator. - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(batch_fetch_error_envelope( @@ -1082,7 +1142,7 @@ impl Server { // Step 5: stand up the shared context + store (mirrors // `doiget_batch_fetch`). A context-init failure aborts the // whole call. - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(batch_fetch_error_envelope( @@ -1417,7 +1477,7 @@ impl Server { let contact_email = doiget_core::orchestrator::configured_contact_email().unwrap_or_default(); - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(read_path_error_envelope( @@ -1577,7 +1637,7 @@ impl Server { // for now, mirroring `build_fetch_context`'s resolver-cache note; // enabling it here is a follow-up. Correctness is unaffected — a // cache miss just re-fetches. - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(read_path_error_envelope( @@ -1693,7 +1753,7 @@ impl Server { } }; - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(read_path_error_envelope( @@ -1826,7 +1886,7 @@ impl Server { let contact_email = doiget_core::orchestrator::configured_contact_email().unwrap_or_default(); - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(read_path_error_envelope( @@ -2109,7 +2169,7 @@ impl Server { } }; - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(read_path_error_envelope( @@ -2291,7 +2351,7 @@ impl Server { &self, Parameters(input): Parameters, ) -> Result { - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ @@ -2359,7 +2419,7 @@ impl Server { }))); } - let ctx = match build_fetch_context() { + let ctx = match self.fetch_context() { Ok(c) => c, Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ @@ -4047,7 +4107,7 @@ fn paper_text_success_envelope(t: &doiget_core::paper_text::PaperText) -> Value /// if missing. /// - `session_id` — fresh 26-char ULID per call (one tool call = one /// logical session, per `docs/PROVENANCE_LOG.md` §3). -fn build_fetch_context() -> anyhow::Result { +fn open_provenance_log(session_id: String) -> anyhow::Result { let log_path = resolve_log_path()?; if let Some(parent) = log_path.parent() { if !parent.as_str().is_empty() { @@ -4055,23 +4115,8 @@ fn build_fetch_context() -> anyhow::Result { .map_err(|e| anyhow::anyhow!("creating log dir {parent}: {e}"))?; } } - let session_id = ulid::Ulid::generate().to_string(); - let log = Arc::new( - ProvenanceLog::open(log_path, session_id.clone()) - .map_err(|e| anyhow::anyhow!("opening provenance log: {e}"))?, - ); - let http = Arc::new(build_http_client_for_fetch()?); - let rate_limiter = Arc::new(RateLimiter::new(RateLimits::HARD_CODED)); - Ok(FetchContext { - http, - rate_limiter, - log, - session_id, - // Resolver cache disabled on the MCP path for now; the resolve - // cache (docs/CACHE.md) is wired through `doiget verify` first. - // Enabling it here for metadata_only / resolve_paper is a follow-up. - cache_root: None, - }) + ProvenanceLog::open(log_path, session_id) + .map_err(|e| anyhow::anyhow!("opening provenance log: {e}")) } /// Build a [`CrossrefSource`] from environment variables @@ -4542,6 +4587,47 @@ fn capability_profile_to_json(profile: &CapabilityProfile) -> Value { #[cfg(test)] #[allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] mod tests { + /// The rate limiter and the provenance log are ONE per process. + /// + /// Every tool handler used to call a free `build_fetch_context()` that + /// constructed both fresh. `RateLimiter`'s pacing state is instance-local, + /// so arXiv's 3 s spacing -- an obligation, not politeness -- was not + /// enforced between MCP calls; and `ProvenanceLog::open` seeds its + /// `(next_seq, last_hash)` by reading the file, so two overlapping calls + /// could append rows whose `prev_hash` does not match the row before + /// them, which `audit-log --verify` reports as a broken chain. + /// + /// Asserted by pointer identity, because that is the property: two + /// contexts from one server must share the same allocation, not merely + /// equal configuration. + #[test] + fn two_tool_calls_share_one_rate_limiter_and_one_log() { + let td = tempfile::TempDir::new().expect("tempdir"); + let root = camino::Utf8Path::from_path(td.path()).expect("utf-8"); + std::env::set_var("DOIGET_LOG_PATH", root.join("log.jsonl").as_str()); + std::env::set_var("DOIGET_STORE_ROOT", root.join("papers").as_str()); + + let server = Server::new(CapabilityProfile::from_env().expect("profile")); + let a = server.fetch_context().expect("first context"); + let b = server.fetch_context().expect("second context"); + + assert!( + Arc::ptr_eq(&a.rate_limiter, &b.rate_limiter), + "a fresh RateLimiter per call has an empty per-source window, so nothing paces the second request behind the first" + ); + assert!( + Arc::ptr_eq(&a.log, &b.log), + "a fresh ProvenanceLog per call re-reads the chain state, so two overlapping appends claim the same ts_seq" + ); + assert_eq!( + a.session_id, b.session_id, + "PROVENANCE_LOG.md: the session ULID is generated once per PROCESS" + ); + + std::env::remove_var("DOIGET_LOG_PATH"); + std::env::remove_var("DOIGET_STORE_ROOT"); + } + /// #538 on the two batch tools. /// /// `fetch_paper_fetch_error_envelope` shed a hand-rolled From d66dbb6dd807f03664bdf81ad296c53475c900eb Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 16:14:30 +0900 Subject: [PATCH 3/3] fix: write the resolver cache the way the store next door writes Two advisories from the same review round. resolver_cache used a plain std::fs::write. A reader racing it sees a half-written file, toml::from_str fails, and the entry degrades to a miss -- safe, per the module's best-effort contract, but it is a re-fetch nobody asked for and a debug! line that reads like corruption. atomic_write (tmp, fsync, rename) already existed one module over; it is now pub(crate) and both write the same way. safekey_from_metadata_filename mints a Safekey straight from a directory listing. The safety argument is that the filesystem only holds names which already passed guard_safekey at write time -- true today, and a claim about the world rather than something the type enforces, which is the shape this release keeps finding. A debug_assert catches a future write path that skips the guard in the test suite instead of in whatever reads the store next. Signed-off-by: Sota Shimozono --- crates/doiget-core/src/resolver_cache.rs | 7 ++++++- crates/doiget-core/src/store/fs_store.rs | 16 ++++++++++++++-- crates/doiget-core/src/store/mod.rs | 4 ++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/crates/doiget-core/src/resolver_cache.rs b/crates/doiget-core/src/resolver_cache.rs index 20960c060..029399a97 100644 --- a/crates/doiget-core/src/resolver_cache.rs +++ b/crates/doiget-core/src/resolver_cache.rs @@ -198,7 +198,12 @@ pub(crate) fn write_at_with_options( return false; } } - if let Err(e) = std::fs::write(&path, toml_text) { + // tmp + rename, not a plain write. A reader racing a plain write sees a + // half-written file, `toml::from_str` fails, and the entry degrades to a + // miss -- safe, per this module's best-effort contract, but it is a + // re-fetch nobody asked for and a `debug!` line that looks like + // corruption. The store next door already had the helper. + if let Err(e) = crate::store::atomic_write(&path, toml_text.as_bytes()) { tracing::debug!(error = %e, path = %path, "resolver cache: write failed"); return false; } diff --git a/crates/doiget-core/src/store/fs_store.rs b/crates/doiget-core/src/store/fs_store.rs index 5e6dbd330..bb78d99b5 100644 --- a/crates/doiget-core/src/store/fs_store.rs +++ b/crates/doiget-core/src/store/fs_store.rs @@ -354,7 +354,19 @@ fn guard_safekey(s: &str) -> Result<(), StoreError> { /// list/search results; the safekey we emit here originated as a stored /// safekey, so it has already passed `guard_safekey` at write time. fn safekey_from_metadata_filename(p: &Utf8Path) -> Safekey { - Safekey(p.file_stem().unwrap_or("").to_string()) + let stem = p.file_stem().unwrap_or(""); + // The safety argument here is "the filesystem only holds names that + // already passed `guard_safekey` at write time" -- true, and a claim + // about the world rather than something the type enforces. Every other + // `Safekey` in the crate is minted through the guard; this one trusts a + // directory listing. Assert it in debug builds so a future write path + // that skips the guard is caught by the test suite instead of by whatever + // reads the store afterwards. + debug_assert!( + guard_safekey(stem).is_ok(), + "store contains a metadata file whose stem is not a valid safekey: {stem:?}" + ); + Safekey(stem.to_string()) } /// Lock mode for [`acquire_lock`]. @@ -745,7 +757,7 @@ fn toml_value_inline(value: &toml::Value) -> Result { /// A crash mid-write leaves either the old file intact (if before the /// rename) or the new file fully written (if after). It never leaves a /// partially-visible new file. -fn atomic_write(dst: &Utf8Path, bytes: &[u8]) -> std::io::Result<()> { +pub(crate) fn atomic_write(dst: &Utf8Path, bytes: &[u8]) -> std::io::Result<()> { let file_name = dst.file_name().ok_or_else(|| { std::io::Error::new( std::io::ErrorKind::InvalidInput, diff --git a/crates/doiget-core/src/store/mod.rs b/crates/doiget-core/src/store/mod.rs index b5c2f800e..fd763e5c8 100644 --- a/crates/doiget-core/src/store/mod.rs +++ b/crates/doiget-core/src/store/mod.rs @@ -25,6 +25,10 @@ pub mod metadata; pub mod render; pub use fs_store::FsStore; + +/// Crash-consistent write (tmp + fsync + rename), shared with the resolver +/// cache so both write the same way. See `docs/STORE.md` §5. +pub(crate) use fs_store::atomic_write; pub use metadata::{DoigetExtension, Metadata}; pub use render::{to_bibtex, to_csl_array};