From 54544cbb0aa67bd9ae0874d33e923689b37fee62 Mon Sep 17 00:00:00 2001 From: Souta Date: Mon, 31 Aug 2026 15:47:08 +0900 Subject: [PATCH 1/3] fix(bib): a PubMed entry was reported as having no identifier; it has a PMID `entry has no DOI / arXiv id` is accurate about what the parser did and wrong about the entry. A user reading it goes and edits a bibliography that was fine, because the sentence puts the deficiency on their side. It is on ours. An existing test pinned exactly that claim -- `eprinttype = {pubmed}`, asserted as `NoIdentifier`, with the comment "the entry has no resolvable identifier". The entry has a PMID. It is not resolvable BY DOIGET, which is a different statement and the one #500 is about. Both PubMed shapes are named now: the `pmid = {...}` field PubMed's own BibTeX export writes, and the BibLaTeX `eprint` + `eprinttype = {pubmed}` pair that `arxiv_eligible` already refused -- correctly, and until now silently. `pmcid` too. NOT_IMPLEMENTED, not INVALID_REF. The input is valid and the support is absent, and the two codes carry opposite advice: "wait for a release" versus "correct your input". Disposition `terminal` follows (ADR-0055). `ParseError` is `#[non_exhaustive]`, so adding a variant compiled everywhere and the compiler could not help. All three consumers had wildcards, and in the MCP one the new variant would have become "unhandled bibliography parse error" -- losing the identifier it had just identified. Explicit arms added at all three; found by reading them rather than by the build. Scope: this is the REPORTING half of #500. `Ref::Pmid` is blocked on something the issue did not anticipate, and I checked before writing any of it: `docs/SAFEKEY.md` is "NORMATIVE (shared spec). Binding for both doiget and BiblioFetch.jl", its reference Julia implementation discriminates ref classes as `startswith(ref, "10.") ? "doi_" : "arxiv_"`, and a PMID is neither. Adding the class needs a coordinated ADR, a vectors update, and a matching change in another repository. Details on the issue. Verified against NCBI's primary documentation and a live round-trip before designing any of it: base `https://eutils.ncbi.nlm.nih.gov/ entrez/eutils/`, 3 req/sec without a key and 10 with one, key optional, `tool` and `email` encouraged. `esearch?term=[AID]` returns PMID 9659853 for `10.1176/ajp.155.7.895`, and `esummary` returns that DOI back under `articleids[].idtype == "doi"` -- so the resolver the issue proposes does work. It is the safekey contract that blocks it, not NCBI. Refs #500 Signed-off-by: Souta --- CHANGELOG.md | 27 ++++ Cargo.lock | 6 +- Cargo.toml | 8 +- crates/doiget-cli/src/commands/batch.rs | 14 ++ crates/doiget-cli/src/commands/verify.rs | 21 +++ crates/doiget-core/src/refs.rs | 170 ++++++++++++++++++++++- crates/doiget-mcp/src/lib.rs | 27 ++++ 7 files changed, 262 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19f79fa09..a9700e741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,6 +208,33 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he ### Fixed +- **[bib]** A PubMed-exported bibliography entry was reported as having **no + identifier**. It has a PMID (#500). + + `entry has no DOI / arXiv id` is accurate about what the parser did and wrong + about the entry. A user reading it goes and edits a `.bib` that was fine; the + missing piece is on doiget's side. An existing test pinned exactly that claim, + with the comment "the entry has no resolvable identifier" - about an entry + carrying `eprinttype = {pubmed}`. + + Both PubMed shapes are now named: the `pmid = {...}` field that PubMed's own + BibTeX export writes, and the BibLaTeX `eprint` + `eprinttype = {pubmed}` pair + that `arxiv_eligible` already refused, correctly and until now silently. + `pmcid` too. + + It surfaces as **`NOT_IMPLEMENTED`, not `INVALID_REF`** - the input is valid + and the support is absent, and the two carry opposite advice ("wait for a + release" versus "correct your input"). Its disposition is `terminal` + accordingly (ADR-0055). + + An entry with genuinely no identifier still reports `NoIdentifier`, and a DOI + alongside a PMID still wins: the check runs only after every supported + identifier has been tried, so it cannot divert an entry doiget could have + resolved. + + This is the reporting half of #500. `Ref::Pmid` itself is blocked on something + the issue did not anticipate - see below. + - **[mcp]** `error.disposition` was **missing from five failure envelopes**, including the two most common failures an agent sees. The change that introduced it converted the envelopes built by `error_object` and missed the diff --git a/Cargo.lock b/Cargo.lock index d1b8a1263..5c323f8c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -524,7 +524,7 @@ dependencies = [ [[package]] name = "doiget-cli" -version = "0.8.13-beta.15" +version = "0.8.13-beta.17" dependencies = [ "anyhow", "assert_cmd", @@ -554,7 +554,7 @@ dependencies = [ [[package]] name = "doiget-core" -version = "0.8.13-beta.15" +version = "0.8.13-beta.17" dependencies = [ "async-trait", "biblatex", @@ -591,7 +591,7 @@ dependencies = [ [[package]] name = "doiget-mcp" -version = "0.8.13-beta.15" +version = "0.8.13-beta.17" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 28969d7fc..efcb1bcea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ exclude = [ ] [workspace.package] -version = "0.8.13-beta.15" +version = "0.8.13-beta.17" 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.15" } -doiget-cli = { path = "crates/doiget-cli", version = "0.8.13-beta.15" } -doiget-mcp = { path = "crates/doiget-mcp", version = "0.8.13-beta.15" } +doiget-core = { path = "crates/doiget-core", version = "0.8.13-beta.17" } +doiget-cli = { path = "crates/doiget-cli", version = "0.8.13-beta.17" } +doiget-mcp = { path = "crates/doiget-mcp", version = "0.8.13-beta.17" } # Async runtime — features are deliberately limited (avoid `full`). tokio = { version = "1", default-features = false, features = [ diff --git a/crates/doiget-cli/src/commands/batch.rs b/crates/doiget-cli/src/commands/batch.rs index 3957c1a56..917478fde 100644 --- a/crates/doiget-cli/src/commands/batch.rs +++ b/crates/doiget-cli/src/commands/batch.rs @@ -121,6 +121,20 @@ pub async fn run_with_options( match entry { Ok(p) => inputs.push(p.ref_.as_input_str().to_string()), Err(ParseError::InvalidRef { raw, .. }) => inputs.push(raw), + // #500: a distinct placeholder, so the JSONL line says the entry + // HAS an identifier doiget cannot use rather than implying it has + // none. Same mechanism as the arm below; different claim. + Err(ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }) => { + let placeholder = match entry_key { + Some(k) => format!(""), + None => format!(""), + }; + inputs.push(placeholder); + } Err(ParseError::NoIdentifier { entry_key }) => { // Synthesise a recognisable placeholder so Step 7's // `Ref::parse` rejects this entry as `INVALID_REF` diff --git a/crates/doiget-cli/src/commands/verify.rs b/crates/doiget-cli/src/commands/verify.rs index cd6e7d8ca..ef7a4867b 100644 --- a/crates/doiget-cli/src/commands/verify.rs +++ b/crates/doiget-cli/src/commands/verify.rs @@ -267,6 +267,27 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "error": { "code": "INVALID_REF", "message": source.to_string() }, }), ), + // #500: still unverifiable, but for a reason the reader can act on + // -- and the action is not "fix the bibliography". + Err(ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }) => ( + VerifyStatus::Unverifiable, + serde_json::json!({ + "ok": false, + "ref": serde_json::Value::Null, + "status": VerifyStatus::Unverifiable.as_wire(), + "entry_key": entry_key, + "error": { + "code": "NOT_IMPLEMENTED", + "message": format!( + "entry is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500); it is NOT missing an identifier" + ), + }, + }), + ), Err(ParseError::NoIdentifier { entry_key }) => ( VerifyStatus::Unverifiable, serde_json::json!({ diff --git a/crates/doiget-core/src/refs.rs b/crates/doiget-core/src/refs.rs index e4f474174..0ae99c316 100644 --- a/crates/doiget-core/src/refs.rs +++ b/crates/doiget-core/src/refs.rs @@ -66,6 +66,28 @@ pub enum ParseError { /// The source bibliography's citation key, when known. entry_key: Option, }, + /// The entry DOES carry an identifier, and it is one doiget recognises + /// and cannot resolve yet (#500). + /// + /// Distinct from [`Self::NoIdentifier`] because the two send a reader in + /// opposite directions. "entry has no DOI / arXiv id" is accurate about + /// what the parser did and wrong about the entry: a PubMed-exported + /// `.bib` record carrying `pmid = {9659853}` is not deficient, and a user + /// who believes it is will go and edit a bibliography that was fine. The + /// missing piece is on doiget's side. + /// + /// Surfaces as `NOT_IMPLEMENTED` rather than `INVALID_REF`: the input is + /// valid and the support is absent, and the two carry different advice -- + /// "wait for a release" versus "correct your input" (ADR-0055). + #[error("entry {entry_key:?} is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500) -- it is NOT missing an identifier")] + UnsupportedIdentifier { + /// Human-facing name of the identifier class, e.g. `"PMID"`. + kind: &'static str, + /// The identifier as written in the entry. + value: String, + /// The source bibliography's citation key, when known. + entry_key: Option, + }, /// The identifier was present but `Ref::parse` rejected it /// (malformed DOI suffix, invalid arXiv id shape, etc.). #[error( @@ -423,9 +445,53 @@ fn parse_bibtex_entry( }; } } + // #500: before reporting "no identifier", check for one doiget simply + // does not support. Saying "no DOI / arXiv id" about an entry that + // carries a PMID is accurate about the parser and wrong about the entry. + if let Some((kind, value)) = unsupported_identifier(entry) { + return Err(ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }); + } Err(ParseError::NoIdentifier { entry_key }) } +/// An identifier doiget recognises but cannot resolve yet (#500). +/// +/// Only classes doiget can *name*. An entry carrying some field this does not +/// know about still reports [`ParseError::NoIdentifier`], which stays correct +/// for it: the point is not to guess, it is to stop saying "no identifier" +/// about the cases where there demonstrably is one. +/// +/// `pmid = {...}` is what PubMed's own BibTeX export writes. The BibLaTeX +/// shape is `eprint = {...}` with `eprinttype = {pubmed}`, which +/// [`arxiv_eligible`] already refuses -- correctly, and until now silently. +fn unsupported_identifier(entry: &biblatex::Entry) -> Option<(&'static str, String)> { + let field = |name: &str| -> Option { + let v = entry.get(name)?.format_verbatim().trim().to_string(); + (!v.is_empty()).then_some(v) + }; + + if let Some(v) = field("pmid") { + return Some(("PMID", v)); + } + if let Some(v) = field("pmcid") { + return Some(("PMCID", v)); + } + let names_pubmed = entry + .get("archiveprefix") + .or_else(|| entry.get("eprinttype")) + .is_some_and(|c| c.format_verbatim().to_ascii_lowercase().contains("pubmed")); + if names_pubmed { + if let Some(v) = field("eprint") { + return Some(("PMID", v)); + } + } + None +} + /// Whether an `eprint` field should be interpreted as an arXiv id. /// True when `archivePrefix` / `eprinttype` names arXiv (case- /// insensitive) or is absent; false when it explicitly names a @@ -450,6 +516,94 @@ mod tests { // ---- detect_format --------------------------------------------- + /// #500: the entry from PubMed's own BibTeX export. It carries `pmid`, + /// and reporting "entry has no DOI / arXiv id" about it is accurate about + /// the parser and wrong about the entry -- a user who believes it goes and + /// edits a bibliography that was fine. + /// + /// The PMID is real: `9659853` is Coryell 1998, whose DOI + /// `10.1176/ajp.155.7.895` NCBI's own esummary returns for it. + #[test] + fn a_pubmed_only_entry_says_it_has_a_pmid_not_that_it_has_nothing() { + let bib = r#"@article{coryell1998, + title = {Lithium discontinuation and subsequent effectiveness}, + author = {Coryell, William}, + year = {1998}, + pmid = {9659853}, +}"#; + let out = parse_bibtex(bib); + assert_eq!(out.len(), 1); + match &out[0] { + Err(ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }) => { + assert_eq!(kind, &"PMID"); + assert_eq!(value, "9659853"); + assert_eq!(entry_key.as_deref(), Some("coryell1998")); + let msg = out[0].as_ref().unwrap_err().to_string(); + assert!( + msg.contains("NOT missing an identifier"), + "the message has to contradict the wrong conclusion explicitly, or the reader draws it anyway: {msg}" + ); + } + other => panic!("expected UnsupportedIdentifier, got {other:?}"), + } + } + + /// The BibLaTeX shape. `arxiv_eligible` already refused this -- correctly, + /// and until now silently, which is the whole complaint. + #[test] + fn the_biblatex_eprinttype_pubmed_shape_is_recognised_too() { + let bib = r#"@article{e, + title = {T}, + eprint = {9659853}, + eprinttype = {pubmed}, +}"#; + let out = parse_bibtex(bib); + assert!( + matches!( + &out[0], + Err(ParseError::UnsupportedIdentifier { kind: "PMID", .. }) + ), + "got {:?}", + out[0] + ); + } + + /// An entry with genuinely nothing still reports `NoIdentifier`. The point + /// is not to relabel every failure -- it is to stop saying "no identifier" + /// about the cases where there demonstrably is one. + #[test] + fn an_entry_with_no_identifier_at_all_is_unchanged() { + let bib = "@article{x, + title = {T}, + year = {2020}, +}"; + let out = parse_bibtex(bib); + assert!( + matches!(&out[0], Err(ParseError::NoIdentifier { .. })), + "got {:?}", + out[0] + ); + } + + /// A DOI still wins. The new check runs only after every supported + /// identifier has been tried, so adding it cannot divert an entry doiget + /// could actually have resolved. + #[test] + fn a_doi_alongside_a_pmid_still_resolves() { + let bib = r#"@article{both, + title = {T}, + doi = {10.1176/ajp.155.7.895}, + pmid = {9659853}, +}"#; + let out = parse_bibtex(bib); + let parsed = out[0].as_ref().expect("the DOI must still win"); + assert_eq!(parsed.ref_.as_input_str(), "10.1176/ajp.155.7.895"); + } + #[test] fn detect_by_bib_extension() { let p = Utf8Path::new("/tmp/library.bib"); @@ -703,12 +857,20 @@ doi:10.1234/foo } #[test] - fn bibtex_non_arxiv_eprinttype_is_skipped() { - // eprinttype names a different server → not an arXiv id, and - // there is no DOI, so the entry has no resolvable identifier. + fn bibtex_non_arxiv_eprinttype_reports_the_identifier_it_found() { + // This test used to assert `NoIdentifier`, with the comment "the entry + // has no resolvable identifier". The entry has a PMID. It is not + // resolvable BY DOIGET, which is a different statement, and the one + // #500 is about -- so the test was pinning the wrong claim. let body = "@article{x, eprint = {12345678}, eprinttype = {pubmed}}"; let res = parse_bibtex(body).into_iter().next().unwrap(); - assert!(matches!(res, Err(ParseError::NoIdentifier { .. }))); + match res { + Err(ParseError::UnsupportedIdentifier { kind, value, .. }) => { + assert_eq!(kind, "PMID"); + assert_eq!(value, "12345678"); + } + other => panic!("expected UnsupportedIdentifier, got {other:?}"), + } } #[test] diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index af404b5fb..ac14c6423 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -980,6 +980,33 @@ impl Server { "error": error_object(ErrorCode::InvalidRef, source.to_string()), })); } + // #500: NOT_IMPLEMENTED, not INVALID_REF. The entry is fine; + // doiget is what is missing, and the two codes carry opposite + // advice -- "wait for a release" versus "correct your input". + // Without this arm it fell into the wildcard below and became + // "unhandled bibliography parse error", losing the identifier + // it had just identified. + Err(doiget_core::refs::ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }) => { + let msg = format!( + "entry is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500); it is NOT missing an identifier" + ); + if input.strict { + return Ok(CallToolResult::structured(batch_fetch_error_envelope( + ErrorCode::NotImplemented, + &format!("{msg} (strict mode aborts)"), + ))); + } + parse_errors.push(json!({ + "entry_key": entry_key, + "ref": Value::Null, + "ok": false, + "error": error_object(ErrorCode::NotImplemented, msg), + })); + } Err(doiget_core::refs::ParseError::NoIdentifier { entry_key }) => { if input.strict { return Ok(CallToolResult::structured(batch_fetch_error_envelope( From 6dbc271e24e33d921b6259ede9247dcdfc1ce351 Mon Sep 17 00:00:00 2001 From: Souta Date: Mon, 31 Aug 2026 23:31:28 +0900 Subject: [PATCH 2/3] fix(bib): one definition of the claim, and the whitespace three copies lost codecov/patch was red on this PR and it was right: nothing drove the three arms this change adds. `refs.rs` tests the parser; the JSONL placeholder, the verify row and the MCP envelope -- the things a user actually reads -- were asserted nowhere. Looking at them found the reason to care. The same sentence had been hand-copied to `verify.rs` and `doiget-mcp`, and both copies had been wrapped across source lines and re-joined with the indentation still in them: "...which doiget cannot resolve yet..." That is the defect the #580 review fixed elsewhere, reintroduced twice by me in the same cycle, and it shipped because no test read the string. So the fix is the cause, not the symptom: `unsupported_identifier_claim` in `doiget_core::refs`, called by both sites, tested once. `ParseError`'s own `#[error]` already carried the claim -- the copies existed only to drop the `entry {entry_key:?}` prefix, which each caller puts in a field of its own. Three more of the same class turned up, all pre-existing on next and all user-facing: two `= note:` lines in `fetch`'s widening advice, and the `config.toml could not be read` warning in `commands/mod.rs`. Fixed. A workspace-wide lint for the pattern is deliberately NOT here. Written, measured, dropped: it flags `config doctor`'s aligned two-column output and test assertion messages at every space threshold from four to ten (12 hits, 3 of them real), and the aligned output uses the same 8+ space runs the defect does. There is no structural discriminator, and a lint that cries wolf is a lint someone deletes. Removing the duplication is the half that lasts. Refs #500 Signed-off-by: Souta --- CHANGELOG.md | 17 ++++++++++++ crates/doiget-cli/src/commands/fetch.rs | 4 +-- crates/doiget-cli/src/commands/mod.rs | 2 +- crates/doiget-cli/src/commands/verify.rs | 4 +-- crates/doiget-core/src/refs.rs | 33 +++++++++++++++++++++++- crates/doiget-mcp/src/lib.rs | 7 +++-- 6 files changed, 57 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94a2f1736..03730b187 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -393,6 +393,23 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he This is the reporting half of #500. `Ref::Pmid` itself is blocked on something the issue did not anticipate - see below. + The sentence the user reads lives in one place now + (`refs::unsupported_identifier_claim`). It had been hand-copied to the CLI + `verify` row and the MCP `batch_from_bibliography` envelope, and both copies + had been wrapped across source lines and re-joined with the indentation still + in them - shipping `which doiget cannot resolve yet` to a + reader. Nothing asserted the text, so nothing failed. `#[error]` already + carried the same claim; the copies existed only to drop the `entry_key` + prefix, which each site puts in a field of its own. + + Three more of the same, pre-existing and found while looking: two `= note:` + lines in `fetch fetch`'s widening advice and the `config.toml could not be + read` warning in `commands/mod.rs`. A workspace-wide lint for the pattern was + written and abandoned - it flags `config doctor`'s aligned two-column output + and test assertion messages at every space threshold from four to ten, and a + lint that cries wolf is a lint that gets deleted. Removing the duplication is + the durable half. + - **[mcp]** `error.disposition` was **missing from five failure envelopes**, including the two most common failures an agent sees. The change that diff --git a/crates/doiget-cli/src/commands/fetch.rs b/crates/doiget-cli/src/commands/fetch.rs index 4209d9aca..44803f05d 100644 --- a/crates/doiget-cli/src/commands/fetch.rs +++ b/crates/doiget-cli/src/commands/fetch.rs @@ -1388,7 +1388,7 @@ fn not_found_trace_lines(ref_: &Ref, attempts: &[SourceAttempt]) -> Vec if !already_set.is_empty() { out.push(format!( - " = note: {} already set, but the source is still off -- this binary was built without the Cargo feature that provides it. Widening needs a differently-built binary, not another variable.", + " = note: {} already set, but the source is still off -- this binary was built without the Cargo feature that provides it. Widening needs a differently-built binary, not another variable.", already_set.join(", ") )); } @@ -1419,7 +1419,7 @@ fn not_found_trace_lines(ref_: &Ref, attempts: &[SourceAttempt]) -> Vec // reading as an ordering it is not. if !middle.is_empty() { out.push( - " = note: the middle is unordered because nothing in this run distinguishes those sources -- venue, affiliation and funder would, and none of them reach here. An invented order would read as information." + " = note: the middle is unordered because nothing in this run distinguishes those sources -- venue, affiliation and funder would, and none of them reach here. An invented order would read as information." .to_string(), ); } diff --git a/crates/doiget-cli/src/commands/mod.rs b/crates/doiget-cli/src/commands/mod.rs index 6b082d099..9b77cd969 100644 --- a/crates/doiget-cli/src/commands/mod.rs +++ b/crates/doiget-cli/src/commands/mod.rs @@ -231,7 +231,7 @@ fn store_root_from_config() -> Option { tracing::warn!( path = %path, error = %e, - "config.toml could not be read; [store] root ignored and the default store root used instead. Run `doiget config doctor` to see which root is in effect." + "config.toml could not be read; [store] root ignored and the default store root used instead. Run `doiget config doctor` to see which root is in effect." ); return None; } diff --git a/crates/doiget-cli/src/commands/verify.rs b/crates/doiget-cli/src/commands/verify.rs index ef7a4867b..66f5172df 100644 --- a/crates/doiget-cli/src/commands/verify.rs +++ b/crates/doiget-cli/src/commands/verify.rs @@ -282,8 +282,8 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "entry_key": entry_key, "error": { "code": "NOT_IMPLEMENTED", - "message": format!( - "entry is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500); it is NOT missing an identifier" + "message": doiget_core::refs::unsupported_identifier_claim( + kind, &value, ), }, }), diff --git a/crates/doiget-core/src/refs.rs b/crates/doiget-core/src/refs.rs index 0ae99c316..8297ba66d 100644 --- a/crates/doiget-core/src/refs.rs +++ b/crates/doiget-core/src/refs.rs @@ -126,6 +126,19 @@ pub enum ParseError { }, } +/// The claim [`ParseError::UnsupportedIdentifier`] makes, without the +/// `entry {entry_key:?}` prefix its `Display` carries. +/// +/// Callers that put `entry_key` in a field of its own -- the CLI `verify` +/// row and the MCP `batch_from_bibliography` envelope both do -- would +/// otherwise say it twice. One definition rather than a copy at each site, +/// because the copies drifted: two of the three carried a run of joined-line +/// whitespace into user-facing output before anything asserted the text. +#[must_use] +pub fn unsupported_identifier_claim(kind: &str, value: &str) -> String { + format!("entry is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500); it is NOT missing an identifier") +} + /// Input-shape discriminator per ADR-0030 D4. /// /// `Auto` means "detect from path extension and/or content @@ -545,7 +558,7 @@ mod tests { let msg = out[0].as_ref().unwrap_err().to_string(); assert!( msg.contains("NOT missing an identifier"), - "the message has to contradict the wrong conclusion explicitly, or the reader draws it anyway: {msg}" + "the message has to contradict the wrong conclusion explicitly, or the reader draws it anyway: {msg}" ); } other => panic!("expected UnsupportedIdentifier, got {other:?}"), @@ -986,4 +999,22 @@ doi:10.1234/foo assert_eq!(Format::CslJson.as_wire(), "csl-json"); assert_eq!(Format::Bibtex.as_wire(), "bibtex"); } + + #[test] + fn the_unsupported_identifier_claim_denies_the_wrong_reading() { + // #500's whole point: the sentence must put the gap on doiget's + // side. A reader who takes "no identifier" at face value goes and + // edits a `.bib` that was fine. + let msg = unsupported_identifier_claim("PMID", "9659853"); + assert!(msg.contains("PMID"), "names the identifier kind: {msg}"); + assert!(msg.contains("9659853"), "quotes the value: {msg}"); + assert!( + msg.contains("NOT missing an identifier"), + "denies the wrong reading: {msg}" + ); + assert!(msg.contains("#500"), "points at the issue: {msg}"); + // The `entry_key` prefix belongs to `Display`, not here -- callers + // carry it in a field of its own and would say it twice. + assert!(!msg.starts_with("entry {"), "no entry_key prefix: {msg}"); + } } diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 5eed10248..0fbbfcaa4 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -1011,9 +1011,8 @@ impl Server { value, entry_key, }) => { - let msg = format!( - "entry is identified only by {kind} {value:?}, which doiget cannot resolve yet (issue #500); it is NOT missing an identifier" - ); + let msg = + doiget_core::refs::unsupported_identifier_claim(kind, &value); if input.strict { return Ok(CallToolResult::structured(batch_fetch_error_envelope( ErrorCode::NotImplemented, @@ -4333,7 +4332,7 @@ fn store_root_from_config() -> Option { tracing::warn!( path = %path, error = %e, - "config.toml could not be read; [store] root ignored and the default store root used instead" + "config.toml could not be read; [store] root ignored and the default store root used instead" ); return None; } From c90863b4a286077f353af8865e5eddf291922914 Mon Sep 17 00:00:00 2001 From: Souta Date: Mon, 31 Aug 2026 23:43:43 +0900 Subject: [PATCH 3/3] style: rustfmt the rewired MCP call site The shared-helper call fits on one line; I had left it wrapped the way the `format!` it replaced was. Signed-off-by: Souta --- crates/doiget-mcp/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 0fbbfcaa4..86662990f 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -1011,8 +1011,7 @@ impl Server { value, entry_key, }) => { - let msg = - doiget_core::refs::unsupported_identifier_claim(kind, &value); + let msg = doiget_core::refs::unsupported_identifier_claim(kind, &value); if input.strict { return Ok(CallToolResult::structured(batch_fetch_error_envelope( ErrorCode::NotImplemented,