diff --git a/CHANGELOG.md b/CHANGELOG.md index 03730b187..2ef931296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,35 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he ## [Unreleased] +### Fixed + +- **[store]** A default `metadata_only` re-write **downgraded a known + `oa_status` and `license` to their not-determined markers**, and said nothing + (#583). + + `docs/STORE.md` §6 does let a re-fetch rewrite the `[doiget]` table, but the + permission is conditional: *"This is intentional, not silent: ... the operator + always learns the entry was downgraded and why."* Since #539, `metadata_only` + only runs the OA lookup when `include_oa_location` is set, so the ordinary call + shape produces `oa_status: None` and `license: "unknown"` — and those won, + with no `note:`, no `pdf.status` and no log row. The condition the permission + rests on was not met. + + Both values are markers, not readings: `oa_status` is "omitted when not + determined" and `license` falls back to `"unknown"`. A paper that genuinely + stops being open access reports `Some("closed")`; a license that changes + reports the new string. So a call that carries the marker did not look, and + preferring the stored value is not a guess about which is newer. + `merge_metadata` now keeps the stored value in exactly that case, and + `"unknown"` has a name (`LICENSE_UNDETERMINED`) so the check does not hang off + a bare literal. [ADR-0056](docs/DECISIONS/0056-not-determined-is-not-an-answer.md), + and `docs/STORE.md` §6's note is amended to scope its claim to determinations. + + The issue as filed said `oa_url` was overwritten with null. It is not — + `merge_opt!(url)` already protected it, which a probe confirmed before any of + this was written. Only the two `[doiget]` fields were affected, and the fix is + a merge rule rather than the store partition the issue proposed. + ### Changed - **[ci]** One test was taking ten minutes and being run five times. The five diff --git a/Cargo.lock b/Cargo.lock index 9a1e96394..5703131e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -524,7 +524,7 @@ dependencies = [ [[package]] name = "doiget-cli" -version = "0.8.13-beta.22" +version = "0.8.13-beta.23" dependencies = [ "anyhow", "assert_cmd", @@ -554,7 +554,7 @@ dependencies = [ [[package]] name = "doiget-core" -version = "0.8.13-beta.22" +version = "0.8.13-beta.23" dependencies = [ "async-trait", "biblatex", @@ -591,7 +591,7 @@ dependencies = [ [[package]] name = "doiget-mcp" -version = "0.8.13-beta.22" +version = "0.8.13-beta.23" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 2fea6098c..6659621d7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ exclude = [ ] [workspace.package] -version = "0.8.13-beta.22" +version = "0.8.13-beta.23" edition = "2021" rust-version = "1.86" license = "MIT" @@ -41,9 +41,9 @@ features-doc-link = "docs/SOURCES.md" [workspace.dependencies] # Core -doiget-core = { path = "crates/doiget-core", version = "0.8.13-beta.22" } -doiget-cli = { path = "crates/doiget-cli", version = "0.8.13-beta.22" } -doiget-mcp = { path = "crates/doiget-mcp", version = "0.8.13-beta.22" } +doiget-core = { path = "crates/doiget-core", version = "0.8.13-beta.23" } +doiget-cli = { path = "crates/doiget-cli", version = "0.8.13-beta.23" } +doiget-mcp = { path = "crates/doiget-mcp", version = "0.8.13-beta.23" } # Async runtime — features are deliberately limited (avoid `full`). tokio = { version = "1", default-features = false, features = [ diff --git a/crates/doiget-core/src/store/fs_store.rs b/crates/doiget-core/src/store/fs_store.rs index 9dc2b58e0..cb91d052d 100644 --- a/crates/doiget-core/src/store/fs_store.rs +++ b/crates/doiget-core/src/store/fs_store.rs @@ -36,7 +36,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use fs2::FileExt; use tracing::warn; -use super::metadata::{DoigetExtension, Metadata}; +use super::metadata::{DoigetExtension, Metadata, LICENSE_UNDETERMINED}; use super::{EntryInfo, Store, StoreError}; use crate::{Safekey, SCHEMA_VERSION}; @@ -532,11 +532,30 @@ fn merge_metadata(existing: Metadata, incoming: Metadata) -> Metadata { out.arxiv_categories = existing.arxiv_categories; } - // [doiget]: doiget owns this table; incoming wins (already in `out`). - // If incoming has no [doiget] but existing did, keep the existing one - // so a metadata-only re-write doesn't silently drop a fetch record. - if out.doiget.is_none() && existing.doiget.is_some() { - out.doiget = existing.doiget; + // [doiget]: doiget owns this table, so a re-write wins (STORE.md §6) -- + // except for the two fields whose "absent" value is a marker rather than + // a reading. `oa_status` is omitted when not determined (#281) and + // `license` falls back to `LICENSE_UNDETERMINED`. A `metadata_only` call + // without `include_oa_location` never runs the OA lookup, so it carries + // exactly those markers; letting them win replaces an answer with the + // absence of one. STORE.md §6 permits a `[doiget]` downgrade because it + // is reported to the operator (#118) -- on this path nothing is, which is + // what ADR-0056 closes. Preserving cannot suppress real news: a paper that + // stops being open access reports `Some("closed")`, and a license that + // changes reports the new string. + match (existing.doiget, out.doiget.as_mut()) { + (Some(existing_d), Some(incoming_d)) => { + if incoming_d.oa_status.is_none() { + incoming_d.oa_status = existing_d.oa_status; + } + if incoming_d.license == LICENSE_UNDETERMINED { + incoming_d.license = existing_d.license; + } + } + // Incoming carries no [doiget] at all: keep the existing fetch record + // rather than dropping it. + (Some(existing_d), None) => out.doiget = Some(existing_d), + (None, _) => {} } // `other` (unknown tables / fields): union, prefer EXISTING on key @@ -870,6 +889,54 @@ mod tests { FsStore::new(root).expect("FsStore::new") } + #[test] + fn a_default_rewrite_does_not_downgrade_a_known_oa_status_or_license() { + // Issue #583. `metadata_only` without `include_oa_location` never + // runs the OA lookup, so it carries `oa_status: None` and + // `license: "unknown"` -- the not-determined markers, not readings. + // Letting them win would replace an answer with the absence of one, + // and STORE.md §6 only permits a [doiget] downgrade that is reported. + let mut existing = sample_metadata(); + existing.url = Some("https://example.org/paper.pdf".to_string()); + let d = existing.doiget.as_mut().expect("sample has [doiget]"); + d.oa_status = Some("gold".to_string()); + d.license = "CC-BY-4.0".to_string(); + + let mut incoming = sample_metadata(); + incoming.url = None; + let d = incoming.doiget.as_mut().expect("sample has [doiget]"); + d.oa_status = None; + d.license = LICENSE_UNDETERMINED.to_string(); + + let out = merge_metadata(existing, incoming); + let d = out.doiget.expect("[doiget] survives"); + assert_eq!(d.oa_status.as_deref(), Some("gold")); + assert_eq!(d.license, "CC-BY-4.0"); + // `url` was already protected by `merge_opt!`; pinned so the two + // halves of #583 cannot drift apart. + assert_eq!(out.url.as_deref(), Some("https://example.org/paper.pdf")); + } + + #[test] + fn a_rewrite_that_determined_a_new_oa_status_or_license_still_wins() { + // The other half: preserving must not suppress real news. A paper + // that stops being open access reports `Some("closed")`, not `None`. + let mut existing = sample_metadata(); + let d = existing.doiget.as_mut().expect("sample has [doiget]"); + d.oa_status = Some("gold".to_string()); + d.license = "CC-BY-4.0".to_string(); + + let mut incoming = sample_metadata(); + let d = incoming.doiget.as_mut().expect("sample has [doiget]"); + d.oa_status = Some("closed".to_string()); + d.license = "CC-BY-NC-4.0".to_string(); + + let out = merge_metadata(existing, incoming); + 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"); + } + #[test] fn merge_metadata_preserves_existing_arxiv_categories() { // Issue #303 / review #318: a later metadata-only re-write that did diff --git a/crates/doiget-core/src/store/metadata.rs b/crates/doiget-core/src/store/metadata.rs index efba2c4a0..5d5f169d5 100644 --- a/crates/doiget-core/src/store/metadata.rs +++ b/crates/doiget-core/src/store/metadata.rs @@ -97,6 +97,14 @@ pub struct Metadata { pub other: std::collections::BTreeMap, } +/// The value `[doiget].license` carries when no license was determined. +/// +/// It is a marker for "the lookup did not produce one", not a reading. A +/// resolver that genuinely reports a license writes that license; nothing +/// reports `"unknown"` as news. `merge_metadata` relies on that to tell an +/// absent answer from a new one. +pub const LICENSE_UNDETERMINED: &str = "unknown"; + /// doiget-specific extension table (`[doiget]`). /// /// Per `docs/STORE.md` §6, doiget owns this table outright and may diff --git a/docs/DECISIONS/0056-not-determined-is-not-an-answer.md b/docs/DECISIONS/0056-not-determined-is-not-an-answer.md new file mode 100644 index 000000000..9dca40144 --- /dev/null +++ b/docs/DECISIONS/0056-not-determined-is-not-an-answer.md @@ -0,0 +1,88 @@ +# 0056 - A not-determined marker never overwrites a determination + +- **Date:** 2026-08-31 +- **Status:** Accepted +- **Supersedes:** - +- **Amends:** [`docs/STORE.md`](../STORE.md) §6 — the re-fetch downgrade note +- **Complements:** [0014](0014-docs-class-system.md) — the ADR a NORMATIVE doc change requires; [0055](0055-error-disposition.md), which drew the same distinction on the wire +- **Source:** #583 (a default `metadata_only` re-write silently downgraded `[doiget].oa_status` and `.license`) + +## Context + +`docs/STORE.md` §6 lets doiget rewrite the `[doiget]` table on a re-fetch, and +says why: + +> A doiget re-fetch of an entry that previously had a PDF but is now +> metadata-only ... rewrites the `[doiget]` table (`source`, `size_bytes`, …) +> in place. **This is intentional, not silent:** as of issue #118 the +> blocked-PDF reason is surfaced to the caller ... so the operator always +> learns the entry was downgraded and why. + +The permission is conditional on the report. Since #539 there is a path where +the report does not exist: `metadata_only` takes `include_oa_location`, and when +it is omitted — the ordinary call shape — the OA lookup never runs. The record +built from that outcome carries `oa_status: None` and `license: "unknown"`, the +merge let them win, and a caller got no `note:` line, no `pdf.status`, and no log +row saying a known `gold` / `CC-BY-4.0` had been replaced with *not determined*. + +Measured before deciding, because the issue as filed claimed the wrong field: + +| field | existing | after a default re-write | +|---|---|---| +| `url` (where `oa_url` lands) | `Some("https://…/paper.pdf")` | `Some(…)` — preserved by `merge_opt!` | +| `[doiget].oa_status` | `Some("gold")` | `None` | +| `[doiget].license` | `"CC-BY-4.0"` | `"unknown"` | + +So the reserved top-level fields were never at risk. The defect is confined to +the two `[doiget]` fields whose absent value is a **marker** rather than a +reading — and both are documented as such: `oa_status` is "omitted when not +determined (#281)", `license` is "an OA license string, or the literal +`unknown`". + +## Decision + +**A marker for "no answer" does not overwrite an answer.** + +In `merge_metadata`, the `[doiget]` arm keeps the existing `oa_status` when the +incoming one is `None`, and the existing `license` when the incoming one is +`LICENSE_UNDETERMINED`. Everything else in the table still follows §6: doiget +owns it and the re-write wins. + +This cannot suppress real news, which is the only reason it is safe: + +- a paper that stops being open access reports `oa_status: Some("closed")` +- a license that changes reports the new string + +Neither reports the marker. `None` and `"unknown"` are only ever produced by a +call that did not look, so preferring the stored value is not a guess about +which is newer — it is the observation that only one of the two is a value. + +`"unknown"` gained a name (`LICENSE_UNDETERMINED`) so the merge does not depend +on a string literal matching the others scattered through the orchestrator. + +## Consequences + +`docs/STORE.md` §6's note says a downgrade guard "is deferred (post-MVP) — it is +a policy choice, not a correctness bug." That remains true of the case it was +written about (#123: an entry loses its PDF because the OA host went +off-allowlist) — there the downgrade is real, and it is reported. It is not true +of a field the caller never asked about, so the note is amended to scope its +claim to determinations rather than markers. + +Nothing BiblioFetch.jl reads changes shape. No field is added, removed or +renamed, `schema_version` does not move, and the reserved top-level fields keep +the `merge_opt!` behaviour they already had. This is a change to which of two +`[doiget]` values doiget itself keeps. + +### Not done + +**Warning on every preserve.** The reserved-field arms `warn!` when they keep an +existing value, because there it means two tools disagree and a human may need +to look. Here it is the ordinary outcome of the ordinary call shape; a log line +per default `metadata_only` would be noise, and the thing worth reporting — +losing the value — no longer happens. + +**Extending this to `source` or `size_bytes`.** They have no marker value. +`size_bytes: 0` is a legitimate reading for a metadata-only entry, and `source` +always names the resolver that actually ran. Treating either as absence would +invent exactly the call-history dependence #583 warned against. diff --git a/docs/DECISIONS/INDEX.md b/docs/DECISIONS/INDEX.md index bf87aeff7..70b1c9c00 100644 --- a/docs/DECISIONS/INDEX.md +++ b/docs/DECISIONS/INDEX.md @@ -72,6 +72,7 @@ Status column reconciled 2026-05-17 against `CHANGELOG.md` slices (issue #150). | 0053 | A DOI resolver is addressing, not hosting | Accepted | `fix/533-resolver-hop-is-addressing` | #533 | | 0054 | An access refusal is a type, and it collapses to `NO_OA_AVAILABLE` | Accepted | `fix/538-typed-access-refusal` | #538 | | 0055 | A failure says what to do about it, in three states | Accepted | `feat/506-error-disposition` | #506 | +| 0056 | A not-determined marker never overwrites a determination | Accepted | `fix/583-store-downgrade` | #583 | ## Conventions diff --git a/docs/STORE.md b/docs/STORE.md index 30b5cd671..c45e67980 100644 --- a/docs/STORE.md +++ b/docs/STORE.md @@ -166,7 +166,19 @@ existing on-disk value WINS over whatever doiget carries (issue #123). > entry's recorded state changes. This is intentional, not silent: as of issue #118 the > blocked-PDF reason is surfaced to the caller (CLI `note:` line / MCP `pdf.status`), > so the operator always learns the entry was downgraded and why. A guard that refuses -> to downgrade is deferred (post-MVP) — it is a policy choice, not a correctness bug. +> to downgrade a **determination** is deferred (post-MVP) — it is a policy choice, not a +> correctness bug. +> +> This permission covers determinations only. Two `[doiget]` fields carry a +> not-determined *marker* rather than a reading — `oa_status` is omitted when not +> determined, and `license` falls back to `"unknown"` — and a re-write that carries +> the marker did not look, so it is not news. Per +> [ADR-0056](DECISIONS/0056-not-determined-is-not-an-answer.md) the merge keeps the +> stored value in that case. A paper that genuinely stops being open access reports +> `oa_status = "closed"`, and a changed license reports the new string; both still +> win. The distinction matters because the permission above is granted on the +> condition that the downgrade is reported, and a field the caller never asked about +> has nothing to report it against (#583). ## 7. TOML normalization diff --git a/site/content/developer/store.md b/site/content/developer/store.md index 0cccce7fd..c229f5e8b 100644 --- a/site/content/developer/store.md +++ b/site/content/developer/store.md @@ -172,7 +172,19 @@ existing on-disk value WINS over whatever doiget carries (issue #123). > entry's recorded state changes. This is intentional, not silent: as of issue #118 the > blocked-PDF reason is surfaced to the caller (CLI `note:` line / MCP `pdf.status`), > so the operator always learns the entry was downgraded and why. A guard that refuses -> to downgrade is deferred (post-MVP) — it is a policy choice, not a correctness bug. +> to downgrade a **determination** is deferred (post-MVP) — it is a policy choice, not a +> correctness bug. +> +> This permission covers determinations only. Two `[doiget]` fields carry a +> not-determined *marker* rather than a reading — `oa_status` is omitted when not +> determined, and `license` falls back to `"unknown"` — and a re-write that carries +> the marker did not look, so it is not news. Per +> [ADR-0056](DECISIONS/0056-not-determined-is-not-an-answer.md) the merge keeps the +> stored value in that case. A paper that genuinely stops being open access reports +> `oa_status = "closed"`, and a changed license reports the new string; both still +> win. The distinction matters because the permission above is granted on the +> condition that the downgrade is reported, and a field the caller never asked about +> has nothing to report it against (#583). ## 7. TOML normalization