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/resolver_cache.rs b/crates/doiget-core/src/resolver_cache.rs index 5963b458b..029399a97 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, @@ -189,20 +198,20 @@ pub 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; } 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-core/src/store/fs_store.rs b/crates/doiget-core/src/store/fs_store.rs index cb91d052d..bb78d99b5 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) - } } // --------------------------------------------------------------------------- @@ -333,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`]. @@ -451,7 +484,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 +584,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. @@ -707,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, @@ -908,7 +958,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 +981,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 +1048,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..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}; @@ -137,6 +141,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 +188,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..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!({ @@ -2538,7 +2598,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 +2746,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_, @@ -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