From e197a950bc2bb915b0048d58d717b6515baa67c0 Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 12:23:41 +0900 Subject: [PATCH 1/5] fix: close the review findings on #580, and cut 0.8.13 Review of the 0.8.13 promotion found one defect class repeated across the release: a fix that landed on one surface and not its sibling. This closes those, corrects the claims that were wrong about the world, and strips the beta. The one that could hurt a caller: `doiget_batch_fetch` and `doiget_batch_from_bibliography` still held verbatim copies of the hand-rolled `FetchError -> ErrorCode` match that `fetch_paper_fetch_error_envelope` shed this cycle, ending in `_ => InternalError`. `disposition` is derived from that code, so a 404 on a batch entry arrived as `retry_after` and an agent was told to keep retrying a DOI that will never resolve. Half-applied fixes, now applied to both surfaces: * #500 `doiget batch` still said INVALID_REF for a PMID-only entry * #534 `doiget search --mode json` still emitted the silent zero envelope * #507 the MCP SessionEnd row said result "ok" beside an error_code * #533 `config doctor --network` still adjudicated with matches, not permits * #500 CSL-JSON had no PMID check; only BibTeX did Claims that did not match the code: * `Formula/doiget.rb` and its generator said the release workflow regenerates the formula. No workflow calls it, and CONTRIBUTING.md says so. * The Homebrew posture check said it fails on a bad checksum. It re-reads the checksums from the file under test. * docs/ERRORS.md named `every_bare_string_error_site_is_a_known_one` as the guard on its bare-string exemptions. The guard did not exist. It does now, and the set it pins is empty. * #462 was filed as the shape of #454 reachable again, and raised as a possible release blocker. It was not: the production branch of both client builders extends with `tier_3_allowlists()` and always did. The test-override branch had no Tier-3 entry, so no wiremock e2e could reach the route on either surface. Registering those keys makes the reproduction pass, so it is no longer #[ignore]d and `route_coverage_e2e` has no remaining Gap. Also: a 404 carrying Retry-After was classified NETWORK_ERROR, because #506 added `retry_after_ms: None` to the arm mapping authoritative absences. The arXiv-only text tools answered a DOI with NO_OA_AVAILABLE, whose disposition sends an agent looking for a config knob that does not exist. `.mcp.json` is pinned rather than resolving npm latest on every start, `just mcp-dev` exists so the pin does not cost dogfooding, and a posture check holds the four release-tracking files to one version. Cut: 0.8.13-beta.23 to 0.8.13. The version-bump gate is red by design on a cut (ADR-0025); it goes green on the next to main promotion this enables. Refs #500, #506, #507, #533, #534, #538, #462, #580 Signed-off-by: Sota Shimozono --- .github/workflows/posture-lint.yml | 42 ++- .mcp.json | 2 +- CHANGELOG.md | 172 +++++++++- CONTRIBUTING.md | 51 ++- Cargo.lock | 6 +- Cargo.toml | 2 +- Formula/doiget.rb | 8 +- crates/doiget-cli/src/commands/batch.rs | 166 ++++++--- crates/doiget-cli/src/commands/config.rs | 7 +- crates/doiget-cli/src/commands/fetch.rs | 29 +- crates/doiget-cli/src/commands/graph.rs | 28 +- crates/doiget-cli/src/commands/search.rs | 23 +- crates/doiget-cli/src/commands/verify.rs | 9 +- crates/doiget-cli/tests/batch_jsonl_e2e.rs | 58 ++++ crates/doiget-core/src/citation_graph.rs | 25 ++ crates/doiget-core/src/discovery.rs | 64 ++++ crates/doiget-core/src/http.rs | 22 +- crates/doiget-core/src/orchestrator.rs | 31 ++ crates/doiget-core/src/refs.rs | 112 ++++++ crates/doiget-core/src/source.rs | 26 +- crates/doiget-mcp/src/lib.rs | 323 +++++++++++------- .../doiget-mcp/tests/error_envelope_shape.rs | 98 ++++++ crates/doiget-mcp/tests/fetch_paper_e2e.rs | 39 ++- crates/doiget-mcp/tests/paper_text_e2e.rs | 17 +- crates/doiget-mcp/tests/route_coverage_e2e.rs | 28 +- docs/ERRORS.md | 2 +- justfile | 26 ++ npm/doiget-cli/test/stage-npm.test.sh | 13 +- scripts/stage-npm.sh | 0 scripts/update-homebrew-formula.sh | 17 +- scripts/update-homebrew-formula.test.sh | 0 site/content/developer/errors.md | 2 +- 32 files changed, 1209 insertions(+), 239 deletions(-) create mode 100644 crates/doiget-mcp/tests/error_envelope_shape.rs mode change 100644 => 100755 scripts/stage-npm.sh mode change 100644 => 100755 scripts/update-homebrew-formula.sh mode change 100644 => 100755 scripts/update-homebrew-formula.test.sh diff --git a/.github/workflows/posture-lint.yml b/.github/workflows/posture-lint.yml index 8769ec19b..05c0ab017 100644 --- a/.github/workflows/posture-lint.yml +++ b/.github/workflows/posture-lint.yml @@ -287,10 +287,48 @@ jobs: shell: bash # `Formula/doiget.rb` is generated from a release's published `.sha256` # assets. #247 was closed as completed while four fifths of it had not - # shipped, so the formula is asserted rather than trusted: this fails on - # a hand-edit, a bad checksum, or a tag/version confusion. + # shipped, so the formula is asserted rather than trusted. + # + # What this fails on: a hand-edit, a MALFORMED checksum, a `v`-prefixed + # version. What it CANNOT fail on, because it re-reads the version and + # the checksums from the very file under test: a well-formed checksum + # carrying the wrong value, or a version that is simply stale. It said + # it caught "a bad checksum" and it does not -- nothing here reaches the + # published `.sha256` assets, offline being the point. The stale-version + # half is covered by the release-sync check below; a wrong-value + # checksum is caught by `brew install` and by nothing before it. run: bash scripts/update-homebrew-formula.test.sh + - name: release-tracking files name one version (#501, #511) + shell: bash + # Four files record "the last published stable", and a stable release + # bumps all four in one maintainer step. Nothing used to hold them + # together, which is how `.claude-plugin/*` sat at 0.8.11 while 0.8.12 + # was the shipped release. Forgetting the step is the only way the + # Homebrew formula or the plugin's pinned server can go stale, so this + # makes forgetting it red. + run: | + set -euo pipefail + formula=$(grep -oE '^ version "[^"]+"' Formula/doiget.rb | grep -oE '[0-9][^"]*') + plugin=$(grep -oE '"version": *"[^"]+"' .claude-plugin/plugin.json | grep -oE '[0-9][^"]*') + market=$(grep -oE '"version": *"[^"]+"' .claude-plugin/marketplace.json | grep -oE '[0-9][^"]*') + pinned=$(grep -oE '"doiget-cli@[^"]+"' .mcp.json | grep -oE '[0-9][^"]*') + echo "formula=$formula plugin=$plugin marketplace=$market .mcp.json=$pinned" + for v in "$plugin" "$market" "$pinned"; do + if [ "$v" != "$formula" ]; then + echo "::error::posture-lint release-sync: release-tracking files disagree. Bump Formula/doiget.rb, .claude-plugin/plugin.json, .claude-plugin/marketplace.json and .mcp.json together (CONTRIBUTING.md, 'one step after a stable release')" + exit 1 + fi + done + # An unpublished pin would send every plugin user to a 404. + case "$pinned" in + *-beta.*|*-rc.*) + echo "::error::posture-lint release-sync: .mcp.json pins a prerelease ($pinned); the plugin must point at a published stable" + exit 1 + ;; + esac + echo "release-tracking files agree on $formula" + - name: npm packaging tests (#511) shell: bash # The name-list greps above cannot catch a wrong binary name, a diff --git a/.mcp.json b/.mcp.json index 5f143481d..7fe049159 100644 --- a/.mcp.json +++ b/.mcp.json @@ -4,7 +4,7 @@ "command": "npx", "args": [ "-y", - "doiget-cli", + "doiget-cli@0.8.12", "serve" ] } diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ef931296..88f7dfb9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,180 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `doiget-core` is the only crate with strict semver guarantees during the 0.x line; CLI flag changes and `doiget-mcp` tool spec changes will be called out explicitly here. -## [Unreleased] +## [0.8.13] - 2026-09-01 ### Fixed +- **[mcp] (breaking)** `doiget_batch_fetch` and `doiget_batch_from_bibliography` + reported a **refused paper as `INTERNAL_ERROR` and a 404 as `NETWORK_ERROR`** + (#538, review of #580). + + `fetch_paper_fetch_error_envelope` shed a hand-rolled copy of + `From<&FetchError> for ErrorCode` this cycle -- the one ending in + `_ => InternalError`. Two verbatim copies of that match stayed behind in + `build_bibliography_envelope` and `batch_fetch_success_envelope`, one screen + below the fix. So the headline of this release, "an access refusal is not an + internal error", held on `doiget_fetch_paper` and on no other fetch surface: + `NotFound`, `Ambiguous` and the brand-new `NotRetrievable` all fell through + the wildcard, and every `Http` status collapsed to `NETWORK_ERROR`. + + The last one is the damaging one. `disposition` is derived from the code, so + a 404 on a batch entry arrived as `retry_after` -- an agent told to keep + retrying a DOI that will never resolve, which is the exact failure + [ADR-0055](docs/DECISIONS/0055-error-disposition.md) exists to prevent. Both + sites call `ErrorCode::from(err)` now, and a test drives the real envelope + builders rather than a re-implementation of them. + +- **[cli] (breaking)** `doiget batch` reported a PMID-only bibliography entry as + `INVALID_REF` (#500). + + The reporting half of #500 landed on `doiget verify` and the MCP bibliography + tool and not here. The verdict the parser reached was flattened into a + placeholder string (``) and handed back to `Ref::parse`, + which has exactly one thing to say. So a PubMed-exported `.bib` record with a + valid `pmid` field was still reported as malformed -- the one claim #500 + exists to stop doiget making -- and the message described the placeholder + rather than the entry. The verdict travels as a value now + (`BatchEntry::Rejected`), so it cannot be flattened, and the comment claiming + the two arms made "different claims" is no longer false. + +- **[cli/mcp] (breaking)** `GraphError::Source` mapped to `NETWORK_ERROR` on the + MCP surface and to the wrapped `FetchError` code on the CLI. + + An OpenAlex 404 during citation-graph expansion was `NOT_FOUND` (terminal) on + `doiget graph` and `NETWORK_ERROR` (`retry_after`) on + `doiget_expand_citation_graph`. The CLI doc comment said the two "cannot say + different things about the same error". `From<&GraphError> for ErrorCode` now + lives beside the enum in `doiget-core`, and all three call sites -- the third + was an inline copy that did not even call the helper directly above it -- go + through it. + +- **[core]** A 404/410/451 carrying a `Retry-After` header was classified + `NETWORK_ERROR` (follow-up to #506). + + Adding `retry_after_ms` to `HttpError::HttpStatus` also added + `retry_after_ms: None` to the arm that maps authoritative absences, narrowing + it without comment or test. The header says how long to wait *if* you retry; + it does not make a 404 provisional. + +- **[mcp] (breaking)** `doiget_paper_text` and `doiget_paper_tex_source` answered + a DOI with `NO_OA_AVAILABLE`, whose disposition is `needs_config`. + + There is no config knob: the tools are arXiv-only and DOI-to-arXiv linking is + not built. `NOT_IMPLEMENTED` is terminal and is what `verify` and + `batch_from_bibliography` already give the same situation for PMIDs -- valid + input, absent support. + +- **[mcp] (breaking)** `doiget_resolve_citation`, `doiget_batch_resolve_citations`, + `doiget_tag` and `doiget_annotate` put a **bare string** in `error`, so a + caller had no `code` to branch on and no `disposition` to decide a retry from. + All thirteen sites build `error_object` now. + + Section 3 of `docs/ERRORS.md` named `every_bare_string_error_site_is_a_known_one` + as the guard that kept that set from growing. **The guard did not exist.** The + document was accurate about the four tools and wrong about the mechanism + keeping the list honest -- a claim resting on code nobody had written, which + is the defect class that document defines. It exists now, the set it pins is + empty, and a second test fails if the document and the list ever disagree. + +- **[core]** `doiget config doctor --network` still adjudicated hosts with + `matches` rather than `permits` (#533). + + The `permits` doc comment said "the four adjudication sites ... all route + through here so they cannot disagree". This was a fifth, and the one whose job + is explaining allowlist refusals: the moment a resolver host entered its probe + list, the command that diagnoses #533 would have reproduced it. + +- **[core]** CSL-JSON entries identified only by a PMID or PMCID reported "entry + has no DOI / arXiv id" (#500). The BibTeX parser learned the distinction; this + format did not, and it is the one a Zotero user is most likely to export. + +- **[cli]** `doiget search --mode json` emitted `{"ok": true, "total_results": 0}` + with no hint for an over-long query (#534). The fix landed on the MCP tool + only, so this surface went on emitting the exact envelope the issue was filed + about. `zero_result_hint` moved to `doiget-core`; both surfaces call it, and + human mode gets a `= note:` on stderr. + +- **[mcp]** `doiget_fetch_paper` logged `SessionEnd` as `result: "ok"` beside a + non-null `error_code` for a blocked PDF leg (#507). The clean-success rule of + the CLI was ported to the MCP surface in the `error_code` half only, so the + row contradicted itself for the one outcome the repeat suppression of #507 has + to be able to see. `FetchPaperOutcome::is_clean_success` is the single + boundary now. + +### Changed + +- **[test]** The Tier-3 TDM-fetched route is asserted end to end and no longer + `#[ignore]`d (#462). + + It was shipped as a failing reproduction, diagnosed as + "`tier_3_allowlists()` is `#[cfg]`-gated and the MCP server does extend its + allowlists with it, so the two disagree somewhere between construction and + use" -- the shape of #454, reachable again -- and raised as something to + decide before cutting this release. + + That diagnosis was wrong, in the subject matter of this very release: + accurate about the code, false about the world. Both client builders have two + branches. The production branch does extend with `tier_3_allowlists()` and was + correct throughout. The test-override branch -- taken whenever any + `DOIGET_*_BASE` is set, which every wiremock test does -- built its allowlist + from a fixed table of Tier-1/2 keys with no Tier-3 entry, so no e2e on either + surface could reach the route. The defect was in the harness. Registering the + Tier-3 keys there is what the test now proves by passing, and + `route_coverage_e2e` has no remaining `Gap`. + + What survives from that diagnosis, and is still true: `fetch_content` is + implemented by APS alone, so three of the four Tier-3 sources cannot reach the + route the tier exists for. Read it as APS coverage, not Tier-3 coverage. + +- **[docs]** `Formula/doiget.rb` and `scripts/update-homebrew-formula.sh` said + the release workflow regenerates the formula "so this file cannot describe a + release that does not exist". No workflow calls the generator, and + `CONTRIBUTING.md` -- added in the same release -- says outright that it is not + automated. The headers describe the real process now. + +- **[ci]** The Homebrew posture check said it "fails on a hand-edit, a bad + checksum, or a tag/version confusion". It re-reads the version and the + checksums from the file under test, so a well-formed checksum carrying the + wrong value, and a version that is simply stale, both pass. The step now says + what it checks and what it does not. + +- **[ci]** New `release-tracking files name one version` posture check. + `Formula/doiget.rb`, `.claude-plugin/plugin.json`, + `.claude-plugin/marketplace.json` and `.mcp.json` all record the last + published stable, nothing held them together, and that is how `.claude-plugin` + sat at 0.8.11 while 0.8.12 was shipping. Forgetting one of the four is red; + pinning a prerelease is red. + +- **[plugin]** `.mcp.json` pins `doiget-cli@0.8.12` instead of resolving the npm + `latest` tag on every server start. The unpinned form also meant that opening + this repository in Claude Code ran the *published release* rather than the + working tree -- editing `crates/doiget-mcp` and testing the build from last + month. `just mcp-dev` registers this checkout as a local-scoped server + instead, and `CONTRIBUTING.md` says so rather than leaving `.mcp.json` to be + edited. + +- **[cli]** `verify` and `batch` wrote wire codes as string literals + (`"INVALID_REF"`, `"NOT_IMPLEMENTED"`) next to a doc comment rejecting exactly + that. They come from `ErrorCode::as_wire` now. + +- **[test]** The `#[ignore]` detector in `route_coverage_e2e` matched any text + containing `#[ignore` in the block above a test, so a doc comment *discussing* + an ignored test made it report that test as skipped. It reads attribute lines. + +- **[docs]** In `crates/doiget-mcp/src/lib.rs`, the doc comment for + `store_root_env_is_usable` had drifted onto `zero_result_hint`. + +- **[scripts]** `scripts/*.sh` are executable, and `CONTRIBUTING.md` no longer + documents a command that fails on a fresh Linux or macOS clone. + +- **[test]** In `npm/doiget-cli/test/stage-npm.test.sh`, a `grep` matching + nothing killed the script under `set -e` *before* the `check no` written to + report that case, so the diagnostic was unreachable in the one situation it + exists for. + +### Fixed (accumulated on `next` since 0.8.12) + - **[store]** A default `metadata_only` re-write **downgraded a known `oa_status` and `license` to their not-determined markers**, and said nothing (#583). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f2725e256..377f5689b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -216,28 +216,55 @@ is the binding spec and full runbook. Summary: to the fixed commit (the pipeline runs the workflow/scripts *as of the tagged tree*). Do not reintroduce a perpetual "release PR". -### Homebrew: one step after a stable release +### One step after a stable release -`Formula/doiget.rb` pins the release binaries by `sha256`, and those checksums -do not exist until the release has published its assets. So the formula lags -the tag by one commit, on purpose, and closing that gap is a maintainer step: +Four files record *the last published stable*, and none of them can be written +before the release exists: `Formula/doiget.rb` pins the binaries by `sha256`, +and `.mcp.json` pins the npm version the plugin runs. So they lag the tag by +one commit, on purpose, and closing that gap is a single maintainer step: ```sh -scripts/update-homebrew-formula.sh 0.8.12 # reads the release's .sha256 assets -bash scripts/update-homebrew-formula.test.sh # the same check CI runs -git commit -s Formula/doiget.rb -m "chore(homebrew): formula for 0.8.12" +bash scripts/update-homebrew-formula.sh 0.8.13 # reads the release's .sha256 assets +# then set 0.8.13 in all three of: +# .claude-plugin/plugin.json "version" +# .claude-plugin/marketplace.json "version" +# .mcp.json args: doiget-cli@0.8.13 +bash scripts/update-homebrew-formula.test.sh # the same check CI runs +git commit -s -m "chore(release): tracking files for 0.8.13" -- Formula/doiget.rb .claude-plugin .mcp.json ``` -Only for **stable** releases. Beta tags are not published to the tap. +Only for **stable** releases. Beta tags are not published to the tap, and the +plugin must never pin a prerelease -- posture-lint fails on both. It is not automated, and that is a decision rather than an omission: committing -the formula back from the release workflow needs a token with write access to a +these back from the release workflow needs a token with write access to a protected branch, and that token is exactly what [#426](https://github.com/QAtlasHub/doiget/issues/426) says is broken. Building the tap's correctness on a known-broken token would be worse than a documented -step. The generator plus `update-homebrew-formula.test.sh` mean the step cannot -be done *wrong* -- a hand-edit, a bad checksum, or a `v`-prefixed version all -fail CI -- only forgotten, which `brew info doiget` makes visible. +step. + +What CI does enforce, so the step cannot be done *wrong* or half-done: + +- `update-homebrew-formula.test.sh` fails on a hand-edit, a malformed + checksum, or a `v`-prefixed version. It does **not** reach the published + `.sha256` assets (it is deliberately offline), so it cannot see a + well-formed checksum carrying the wrong value. +- The `release-tracking files name one version` posture check fails when the + four files disagree. Bumping three and forgetting the fourth is red; + forgetting all four is not, which is what `brew info doiget` is for. + +### Running the MCP server from your working tree + +`.mcp.json` is the plugin's server declaration, so it pins the last published +release. Opening this repo in Claude Code therefore runs *the release*, not +your checkout -- you can edit `crates/doiget-mcp` and be testing the version +you shipped last month. Do not edit `.mcp.json` to work around that; a +local-scoped server takes precedence over the project-scoped file: + +```sh +just mcp-dev # registers this checkout's build as `doiget-dev` +just mcp-dev-off # removes it again +``` ### npm: the one-time bootstrap diff --git a/Cargo.lock b/Cargo.lock index 5703131e4..cb76a31cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -524,7 +524,7 @@ dependencies = [ [[package]] name = "doiget-cli" -version = "0.8.13-beta.23" +version = "0.8.13" dependencies = [ "anyhow", "assert_cmd", @@ -554,7 +554,7 @@ dependencies = [ [[package]] name = "doiget-core" -version = "0.8.13-beta.23" +version = "0.8.13" dependencies = [ "async-trait", "biblatex", @@ -591,7 +591,7 @@ dependencies = [ [[package]] name = "doiget-mcp" -version = "0.8.13-beta.23" +version = "0.8.13" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 6659621d7..cb8667bf6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ exclude = [ ] [workspace.package] -version = "0.8.13-beta.23" +version = "0.8.13" edition = "2021" rust-version = "1.86" license = "MIT" diff --git a/Formula/doiget.rb b/Formula/doiget.rb index 35a055689..5492c1e86 100644 --- a/Formula/doiget.rb +++ b/Formula/doiget.rb @@ -1,7 +1,11 @@ # GENERATED by scripts/update-homebrew-formula.sh -- do not edit by hand. # -# Regenerated by the release workflow for every published version, so this -# file cannot describe a release that does not exist (#501). +# Regenerate with `scripts/update-homebrew-formula.sh ` after a stable +# release has published its assets. That is a MAINTAINER step, not a workflow +# step -- committing the formula back from CI needs a token with write access +# to a protected branch, and #426 says that token is broken. So the formula +# lags the tag until someone runs the script, and CONTRIBUTING.md says so +# (#501). class Doiget < Formula desc "Open Access paper fetcher with an MCP server" homepage "https://github.com/QAtlasHub/doiget" diff --git a/crates/doiget-cli/src/commands/batch.rs b/crates/doiget-cli/src/commands/batch.rs index 917478fde..fa9f3f3ab 100644 --- a/crates/doiget-cli/src/commands/batch.rs +++ b/crates/doiget-cli/src/commands/batch.rs @@ -51,6 +51,32 @@ use super::fetch::{ }; use super::resolve_store_root; +/// One entry of the input file, on its way into the dispatch window. +/// +/// `Rejected` carries the verdict the bibliography parser already reached. +/// Before #500's reporting half this was a plain `Vec`: every entry +/// the parser could not turn into a `Ref` became a synthetic placeholder +/// (``, ``) that was handed back to +/// `Ref::parse`, whose only possible answer is `INVALID_REF`. So a PubMed +/// `.bib` record with a valid `pmid` field was reported to the operator as a +/// malformed reference -- the one claim #500 exists to stop doiget making -- +/// and the accompanying message described the placeholder rather than the +/// entry. The verdict travels as a value now, so it cannot be flattened. +enum BatchEntry { + /// A ref string still to be parsed. + Ref(String), + /// Unfetchable, with the reason already decided. + Rejected { + /// What to show in the JSONL `ref` field and the failure digest. + display: String, + /// The closed-set code the operator should see. + code: ErrorCode, + /// The parser's own message, about the entry rather than a + /// placeholder synthesised from it. + message: String, + }, +} + /// Run the `doiget batch ` subcommand. /// /// When `dry_run` is `true` (per ADR-0022 §1 + §3): read the input @@ -116,37 +142,63 @@ pub async fn run_with_options( // error rather than a silently-empty batch. let path_utf8 = Utf8Path::new(&path); let parsed = refs::parse_input(&raw, Format::Auto, Some(path_utf8)); - let mut inputs: Vec = Vec::with_capacity(parsed.len()); + let mut inputs: Vec = Vec::with_capacity(parsed.len()); for entry in parsed { 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 { + Ok(p) => inputs.push(BatchEntry::Ref(p.ref_.as_input_str().to_string())), + // The raw identifier verbatim, so the downstream `Ref::parse` + // reports `INVALID_REF` against the string the user wrote. That + // IS the right claim here. + Err(ParseError::InvalidRef { raw, .. }) => inputs.push(BatchEntry::Ref(raw)), + // #500. The verdict travels as a value. It used to travel as a + // placeholder string handed back to `Ref::parse`, and `Ref::parse` + // has exactly one thing to say -- `INVALID_REF` -- so this arm and + // the `NoIdentifier` arm below reached the user as the SAME claim + // ("your bibliography is malformed") despite the comment here + // saying they differed, and the message described the synthetic + // placeholder rather than the user's entry. That claim is what + // #500 exists to stop doiget making about an entry that is fine. + Err(err @ ParseError::UnsupportedIdentifier { .. }) => { + let message = err.to_string(); + let ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + } = err + else { + unreachable!("guarded by the pattern above") + }; + let display = match entry_key { Some(k) => format!(""), None => format!(""), }; - inputs.push(placeholder); + inputs.push(BatchEntry::Rejected { + display, + // The input is valid and the support is absent. The same + // code the MCP `batch_from_bibliography` and CLI `verify` + // surfaces already gave this entry. + code: ErrorCode::NotImplemented, + message, + }); } - Err(ParseError::NoIdentifier { entry_key }) => { - // Synthesise a recognisable placeholder so Step 7's - // `Ref::parse` rejects this entry as `INVALID_REF` - // with the operator's citation key visible in the - // JSONL `ref` field. A future slice will plumb the - // structured `entry_key` into a dedicated error - // object field (ADR-0030 §6). - let placeholder = match entry_key { + Err(err @ ParseError::NoIdentifier { .. }) => { + let message = err.to_string(); + let ParseError::NoIdentifier { entry_key } = err else { + unreachable!("guarded by the pattern above") + }; + // The operator's citation key stays visible in the JSONL + // `ref` field. A future slice will plumb the structured + // `entry_key` into a dedicated error-object field + // (ADR-0030 §6). + let display = match entry_key { Some(k) => format!(""), None => "".to_string(), }; - inputs.push(placeholder); + inputs.push(BatchEntry::Rejected { + display, + code: ErrorCode::InvalidRef, + message, + }); } Err(ParseError::Decode { format, message }) => { return Err(anyhow!("input did not deserialise as {format}: {message}")); @@ -171,7 +223,11 @@ pub async fn run_with_options( "encountered unknown ParseError variant; batch continues with placeholder \ INVALID_REF — this should never happen on a current doiget-core build" ); - inputs.push(format!("")); + inputs.push(BatchEntry::Rejected { + display: format!(""), + code: ErrorCode::InvalidRef, + message: other.to_string(), + }); } } } @@ -195,7 +251,24 @@ pub async fn run_with_options( if dry_run { let store_root = resolve_store_root()?; let mut parse_errors: usize = 0; - for input in &inputs { + for entry in &inputs { + let input = match entry { + BatchEntry::Ref(input) => input, + BatchEntry::Rejected { + display: shown, + code, + message, + } => { + parse_errors += 1; + tracing::warn!( + input = %shown, + error = %message, + code = code.as_wire(), + "skipping unfetchable batch entry in dry-run mode", + ); + continue; + } + }; match Ref::parse(input) { Ok(ref_) => { let plan = build_fetch_plan(&ref_, &store_root); @@ -258,24 +331,40 @@ pub async fn run_with_options( // Skip delay before the very first fetch; apply between all subsequent ones. let mut is_first_spawn = true; loop { - let window: Vec = remaining.by_ref().take(MCP_BATCH_MAX_SIZE).collect(); + let window: Vec = remaining.by_ref().take(MCP_BATCH_MAX_SIZE).collect(); if window.is_empty() { break; } let mut joins: tokio::task::JoinSet = tokio::task::JoinSet::new(); - for input in window { - let ref_ = match Ref::parse(&input) { - Ok(r) => r, - Err(e) => { + for entry in window { + // Either the parser already reached a verdict about this entry, or + // `Ref::parse` reaches one now. Both land in the SAME reporting + // block below -- which is the point: a verdict that is not + // `INVALID_REF` now survives to the caller instead of being + // laundered through a placeholder string. + let parsed = match entry { + BatchEntry::Ref(input) => match Ref::parse(&input) { + Ok(r) => Ok((input, r)), + Err(e) => Err((input, ErrorCode::InvalidRef, e.to_string())), + }, + BatchEntry::Rejected { + display, + code, + message, + } => Err((display, code, message)), + }; + let (input, ref_) = match parsed { + Ok(pair) => pair, + Err((input, code, message)) => { parse_errors += 1; - failures.push((input.clone(), "INVALID_REF")); + failures.push((input.clone(), code.as_wire())); if json_mode { - // #205: parse failures get an INVALID_REF JSONL line - // with the human message in `error.message`. Per - // ERRORS.md §3.1 there is no `denial_context` on - // INVALID_REF (the input never reached a guard). - emit_jsonl_failure(Some(&input), "INVALID_REF", &e.to_string()); + // #205: an unfetchable entry gets a JSONL line with the + // human message in `error.message`. Per ERRORS.md §3.1 + // there is no `denial_context` on these -- the input + // never reached a guard. + emit_jsonl_failure(Some(&input), code.as_wire(), &message); } // Best-effort `Resolve` row capturing the parse failure; // we do NOT abort the batch on a single bad line. @@ -285,7 +374,7 @@ pub async fn run_with_options( capability: Capability::Oa, ref_: Some(&input), source: None, - error_code: Some("INVALID_REF"), + error_code: Some(code.as_wire()), size_bytes: None, license: None, store_path: None, @@ -296,8 +385,9 @@ pub async fn run_with_options( }); tracing::warn!( %input, - error = %e, - "skipping malformed batch entry", + error = %message, + code = code.as_wire(), + "skipping unfetchable batch entry", ); continue; } diff --git a/crates/doiget-cli/src/commands/config.rs b/crates/doiget-cli/src/commands/config.rs index 10b357e32..76a9e84a8 100644 --- a/crates/doiget-cli/src/commands/config.rs +++ b/crates/doiget-cli/src/commands/config.rs @@ -861,7 +861,12 @@ async fn network_report(cfg: &ResolvedConfig) { ("doaj.org", "https://doaj.org/robots.txt"), ]; for (host, url) in PROBES { - let verdict = if !allow.matches(host) { + // `permits`, not `matches` (#533). This is the fifth adjudication + // site, and the one whose whole job is telling a user why a host was + // refused: with `matches` it would report a DOI resolver as + // `NotAllowlisted` while the real fetch path follows it, which is the + // exact wrong answer #533 was about. + let verdict = if !allow.permits(host) { ProbeVerdict::NotAllowlisted } else { match url::Url::parse(url) { diff --git a/crates/doiget-cli/src/commands/fetch.rs b/crates/doiget-cli/src/commands/fetch.rs index 44803f05d..541d77977 100644 --- a/crates/doiget-cli/src/commands/fetch.rs +++ b/crates/doiget-cli/src/commands/fetch.rs @@ -233,6 +233,14 @@ pub(crate) fn build_http_client(user_agent: Option<&str>) -> Result // mirroring `DOIGET_ARXIV_BASE`. let ar5iv_base = std::env::var("DOIGET_AR5IV_BASE").ok(); + #[cfg(feature = "tdm-aps")] + let tdm_aps = std::env::var("DOIGET_APS_BASE").ok(); + #[cfg(feature = "tdm-elsevier")] + let tdm_elsevier = std::env::var("DOIGET_ELSEVIER_BASE").ok(); + #[cfg(feature = "tdm-springer")] + let tdm_springer = std::env::var("DOIGET_SPRINGER_BASE").ok(); + #[cfg(feature = "tdm-ieee")] + let tdm_ieee = std::env::var("DOIGET_IEEE_BASE").ok(); if arxiv.is_none() && crossref.is_none() && unpaywall.is_none() @@ -345,8 +353,25 @@ pub(crate) fn build_http_client(user_agent: Option<&str>) -> Result // Test-base mode: build a relaxed client per overridden source. let mut owned: Vec<(String, String)> = Vec::new(); + // Tier-3 test bases, mirroring the MCP builder. Without these a wiremock + // e2e cannot reach the TDM-fetched route on this surface either: the + // override branch's table held only Tier-1/2 keys, so `tdm-aps` was absent + // from the client's map and the attempt died as `no allowlist registered + // for source tdm-aps` -- a harness gap that read like #454 coming back. + // + // Deliberately NOT part of the production-branch test above: setting only + // `DOIGET_APS_BASE` to replay a recorded fixture must not silently switch + // the process to the allow-http test client. for (source, base) in [ ("arxiv", arxiv.as_deref()), + #[cfg(feature = "tdm-aps")] + ("tdm-aps", tdm_aps.as_deref()), + #[cfg(feature = "tdm-elsevier")] + ("tdm-elsevier", tdm_elsevier.as_deref()), + #[cfg(feature = "tdm-springer")] + ("tdm-springer", tdm_springer.as_deref()), + #[cfg(feature = "tdm-ieee")] + ("tdm-ieee", tdm_ieee.as_deref()), ("crossref", crossref.as_deref()), ("unpaywall", unpaywall.as_deref()), ("oa-publisher", oa_publisher.as_deref()), @@ -579,7 +604,9 @@ impl FetchHarness { /// Pulled out so both `run_with_options` and `commands::batch` agree on /// the failure boundary. pub(crate) fn outcome_is_clean_success(outcome: &FetchPaperOutcome) -> bool { - !matches!(outcome.pdf_leg, PdfLegStatus::Blocked { .. }) + // The rule lives in `doiget-core` now, because the MCP surface needs the + // same boundary and had only half of it. + outcome.is_clean_success() } /// CLI-only one-line success message on stderr (ADR-0001 stdio diff --git a/crates/doiget-cli/src/commands/graph.rs b/crates/doiget-cli/src/commands/graph.rs index 272e0ccad..b876c8da2 100644 --- a/crates/doiget-cli/src/commands/graph.rs +++ b/crates/doiget-cli/src/commands/graph.rs @@ -40,24 +40,18 @@ use super::output::print_err; /// /// The `map_err` below already decides this per variant, but it does so while /// building a user-facing message and an exit code, so the value is not -/// available to the provenance bookend that runs first. Kept next to it, and -/// exhaustive, so the two cannot say different things about the same error. +/// available to the provenance bookend that runs first. +/// +/// This used to hold its own exhaustive match, above a claim that "the two +/// cannot say different things about the same error". Two is the wrong count: +/// the MCP surface held a third copy, which flattened `Source(_)` to +/// `NETWORK_ERROR`, so an OpenAlex 404 was terminal here and retriable there. +/// The mapping lives beside the enum in `doiget-core` now. fn graph_error_code(e: &GraphError) -> ErrorCode { - match e { - GraphError::CapabilityDenied => ErrorCode::CapabilityDenied, - // An indexing gap upstream: the seed is a valid DOI that OpenAlex does - // not hold. `NO_OA_AVAILABLE`, matching what the MCP tool has always - // reported for the same condition -- I wrote `NOT_FOUND` here without - // checking the sibling surface, which would have had the two disagree - // about one error. - GraphError::SeedNotIndexed => ErrorCode::NoOaAvailable, - // These two already own a mapping; reuse it rather than restate it. - GraphError::Source(fe) => ErrorCode::from(fe), - GraphError::Log(_) => ErrorCode::LogError, - // `GraphError` is `#[non_exhaustive]`, so a wildcard is required from - // this crate. Kept last and explicit rather than implied. - _ => ErrorCode::InternalError, - } + // The mapping lives in `doiget_core::citation_graph`, next to the enum, so + // this surface and the MCP one cannot drift apart again -- they did, on + // `Source(_)`, which the MCP copy flattened to `NETWORK_ERROR`. + ErrorCode::from(e) } /// Run the `graph` subcommand against the live source set. diff --git a/crates/doiget-cli/src/commands/search.rs b/crates/doiget-cli/src/commands/search.rs index a8abfb31b..4c1fb4c8f 100644 --- a/crates/doiget-cli/src/commands/search.rs +++ b/crates/doiget-cli/src/commands/search.rs @@ -260,6 +260,14 @@ async fn run_external( // year / OA / DOI / title. Tab-separated, `cut(1)`-compatible. writeln!(out, "cited_by\tyear\toa\tdoi\ttitle") .context("failed to write search header to stdout")?; + // #534, human half: a header with no rows under it says "not indexed" just + // as flatly as the JSON envelope did. The note goes to stderr so the table + // on stdout stays `cut(1)`-clean (ADR-0001). + if results.results.is_empty() { + if let Some(hint) = doiget_core::discovery::zero_result_hint(query) { + print_err(format_args!(" = note: {hint}")); + } + } for hit in &results.results { let year = dash_or(hit.year); let oa = hit.oa_status.as_deref().unwrap_or("-"); @@ -299,14 +307,25 @@ fn local_envelope(query: &str, entries: &[EntryInfo]) -> serde_json::Value { /// `{ ok, scope, query, total_results, count, results }`. Extracted as a pure /// function so the wire shape is unit-testable without capturing stdout. fn external_envelope(query: &str, results: &PaperSearchResults) -> serde_json::Value { - serde_json::json!({ + let mut envelope = serde_json::json!({ "ok": true, "scope": "external", "query": query, "total_results": results.total_results, "count": results.results.len(), "results": results.results, - }) + }); + // #534. `{"ok": true, "total_results": 0}` is a success envelope, so + // nothing in the error machinery reaches it, and a script or agent reading + // it takes zero results as a fact about the literature and stops. The fix + // first landed on the MCP tool only -- this surface went on emitting the + // exact envelope the issue was filed about, byte for byte. + if results.results.is_empty() { + if let Some(hint) = doiget_core::discovery::zero_result_hint(query) { + envelope["hint"] = serde_json::json!(hint); + } + } + envelope } /// Pretty-serialize a JSON value and write it as one line to `out`. Shared diff --git a/crates/doiget-cli/src/commands/verify.rs b/crates/doiget-cli/src/commands/verify.rs index 66f5172df..2b1fc727f 100644 --- a/crates/doiget-cli/src/commands/verify.rs +++ b/crates/doiget-cli/src/commands/verify.rs @@ -40,6 +40,7 @@ use doiget_core::orchestrator::resolve_only; use doiget_core::refs::{parse_input, Format, ParseError}; use doiget_core::verify_config::{self, OnMissingId}; use doiget_core::CapabilityProfile; +use doiget_core::ErrorCode; use super::fetch::CliExit; use super::output::OutputMode; @@ -264,7 +265,7 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "ref": raw, "status": VerifyStatus::Illegal.as_wire(), "entry_key": entry_key, - "error": { "code": "INVALID_REF", "message": source.to_string() }, + "error": { "code": ErrorCode::InvalidRef.as_wire(), "message": source.to_string() }, }), ), // #500: still unverifiable, but for a reason the reader can act on @@ -281,7 +282,7 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "status": VerifyStatus::Unverifiable.as_wire(), "entry_key": entry_key, "error": { - "code": "NOT_IMPLEMENTED", + "code": ErrorCode::NotImplemented.as_wire(), "message": doiget_core::refs::unsupported_identifier_claim( kind, &value, ), @@ -295,7 +296,7 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "ref": serde_json::Value::Null, "status": VerifyStatus::Unverifiable.as_wire(), "entry_key": entry_key, - "error": { "code": "INVALID_REF", "message": "entry has no DOI / arXiv id" }, + "error": { "code": ErrorCode::InvalidRef.as_wire(), "message": "entry has no DOI / arXiv id" }, }), ), Err(ParseError::Decode { format, message }) => ( @@ -304,7 +305,7 @@ pub async fn run(path: String, format: String, cli_strict: bool, mode: OutputMod "ok": false, "status": VerifyStatus::Illegal.as_wire(), "error": { - "code": "INVALID_REF", + "code": ErrorCode::InvalidRef.as_wire(), "message": format!("input did not parse as {format}: {message}"), }, }), diff --git a/crates/doiget-cli/tests/batch_jsonl_e2e.rs b/crates/doiget-cli/tests/batch_jsonl_e2e.rs index 87fede5b6..ed140117e 100644 --- a/crates/doiget-cli/tests/batch_jsonl_e2e.rs +++ b/crates/doiget-cli/tests/batch_jsonl_e2e.rs @@ -366,3 +366,61 @@ fn batch_failure_digest_includes_parse_errors() { "digest must list the parse failure: {stderr}" ); } + +/// #500 on the CLI `batch` surface. +/// +/// A PubMed-exported `.bib` record carries `pmid = {9659853}` and no DOI. The +/// entry is fine; doiget cannot resolve that identifier class yet. Reporting +/// `INVALID_REF` sends the user to edit a bibliography that is correct, which +/// is the one claim #500 exists to stop doiget making. +/// +/// It reported `INVALID_REF` anyway, on this surface only: the parser's +/// verdict was flattened into a placeholder string and handed back to +/// `Ref::parse`, whose only possible answer is `INVALID_REF`. The MCP tool and +/// `doiget verify` had said `NOT_IMPLEMENTED` all along. +#[test] +fn batch_json_pmid_only_entry_is_not_implemented_not_invalid_ref() { + let dir = TempDir::new().expect("tempdir"); + let bib = dir.path().join("refs.bib"); + { + let mut f = std::fs::File::create(&bib).expect("create bib file"); + f.write_all( + b"@article{Smith2020, + title = {A PubMed-only record}, + pmid = {9659853}, +} +", + ) + .expect("write bib"); + } + + let out = doiget(&dir) + .args(["batch", bib.to_str().expect("utf-8"), "--mode", "json"]) + .output() + .expect("run batch"); + + let stdout = String::from_utf8(out.stdout).expect("utf-8 stdout"); + let line = stdout + .lines() + .find(|l| l.contains("\"ok\"")) + .unwrap_or_else(|| panic!("no JSONL record in: {stdout}")); + let v: Value = serde_json::from_str(line).expect("JSONL record parses"); + + assert_eq!(v["ok"], serde_json::json!(false), "record: {v}"); + assert_eq!( + v["error"]["code"], + serde_json::json!("NOT_IMPLEMENTED"), + "a PMID entry is unsupported, not malformed: {v}" + ); + // The message must describe the user's entry, not the synthetic + // placeholder the old code round-tripped through `Ref::parse`. + let message = v["error"]["message"].as_str().unwrap_or_default(); + assert!( + message.contains("PMID") && message.contains("9659853"), + "message names the identifier the entry actually carries: {message:?}" + ); + assert!( + !message.contains("` and the MCP surface flattened to `NETWORK_ERROR` -- so +/// an OpenAlex 404 during expansion was `NOT_FOUND` (terminal) on one surface +/// and `NETWORK_ERROR` (retry_after) on the other, telling an agent to retry a +/// seed that will never resolve. Defined here, in the crate that owns the +/// error, so the wildcard `#[non_exhaustive]` forces on downstream crates +/// cannot silently absorb a new variant again. +impl From<&GraphError> for crate::ErrorCode { + fn from(e: &GraphError) -> crate::ErrorCode { + match e { + GraphError::CapabilityDenied => crate::ErrorCode::CapabilityDenied, + // An indexing gap upstream: the seed is a valid DOI that OpenAlex + // does not hold. + GraphError::SeedNotIndexed => crate::ErrorCode::NoOaAvailable, + // Already owns an exhaustive mapping; reuse it rather than + // restate it. + GraphError::Source(fe) => crate::ErrorCode::from(fe), + GraphError::Log(_) => crate::ErrorCode::LogError, + } + } +} + /// Expand the citation graph for `seed_doi` via OpenAlex. pub async fn expand( seed_doi: &Doi, diff --git a/crates/doiget-core/src/discovery.rs b/crates/doiget-core/src/discovery.rs index 19981878e..7817d23a3 100644 --- a/crates/doiget-core/src/discovery.rs +++ b/crates/doiget-core/src/discovery.rs @@ -1231,9 +1231,73 @@ pub async fn frontier_view( // Tests // --------------------------------------------------------------------------- +/// Advice to attach to a paper search that matched nothing (#534). +/// +/// OpenAlex free-text matching degrades sharply as a query lengthens: past +/// roughly eight terms it returns nothing at all rather than a partial match. +/// A human reading `0 results` shortens the query and tries again. An agent +/// reading `ok: true` with an empty array reads it as a fact about the world +/// and stops -- in the session that produced #534, eleven consecutive searches +/// returned zero for papers a three-to-five term query then found immediately, +/// and a known study was written off as unavailable. +/// +/// Returns `None` for short queries: a zero-result two-term search really may +/// mean the work is not indexed, and a hint on every empty result would train +/// readers to skip it. +/// +/// Lives here rather than on either front end because both surfaces call +/// [`paper_search`] and both can return the empty envelope. The first fix +/// landed only on the MCP tool, so `doiget search --mode json` went on +/// emitting the exact envelope #534 was filed about. +#[must_use] +pub fn zero_result_hint(query: &str) -> Option { + // Where OpenAlex free-text matching starts failing outright. Not a hard + // boundary -- a bound observed from the queries in #534, which is why the + // wording says "roughly". + const DEGRADES_PAST: usize = 8; + + let terms = query.split_whitespace().count(); + if terms <= DEGRADES_PAST { + return None; + } + Some(format!( + "This query has {terms} terms. OpenAlex free-text matching degrades sharply past roughly {DEGRADES_PAST} and returns nothing rather than a partial match, so zero results here is more likely to be about the query than about the literature. Retry with 3-5 distinctive terms - an author surname, a coined phrase, the distinguishing noun - before concluding the work is not indexed." + )) +} + #[cfg(test)] #[allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] mod tests { + /// #534: a long query matching nothing is far more likely to be a + /// query-length problem than an absent literature, and the agent reading + /// the envelope is the one who cannot tell the difference. + #[test] + fn a_long_zero_result_query_is_told_why_it_may_be_zero() { + let q = "lithium refractoriness after discontinuation kindling sensitization course of illness Post"; + let hint = zero_result_hint(q).expect("10 terms is past the threshold"); + assert!(hint.contains("10 terms"), "names the count: {hint}"); + assert!( + hint.contains("3-5"), + "says what to do instead, not only what went wrong: {hint}" + ); + } + + /// A short query returning nothing may genuinely mean nothing is indexed. + /// Hinting on every empty result would teach readers to skip the hint. + #[test] + fn a_short_zero_result_query_is_left_alone() { + assert!(zero_result_hint("depersonalization derealization").is_none()); + assert!(zero_result_hint("").is_none()); + } + + /// The threshold counts terms, not characters: one very long term is still + /// one term, and "shorten it" is not the advice to give. + #[test] + fn the_threshold_counts_terms_not_length() { + assert!(zero_result_hint(&"a".repeat(400)).is_none()); + assert!(zero_result_hint("a b c d e f g h i").is_some()); + } + use super::*; use std::sync::Arc; diff --git a/crates/doiget-core/src/http.rs b/crates/doiget-core/src/http.rs index 6b1c28c3e..9586bb8d8 100644 --- a/crates/doiget-core/src/http.rs +++ b/crates/doiget-core/src/http.rs @@ -185,10 +185,15 @@ impl SourceAllowlist { /// means the resolver set is not silently reported as part of any /// source's `expected_hosts`. /// - /// The four adjudication sites (the pre-fetch OA-URL check in - /// `orchestrator`, the two redirect-policy closures, and `probe`) all - /// route through here so they cannot disagree -- #533 was found because - /// only one of them was ever walked end to end. + /// The five adjudication sites -- the pre-fetch OA-URL check in + /// `orchestrator`, the two redirect-policy closures, `probe`, and the + /// pre-check `doiget config doctor --network` runs before calling + /// `probe` -- all route through here so they cannot disagree. #533 was + /// found because only one of them was ever walked end to end, and the + /// doctor's was missed on the first pass at this very doc comment: it + /// said "four" while still calling `matches`, which would have had the + /// command that explains allowlist refusals reproduce #533 the moment a + /// resolver host entered its probe list. #[must_use] pub fn permits(&self, host: &str) -> bool { is_transparent_resolver(host) || self.matches(host) @@ -216,6 +221,15 @@ impl SourceAllowlist { /// that actually serves the bytes is adjudicated exactly as before, so this /// is transparent to the invariant rather than an exception to it. /// +/// One edge the sentence above does not cover: a chain that TERMINATES at a +/// resolver -- a `200` straight from `doi.org` rather than the `302` it +/// exists to send -- is served by a host `permits` allowed and `matches` +/// would not. The bound still holds in practice because these three hosts are +/// operated by the DOI Foundation and CNRI and do not serve article bytes, +/// which is why the set is closed and exact; but the invariant is "the +/// resolver is trusted to redirect", not "only allowlisted hosts ever send +/// bytes". +/// /// # Why a closed set and not "the terminal host" /// /// #533's first suggestion was to adjudicate only the final host of a diff --git a/crates/doiget-core/src/orchestrator.rs b/crates/doiget-core/src/orchestrator.rs index 134b7c318..dc644c05c 100644 --- a/crates/doiget-core/src/orchestrator.rs +++ b/crates/doiget-core/src/orchestrator.rs @@ -1165,6 +1165,21 @@ pub struct FetchPaperOutcome { } impl FetchPaperOutcome { + /// `true` when this outcome is a success with nothing withheld. + /// + /// A `Blocked` PDF leg is an `Ok` outcome whose payload was refused, so + /// `Result::is_ok` alone calls it a clean success. Both front ends need + /// the same boundary and had drifted: the CLI special-cased `Blocked` + /// for its exit code and its `SessionEnd` row, the MCP server only for + /// the row's `error_code`, so `doiget_fetch_paper` logged + /// `result: "ok"` beside a non-null code -- a self-contradictory row for + /// the one outcome an agent is most likely to retry, and the outcome + /// #507's repeat suppression has to be able to see. + #[must_use] + pub fn is_clean_success(&self) -> bool { + !matches!(self.pdf_leg, PdfLegStatus::Blocked { .. }) + } + /// Test-only constructor for downstream crates (`doiget-cli`, /// `doiget-mcp`) that need to drive classification / rendering /// logic without running the full orchestrator. Produces a @@ -2884,6 +2899,22 @@ pub struct BatchOutcome { pub results: Vec, } +impl BatchOutcome { + /// Test-only constructor for downstream crates, mirroring + /// [`FetchPaperOutcome::for_test_synthetic`]. + /// + /// `BatchOutcome` is `#[non_exhaustive]`, so `doiget-mcp` cannot build one + /// to drive its envelope builders -- which is part of why the two batch + /// tools' error mapping went untested long enough to drift (#538). + /// + /// `#[doc(hidden)]`: not a stable public API. + #[doc(hidden)] + #[must_use] + pub fn for_test_synthetic(results: Vec) -> Self { + Self { results } + } +} + /// Iterate over `refs` through [`fetch_paper`], collecting one /// [`BatchResultEntry`] per ref. /// diff --git a/crates/doiget-core/src/refs.rs b/crates/doiget-core/src/refs.rs index 8297ba66d..3133bad2a 100644 --- a/crates/doiget-core/src/refs.rs +++ b/crates/doiget-core/src/refs.rs @@ -382,9 +382,66 @@ fn parse_csl_entry( } } } + // #500, CSL-JSON half. The BibTeX parser learned to say "this entry HAS an + // identifier I cannot use" and this one did not, so the same PubMed record + // exported as CSL-JSON still got "entry has no DOI / arXiv id" -- the claim + // #500 exists to stop doiget making, surviving in the format a Zotero user + // is most likely to hand it. + if let Some((kind, value)) = csl_unsupported_identifier(entry) { + return Err(ParseError::UnsupportedIdentifier { + kind, + value, + entry_key, + }); + } Err(ParseError::NoIdentifier { entry_key }) } +/// The CSL-JSON counterpart of [`unsupported_identifier`]: an identifier +/// doiget recognises and cannot resolve yet (#500). +/// +/// CSL-JSON has no standard PMID field, so exporters improvise. Zotero writes +/// `PMID: 9659853` into `note`; some tools emit a top-level `PMID` key. Both +/// are checked, and the note scan mirrors the arXiv one directly above it. +fn csl_unsupported_identifier(entry: &serde_json::Value) -> Option<(&'static str, String)> { + let direct = |name: &str| -> Option { + let v = entry.get(name)?; + let s = match v { + serde_json::Value::String(s) => s.trim().to_string(), + serde_json::Value::Number(n) => n.to_string(), + _ => return None, + }; + (!s.is_empty()).then_some(s) + }; + for (field, kind) in [ + ("PMID", "PMID"), + ("pmid", "PMID"), + ("PMCID", "PMCID"), + ("pmcid", "PMCID"), + ] { + if let Some(v) = direct(field) { + return Some((kind, v)); + } + } + + // Zotero's `note` carries `PMID: 9659853` / `PMCID: PMC1234567`. + let note = entry.get("note").and_then(|v| v.as_str())?; + for (needle, kind) in [("pmcid:", "PMCID"), ("pmid:", "PMID")] { + let lower = note.to_ascii_lowercase(); + if let Some(at) = lower.find(needle) { + let value: String = note[at + needle.len()..] + .trim_start() + .chars() + .take_while(|c| c.is_ascii_alphanumeric()) + .collect(); + if !value.is_empty() { + return Some((kind, value)); + } + } + } + None +} + /// Parse a BibTeX / BibLaTeX document via the `biblatex` crate /// (ADR-0030 D2). One `@entrytype{KEY, …}` produces one entry; /// `entry_key` is the citation key verbatim. @@ -525,6 +582,61 @@ fn arxiv_eligible(entry: &biblatex::Entry) -> bool { #[cfg(test)] #[allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] mod tests { + /// #500, CSL-JSON. The BibTeX parser learned to distinguish "has no + /// identifier" from "has one I cannot use"; this format did not, so the + /// same PubMed record exported from Zotero still got the claim #500 exists + /// to prevent -- in the format a Zotero user is most likely to hand over. + #[test] + fn a_csl_entry_identified_only_by_a_pmid_is_not_missing_an_identifier() { + let doc = r#"[{"id":"Smith2020","type":"article-journal","PMID":"9659853"}]"#; + let out = parse_csl_json(doc); + assert_eq!(out.len(), 1); + match &out[0] { + Err(ParseError::UnsupportedIdentifier { kind, value, .. }) => { + assert_eq!(*kind, "PMID"); + assert_eq!(value, "9659853"); + } + other => panic!("expected UnsupportedIdentifier, got {other:?}"), + } + } + + /// Zotero does not emit a top-level `PMID`; it writes it into `note`. + /// A checker that only looked at the field would have missed the exporter + /// that produces most of these files. + #[test] + fn a_csl_note_carrying_a_pmid_is_read_the_same_way() { + let doc = r#"[{"id":"S","type":"article-journal","note":"PMID: 9659853; see PubMed"}]"#; + match &parse_csl_json(doc)[0] { + Err(ParseError::UnsupportedIdentifier { kind, value, .. }) => { + assert_eq!(*kind, "PMID"); + assert_eq!(value, "9659853"); + } + other => panic!("expected UnsupportedIdentifier, got {other:?}"), + } + } + + /// A numeric PMID must not be coerced into anything, and a DOI alongside + /// one still wins: the check runs only after every supported identifier + /// has been tried. + #[test] + fn a_csl_doi_still_wins_over_a_pmid() { + let doc = r#"[{"id":"S","type":"article-journal","DOI":"10.1234/x","PMID":9659853}]"#; + let parsed = parse_csl_json(doc); + let entry = parsed[0].as_ref().expect("the DOI must still win"); + assert_eq!(entry.ref_.as_input_str(), "10.1234/x"); + } + + /// An entry with genuinely nothing must keep saying so -- the new check + /// must not turn every unidentifiable entry into "unsupported". + #[test] + fn a_csl_entry_with_no_identifier_at_all_still_says_so() { + let doc = r#"[{"id":"S","type":"article-journal","title":"No ids here"}]"#; + assert!(matches!( + &parse_csl_json(doc)[0], + Err(ParseError::NoIdentifier { .. }) + )); + } + use super::*; // ---- detect_format --------------------------------------------- diff --git a/crates/doiget-core/src/source.rs b/crates/doiget-core/src/source.rs index 4c7689927..dc10a338b 100644 --- a/crates/doiget-core/src/source.rs +++ b/crates/doiget-core/src/source.rs @@ -255,7 +255,6 @@ impl From<&FetchError> for crate::ErrorCode { // is treated as transient. FetchError::Http(HttpError::HttpStatus { status: 404 | 410 | 451, - retry_after_ms: None, .. }) => crate::ErrorCode::NotFound, FetchError::Http(HttpError::HttpStatus { @@ -571,6 +570,31 @@ mod tests { "status {status} should map to NotFound" ); } + // ...and a `Retry-After` on that response does not change it. #506 + // added `retry_after_ms` to this variant, and the arm above briefly + // matched `retry_after_ms: None`, which silently sent a 404 carrying + // the header to `NETWORK_ERROR` -- disposition `retry_after` -- so an + // agent was told to retry a DOI that will never resolve. The header + // says how long to wait IF you retry; it does not make an + // authoritative absence provisional. + for status in [404u16, 410, 451] { + let e: ErrorCode = FetchError::Http(HttpError::HttpStatus { + status, + retry_after_ms: Some(30_000), + url: "https://api.crossref.org/works/10.5555/absent".into(), + }) + .into(); + assert_eq!( + e, + ErrorCode::NotFound, + "status {status} with Retry-After is still NotFound" + ); + assert_eq!( + e.disposition(), + crate::Disposition::Terminal, + "and stays terminal, so nothing tells the agent to retry it" + ); + } // A non-HTTP authoritative absence (e.g. arXiv's empty Atom feed) // also maps to NotFound. let e: ErrorCode = FetchError::NotFound { diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 86662990f..051822cb0 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -658,14 +658,16 @@ impl Server { let outcome = core_fetch_paper(&ref_, &self.profile, &ctx, &store, &store_root).await; - let session_ok = outcome.is_ok(); // #507, second surface. `core_fetch_paper` returns `Ok` with a FAILED - // leg when an OA URL was found and refused, so `outcome.err()` is - // `None` and the row would say the call succeeded with no error code - // -- for the one outcome an agent is most likely to retry. The CLI - // sibling special-cases exactly this (`commands/fetch.rs`); the MCP - // path did not, so the same fix landed on one surface and not the - // other. Found by review of this change. + // leg when an OA URL was found and refused, so `Result::is_ok` alone + // calls that a clean success -- for the one outcome an agent is most + // likely to retry. The CLI sibling special-cases exactly this; the + // first pass at this fix ported only the `error_code` half below, so + // the row said `result: "ok"` AND carried a code. Both halves now go + // through `FetchPaperOutcome::is_clean_success`. + let session_ok = outcome + .as_ref() + .is_ok_and(FetchPaperOutcome::is_clean_success); let blocked_code = match outcome.as_ref() { Ok(o) => match &o.pdf_leg { doiget_core::orchestrator::PdfLegStatus::Blocked { code, .. } => { @@ -1478,7 +1480,7 @@ impl Server { // so at the exact point an agent would otherwise conclude // absence. if results.results.is_empty() { - if let Some(hint) = zero_result_hint(&input.query) { + if let Some(hint) = doiget_core::discovery::zero_result_hint(&input.query) { envelope["hint"] = json!(hint); } } @@ -1525,7 +1527,16 @@ impl Server { Ok(doiget_core::Ref::Doi(_)) => { return Ok(CallToolResult::structured(read_path_error_envelope( Some(&input.ref_), - ErrorCode::NoOaAvailable, + // Not `NO_OA_AVAILABLE`. That code's disposition is + // `needs_config` -- "a named change makes it" -- and there + // is no config knob here: this tool is arXiv-only and + // DOI to arXiv linking is not built. `NOT_IMPLEMENTED` is + // terminal and says the true thing (ERRORS.md: "wait for + // next minor release; do not retry"), and it is the code + // `verify` / `batch_from_bibliography` already give the + // same situation for PMIDs (#500): valid input, absent + // support. + ErrorCode::NotImplemented, "no full-text source for a DOI — pass the arXiv id if a preprint exists \ (DOI→arXiv linking is #281 item 5)", ))); @@ -1644,7 +1655,9 @@ impl Server { Ok(doiget_core::Ref::Doi(_)) => { return Ok(CallToolResult::structured(read_path_error_envelope( Some(&input.ref_), - ErrorCode::NoOaAvailable, + // Same reasoning as `doiget_paper_text` above: absent + // support, not absent open access. + ErrorCode::NotImplemented, "no TeX source for a DOI — pass the arXiv id if a preprint exists", ))); } @@ -2182,21 +2195,11 @@ impl Server { "truncated": graph.truncated, "total_visited": graph.total_visited, }))), + // Was a third copy of the same match, inline, which did not + // even call the helper directly above it. Err(e) => Ok(CallToolResult::structured(read_path_error_envelope( Some(&input.ref_), - match &e { - doiget_core::citation_graph::GraphError::CapabilityDenied => { - ErrorCode::CapabilityDenied - } - doiget_core::citation_graph::GraphError::SeedNotIndexed => { - ErrorCode::NoOaAvailable - } - doiget_core::citation_graph::GraphError::Log(_) => ErrorCode::LogError, - doiget_core::citation_graph::GraphError::Source(_) => { - ErrorCode::NetworkError - } - _ => ErrorCode::InternalError, - }, + graph_error_code(&e), &format!("citation graph expansion failed: {e}"), ))), } @@ -2281,7 +2284,10 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": format!("context initialization failed: {e}"), + "error": error_object( + ErrorCode::InternalError, + format!("context initialization failed: {e}"), + ), }))); } }; @@ -2291,7 +2297,7 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": e, + "error": error_object(ErrorCode::CapabilityDenied, e), }))); } }; @@ -2307,7 +2313,7 @@ impl Server { }))), Err(e) => Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": format!("resolve failed: {e}"), + "error": error_object(ErrorCode::from(&e), format!("resolve failed: {e}")), }))), } } @@ -2334,7 +2340,10 @@ impl Server { if input.queries.len() > 50 { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": "At most 50 queries per call.", + "error": error_object( + ErrorCode::InvalidRef, + "At most 50 queries per call.", + ), }))); } @@ -2343,7 +2352,10 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": format!("context initialization failed: {e}"), + "error": error_object( + ErrorCode::InternalError, + format!("context initialization failed: {e}"), + ), }))); } }; @@ -2353,7 +2365,7 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": e, + "error": error_object(ErrorCode::CapabilityDenied, e), }))); } }; @@ -2370,7 +2382,10 @@ impl Server { Err(e) => { results.push(serde_json::json!({ "query": query, - "error": format!("resolve failed: {e}"), + "error": error_object( + ErrorCode::from(&e), + format!("resolve failed: {e}"), + ), })); } } @@ -2444,7 +2459,10 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("no store entry for {}; fetch the paper first", input.ref_), + "error": error_object( + ErrorCode::NotFound, + format!("no store entry for {}; fetch the paper first", input.ref_), + ), }))); } Err(e) => { @@ -2461,7 +2479,10 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("entry {} has no [doiget] table; fetch it first", input.ref_), + "error": error_object( + ErrorCode::NotFound, + format!("entry {} has no [doiget] table; fetch it first", input.ref_), + ), }))); } }; @@ -2562,7 +2583,10 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("no store entry for {}; fetch the paper first", input.ref_), + "error": error_object( + ErrorCode::NotFound, + format!("no store entry for {}; fetch the paper first", input.ref_), + ), }))); } Err(e) => { @@ -2579,7 +2603,10 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("entry {} has no [doiget] table; fetch it first", input.ref_), + "error": error_object( + ErrorCode::NotFound, + format!("entry {} has no [doiget] table; fetch it first", input.ref_), + ), }))); } }; @@ -2590,14 +2617,20 @@ impl Server { if text.is_empty() { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": "annotation text must not be empty; set clear:true to remove it", + "error": error_object( + ErrorCode::InvalidRef, + "annotation text must not be empty; set clear:true to remove it", + ), }))); } ext.annotation = Some(text.clone()); } else { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": "provide 'text' to set an annotation, or 'clear: true' to remove it", + "error": error_object( + ErrorCode::InvalidRef, + "provide 'text' to set an annotation, or 'clear: true' to remove it", + ), }))); } @@ -3231,14 +3264,11 @@ fn entry_info_to_json(entry: &EntryInfo) -> Value { /// cannot say different things about the same error. #[cfg(feature = "citation")] fn graph_error_code(e: &doiget_core::citation_graph::GraphError) -> ErrorCode { - use doiget_core::citation_graph::GraphError; - match e { - GraphError::CapabilityDenied => ErrorCode::CapabilityDenied, - GraphError::SeedNotIndexed => ErrorCode::NoOaAvailable, - GraphError::Log(_) => ErrorCode::LogError, - GraphError::Source(_) => ErrorCode::NetworkError, - _ => ErrorCode::InternalError, - } + // Delegate. This copy flattened `Source(_)` to `NETWORK_ERROR`, so an + // OpenAlex 404 during expansion arrived here as `retry_after` while the + // CLI called the same condition `NOT_FOUND` -- one error, two answers, + // and the retriable one was wrong. + ErrorCode::from(e) } /// The `error` object every failure envelope carries (#506). @@ -3695,16 +3725,16 @@ fn build_bibliography_envelope( "pdf": pdf_leg_json(&outcome.pdf_leg), }), Err(err) => { - let code: ErrorCode = match err { - FetchError::NotEligible { .. } => ErrorCode::CapabilityDenied, - FetchError::NoOaAvailable => ErrorCode::NoOaAvailable, - FetchError::Http(_) => ErrorCode::NetworkError, - FetchError::Log(_) => ErrorCode::LogError, - FetchError::InvalidRef(_) => ErrorCode::InvalidRef, - FetchError::SourceSchema { .. } => ErrorCode::InternalError, - FetchError::TooManyRefs { .. } => ErrorCode::InvalidRef, - _ => ErrorCode::InternalError, - }; + // Delegate, do not re-implement (#538). This was the same + // hand-rolled copy `fetch_paper_fetch_error_envelope` shed one + // screen up, left behind on the two batch tools: ending in + // `_ => InternalError` it reported `NotFound`, `Ambiguous` and + // -- once #538 landed -- `NotRetrievable` as internal errors, + // and collapsed every `Http` status to `NETWORK_ERROR`, so a + // 404 told an agent to retry a DOI that will never resolve. + // `disposition` is derived from this code below, so a wrong + // code was also a wrong disposition. + let code: ErrorCode = ErrorCode::from(err); let denial: Option = err.into(); let mut error_obj = serde_json::Map::new(); error_obj.insert("code".into(), json!(code)); @@ -3803,16 +3833,16 @@ fn batch_fetch_success_envelope( "pdf": pdf_leg_json(&outcome.pdf_leg), }), Err(err) => { - let code: ErrorCode = match err { - FetchError::NotEligible { .. } => ErrorCode::CapabilityDenied, - FetchError::NoOaAvailable => ErrorCode::NoOaAvailable, - FetchError::Http(_) => ErrorCode::NetworkError, - FetchError::Log(_) => ErrorCode::LogError, - FetchError::InvalidRef(_) => ErrorCode::InvalidRef, - FetchError::SourceSchema { .. } => ErrorCode::InternalError, - FetchError::TooManyRefs { .. } => ErrorCode::InvalidRef, - _ => ErrorCode::InternalError, - }; + // Delegate, do not re-implement (#538). This was the same + // hand-rolled copy `fetch_paper_fetch_error_envelope` shed one + // screen up, left behind on the two batch tools: ending in + // `_ => InternalError` it reported `NotFound`, `Ambiguous` and + // -- once #538 landed -- `NotRetrievable` as internal errors, + // and collapsed every `Http` status to `NETWORK_ERROR`, so a + // 404 told an agent to retry a DOI that will never resolve. + // `disposition` is derived from this code below, so a wrong + // code was also a wrong disposition. + let code: ErrorCode = ErrorCode::from(err); let denial: Option = err.into(); let mut error_obj = serde_json::Map::new(); error_obj.insert("code".into(), json!(code)); @@ -4031,6 +4061,14 @@ fn build_http_client_for_fetch() -> anyhow::Result { // `DOIGET_ARXIV_BASE` is absent. let arxiv_src = std::env::var("DOIGET_ARXIV_SRC_BASE").ok(); + #[cfg(feature = "tdm-aps")] + let tdm_aps = std::env::var("DOIGET_APS_BASE").ok(); + #[cfg(feature = "tdm-elsevier")] + let tdm_elsevier = std::env::var("DOIGET_ELSEVIER_BASE").ok(); + #[cfg(feature = "tdm-springer")] + let tdm_springer = std::env::var("DOIGET_SPRINGER_BASE").ok(); + #[cfg(feature = "tdm-ieee")] + let tdm_ieee = std::env::var("DOIGET_IEEE_BASE").ok(); if arxiv.is_none() && arxiv_src.is_none() && crossref.is_none() @@ -4122,8 +4160,30 @@ fn build_http_client_for_fetch() -> anyhow::Result { // When DOIGET_ARXIV_BASE is set use it; otherwise fall back to // DOIGET_ARXIV_SRC_BASE (they share the "arxiv" HTTP source key). let arxiv_entry = arxiv.as_deref().or(arxiv_src.as_deref()); + // Tier-3 test bases. A wiremock e2e could not reach the TDM-fetched + // route at all before this: the override branch built its allowlist + // from a fixed table of Tier-1/2 keys, so `tdm-aps` was simply not in + // the client's map and every attempt died as + // `no allowlist registered for source tdm-aps`. That was read as + // "#454's shape, reachable again" and filed as a possible production + // regression -- the production branch above extends with + // `tier_3_allowlists()` and was correct all along. The defect was + // here, in the harness, which is why only a route assertion found it. + // + // Not added to the production-branch test above on purpose: setting + // only `DOIGET_APS_BASE` (which `tdm_ieee.rs` documents as a way to + // replay a recorded fixture) must NOT drop the process into the + // allow-http test client. for (source, base) in [ ("arxiv", arxiv_entry), + #[cfg(feature = "tdm-aps")] + ("tdm-aps", tdm_aps.as_deref()), + #[cfg(feature = "tdm-elsevier")] + ("tdm-elsevier", tdm_elsevier.as_deref()), + #[cfg(feature = "tdm-springer")] + ("tdm-springer", tdm_springer.as_deref()), + #[cfg(feature = "tdm-ieee")] + ("tdm-ieee", tdm_ieee.as_deref()), ("crossref", crossref.as_deref()), ("unpaywall", unpaywall.as_deref()), ("oa-publisher", oa_publisher.as_deref()), @@ -4253,34 +4313,6 @@ impl ServerHandler for Server { /// an unexpanded `${...}` placeholder. A Desktop-Extension config left blank /// passes the literal `${user_config.store_root}`, which must never become a /// filesystem path (it produced `os error 5` access-denied). See #369. -/// Advice to attach to a `doiget_paper_search` that matched nothing (#534). -/// -/// OpenAlex free-text matching degrades sharply as a query lengthens: past -/// roughly eight terms it returns nothing at all rather than a partial match. -/// A human reading `0 results` shortens the query and tries again. An agent -/// reading `ok: true` with an empty array reads it as a fact about the world -/// and stops -- in the session that produced #534, eleven consecutive searches -/// returned zero for papers a three-to-five term query then found immediately, -/// and a known study was written off as unavailable. -/// -/// Returns `None` for short queries: a zero-result two-term search really may -/// mean the work is not indexed, and a hint on every empty result would train -/// readers to skip it. -fn zero_result_hint(query: &str) -> Option { - // Where OpenAlex free-text matching starts failing outright. Not a hard - // boundary -- a bound observed from the queries in #534, which is why the - // wording says "roughly". - const DEGRADES_PAST: usize = 8; - - let terms = query.split_whitespace().count(); - if terms <= DEGRADES_PAST { - return None; - } - Some(format!( - "This query has {terms} terms. OpenAlex free-text matching degrades sharply past roughly {DEGRADES_PAST} and returns nothing rather than a partial match, so zero results here is more likely to be about the query than about the literature. Retry with 3-5 distinctive terms - an author surname, a coined phrase, the distinguishing noun - before concluding the work is not indexed." - )) -} - fn store_root_env_is_usable(value: &str) -> bool { let v = value.trim(); !v.is_empty() && !v.contains("${") @@ -4460,38 +4492,95 @@ fn capability_profile_to_json(profile: &CapabilityProfile) -> Value { #[cfg(test)] #[allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] mod tests { - use super::*; - - /// #534: a long query matching nothing is far more likely to be a - /// query-length problem than an absent literature, and the agent reading - /// the envelope is the one who cannot tell the difference. + /// #538 on the two batch tools. + /// + /// `fetch_paper_fetch_error_envelope` shed a hand-rolled + /// `From<&FetchError>` copy ending in `_ => InternalError`. Two verbatim + /// copies of that match stayed behind in `build_bibliography_envelope` and + /// `batch_fetch_success_envelope`, so on those two surfaces a refused + /// paper was an INTERNAL_ERROR and -- worse -- a 404 was a NETWORK_ERROR + /// with disposition `retry_after`, telling an agent to keep retrying a DOI + /// that will never resolve. `disposition` is derived from the code, so the + /// wrong code was also the wrong advice. + /// + /// Driven through the real envelope builders, not a re-implementation. #[test] - fn a_long_zero_result_query_is_told_why_it_may_be_zero() { - let q = "lithium refractoriness after discontinuation kindling sensitization course of illness Post"; - let hint = zero_result_hint(q).expect("10 terms is past the threshold"); - assert!(hint.contains("10 terms"), "names the count: {hint}"); - assert!( - hint.contains("3-5"), - "says what to do instead, not only what went wrong: {hint}" - ); - } + fn a_batch_entry_gets_the_canonical_code_not_internal_error() { + use doiget_core::http::HttpError; + use doiget_core::orchestrator::{BatchOutcome, BatchResultEntry}; + use doiget_core::source::FetchError; + + let cases: Vec<(FetchError, &str, &str)> = vec![ + ( + FetchError::NotRetrievable { + source_key: "europepmc".into(), + detail: "record has no open-access full text".into(), + }, + "NO_OA_AVAILABLE", + "needs_config", + ), + ( + FetchError::Http(HttpError::HttpStatus { + status: 404, + url: "https://api.crossref.org/works/10.5555/absent".into(), + retry_after_ms: None, + }), + "NOT_FOUND", + "terminal", + ), + ]; - /// A short query returning nothing may genuinely mean nothing is indexed. - /// Hinting on every empty result would teach readers to skip the hint. - #[test] - fn a_short_zero_result_query_is_left_alone() { - assert!(zero_result_hint("depersonalization derealization").is_none()); - assert!(zero_result_hint("").is_none()); + for (err, want_code, want_disposition) in cases { + let ref_ = Ref::parse("10.1234/x").expect("valid doi"); + let batch = BatchOutcome::for_test_synthetic(vec![BatchResultEntry { + ref_: ref_.clone(), + outcome: Err(err), + }]); + + let bib = build_bibliography_envelope( + &batch, + std::slice::from_ref(&ref_), + &[None], + Vec::new(), + ); + let e = &bib["results"][0]["error"]; + assert_eq!( + e["code"], + json!(want_code), + "batch_from_bibliography: {bib:?}" + ); + assert_eq!(e["disposition"], json!(want_disposition), "{bib:?}"); + + let raw = vec![ref_.as_input_str().to_string()]; + let bf = batch_fetch_success_envelope(&batch, &raw); + let e = &bf["results"][0]["error"]; + assert_eq!(e["code"], json!(want_code), "batch_fetch: {bf:?}"); + assert_eq!(e["disposition"], json!(want_disposition), "{bf:?}"); + } } - /// The threshold counts terms, not characters: one very long term is still - /// one term, and "shorten it" is not the advice to give. + /// One error, one answer, on both front ends. The CLI delegated + /// `GraphError::Source` to `From<&FetchError>` and this crate flattened it + /// to `NETWORK_ERROR`, so an OpenAlex 404 during expansion was terminal on + /// one surface and retriable on the other. + #[cfg(feature = "citation")] #[test] - fn the_threshold_counts_terms_not_length() { - assert!(zero_result_hint(&"a".repeat(400)).is_none()); - assert!(zero_result_hint("a b c d e f g h i").is_some()); + fn a_graph_source_error_keeps_the_wrapped_code() { + use doiget_core::citation_graph::GraphError; + use doiget_core::http::HttpError; + use doiget_core::source::FetchError; + + let e = GraphError::Source(FetchError::Http(HttpError::HttpStatus { + status: 404, + url: "https://api.openalex.org/works/doi:10.5555/absent".into(), + retry_after_ms: None, + })); + assert_eq!(graph_error_code(&e), ErrorCode::NotFound); + assert_eq!(graph_error_code(&e).disposition().as_wire(), "terminal"); } + use super::*; + /// #406: the store-writability probe MUST NOT create anything. /// `doiget_health` is annotated `read_only_hint = true`, and the old /// `create_dir_all` implementation made a health check materialise diff --git a/crates/doiget-mcp/tests/error_envelope_shape.rs b/crates/doiget-mcp/tests/error_envelope_shape.rs new file mode 100644 index 000000000..d962ef568 --- /dev/null +++ b/crates/doiget-mcp/tests/error_envelope_shape.rs @@ -0,0 +1,98 @@ +//! Every `ok: false` envelope carries a structured `error` OBJECT (ADR-0055). +//! +//! `docs/ERRORS.md` §3 names `every_bare_string_error_site_is_a_known_one` as +//! the guard that pins which tools have not got there yet, "so it can shrink +//! but not grow". The guard did not exist. The document was accurate about +//! the four tools it listed and wrong about the mechanism keeping the list +//! honest -- a claim about the world resting on code that was never written, +//! which is the defect class that document defines. +//! +//! It exists now, and the set it pins is EMPTY: `doiget_resolve_citation`, +//! `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate` build +//! `error_object` like every other tool. Kept as a guard rather than deleted +//! along with the exemption, because the failure mode is a new `json!` literal +//! typing `"error": format!(...)` -- which is how the four got there. + +#![allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] + +use camino::Utf8PathBuf; + +/// Tools still permitted to answer with a bare string in `error`. +/// +/// EMPTY, deliberately. An entry here is a promise to a caller that it cannot +/// branch on the failure, so adding one needs a reason in `docs/ERRORS.md` +/// §3 rather than a line here. +const KNOWN_BARE_STRING_TOOLS: &[&str] = &[]; + +fn router_src() -> Utf8PathBuf { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs") +} + +/// The accepted right-hand sides for an `"error":` key. +/// +/// `error_object(..)` is the builder; `error_obj` is the `serde_json::Map` +/// the five hand-assembled envelopes fill in and insert. Both are objects. +fn is_structured(value: &str) -> bool { + let v = value.trim_start(); + v.starts_with("error_object(") || v.starts_with("error_obj") +} + +#[test] +fn every_bare_string_error_site_is_a_known_one() { + let src = std::fs::read_to_string(router_src()).expect("router source is readable"); + + let mut bare = Vec::new(); + for (lineno, line) in src.lines().enumerate() { + let trimmed = line.trim_start(); + // Prose is not an envelope. + if trimmed.starts_with("//") { + continue; + } + let Some(rest) = trimmed.split_once("\"error\":").map(|(_, r)| r) else { + continue; + }; + // A key with its value on the next line is written `"error":` alone; + // the builder call is what follows, so look there instead. + let value = if rest.trim().is_empty() { + src.lines().nth(lineno + 1).unwrap_or_default() + } else { + rest + }; + if !is_structured(value) { + bare.push(format!("src/lib.rs:{}: {}", lineno + 1, trimmed)); + } + } + + assert!( + bare.is_empty() || !KNOWN_BARE_STRING_TOOLS.is_empty(), + "an `ok:false` envelope answers with a bare string instead of \ + `error_object(..)`, so the caller has no `code` to branch on and no \ + `disposition` to decide a retry from (ADR-0055). Either build the \ + object, or add the tool to KNOWN_BARE_STRING_TOOLS with a reason in \ + docs/ERRORS.md §3:\n {}", + bare.join("\n ") + ); + assert!( + bare.is_empty(), + "docs/ERRORS.md §3 says this set can shrink but not grow; these are \ + not in KNOWN_BARE_STRING_TOOLS:\n {}", + bare.join("\n ") + ); +} + +/// The exemption list is a liability, not a feature: if it is empty the +/// document must not still be telling readers four tools are exempt. +#[test] +fn the_exemption_list_and_the_document_agree() { + let errors_md = Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../docs/ERRORS.md"); + let doc = std::fs::read_to_string(&errors_md).expect("docs/ERRORS.md is readable"); + let doc_claims_exemptions = doc.contains("do not yet emit that object at all"); + assert_eq!( + doc_claims_exemptions, + !KNOWN_BARE_STRING_TOOLS.is_empty(), + "docs/ERRORS.md §3 and KNOWN_BARE_STRING_TOOLS disagree about whether \ + any tool still answers with a bare string (doc says {doc_claims_exemptions}, \ + list has {} entries)", + KNOWN_BARE_STRING_TOOLS.len() + ); +} diff --git a/crates/doiget-mcp/tests/fetch_paper_e2e.rs b/crates/doiget-mcp/tests/fetch_paper_e2e.rs index 84b310f73..972fbb340 100644 --- a/crates/doiget-mcp/tests/fetch_paper_e2e.rs +++ b/crates/doiget-mcp/tests/fetch_paper_e2e.rs @@ -865,34 +865,41 @@ async fn fetch_paper_doi_with_no_oa_anywhere_reports_the_no_oa_url_route() -> an Ok(()) } -/// **`#[ignore]`d because it currently fails, and that is the finding.** The -/// Tier-3 chain DOES fire -- the attempts row shows `tdm-aps` consulted -- and -/// then: +/// The Tier-3 TDM-fetched route, asserted end to end over MCP. +/// +/// Written as an `#[ignore]`d reproduction first, because it failed with: /// /// ```text /// "detail": "network error: no allowlist registered for source tdm-aps" /// ``` /// -/// which is `HttpError::UnknownSource`: the source key is not in the client's -/// map at all. `tier_3_allowlists()` is purely `#[cfg]`-gated and the MCP +/// -- `HttpError::UnknownSource`, the source key absent from the client's map. +/// That was diagnosed as "`tier_3_allowlists()` is `#[cfg]`-gated and the MCP /// server does extend its allowlists with it, so the two disagree somewhere -/// between construction and use. That is #454's shape -- "Tier-3 allowlists -/// never registered in the client" -- reachable again, and it is exactly the -/// class of defect a route assertion exists to surface. Left as a -/// reproduction rather than muted or worked around. +/// between construction and use", i.e. #454's shape reachable again, and it was +/// raised as something to decide before cutting a release. +/// +/// The diagnosis was wrong, and wrong in this file's own subject matter: a +/// statement accurate about the code and false about the world. Both client +/// builders have two branches. The production branch does extend with +/// `tier_3_allowlists()` and was correct throughout. The test-override branch +/// -- taken whenever ANY `DOIGET_*_BASE` is set, which every wiremock test +/// does -- built its allowlist from a fixed table of Tier-1/2 keys with no +/// Tier-3 entry, so no e2e on either surface could reach this route. The +/// defect was in the harness. Registering the Tier-3 keys there is what this +/// test now proves, by passing. +/// +/// What survives from that diagnosis, and is still true: `fetch_content` is +/// implemented by APS alone. Elsevier, Springer and IEEE inherit the default +/// `Ok(None)` and are metadata-only, so three of the four Tier-3 sources +/// cannot reach the route the tier exists for. Read this as APS coverage, not +/// as Tier-3 coverage. /// /// #462: the Tier-3 route, which had no assertion anywhere -- which is how /// #458, "the Tier-3 chain is skipped whenever Crossref answers", shipped. -/// -/// Only `tdm-aps` can produce it. `fetch_content` is implemented by APS alone; -/// Elsevier, Springer and IEEE inherit the default `Ok(None)` and are -/// metadata-only, so three of the four Tier-3 sources cannot reach the route -/// the tier exists for. Worth knowing before reading this as APS-specific -/// coverage: it is the whole of it. #[cfg(feature = "tdm-aps")] #[tokio::test] #[serial_test::serial] -#[ignore = "#462: reproduces `no allowlist registered for source tdm-aps` over MCP; run with --ignored --features tdm-aps"] async fn fetch_paper_doi_served_by_the_publisher_reports_the_tdm_fetched_route( ) -> anyhow::Result<()> { use wiremock::matchers::{header, method, path}; diff --git a/crates/doiget-mcp/tests/paper_text_e2e.rs b/crates/doiget-mcp/tests/paper_text_e2e.rs index 654ebc39d..b0c7f8eb2 100644 --- a/crates/doiget-mcp/tests/paper_text_e2e.rs +++ b/crates/doiget-mcp/tests/paper_text_e2e.rs @@ -180,9 +180,15 @@ async fn paper_text_max_chars_truncates() -> anyhow::Result<()> { #[tokio::test] #[serial_test::serial] -async fn paper_text_doi_maps_to_no_oa_available() -> anyhow::Result<()> { - // A DOI has no full-text source in this slice (ADR-0032 D5): the tool - // must return a structured NO_OA_AVAILABLE envelope, no network touched. +async fn paper_text_doi_is_terminal_not_a_config_problem() -> anyhow::Result<()> { + // A DOI has no full-text source in this slice (ADR-0032 D5). The code says + // WHICH kind of "no": `NO_OA_AVAILABLE` carries disposition `needs_config` + // -- "a named change makes it" -- and sends an agent looking for a config + // knob that does not exist, because the missing piece is DOI-to-arXiv + // linking (#281 item 5), not a grant. `NOT_IMPLEMENTED` is terminal and + // is the code `verify` / `batch_from_bibliography` already give the same + // situation for PMIDs (#500): valid input, absent support. + // No network touched either way. let env = EnvGuard::new(ENV_KEYS); let (client, server_handle) = boot_in_memory_server().await?; @@ -196,7 +202,10 @@ async fn paper_text_doi_maps_to_no_oa_available() -> anyhow::Result<()> { let s = result.structured_content.as_ref().expect("structured"); assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); - assert_eq!(s["error"]["code"], serde_json::json!("NO_OA_AVAILABLE")); + assert_eq!(s["error"]["code"], serde_json::json!("NOT_IMPLEMENTED")); + // The code is only half the answer; the disposition is what an agent + // branches on, and it is the half that was wrong. + assert_eq!(s["error"]["disposition"], serde_json::json!("terminal")); // MCP_TOOLS.md §5: an ok:false envelope echoes the input `ref`. assert_eq!(s["ref"], serde_json::json!("10.1234/example")); diff --git a/crates/doiget-mcp/tests/route_coverage_e2e.rs b/crates/doiget-mcp/tests/route_coverage_e2e.rs index df1ece2d2..0c1982aea 100644 --- a/crates/doiget-mcp/tests/route_coverage_e2e.rs +++ b/crates/doiget-mcp/tests/route_coverage_e2e.rs @@ -36,6 +36,10 @@ use camino::Utf8PathBuf; /// How a route is covered. #[derive(Debug)] +#[allow( + dead_code, + reason = "no route is uncovered today; the variant is how the next one gets recorded instead of silently omitted" +)] enum Coverage { /// Asserted by `fn` in the given test file. By { @@ -82,8 +86,13 @@ const ROUTES: &[(&str, Coverage)] = &[ ), ( "tdm_fetched", - Coverage::Gap { - why: "`fetch_paper_doi_served_by_the_publisher_reports_the_tdm_fetched_route` is written and #[ignore]d because it FAILS: the chain fires, and the client answers `no allowlist registered for source tdm-aps`. Only tdm-aps implements `fetch_content` at all, so that is the whole route. #454's shape, reachable again", + // Was the file's one `Gap`, on a reproduction that could not pass + // because the test harness had no way to register a Tier-3 allowlist + // entry -- not, as the reason here claimed, because production had + // regressed to #454's shape. + Coverage::By { + file: "fetch_paper_e2e.rs", + test_fn: "fetch_paper_doi_served_by_the_publisher_reports_the_tdm_fetched_route", }, ), ]; @@ -131,7 +140,18 @@ fn every_claimed_covering_test_exists_and_asserts_its_route() { // not run cannot be evidence that a route is covered. let line_start = src[..def].rfind('\n').map_or(0, |i| i + 1); let attrs_from = src[..line_start].rfind("\n\n").map_or(0, |i| i + 2); - if src[attrs_from..line_start].contains("#[ignore") { + // Line-by-line, and only lines that ARE an attribute. A substring search + // over the whole block also matches a doc comment that DISCUSSES + // `#[ignore]` -- which the corrected write-up of the TDM route now does, + // and it made this checker report the very test it was reading about as + // skipped. Prose is not an attribute; a checker that cannot tell the + // difference is the defect it exists to catch. + let is_ignored = src[attrs_from..line_start] + .lines() + .map(str::trim_start) + .filter(|l| l.starts_with("#[")) + .any(|l| l.starts_with("#[ignore")); + if is_ignored { problems.push(format!( "{route}: `{test_fn}` is #[ignore]d, so CI never runs it -- that is a Gap with a reason, not coverage" )); @@ -247,7 +267,7 @@ fn every_route_is_either_covered_or_has_a_stated_reason() { // might read into a number that has to be updated when it changes. assert_eq!( uncovered.len(), - 1, + 0, "known gaps changed: {uncovered:?}. If a route just gained an assertion, move it from `Gap` to `By` and drop this count by one -- the point is that closing a gap is a visible edit, not a silent improvement." ); } diff --git a/docs/ERRORS.md b/docs/ERRORS.md index 55aa21977..8a1c2a568 100644 --- a/docs/ERRORS.md +++ b/docs/ERRORS.md @@ -55,7 +55,7 @@ Wire form (JSON / MCP): `"INVALID_REF"`, `"NO_OA_AVAILABLE"`, etc. | Persona | Surface | |---|---| -| Agent (MCP) | Structured, never throws. On failure: `{ ok: false, error: { code, message, disposition, retry_after_ms?, remediation?, denial_context? } }`. `disposition` is present on every failure that carries a structured `error` OBJECT, and is the field to branch a retry on — see §2 and ADR-0055. **Four tools do not yet emit that object at all** and instead put a bare string in `error`: `doiget_resolve_citation`, `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate`. That set is pinned by `every_bare_string_error_site_is_a_known_one` so it can shrink but not grow; this document said "ALWAYS present" before that was checked, which was the same overclaim it exists to prevent. It is derived from `code` by `ErrorCode::disposition`, never hand-written per call site. `remediation` IS carried here when the failure has a `denial_context`, from the same `remediation::for_denial` the blocked leg and the CLI `= help:` block use — this document previously said it was not, which meant the one field naming the fix was present when a call succeeded with a blocked leg and absent when the call actually failed (#506). It is omitted when there is no named fix rather than emitted empty. `attempts` remains an `ok: true` field. A blocked PDF leg is still an `ok: true` result with a failed leg, not an `ok: false` call, and it carries `pdf.remediation` plus `pdf.disposition`. `retry_after_ms` is present only when the SERVER sent a `Retry-After` on the response that ended the attempt; it is never backfilled from doiget's internal backoff, because a guess about the server wearing the name of a server-supplied value is the defect these fields exist to remove (#506). | +| Agent (MCP) | Structured, never throws. On failure: `{ ok: false, error: { code, message, disposition, retry_after_ms?, remediation?, denial_context? } }`. `disposition` is present on every failure that carries a structured `error` OBJECT, and is the field to branch a retry on — see §2 and ADR-0055. `disposition` is now present on **every** `ok: false` envelope. Four tools -- `doiget_resolve_citation`, `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate` -- used to put a bare string in `error` instead, so a caller had nothing to branch on; they build the object like every other tool as of 0.8.13 **(breaking for anyone reading `error` as a string on those four)**. The set is pinned EMPTY by `every_bare_string_error_site_is_a_known_one`, and `the_exemption_list_and_the_document_agree` fails if this paragraph and that list ever disagree. Both are new: this document already named the guard as the reason the set "can shrink but not grow", and the guard had not been written -- a claim about the world resting on code that did not exist, which is exactly what this document defines. It is derived from `code` by `ErrorCode::disposition`, never hand-written per call site. `remediation` IS carried here when the failure has a `denial_context`, from the same `remediation::for_denial` the blocked leg and the CLI `= help:` block use — this document previously said it was not, which meant the one field naming the fix was present when a call succeeded with a blocked leg and absent when the call actually failed (#506). It is omitted when there is no named fix rather than emitted empty. `attempts` remains an `ok: true` field. A blocked PDF leg is still an `ok: true` result with a failed leg, not an `ok: false` call, and it carries `pdf.remediation` plus `pdf.disposition`. `retry_after_ms` is present only when the SERVER sent a `Retry-After` on the response that ended the attempt; it is never backfilled from doiget's internal backoff, because a guess about the server wearing the name of a server-supplied value is the defect these fields exist to remove (#506). | | Researcher (CLI human) | `cargo`-style stderr: `error[E0007]: rate limited from unpaywall: retry after 1s`. Exit code 1. | | CI / Batch (CLI `--json`) | JSON Lines record per ref with `{"ok":false, "error":{"code":"...","message":"...","denial_context":{...}?,"remediation":[...]?,"attempts":[...]?}}`. Exit code = number of failures (capped at 255). **Records are emitted in completion order, not input order** — see below. | | Library (Rust) | `Err(FetchError)` (typed via `thiserror`). | diff --git a/justfile b/justfile index ad4a52e8a..abfc70497 100644 --- a/justfile +++ b/justfile @@ -25,6 +25,32 @@ lint: test: cargo test --workspace --all-targets --no-default-features --features oa-only +# Mirrors CI `test (slow)` — the `#[ignore]`d tests, which the job above skips. +# One of them obeys arXiv's published 3 s/request rate and takes ~10 minutes; +# that is the whole reason it is not in `test`. +test-slow: + cargo test --workspace --all-targets --no-default-features --features oa-only -- --ignored + +# Register THIS checkout's build as the MCP server for Claude Code, without +# touching `.mcp.json`. +# +# `.mcp.json` is the plugin's server declaration and pins the last published +# release, so opening this repo in Claude Code otherwise runs the RELEASE, not +# your working tree — you can edit `crates/doiget-mcp` all day and test the +# version you shipped last month. A local-scoped server wins over the +# project-scoped `.mcp.json`, so this shadows it for you alone and leaves the +# file (and everyone else's) unchanged. +# +# `just mcp-dev-off` removes it again. +mcp-dev: + cargo build --no-default-features --features oa-only -p doiget-cli + claude mcp remove doiget-dev --scope local || true + claude mcp add doiget-dev --scope local -- cargo run --quiet --no-default-features --features oa-only -p doiget-cli -- serve + +# Drop the local-scoped dev server registered by `just mcp-dev`. +mcp-dev-off: + claude mcp remove doiget-dev --scope local + # Mirrors CI `msrv` job (build of the published surface). build: cargo build --workspace --no-default-features --features oa-only diff --git a/npm/doiget-cli/test/stage-npm.test.sh b/npm/doiget-cli/test/stage-npm.test.sh index fc1e179d1..6a0f2d8fa 100644 --- a/npm/doiget-cli/test/stage-npm.test.sh +++ b/npm/doiget-cli/test/stage-npm.test.sh @@ -162,10 +162,19 @@ if command -v npm > /dev/null 2>&1; then fi # The platform list the loop actually iterates, from the same source it reads. - looped="$(grep -oE '^doiget-[a-z0-9-]+:' "$ROOT/scripts/stage-npm.sh" | tr -d ':' | sort -u | sed 's#^#./npm-stage/#')" - direct="$(grep -oE 'npm publish [^ "$]+' "$WORKFLOW" | awk '{print $3}')" + # + # `|| true` is load-bearing, not defensive noise. Under `set -euo pipefail` a + # bare assignment takes the exit status of its command substitution, so a + # `grep` that matches nothing kills the script HERE -- before the `check no` + # written two lines down to report exactly that. The diagnostic was + # unreachable in the one case it exists for, and CI would show a bare + # non-zero exit with none of the ok/FAIL lines around it. + looped="$(grep -oE '^doiget-[a-z0-9-]+:' "$ROOT/scripts/stage-npm.sh" | tr -d ':' | sort -u | sed 's#^#./npm-stage/#' || true)" + direct="$(grep -oE 'npm publish [^ "$]+' "$WORKFLOW" | awk '{print $3}' || true)" if [ -z "$looped" ] || [ -z "$direct" ]; then check no "found the npm publish invocations in release-plz.yml" + else + check ok "found the npm publish invocations in release-plz.yml" fi all_specs="$(printf '%s diff --git a/scripts/stage-npm.sh b/scripts/stage-npm.sh old mode 100644 new mode 100755 diff --git a/scripts/update-homebrew-formula.sh b/scripts/update-homebrew-formula.sh old mode 100644 new mode 100755 index a2b1f31f5..23c4464ba --- a/scripts/update-homebrew-formula.sh +++ b/scripts/update-homebrew-formula.sh @@ -3,8 +3,13 @@ # # The formula is GENERATED, never hand-edited: #247 was closed as completed # while four fifths of it had not shipped, and a hand-maintained formula is -# exactly the kind of thing that half-lands the same way. The release workflow -# runs this, so the formula cannot describe a version that was never published. +# exactly the kind of thing that half-lands the same way. +# +# A maintainer runs this after a stable release, not the release workflow -- +# see CONTRIBUTING.md and #426. The generator cannot be run WRONG (posture-lint +# asserts the committed formula is its output, and refuses malformed +# checksums); it can only be FORGOTTEN, which the release-sync posture check +# turns red as soon as the other release-tracking files are bumped. # # Usage: # scripts/update-homebrew-formula.sh @@ -53,8 +58,12 @@ mkdir -p "$(dirname "$OUT")" cat > "$OUT" <\` after a stable +# release has published its assets. That is a MAINTAINER step, not a workflow +# step -- committing the formula back from CI needs a token with write access +# to a protected branch, and #426 says that token is broken. So the formula +# lags the tag until someone runs the script, and CONTRIBUTING.md says so +# (#501). class Doiget < Formula desc "Open Access paper fetcher with an MCP server" homepage "https://github.com/${REPO}" diff --git a/scripts/update-homebrew-formula.test.sh b/scripts/update-homebrew-formula.test.sh old mode 100644 new mode 100755 diff --git a/site/content/developer/errors.md b/site/content/developer/errors.md index a20c3f9c5..b53263c66 100644 --- a/site/content/developer/errors.md +++ b/site/content/developer/errors.md @@ -61,7 +61,7 @@ Wire form (JSON / MCP): `"INVALID_REF"`, `"NO_OA_AVAILABLE"`, etc. | Persona | Surface | |---|---| -| Agent (MCP) | Structured, never throws. On failure: `{ ok: false, error: { code, message, disposition, retry_after_ms?, remediation?, denial_context? } }`. `disposition` is present on every failure that carries a structured `error` OBJECT, and is the field to branch a retry on — see §2 and ADR-0055. **Four tools do not yet emit that object at all** and instead put a bare string in `error`: `doiget_resolve_citation`, `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate`. That set is pinned by `every_bare_string_error_site_is_a_known_one` so it can shrink but not grow; this document said "ALWAYS present" before that was checked, which was the same overclaim it exists to prevent. It is derived from `code` by `ErrorCode::disposition`, never hand-written per call site. `remediation` IS carried here when the failure has a `denial_context`, from the same `remediation::for_denial` the blocked leg and the CLI `= help:` block use — this document previously said it was not, which meant the one field naming the fix was present when a call succeeded with a blocked leg and absent when the call actually failed (#506). It is omitted when there is no named fix rather than emitted empty. `attempts` remains an `ok: true` field. A blocked PDF leg is still an `ok: true` result with a failed leg, not an `ok: false` call, and it carries `pdf.remediation` plus `pdf.disposition`. `retry_after_ms` is present only when the SERVER sent a `Retry-After` on the response that ended the attempt; it is never backfilled from doiget's internal backoff, because a guess about the server wearing the name of a server-supplied value is the defect these fields exist to remove (#506). | +| Agent (MCP) | Structured, never throws. On failure: `{ ok: false, error: { code, message, disposition, retry_after_ms?, remediation?, denial_context? } }`. `disposition` is present on every failure that carries a structured `error` OBJECT, and is the field to branch a retry on — see §2 and ADR-0055. `disposition` is now present on **every** `ok: false` envelope. Four tools -- `doiget_resolve_citation`, `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate` -- used to put a bare string in `error` instead, so a caller had nothing to branch on; they build the object like every other tool as of 0.8.13 **(breaking for anyone reading `error` as a string on those four)**. The set is pinned EMPTY by `every_bare_string_error_site_is_a_known_one`, and `the_exemption_list_and_the_document_agree` fails if this paragraph and that list ever disagree. Both are new: this document already named the guard as the reason the set "can shrink but not grow", and the guard had not been written -- a claim about the world resting on code that did not exist, which is exactly what this document defines. It is derived from `code` by `ErrorCode::disposition`, never hand-written per call site. `remediation` IS carried here when the failure has a `denial_context`, from the same `remediation::for_denial` the blocked leg and the CLI `= help:` block use — this document previously said it was not, which meant the one field naming the fix was present when a call succeeded with a blocked leg and absent when the call actually failed (#506). It is omitted when there is no named fix rather than emitted empty. `attempts` remains an `ok: true` field. A blocked PDF leg is still an `ok: true` result with a failed leg, not an `ok: false` call, and it carries `pdf.remediation` plus `pdf.disposition`. `retry_after_ms` is present only when the SERVER sent a `Retry-After` on the response that ended the attempt; it is never backfilled from doiget's internal backoff, because a guess about the server wearing the name of a server-supplied value is the defect these fields exist to remove (#506). | | Researcher (CLI human) | `cargo`-style stderr: `error[E0007]: rate limited from unpaywall: retry after 1s`. Exit code 1. | | CI / Batch (CLI `--json`) | JSON Lines record per ref with `{"ok":false, "error":{"code":"...","message":"...","denial_context":{...}?,"remediation":[...]?,"attempts":[...]?}}`. Exit code = number of failures (capped at 255). **Records are emitted in completion order, not input order** — see below. | | Library (Rust) | `Err(FetchError)` (typed via `thiserror`). | From 5cfcd5bc5d5e948ba0798496f3d7221de756cbdc Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 12:27:08 +0900 Subject: [PATCH 2/5] fix(release): bump the inter-crate version requirements too The cut changed [workspace.package].version and left [workspace.dependencies] declaring ^0.8.13-beta.23, so cargo could not resolve doiget-core and the release gate G2 failed. Local builds hid it: path dependencies win over the version requirement. scripts/release-version-gate.sh v0.8.13 now passes G0-G6 (G7 skips pre-tag). Signed-off-by: Sota Shimozono --- Cargo.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index cb8667bf6..489652f8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,9 +41,9 @@ features-doc-link = "docs/SOURCES.md" [workspace.dependencies] # Core -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" } +doiget-core = { path = "crates/doiget-core", version = "0.8.13" } +doiget-cli = { path = "crates/doiget-cli", version = "0.8.13" } +doiget-mcp = { path = "crates/doiget-mcp", version = "0.8.13" } # Async runtime — features are deliberately limited (avoid `full`). tokio = { version = "1", default-features = false, features = [ From 3e5ea17b89163a5124e633f9554170716edc00ba Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 13:07:33 +0900 Subject: [PATCH 3/5] fix(ci): make the release-sync check say why it failed, and catch every prerelease Two defects in the check added one commit ago, both found by reviewing it. An empty extraction killed the step at the assignment, under set -euo pipefail, before the ::error:: written to explain it -- so deleting the doiget-cli@ pin from .mcp.json failed closed and silently. That is the shape fixed one file over in stage-npm.test.sh in the same PR. The prerelease guard enumerated spellings (*-beta.*|*-rc.*) and let 0.9.0-alpha.2 through. Any - suffix is a prerelease per SemVer. Probed both: an empty extraction now names the file, and beta/rc/alpha/pre/ rc1 are all rejected while 0.8.12 passes. Signed-off-by: Sota Shimozono --- .github/workflows/posture-lint.yml | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/workflows/posture-lint.yml b/.github/workflows/posture-lint.yml index 05c0ab017..56b1e7879 100644 --- a/.github/workflows/posture-lint.yml +++ b/.github/workflows/posture-lint.yml @@ -309,10 +309,24 @@ jobs: # makes forgetting it red. run: | set -euo pipefail - formula=$(grep -oE '^ version "[^"]+"' Formula/doiget.rb | grep -oE '[0-9][^"]*') - plugin=$(grep -oE '"version": *"[^"]+"' .claude-plugin/plugin.json | grep -oE '[0-9][^"]*') - market=$(grep -oE '"version": *"[^"]+"' .claude-plugin/marketplace.json | grep -oE '[0-9][^"]*') - pinned=$(grep -oE '"doiget-cli@[^"]+"' .mcp.json | grep -oE '[0-9][^"]*') + # `|| true` on each is load-bearing. Under `set -euo pipefail` a bare + # assignment takes the exit status of its command substitution, so a + # `grep` that matches nothing -- somebody deletes the `@version` pin from + # .mcp.json, or reformats a manifest -- kills the step HERE, before the + # `::error::` written to explain it. It failed closed, which is right, and + # said nothing, which is the defect this whole job exists to catch. Found + # by review of this change; the same shape was fixed one file over in + # npm/doiget-cli/test/stage-npm.test.sh. + formula=$(grep -oE '^ version "[^"]+"' Formula/doiget.rb | grep -oE '[0-9][^"]*' || true) + plugin=$(grep -oE '"version": *"[^"]+"' .claude-plugin/plugin.json | grep -oE '[0-9][^"]*' || true) + market=$(grep -oE '"version": *"[^"]+"' .claude-plugin/marketplace.json | grep -oE '[0-9][^"]*' || true) + pinned=$(grep -oE '"doiget-cli@[^"]+"' .mcp.json | grep -oE '[0-9][^"]*' || true) + for pair in "Formula/doiget.rb:$formula" ".claude-plugin/plugin.json:$plugin" ".claude-plugin/marketplace.json:$market" ".mcp.json:$pinned"; do + if [ -z "${pair#*:}" ]; then + echo "::error::posture-lint release-sync: no version found in ${pair%%:*} -- the file was reformatted, or .mcp.json lost its doiget-cli@ pin" + exit 1 + fi + done echo "formula=$formula plugin=$plugin marketplace=$market .mcp.json=$pinned" for v in "$plugin" "$market" "$pinned"; do if [ "$v" != "$formula" ]; then @@ -320,9 +334,13 @@ jobs: exit 1 fi done - # An unpublished pin would send every plugin user to a 404. + # An unpublished pin would send every plugin user to a 404. Any `-` + # suffix is a prerelease per SemVer; the first version of this listed + # spellings (`*-beta.*|*-rc.*`) and let `0.9.0-alpha.2` straight + # through, which is the enumerate-the-known-cases mistake this + # release is otherwise about. case "$pinned" in - *-beta.*|*-rc.*) + *-*) echo "::error::posture-lint release-sync: .mcp.json pins a prerelease ($pinned); the plugin must point at a published stable" exit 1 ;; From 316c2630098cc5067c6f5ad971fceae021d533ee Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 14:06:58 +0900 Subject: [PATCH 4/5] fix: close the review findings on this PR itself A six-agent review of #588 found the PR reproducing the defect it exists to fix. Every item below is mine, from the previous two commits. Fixes that landed on one surface and not its sibling -- the exact criticism this release makes of the code it changes: * doiget_paper_text / doiget_paper_tex_source were re-coded to NOT_IMPLEMENTED and the three CLI siblings (text, source, tex-source) were left on NO_OA_AVAILABLE, so one binary answered the same question two ways. The tools' own description strings, their input-schema field docs and docs/MCP_TOOLS.md also still named the old code. * doiget_fetch_paper got FetchPaperOutcome::is_clean_success and the two batch tools kept .all(|r| r.outcome.is_ok()), calling a batch where every entry was refused a success. A fourth site in the CLI had hand-rolled the same predicate. Guards that did not guard: * error_envelope_shape scanned for the literal "error": and missed insert("error".into(), ..) -- which is how fetch_paper_error_envelope writes it, the builder behind the INVALID_REF / STORE_ERROR / LOG_ERROR / INTERNAL_ERROR arms of doiget_fetch_paper. Proven by injecting a bare string there and watching the guard stay green. Its is_structured also accepted any identifier starting with error_obj. * Its KNOWN_BARE_STRING_TOOLS exemption told a failing contributor to add their tool to a list the second assertion never consulted. Removed. * route_coverage_e2e claimed tdm_fetched was covered by a test that is cfg-gated on tdm-aps, so in the two REQUIRED CI jobs (--features oa-only) the function is not in the binary at all. Coverage::By now records the feature and the checker cross-checks it against the source. * Its ignore detector missed cfg_attr(, ignore). Codes that made a claim the docs contradict: * doiget_tag / doiget_annotate reported a store miss as NOT_FOUND, which docs/ERRORS.md defines as "a metadata source authoritatively reported the id does not exist ... a definite dead reference". Those sites had no code at all before this PR, so the wrong claim is new. STORE_ERROR now, whose needs_config disposition fits "fetch it first", and ERRORS.md says so. And, one function over from the CHANGELOG line about a doc comment drifting onto the wrong item, resolve_store_root's doc had drifted onto store_root_env_is_usable. Also: the CONTRIBUTING release runbook gave three of four edits as comments between two real commands, so copy-pasting it produced a commit whose message claimed four files were bumped when one was; just mcp-dev registered doiget-dev, and local scope only shadows a project-scoped server of the SAME name, so both stayed connected and the pinned release remained callable; the release-sync check overclaimed what it catches; and the CSL-JSON bullet was missing its (breaking) marker, leaving five where the PR body says six. Signed-off-by: Sota Shimozono --- .github/workflows/posture-lint.yml | 20 ++- CHANGELOG.md | 43 ++++-- CONTRIBUTING.md | 30 +++-- crates/doiget-cli/src/commands/fetch.rs | 2 +- crates/doiget-cli/src/commands/source.rs | 9 +- crates/doiget-cli/src/commands/tex_source.rs | 9 +- crates/doiget-cli/src/commands/text.rs | 9 +- crates/doiget-core/src/lib.rs | 6 +- crates/doiget-core/src/refs.rs | 19 +++ crates/doiget-mcp/src/lib.rs | 122 ++++++++++++++---- .../doiget-mcp/tests/error_envelope_shape.rs | 92 ++++++++----- crates/doiget-mcp/tests/route_coverage_e2e.rs | 50 ++++++- docs/ERRORS.md | 2 +- docs/MCP_TOOLS.md | 2 +- justfile | 14 +- site/content/developer/errors.md | 2 +- site/content/developer/mcp-tools.md | 2 +- 17 files changed, 329 insertions(+), 104 deletions(-) diff --git a/.github/workflows/posture-lint.yml b/.github/workflows/posture-lint.yml index 56b1e7879..017c84276 100644 --- a/.github/workflows/posture-lint.yml +++ b/.github/workflows/posture-lint.yml @@ -304,9 +304,15 @@ jobs: # Four files record "the last published stable", and a stable release # bumps all four in one maintainer step. Nothing used to hold them # together, which is how `.claude-plugin/*` sat at 0.8.11 while 0.8.12 - # was the shipped release. Forgetting the step is the only way the - # Homebrew formula or the plugin's pinned server can go stale, so this - # makes forgetting it red. + # was the shipped release. + # + # What this CANNOT catch, stated so the check does not repeat the + # overclaim it exists to fix: forgetting the step ENTIRELY leaves all + # four at the previous release, mutually consistent, and green. + # Nothing in the repository knows which version is currently published. + # What it does catch is bumping some and not others, and pinning a + # version that was never released -- CHANGELOG.md is the only + # independent source of truth available offline. run: | set -euo pipefail # `|| true` on each is load-bearing. Under `set -euo pipefail` a bare @@ -345,7 +351,13 @@ jobs: exit 1 ;; esac - echo "release-tracking files agree on $formula" + # The one independent check available without a network: a version + # these files claim to track must have a released CHANGELOG section. + if ! grep -qE "^## \[$formula\]" CHANGELOG.md; then + echo "::error::posture-lint release-sync: the tracking files name $formula, which has no '## [$formula]' section in CHANGELOG.md -- they point at a version that was never released" + exit 1 + fi + echo "release-tracking files agree on $formula, and CHANGELOG.md has its section" - name: npm packaging tests (#511) shell: bash diff --git a/CHANGELOG.md b/CHANGELOG.md index 88f7dfb9c..ece17a414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,14 +64,21 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he it without comment or test. The header says how long to wait *if* you retry; it does not make a 404 provisional. -- **[mcp] (breaking)** `doiget_paper_text` and `doiget_paper_tex_source` answered - a DOI with `NO_OA_AVAILABLE`, whose disposition is `needs_config`. +- **[cli/mcp] (breaking)** The arXiv-only text tools answered a DOI with + `NO_OA_AVAILABLE`, whose disposition is `needs_config`. - There is no config knob: the tools are arXiv-only and DOI-to-arXiv linking is - not built. `NOT_IMPLEMENTED` is terminal and is what `verify` and + There is no config knob: they are arXiv-only and DOI-to-arXiv linking is not + built. `NOT_IMPLEMENTED` is terminal and is what `verify` and `batch_from_bibliography` already give the same situation for PMIDs -- valid input, absent support. + All five surfaces, not two: `doiget_paper_text` and `doiget_paper_tex_source` + over MCP, and `doiget text` / `doiget source` / `doiget tex-source` on the + CLI. The first pass changed the MCP pair only, which would have left one + binary answering the same question two ways -- and left the tools' own + advertised `description` strings, their input-schema field docs and + `docs/MCP_TOOLS.md` naming a code they no longer return. + - **[mcp] (breaking)** `doiget_resolve_citation`, `doiget_batch_resolve_citations`, `doiget_tag` and `doiget_annotate` put a **bare string** in `error`, so a caller had no `code` to branch on and no `disposition` to decide a retry from. @@ -92,9 +99,13 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he is explaining allowlist refusals: the moment a resolver host entered its probe list, the command that diagnoses #533 would have reproduced it. -- **[core]** CSL-JSON entries identified only by a PMID or PMCID reported "entry - has no DOI / arXiv id" (#500). The BibTeX parser learned the distinction; this - format did not, and it is the one a Zotero user is most likely to export. +- **[core] (breaking)** CSL-JSON entries identified only by a PMID or PMCID + reported "entry has no DOI / arXiv id" (#500), and came out as `INVALID_REF`. + The BibTeX parser learned the distinction; this format did not, and it is the + one a Zotero user is most likely to export. Marked breaking for the same + reason as the `doiget batch` bullet above: the wire code changes to + `NOT_IMPLEMENTED` on `verify`, `batch` and the MCP bibliography tool for this + input shape. - **[cli]** `doiget search --mode json` emitted `{"ok": true, "total_results": 0}` with no hint for an over-long query (#534). The fix landed on the MCP tool @@ -102,12 +113,18 @@ flag changes and `doiget-mcp` tool spec changes will be called out explicitly he about. `zero_result_hint` moved to `doiget-core`; both surfaces call it, and human mode gets a `= note:` on stderr. -- **[mcp]** `doiget_fetch_paper` logged `SessionEnd` as `result: "ok"` beside a - non-null `error_code` for a blocked PDF leg (#507). The clean-success rule of - the CLI was ported to the MCP surface in the `error_code` half only, so the - row contradicted itself for the one outcome the repeat suppression of #507 has - to be able to see. `FetchPaperOutcome::is_clean_success` is the single - boundary now. +- **[mcp]** `doiget_fetch_paper`, `doiget_batch_fetch` and + `doiget_batch_from_bibliography` logged `SessionEnd` as `result: "ok"` for a + blocked PDF leg (#507) -- on the single-ref tool, beside a non-null + `error_code`, so the row contradicted itself. + + The clean-success rule of the CLI was ported to the MCP surface in the + `error_code` half only, and then only to the single-ref tool: the two batch + tools went on calling `.all(|r| r.outcome.is_ok())` a success for a batch + where every entry was refused. That is the outcome the repeat suppression of + #507 has to be able to see. `FetchPaperOutcome::is_clean_success` is the + single boundary for all four call sites now, including the identity-line + check in the CLI that had hand-rolled it a fourth time. ### Changed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 377f5689b..1920928bb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -224,15 +224,21 @@ and `.mcp.json` pins the npm version the plugin runs. So they lag the tag by one commit, on purpose, and closing that gap is a single maintainer step: ```sh -bash scripts/update-homebrew-formula.sh 0.8.13 # reads the release's .sha256 assets -# then set 0.8.13 in all three of: -# .claude-plugin/plugin.json "version" -# .claude-plugin/marketplace.json "version" -# .mcp.json args: doiget-cli@0.8.13 -bash scripts/update-homebrew-formula.test.sh # the same check CI runs -git commit -s -m "chore(release): tracking files for 0.8.13" -- Formula/doiget.rb .claude-plugin .mcp.json +V=0.8.13 # the version that was just published + +bash scripts/update-homebrew-formula.sh "$V" # reads that release's .sha256 assets +sed -i "s/\"version\": \"[^\"]*\"/\"version\": \"$V\"/" .claude-plugin/plugin.json .claude-plugin/marketplace.json +sed -i "s/doiget-cli@[^\"]*/doiget-cli@$V/" .mcp.json + +bash scripts/update-homebrew-formula.test.sh # the same check CI runs +git commit -s -m "chore(release): tracking files for $V" -- Formula/doiget.rb .claude-plugin .mcp.json ``` +All four edits are commands on purpose. The first version of this block gave +three of them as `#` comments between two real commands, so copy-pasting it +bumped the formula, silently skipped the other three, and produced a commit +whose message said all four had been done. + Only for **stable** releases. Beta tags are not published to the tap, and the plugin must never pin a prerelease -- posture-lint fails on both. @@ -259,13 +265,17 @@ What CI does enforce, so the step cannot be done *wrong* or half-done: release. Opening this repo in Claude Code therefore runs *the release*, not your checkout -- you can edit `crates/doiget-mcp` and be testing the version you shipped last month. Do not edit `.mcp.json` to work around that; a -local-scoped server takes precedence over the project-scoped file: +local-scoped server of the **same name** shadows the project-scoped one: ```sh -just mcp-dev # registers this checkout's build as `doiget-dev` -just mcp-dev-off # removes it again +just mcp-dev # registers this checkout's build, as `doiget` +just mcp-dev-off # removes it; `.mcp.json`'s pinned entry reappears ``` +The name matters. `claude mcp add doiget-dev --scope local` leaves BOTH servers +connected, so the release-pinned tools stay callable and nothing is shadowed -- +which is how the first version of this section was written. + ### npm: the one-time bootstrap The `publish to npm` job authenticates over OIDC and holds no `NPM_TOKEN`. diff --git a/crates/doiget-cli/src/commands/fetch.rs b/crates/doiget-cli/src/commands/fetch.rs index 541d77977..8aa0d39b3 100644 --- a/crates/doiget-cli/src/commands/fetch.rs +++ b/crates/doiget-cli/src/commands/fetch.rs @@ -707,7 +707,7 @@ fn emit_success_line(ref_: &Ref, outcome: &FetchPaperOutcome) { // paper landed without a second `doiget info` call. Skipped for the // Blocked fail-closed arm (it rendered an `error[CODE]:` line above, not // a success). - if !matches!(outcome.pdf_leg, PdfLegStatus::Blocked { .. }) { + if outcome.is_clean_success() { emit_identity_line(outcome); } } diff --git a/crates/doiget-cli/src/commands/source.rs b/crates/doiget-cli/src/commands/source.rs index 3f25f4180..d98a0a21e 100644 --- a/crates/doiget-cli/src/commands/source.rs +++ b/crates/doiget-cli/src/commands/source.rs @@ -52,7 +52,14 @@ pub async fn run( let id: ArxivId = match parsed { Ref::Arxiv(a) => a, Ref::Doi(_) => { - let code = ErrorCode::NoOaAvailable; + // `NOT_IMPLEMENTED`, not `NO_OA_AVAILABLE`. The latter carries + // disposition `needs_config` -- "a named change makes it" -- and + // there is no knob: this command is arXiv-only and DOI-to-arXiv + // linking is not built (#281 item 5). Kept identical to the MCP + // sibling; changing one surface and not the other is the defect + // this release is about, and the first pass at this fix did + // exactly that. + let code = ErrorCode::NotImplemented; print_err(format_args!( "error[{}]: no source bundle for a bare DOI — if an arXiv preprint exists, \ pass its id (e.g. `doiget source arxiv:2401.12345 --out ./src`)", diff --git a/crates/doiget-cli/src/commands/tex_source.rs b/crates/doiget-cli/src/commands/tex_source.rs index 39e7576d8..4cb2ca89a 100644 --- a/crates/doiget-cli/src/commands/tex_source.rs +++ b/crates/doiget-cli/src/commands/tex_source.rs @@ -42,7 +42,14 @@ pub async fn run( let id: ArxivId = match parsed { Ref::Arxiv(a) => a, Ref::Doi(_) => { - let code = ErrorCode::NoOaAvailable; + // `NOT_IMPLEMENTED`, not `NO_OA_AVAILABLE`. The latter carries + // disposition `needs_config` -- "a named change makes it" -- and + // there is no knob: this command is arXiv-only and DOI-to-arXiv + // linking is not built (#281 item 5). Kept identical to the MCP + // sibling; changing one surface and not the other is the defect + // this release is about, and the first pass at this fix did + // exactly that. + let code = ErrorCode::NotImplemented; print_err(format_args!( "error[{}]: no TeX source for a bare DOI — if an arXiv preprint exists, \ pass its id (e.g. `doiget tex-source arxiv:2401.12345`)", diff --git a/crates/doiget-cli/src/commands/text.rs b/crates/doiget-cli/src/commands/text.rs index c7c81a9f6..911bdae1a 100644 --- a/crates/doiget-cli/src/commands/text.rs +++ b/crates/doiget-cli/src/commands/text.rs @@ -52,7 +52,14 @@ pub async fn run( // No full-text HTML source for a bare DOI in PR4; DOI→arXiv // linking is #281 item 5 (ADR-0032 D5). Report honestly rather // than silently failing. - let code = ErrorCode::NoOaAvailable; + // `NOT_IMPLEMENTED`, not `NO_OA_AVAILABLE`. The latter carries + // disposition `needs_config` -- "a named change makes it" -- and + // there is no knob: this command is arXiv-only and DOI-to-arXiv + // linking is not built (#281 item 5). Kept identical to the MCP + // sibling; changing one surface and not the other is the defect + // this release is about, and the first pass at this fix did + // exactly that. + let code = ErrorCode::NotImplemented; print_err(format_args!( "error[{}]: no full-text source for a DOI — if an arXiv preprint exists, \ pass its id (e.g. `doiget text arxiv:2401.12345`)", diff --git a/crates/doiget-core/src/lib.rs b/crates/doiget-core/src/lib.rs index 4e13850af..dbfdf1c3e 100644 --- a/crates/doiget-core/src/lib.rs +++ b/crates/doiget-core/src/lib.rs @@ -705,7 +705,11 @@ pub enum ErrorCode { /// message lists the candidate matches. Wire form: `"AMBIGUOUS"`. /// Raised by `doiget search`'s name-filter resolution (ADR-0031 D5). Ambiguous, - /// Filesystem write failed. + /// The local store could not serve the request: a filesystem write + /// failed, or a mutating tool was asked to change an entry that has not + /// been fetched. Deliberately not [`Self::NotFound`] in the second case -- + /// that code says a metadata source reported the id does not exist, and a + /// caller acting on it would treat a perfectly good reference as dead. StoreError, /// Provenance log write failed; the fetch was aborted. LogError, diff --git a/crates/doiget-core/src/refs.rs b/crates/doiget-core/src/refs.rs index 3133bad2a..765f9d174 100644 --- a/crates/doiget-core/src/refs.rs +++ b/crates/doiget-core/src/refs.rs @@ -615,6 +615,25 @@ mod tests { } } + /// PMCID is the other half of the check and was written without a test. + /// Both the direct field and the Zotero `note` form. + #[test] + fn a_csl_entry_identified_only_by_a_pmcid_is_read_as_pmcid() { + for doc in [ + r#"[{"id":"S","type":"article-journal","PMCID":"PMC1234567"}]"#, + r#"[{"id":"S","type":"article-journal","pmcid":"PMC1234567"}]"#, + r#"[{"id":"S","type":"article-journal","note":"PMCID: PMC1234567"}]"#, + ] { + match &parse_csl_json(doc)[0] { + Err(ParseError::UnsupportedIdentifier { kind, value, .. }) => { + assert_eq!(*kind, "PMCID", "for {doc}"); + assert_eq!(value, "PMC1234567", "for {doc}"); + } + other => panic!("expected UnsupportedIdentifier for {doc}, got {other:?}"), + } + } + } + /// A numeric PMID must not be coerced into anything, and a DOI alongside /// one still wins: the check runs only after every supported identifier /// has been tried. diff --git a/crates/doiget-mcp/src/lib.rs b/crates/doiget-mcp/src/lib.rs index 051822cb0..86be2518c 100644 --- a/crates/doiget-mcp/src/lib.rs +++ b/crates/doiget-mcp/src/lib.rs @@ -846,10 +846,19 @@ impl Server { let batch_outcome = core_batch_fetch(&parsed, &self.profile, &ctx, &store, &store_root).await; - let session_ok = batch_outcome - .as_ref() - .map(|b| b.results.iter().all(|r| r.outcome.is_ok())) - .unwrap_or(false); + // #507, third and fourth surfaces. `.is_ok()` on a `BatchResultEntry` + // calls a Blocked PDF leg a success, exactly as the single-ref tool + // did before this release fixed it -- and the first pass at that fix + // stopped at `doiget_fetch_paper`, leaving the two batch tools writing + // `result: ok` for a call where every entry was refused. One boundary, + // `FetchPaperOutcome::is_clean_success`, for all four. + let session_ok = batch_outcome.as_ref().is_ok_and(|b| { + b.results.iter().all(|r| { + r.outcome + .as_ref() + .is_ok_and(FetchPaperOutcome::is_clean_success) + }) + }); let _ = ctx.log.append(RowInput { event: LogEvent::SessionEnd, @@ -1126,11 +1135,14 @@ impl Server { let entry_keys: Vec> = to_fetch.iter().map(|(_, k)| k.clone()).collect(); let batch_outcome = core_batch_fetch(&refs, &self.profile, &ctx, &store, &store_root).await; - let session_ok = batch_outcome - .as_ref() - .map(|b| b.results.iter().all(|r| r.outcome.is_ok())) - .unwrap_or(false) - && parse_errors.is_empty(); + // Same boundary as the sibling batch tool above (#507). + let session_ok = batch_outcome.as_ref().is_ok_and(|b| { + b.results.iter().all(|r| { + r.outcome + .as_ref() + .is_ok_and(FetchPaperOutcome::is_clean_success) + }) + }) && parse_errors.is_empty(); let _ = ctx.log.append(RowInput { event: LogEvent::SessionEnd, @@ -1500,7 +1512,7 @@ impl Server { /// #281 "read" step; ADR-0032). Fetches the ar5iv LaTeXML-XHTML /// rendering of an **arXiv** paper and returns it as sectioned plain /// text. The PDF blob is never opened (ADR-0032 D1). Tier-1 OA, - /// always-on. A bare DOI returns `NO_OA_AVAILABLE` (DOI→arXiv linking + /// always-on. A bare DOI returns `NOT_IMPLEMENTED` (DOI→arXiv linking /// is #281 item 5). #[tool( description = "WHEN TO USE: Read a paper's full text (arXiv only) without an external pdf-to-text tool — the 'read' step after discovery/fetch.\n\ @@ -1508,7 +1520,7 @@ impl Server { OUTPUTS: { ok: true, arxiv_id, source: \"ar5iv\", title, sections: [{ heading, text }], char_count, truncated, retrieved_from } OR { ok:false, error }.\n\ COSTS: 1 ar5iv HTTP request (HTML), then parse; large papers can be sizeable — use max_chars to bound.\n\ SIDE EFFECTS: Emits an OA provenance row. NEVER opens the PDF blob; NEVER writes the store.\n\ - LIMITS: arXiv only (a DOI → NO_OA_AVAILABLE; pass the arXiv id). A paper not converted by ar5iv → NOT_FOUND. Best-effort extraction (truncation flagged on `truncated`).", + LIMITS: arXiv only (a DOI → NOT_IMPLEMENTED; pass the arXiv id). A paper not converted by ar5iv → NOT_FOUND. Best-effort extraction (truncation flagged on `truncated`).", annotations( read_only_hint = true, destructive_hint = false, @@ -1521,7 +1533,7 @@ impl Server { Parameters(input): Parameters, ) -> Result { // Validate the ref. A DOI has no full-text source in this slice; - // report NO_OA_AVAILABLE rather than silently failing (ADR-0032 D5). + // report the absence rather than silently failing (ADR-0032 D5). let id = match doiget_core::Ref::parse(&input.ref_) { Ok(doiget_core::Ref::Arxiv(a)) => a, Ok(doiget_core::Ref::Doi(_)) => { @@ -1638,7 +1650,7 @@ impl Server { OUTPUTS: { ok: true, arxiv_id, main_file, tex_source, char_count, truncated, retrieved_from } OR { ok:false, error }.\n\ COSTS: 1 arXiv source API request (gzip'd tar download); large papers can be sizeable — use max_chars to bound.\n\ SIDE EFFECTS: Emits an OA provenance row. NEVER writes the store; NEVER opens a PDF blob.\n\ - LIMITS: arXiv only (a DOI → NO_OA_AVAILABLE; pass the arXiv id). PDF-only submissions → TEXT_UNAVAILABLE.", + LIMITS: arXiv only (a DOI → NOT_IMPLEMENTED; pass the arXiv id). PDF-only submissions → TEXT_UNAVAILABLE.", annotations( read_only_hint = true, destructive_hint = false, @@ -2459,8 +2471,24 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, + // NOT `NOT_FOUND`. docs/ERRORS.md defines that as "a + // metadata source authoritatively reported the id does not + // exist ... doiget verify treats it as a definite dead + // reference", so an agent reading it would conclude the DOI + // is retracted or mistyped when the truth is that nobody has + // fetched it yet. `doiget_info` / `doiget_paper_pdf_path` / + // `doiget_search_local` all decline to call a store miss + // NOT_FOUND for exactly this reason; they answer ok:true with + // a null payload. These two mutate an entry, so they cannot, + // but they must not make the stronger claim either. + // + // `STORE_ERROR` is the closest fit in the closed set and its + // disposition -- `needs_config`, "will not change by itself; + // a named change makes it" -- is exactly right: the entry + // appears when you fetch it, which the message says. The code + // is approximate and the closed set has a real gap here. "error": error_object( - ErrorCode::NotFound, + ErrorCode::StoreError, format!("no store entry for {}; fetch the paper first", input.ref_), ), }))); @@ -2479,8 +2507,11 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, + // Same reasoning as the store-miss arm above: the entry + // exists and is incomplete, which is not "the id does not + // exist". "error": error_object( - ErrorCode::NotFound, + ErrorCode::StoreError, format!("entry {} has no [doiget] table; fetch it first", input.ref_), ), }))); @@ -2583,8 +2614,24 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, + // NOT `NOT_FOUND`. docs/ERRORS.md defines that as "a + // metadata source authoritatively reported the id does not + // exist ... doiget verify treats it as a definite dead + // reference", so an agent reading it would conclude the DOI + // is retracted or mistyped when the truth is that nobody has + // fetched it yet. `doiget_info` / `doiget_paper_pdf_path` / + // `doiget_search_local` all decline to call a store miss + // NOT_FOUND for exactly this reason; they answer ok:true with + // a null payload. These two mutate an entry, so they cannot, + // but they must not make the stronger claim either. + // + // `STORE_ERROR` is the closest fit in the closed set and its + // disposition -- `needs_config`, "will not change by itself; + // a named change makes it" -- is exactly right: the entry + // appears when you fetch it, which the message says. The code + // is approximate and the closed set has a real gap here. "error": error_object( - ErrorCode::NotFound, + ErrorCode::StoreError, format!("no store entry for {}; fetch the paper first", input.ref_), ), }))); @@ -2603,8 +2650,11 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, + // Same reasoning as the store-miss arm above: the entry + // exists and is incomplete, which is not "the id does not + // exist". "error": error_object( - ErrorCode::NotFound, + ErrorCode::StoreError, format!("entry {} has no [doiget] table; fetch it first", input.ref_), ), }))); @@ -2861,7 +2911,7 @@ pub struct PaperSearchInput { #[schemars(deny_unknown_fields)] pub struct PaperTextInput { /// arXiv id (e.g. "arxiv:2401.12345"), validated via `Ref::parse`. A - /// bare DOI returns `NO_OA_AVAILABLE` (pass the arXiv id; + /// bare DOI returns `NOT_IMPLEMENTED` (pass the arXiv id; /// DOI→arXiv linking is #281 item 5). #[serde(rename = "ref")] #[schemars(rename = "ref")] @@ -2878,7 +2928,7 @@ pub struct PaperTextInput { #[schemars(deny_unknown_fields)] pub struct PaperTexSourceInput { /// arXiv id (e.g. "arxiv:2401.12345"), validated via `Ref::parse`. A - /// bare DOI returns `NO_OA_AVAILABLE` (pass the arXiv id). + /// bare DOI returns `NOT_IMPLEMENTED` (pass the arXiv id). #[serde(rename = "ref")] #[schemars(rename = "ref")] pub ref_: String, @@ -4292,6 +4342,15 @@ impl ServerHandler for Server { // Helpers // --------------------------------------------------------------------------- +/// Whether a `DOIGET_STORE_ROOT` value is usable as a path: non-empty and not +/// an unexpanded `${...}` placeholder. A Desktop-Extension config left blank +/// passes the literal `${user_config.store_root}`, which must never become a +/// filesystem path (it produced `os error 5` access-denied). See #369. +fn store_root_env_is_usable(value: &str) -> bool { + let v = value.trim(); + !v.is_empty() && !v.contains("${") +} + /// Resolve the on-disk store root using the same precedence the CLI /// applies (`docs/CONFIG.md` §4): /// @@ -4309,15 +4368,6 @@ impl ServerHandler for Server { /// `doiget-cli -> doiget-mcp` wiring established by this PR and pull /// `clap` etc. into the MCP crate. Lifting this helper into `doiget-core` /// is a viable Phase-3 follow-up but is out of scope for this foundation. -/// Whether a `DOIGET_STORE_ROOT` value is usable as a path: non-empty and not -/// an unexpanded `${...}` placeholder. A Desktop-Extension config left blank -/// passes the literal `${user_config.store_root}`, which must never become a -/// filesystem path (it produced `os error 5` access-denied). See #369. -fn store_root_env_is_usable(value: &str) -> bool { - let v = value.trim(); - !v.is_empty() && !v.contains("${") -} - fn resolve_store_root() -> Option { if let Ok(s) = std::env::var("DOIGET_STORE_ROOT") { if store_root_env_is_usable(&s) { @@ -4528,6 +4578,22 @@ mod tests { "NOT_FOUND", "terminal", ), + // The replaced comment names three variants the wildcard swallowed; + // the first pass at this test drove one of them. + ( + FetchError::NotFound { + hint: "no Crossref record".into(), + }, + "NOT_FOUND", + "terminal", + ), + ( + FetchError::Ambiguous { + hint: "3 authors matched".into(), + }, + "AMBIGUOUS", + "terminal", + ), ]; for (err, want_code, want_disposition) in cases { diff --git a/crates/doiget-mcp/tests/error_envelope_shape.rs b/crates/doiget-mcp/tests/error_envelope_shape.rs index d962ef568..601c42e77 100644 --- a/crates/doiget-mcp/tests/error_envelope_shape.rs +++ b/crates/doiget-mcp/tests/error_envelope_shape.rs @@ -17,24 +17,52 @@ use camino::Utf8PathBuf; -/// Tools still permitted to answer with a bare string in `error`. -/// -/// EMPTY, deliberately. An entry here is a promise to a caller that it cannot -/// branch on the failure, so adding one needs a reason in `docs/ERRORS.md` -/// §3 rather than a line here. -const KNOWN_BARE_STRING_TOOLS: &[&str] = &[]; - fn router_src() -> Utf8PathBuf { Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs") } -/// The accepted right-hand sides for an `"error":` key. +/// Every place the router names an `error` field, in BOTH forms it uses. +/// +/// Two forms exist, and the first version of this file knew about one: +/// +/// ```text +/// json!({ ..., "error": }) // matched +/// map.insert("error".into(), ) // INVISIBLE +/// ``` +/// +/// `fetch_paper_error_envelope` uses the second, and it backs the +/// `INVALID_REF` / `STORE_ERROR` / `LOG_ERROR` / `INTERNAL_ERROR` arms of +/// `doiget_fetch_paper` -- among the most reachable failures in the crate. +/// So the guard whose docstring called the exempt set EMPTY could not see the +/// busiest envelope builder in the file. Found by review. +fn error_field_value(line: &str) -> Option<&str> { + for key in [ + "\"error\":", + "insert(\"error\".into(),", + "insert(\"error\".to_string(),", + ] { + if let Some((_, rest)) = line.split_once(key) { + return Some(rest); + } + } + None +} + +/// The accepted right-hand sides for an `error` field. /// /// `error_object(..)` is the builder; `error_obj` is the `serde_json::Map` -/// the five hand-assembled envelopes fill in and insert. Both are objects. +/// the hand-assembled envelopes fill in and insert. Both are objects. fn is_structured(value: &str) -> bool { let v = value.trim_start(); - v.starts_with("error_object(") || v.starts_with("error_obj") + if v.starts_with("error_object(") { + return true; + } + // Exact identifier, not a prefix. `v.starts_with("error_obj")` also + // accepted `error_obj_msg`, so a bare-string regression could be waved + // through by naming the local variable carefully -- the guard defeated by + // spelling, which is the failure it exists to prevent. + v.strip_prefix("error_obj") + .is_some_and(|rest| !rest.starts_with(|c: char| c.is_alphanumeric() || c == '_')) } #[test] @@ -48,7 +76,7 @@ fn every_bare_string_error_site_is_a_known_one() { if trimmed.starts_with("//") { continue; } - let Some(rest) = trimmed.split_once("\"error\":").map(|(_, r)| r) else { + let Some(rest) = error_field_value(trimmed) else { continue; }; // A key with its value on the next line is written `"error":` alone; @@ -63,36 +91,32 @@ fn every_bare_string_error_site_is_a_known_one() { } } - assert!( - bare.is_empty() || !KNOWN_BARE_STRING_TOOLS.is_empty(), - "an `ok:false` envelope answers with a bare string instead of \ - `error_object(..)`, so the caller has no `code` to branch on and no \ - `disposition` to decide a retry from (ADR-0055). Either build the \ - object, or add the tool to KNOWN_BARE_STRING_TOOLS with a reason in \ - docs/ERRORS.md §3:\n {}", - bare.join("\n ") - ); + // One assertion, not two. The first version kept a + // `KNOWN_BARE_STRING_TOOLS` exemption list and told a failing + // contributor to add their tool to it -- advice that could not work, + // because the second assertion checked `bare.is_empty()` + // unconditionally and never consulted the list. An escape hatch that + // does not open is worse than none: it sends the next person down a + // path ending in rewriting the test anyway. assert!( bare.is_empty(), - "docs/ERRORS.md §3 says this set can shrink but not grow; these are \ - not in KNOWN_BARE_STRING_TOOLS:\n {}", - bare.join("\n ") + "an ok:false envelope answers with a bare string instead of error_object(..), so the caller has no code to branch on and no disposition to decide a retry from (ADR-0055). Build the object; if a tool genuinely cannot, say so in docs/ERRORS.md and change this test deliberately: + {}", + bare.join(" + ") ); } -/// The exemption list is a liability, not a feature: if it is empty the -/// document must not still be telling readers four tools are exempt. +/// The document must not still tell readers that four tools are exempt when +/// none are. This is the half of the original pairing that was doing real +/// work -- a doc describing a state the code left behind is the historical +/// defect -- and it needs no exemption list to detect it. #[test] -fn the_exemption_list_and_the_document_agree() { +fn the_document_does_not_claim_exemptions_that_no_longer_exist() { let errors_md = Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../docs/ERRORS.md"); let doc = std::fs::read_to_string(&errors_md).expect("docs/ERRORS.md is readable"); - let doc_claims_exemptions = doc.contains("do not yet emit that object at all"); - assert_eq!( - doc_claims_exemptions, - !KNOWN_BARE_STRING_TOOLS.is_empty(), - "docs/ERRORS.md §3 and KNOWN_BARE_STRING_TOOLS disagree about whether \ - any tool still answers with a bare string (doc says {doc_claims_exemptions}, \ - list has {} entries)", - KNOWN_BARE_STRING_TOOLS.len() + assert!( + !doc.contains("do not yet emit that object at all"), + "docs/ERRORS.md section 3 still says some tools answer with a bare string in error, but every_bare_string_error_site_is_a_known_one finds none. Update the document, or this guard is vouching for prose nobody checked." ); } diff --git a/crates/doiget-mcp/tests/route_coverage_e2e.rs b/crates/doiget-mcp/tests/route_coverage_e2e.rs index 0c1982aea..8ee8d9898 100644 --- a/crates/doiget-mcp/tests/route_coverage_e2e.rs +++ b/crates/doiget-mcp/tests/route_coverage_e2e.rs @@ -45,6 +45,18 @@ enum Coverage { By { file: &'static str, test_fn: &'static str, + /// The Cargo feature the covering test is `#[cfg]`-gated behind, if + /// any. `None` means it compiles in the default `oa-only` surface. + /// + /// This exists because the checks below read the test file as TEXT. + /// Text cannot tell "this function is compiled into the binary" from + /// "this function's source is present in the repository", so a + /// feature-gated test was accepted as unconditional coverage -- and + /// the two REQUIRED CI jobs run `--features oa-only`, where that + /// function does not exist. The registry vouched for a route with no + /// coverage in the builds that gate merges: this file's own stated + /// failure class, inside this file. + feature: Option<&'static str>, }, /// Not asserted yet, and why. Deliberately not `None`: the reason is the /// difference between a known gap and an oversight. @@ -61,6 +73,7 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_arxiv_happy_path_writes_pdf_and_returns_envelope", + feature: None, }, ), ( @@ -68,6 +81,7 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_doi_with_no_oa_anywhere_reports_the_no_oa_url_route", + feature: None, }, ), ( @@ -75,6 +89,7 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_doi_blocked_pdf_includes_suggested_arxiv_id", + feature: None, }, ), ( @@ -82,6 +97,7 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_doi_falls_back_to_the_arxiv_preprint", + feature: None, }, ), ( @@ -93,6 +109,8 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_doi_served_by_the_publisher_reports_the_tdm_fetched_route", + // Only the `test (tdm features)` CI job compiles this. + feature: Some("tdm-aps"), }, ), ]; @@ -120,7 +138,12 @@ fn tests_dir() -> Utf8PathBuf { fn every_claimed_covering_test_exists_and_asserts_its_route() { let mut problems = Vec::new(); for (route, cov) in ROUTES { - let Coverage::By { file, test_fn } = cov else { + let Coverage::By { + file, + test_fn, + feature, + } = cov + else { continue; }; let path = tests_dir().join(file); @@ -150,7 +173,30 @@ fn every_claimed_covering_test_exists_and_asserts_its_route() { .lines() .map(str::trim_start) .filter(|l| l.starts_with("#[")) - .any(|l| l.starts_with("#[ignore")); + // `#[ignore` catches the plain attribute; `ignore)` catches + // `#[cfg_attr(, ignore)]`, which cargo skips just as + // completely and which the first version of this check waved + // through -- it only compared the start of the line, so a + // conditionally-ignored test could be claimed as coverage. + // rustfmt leaves `cfg_attr` on one line, so `cargo fmt` does not + // rescue us here the way it does for `#[test] #[ignore]`. + .any(|l| l.starts_with("#[ignore") || l.contains("ignore)")); + // The registry's feature claim must match the source. A test the + // default build does not compile is not unconditional coverage, and + // saying so here is the only place a reader learns it: `cargo test` + // under `--features oa-only` cannot report a function it never built. + let attrs = &src[attrs_from..line_start]; + let src_feature = attrs.lines().map(str::trim_start).find_map(|l| { + let rest = l.strip_prefix("#[cfg(feature = \"")?; + rest.split('"').next() + }); + if src_feature != *feature { + problems.push(format!( + "{route}: registry says feature={feature:?} but `{test_fn}` is gated on {src_feature:?}. A feature-gated test is coverage only in a CI job that enables it" + )); + continue; + } + if is_ignored { problems.push(format!( "{route}: `{test_fn}` is #[ignore]d, so CI never runs it -- that is a Gap with a reason, not coverage" diff --git a/docs/ERRORS.md b/docs/ERRORS.md index 8a1c2a568..39ed6c3fb 100644 --- a/docs/ERRORS.md +++ b/docs/ERRORS.md @@ -41,7 +41,7 @@ Wire form (JSON / MCP): `"INVALID_REF"`, `"NO_OA_AVAILABLE"`, etc. | `NETWORK_ERROR` | Transport / DNS / TLS failure. **Does NOT cover a deliberate supply-chain policy block** — see §6.1: an off-allowlist / redirect-denied / insecure-scheme OA-PDF leg is `CAPABILITY_DENIED`, not `NETWORK_ERROR`. | `retry_after` | Retry usually fine. | | `NOT_FOUND` | Metadata source authoritatively reported the id does not exist: HTTP `404` / `410` / `451`, or a source-specific absence (arXiv returns HTTP 200 with an empty `` for an unknown id). Network-independent and reproducible — distinct from the transient `NETWORK_ERROR` / `RATE_LIMITED`. `doiget verify` treats it as a definite dead reference (`absent`). For a DOI it is emitted only when all configured sources (Crossref, Unpaywall) fail to resolve it; a DataCite-only DOI may thus be reported `NOT_FOUND`. | `terminal` | No (the id is wrong or retracted). | | `AMBIGUOUS` | A name filter (`--author` / `--venue` / `--publisher`) matched several entities with no clear winner; the error lists the candidates. Distinct from `NOT_FOUND` ("matched nothing"). CLI exit `2`. | `terminal` | Yes — narrow the name (add a first name / fuller title) or pass an exact id. | -| `STORE_ERROR` | Filesystem write failed (disk, permission, etc.). | `needs_config` | Depends on cause. | +| `STORE_ERROR` | The local store could not serve the request: a filesystem write failed (disk, permission, etc.), or the entry a mutating tool needs is not there yet (`doiget_tag` / `doiget_annotate` on a ref nobody has fetched). Deliberately NOT `NOT_FOUND` in that second case: the id is fine, and `NOT_FOUND` would tell a caller the reference is dead. The read-only tools (`doiget_info`, `doiget_paper_pdf_path`, `doiget_search_local`) report a store miss as `ok: true` with a null payload instead, which is preferable where the operation has nothing to mutate. | `needs_config` | Depends on cause; for a store miss, fetch the paper first. | | `LOG_ERROR` | Provenance log write failed. **Fetch is aborted.** | `needs_config` | Free disk / fix perms. | | `CAPABILITY_DENIED` | Source not in `CapabilityProfile`. | `needs_config` | User opts in, or pick different source. | | `FETCH_TIMEOUT` | Per-request timeout exceeded. | `retry_after` | Retry. | diff --git a/docs/MCP_TOOLS.md b/docs/MCP_TOOLS.md index 5d8b04ab4..5c4e14b2f 100644 --- a/docs/MCP_TOOLS.md +++ b/docs/MCP_TOOLS.md @@ -17,7 +17,7 @@ speaks **stdio only** ([ADR-0001](DECISIONS/), [`SCOPE.md`](SCOPE.md) §non-goal | `doiget_info` | Retrieve a store entry's metadata. | | `doiget_search_local` | Search store metadata (title / authors / venue). | | `doiget_paper_search` | External literature discovery over OpenAlex (`/works?search=`); abstract-bearing candidates for triage. Tier-1 OA metadata, always-on; **never fetches a PDF** (ADR-0031). | -| `doiget_paper_text` | Extract an **arXiv** paper's full text from ar5iv as sectioned plain text (`ref`, optional `max_chars`). Tier-1 OA, always-on; **never opens the PDF blob** (ADR-0032). A DOI → `NO_OA_AVAILABLE`. | +| `doiget_paper_text` | Extract an **arXiv** paper's full text from ar5iv as sectioned plain text (`ref`, optional `max_chars`). Tier-1 OA, always-on; **never opens the PDF blob** (ADR-0032). A DOI → `NOT_IMPLEMENTED` (terminal: the tool is arXiv-only and DOI→arXiv linking is #281 item 5, not a config knob). | | `doiget_link` | Resolve a **DOI** to its arXiv preprint + identity cluster (`{ doi, arxiv, openalex_id, title }`) over OpenAlex, for reading or dedup (#281 item 5). Tier-1 OA, always-on; **never fetches a PDF**. arXiv → DOI is a follow-up; a non-DOI ref → `INVALID_REF`. | | `doiget_list_recent` | Last N fetched entries. | | `doiget_paper_pdf_path` | Return the local path of a cached PDF. **Does not read, parse, or transmit content.** | diff --git a/justfile b/justfile index abfc70497..55265dde8 100644 --- a/justfile +++ b/justfile @@ -41,15 +41,21 @@ test-slow: # project-scoped `.mcp.json`, so this shadows it for you alone and leaves the # file (and everyone else's) unchanged. # +# The name MUST match `.mcp.json`'s (`doiget`). Local scope only shadows a +# project-scoped server of the SAME name -- registering `doiget-dev` alongside +# it leaves BOTH connected, so the release-pinned tools stay callable and the +# problem this recipe exists for is not solved. Verified by review. +# # `just mcp-dev-off` removes it again. mcp-dev: cargo build --no-default-features --features oa-only -p doiget-cli - claude mcp remove doiget-dev --scope local || true - claude mcp add doiget-dev --scope local -- cargo run --quiet --no-default-features --features oa-only -p doiget-cli -- serve + claude mcp remove doiget --scope local || true + claude mcp add doiget --scope local -- cargo run --quiet --no-default-features --features oa-only -p doiget-cli -- serve -# Drop the local-scoped dev server registered by `just mcp-dev`. +# Drop the local-scoped dev server registered by `just mcp-dev`; the +# project-scoped `.mcp.json` entry becomes visible again. mcp-dev-off: - claude mcp remove doiget-dev --scope local + claude mcp remove doiget --scope local # Mirrors CI `msrv` job (build of the published surface). build: diff --git a/site/content/developer/errors.md b/site/content/developer/errors.md index b53263c66..3005514ed 100644 --- a/site/content/developer/errors.md +++ b/site/content/developer/errors.md @@ -47,7 +47,7 @@ Wire form (JSON / MCP): `"INVALID_REF"`, `"NO_OA_AVAILABLE"`, etc. | `NETWORK_ERROR` | Transport / DNS / TLS failure. **Does NOT cover a deliberate supply-chain policy block** — see §6.1: an off-allowlist / redirect-denied / insecure-scheme OA-PDF leg is `CAPABILITY_DENIED`, not `NETWORK_ERROR`. | `retry_after` | Retry usually fine. | | `NOT_FOUND` | Metadata source authoritatively reported the id does not exist: HTTP `404` / `410` / `451`, or a source-specific absence (arXiv returns HTTP 200 with an empty `` for an unknown id). Network-independent and reproducible — distinct from the transient `NETWORK_ERROR` / `RATE_LIMITED`. `doiget verify` treats it as a definite dead reference (`absent`). For a DOI it is emitted only when all configured sources (Crossref, Unpaywall) fail to resolve it; a DataCite-only DOI may thus be reported `NOT_FOUND`. | `terminal` | No (the id is wrong or retracted). | | `AMBIGUOUS` | A name filter (`--author` / `--venue` / `--publisher`) matched several entities with no clear winner; the error lists the candidates. Distinct from `NOT_FOUND` ("matched nothing"). CLI exit `2`. | `terminal` | Yes — narrow the name (add a first name / fuller title) or pass an exact id. | -| `STORE_ERROR` | Filesystem write failed (disk, permission, etc.). | `needs_config` | Depends on cause. | +| `STORE_ERROR` | The local store could not serve the request: a filesystem write failed (disk, permission, etc.), or the entry a mutating tool needs is not there yet (`doiget_tag` / `doiget_annotate` on a ref nobody has fetched). Deliberately NOT `NOT_FOUND` in that second case: the id is fine, and `NOT_FOUND` would tell a caller the reference is dead. The read-only tools (`doiget_info`, `doiget_paper_pdf_path`, `doiget_search_local`) report a store miss as `ok: true` with a null payload instead, which is preferable where the operation has nothing to mutate. | `needs_config` | Depends on cause; for a store miss, fetch the paper first. | | `LOG_ERROR` | Provenance log write failed. **Fetch is aborted.** | `needs_config` | Free disk / fix perms. | | `CAPABILITY_DENIED` | Source not in `CapabilityProfile`. | `needs_config` | User opts in, or pick different source. | | `FETCH_TIMEOUT` | Per-request timeout exceeded. | `retry_after` | Retry. | diff --git a/site/content/developer/mcp-tools.md b/site/content/developer/mcp-tools.md index 3cf8d30b5..eb49ad8d4 100644 --- a/site/content/developer/mcp-tools.md +++ b/site/content/developer/mcp-tools.md @@ -23,7 +23,7 @@ speaks **stdio only** ([ADR-0001](DECISIONS/), [`SCOPE.md`](SCOPE.md) §non-goal | `doiget_info` | Retrieve a store entry's metadata. | | `doiget_search_local` | Search store metadata (title / authors / venue). | | `doiget_paper_search` | External literature discovery over OpenAlex (`/works?search=`); abstract-bearing candidates for triage. Tier-1 OA metadata, always-on; **never fetches a PDF** (ADR-0031). | -| `doiget_paper_text` | Extract an **arXiv** paper's full text from ar5iv as sectioned plain text (`ref`, optional `max_chars`). Tier-1 OA, always-on; **never opens the PDF blob** (ADR-0032). A DOI → `NO_OA_AVAILABLE`. | +| `doiget_paper_text` | Extract an **arXiv** paper's full text from ar5iv as sectioned plain text (`ref`, optional `max_chars`). Tier-1 OA, always-on; **never opens the PDF blob** (ADR-0032). A DOI → `NOT_IMPLEMENTED` (terminal: the tool is arXiv-only and DOI→arXiv linking is #281 item 5, not a config knob). | | `doiget_link` | Resolve a **DOI** to its arXiv preprint + identity cluster (`{ doi, arxiv, openalex_id, title }`) over OpenAlex, for reading or dedup (#281 item 5). Tier-1 OA, always-on; **never fetches a PDF**. arXiv → DOI is a follow-up; a non-DOI ref → `INVALID_REF`. | | `doiget_list_recent` | Last N fetched entries. | | `doiget_paper_pdf_path` | Return the local path of a cached PDF. **Does not read, parse, or transmit content.** | From 9ee9baa87a03c0f148f8a9890222482c6cc36d47 Mon Sep 17 00:00:00 2001 From: Sota Shimozono Date: Tue, 1 Sep 2026 14:36:32 +0900 Subject: [PATCH 5/5] test: cover the tools that had no tests at all doiget_tag and doiget_annotate had no test of any kind -- not an error path, not even a success path. That is how a wrong code got into them: this release gave their failure arms a structured error object for the first time and chose NOT_FOUND for a store miss, which docs/ERRORS.md defines as a metadata source authoritatively reporting the id does not exist. Nothing in the suite could notice, and the review had to find it by reading. Four e2e tests over the real MCP transport: a store miss is not a dead reference (and stays needs_config), a malformed ref stays INVALID_REF so the two remain distinguishable, and a request-shape failure carries an object rather than a bare string. Also covered, both flagged by the same review as changed-but-unasserted: * doiget_paper_tex_source given a DOI. Its sibling doiget_paper_text had a test for the identical arm and this one had none, so the pair could drift the moment either was edited. * the zero-result hint on the CLI search surface (#534), through the real external_envelope: present on a long empty result, absent on a short one, absent when there were results. 922 tests on oa-only, 1039 with the tdm features. Signed-off-by: Sota Shimozono --- crates/doiget-cli/src/commands/search.rs | 46 ++++ crates/doiget-mcp/tests/paper_text_e2e.rs | 29 +++ crates/doiget-mcp/tests/tag_annotate_e2e.rs | 255 ++++++++++++++++++++ 3 files changed, 330 insertions(+) create mode 100644 crates/doiget-mcp/tests/tag_annotate_e2e.rs diff --git a/crates/doiget-cli/src/commands/search.rs b/crates/doiget-cli/src/commands/search.rs index 4c1fb4c8f..1d1aa6240 100644 --- a/crates/doiget-cli/src/commands/search.rs +++ b/crates/doiget-cli/src/commands/search.rs @@ -380,6 +380,52 @@ mod tests { assert_eq!(v["results"][0]["abstract"], "abs"); } + /// #534 on THIS surface. The fix landed on the MCP tool first, so + /// `doiget search --mode json` went on emitting the exact envelope the + /// issue was filed about -- `ok: true`, `total_results: 0`, nothing to + /// tell a script that the query, not the literature, was the problem. + /// + /// Driven through the real `external_envelope`, the function `--mode json` + /// prints. + #[test] + fn a_long_query_that_matched_nothing_carries_the_hint() { + let results = PaperSearchResults { + results: vec![], + total_results: Some(0), + }; + let q = "lithium refractoriness after discontinuation kindling sensitization course of illness Post"; + let v = external_envelope(q, &results); + assert_eq!(v["ok"], true, "still a success envelope: {v}"); + assert_eq!(v["count"], 0); + let hint = v["hint"].as_str().unwrap_or_default(); + assert!(hint.contains("10 terms"), "names the count: {hint:?}"); + assert!(hint.contains("3-5"), "says what to do instead: {hint:?}"); + } + + /// A short query matching nothing may genuinely mean nothing is indexed, + /// and a hint on every empty result would train readers to skip it. + #[test] + fn a_short_query_that_matched_nothing_is_left_alone() { + let results = PaperSearchResults { + results: vec![], + total_results: Some(0), + }; + let v = external_envelope("depersonalization derealization", &results); + assert!(v.get("hint").is_none(), "no hint on a short query: {v}"); + } + + /// And a query that DID match carries no hint, however long it is. + #[test] + fn a_long_query_with_results_carries_no_hint() { + let results = PaperSearchResults { + results: vec![hit()], + total_results: Some(1), + }; + let q = "a b c d e f g h i j k l"; + let v = external_envelope(q, &results); + assert!(v.get("hint").is_none(), "results present: {v}"); + } + #[test] fn sort_arg_lowers_to_core() { // Relevance is the only sort (#290); `cited` / `recent` were removed. diff --git a/crates/doiget-mcp/tests/paper_text_e2e.rs b/crates/doiget-mcp/tests/paper_text_e2e.rs index b0c7f8eb2..8d378e8c6 100644 --- a/crates/doiget-mcp/tests/paper_text_e2e.rs +++ b/crates/doiget-mcp/tests/paper_text_e2e.rs @@ -258,3 +258,32 @@ async fn paper_text_unconverted_paper_maps_to_text_unavailable() -> anyhow::Resu drop(td); Ok(()) } + +/// The sibling tool got the same re-code and had no test of its own, so the +/// two could have drifted the moment one was edited -- which is the failure +/// this release is about. +#[tokio::test] +#[serial_test::serial] +async fn paper_tex_source_doi_is_terminal_not_a_config_problem() -> anyhow::Result<()> { + let env = EnvGuard::new(ENV_KEYS); + + let (client, server_handle) = boot_in_memory_server().await?; + + let mut args = serde_json::Map::new(); + args.insert("ref".to_string(), serde_json::json!("10.1234/example")); + let result = client + .peer() + .call_tool(CallToolRequestParams::new("doiget_paper_tex_source").with_arguments(args)) + .await?; + let s = result.structured_content.as_ref().expect("structured"); + + assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); + assert_eq!(s["error"]["code"], serde_json::json!("NOT_IMPLEMENTED")); + assert_eq!(s["error"]["disposition"], serde_json::json!("terminal")); + assert_eq!(s["ref"], serde_json::json!("10.1234/example")); + + client.cancel().await?; + server_handle.await??; + drop(env); + Ok(()) +} diff --git a/crates/doiget-mcp/tests/tag_annotate_e2e.rs b/crates/doiget-mcp/tests/tag_annotate_e2e.rs new file mode 100644 index 000000000..2e8dcb31e --- /dev/null +++ b/crates/doiget-mcp/tests/tag_annotate_e2e.rs @@ -0,0 +1,255 @@ +//! E2E coverage for `doiget_tag` and `doiget_annotate`. +//! +//! These two tools had **no test of any kind** — not an error path, not even a +//! success path. That is how a wrong error code got into them and out again: +//! this release gave every one of their failure arms a structured `error` +//! object for the first time, and picked `NOT_FOUND` for "the ref is not in +//! the store yet". `docs/ERRORS.md` defines that code as *a metadata source +//! authoritatively reported the id does not exist*, which `doiget verify` +//! treats as a definite dead reference — so the tools would have told an agent +//! a perfectly good DOI was retracted. Nothing in the suite could notice. +//! +//! Every assertion below drives the real MCP tool over a real transport. + +#![allow(clippy::expect_used, clippy::unwrap_used, clippy::panic)] + +use doiget_core::CapabilityProfile; +use doiget_mcp::Server; +use rmcp::{model::CallToolRequestParams, ServiceExt}; + +struct EnvGuard { + keys: Vec<&'static str>, +} + +impl EnvGuard { + fn new(keys: &[&'static str]) -> Self { + for k in keys { + std::env::remove_var(k); + } + Self { + keys: keys.to_vec(), + } + } + fn set(&self, key: &str, val: &str) { + std::env::set_var(key, val); + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + for k in &self.keys { + std::env::remove_var(k); + } + } +} + +const ENV_KEYS: &[&str] = &["DOIGET_STORE_ROOT", "DOIGET_LOG_PATH"]; + +async fn boot_in_memory_server() -> anyhow::Result<( + rmcp::service::RunningService, + tokio::task::JoinHandle>, +)> { + let profile = CapabilityProfile::from_env().expect("clean env never errors"); + let server = Server::new(profile); + let (server_transport, client_transport) = tokio::io::duplex(64 * 1024); + let server_handle = tokio::spawn(async move { + let service = server.serve(server_transport).await?; + service.waiting().await?; + anyhow::Ok(()) + }); + let client = ().serve(client_transport).await?; + Ok((client, server_handle)) +} + +fn args(pairs: &[(&str, serde_json::Value)]) -> serde_json::Map { + let mut m = serde_json::Map::new(); + for (k, v) in pairs { + m.insert((*k).to_string(), v.clone()); + } + m +} + +/// A ref nobody has fetched is not a dead reference. +/// +/// The code must not be `NOT_FOUND`: `docs/ERRORS.md` reserves that for "a +/// metadata source authoritatively reported the id does not exist", and an +/// agent acting on it would drop a citation that is fine. The remedy here is +/// an action the caller can take, which is what `needs_config` means. +#[tokio::test] +#[serial_test::serial] +async fn tag_on_an_unfetched_ref_does_not_call_it_a_dead_reference() -> anyhow::Result<()> { + let td = tempfile::TempDir::new().expect("tempdir"); + let root = camino::Utf8Path::from_path(td.path()).expect("utf-8 tempdir"); + + let env = EnvGuard::new(ENV_KEYS); + env.set("DOIGET_STORE_ROOT", root.join("papers").as_str()); + env.set("DOIGET_LOG_PATH", root.join("log.jsonl").as_str()); + + let (client, server_handle) = boot_in_memory_server().await?; + let result = client + .peer() + .call_tool( + CallToolRequestParams::new("doiget_tag").with_arguments(args(&[ + ("ref", serde_json::json!("10.1234/never-fetched")), + ("add", serde_json::json!(["to-read"])), + ])), + ) + .await?; + let s = result.structured_content.as_ref().expect("structured"); + + assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); + assert_ne!( + s["error"]["code"], + serde_json::json!("NOT_FOUND"), + "a ref that has not been fetched is not a dead reference: {s:?}" + ); + assert_eq!( + s["error"]["code"], + serde_json::json!("STORE_ERROR"), + "{s:?}" + ); + assert_eq!( + s["error"]["disposition"], + serde_json::json!("needs_config"), + "the remedy is a named action (fetch it), not a retry: {s:?}" + ); + let message = s["error"]["message"].as_str().unwrap_or_default(); + assert!( + message.contains("fetch"), + "the message names the action: {message:?}" + ); + + client.cancel().await?; + server_handle.await??; + drop(env); + drop(td); + Ok(()) +} + +/// Same rule on the sibling tool, which had the same code and the same +/// absence of tests. +#[tokio::test] +#[serial_test::serial] +async fn annotate_on_an_unfetched_ref_does_not_call_it_a_dead_reference() -> anyhow::Result<()> { + let td = tempfile::TempDir::new().expect("tempdir"); + let root = camino::Utf8Path::from_path(td.path()).expect("utf-8 tempdir"); + + let env = EnvGuard::new(ENV_KEYS); + env.set("DOIGET_STORE_ROOT", root.join("papers").as_str()); + env.set("DOIGET_LOG_PATH", root.join("log.jsonl").as_str()); + + let (client, server_handle) = boot_in_memory_server().await?; + let result = client + .peer() + .call_tool( + CallToolRequestParams::new("doiget_annotate").with_arguments(args(&[ + ("ref", serde_json::json!("10.1234/never-fetched")), + ("text", serde_json::json!("read the appendix")), + ])), + ) + .await?; + let s = result.structured_content.as_ref().expect("structured"); + + assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); + assert_ne!(s["error"]["code"], serde_json::json!("NOT_FOUND"), "{s:?}"); + assert_eq!( + s["error"]["code"], + serde_json::json!("STORE_ERROR"), + "{s:?}" + ); + assert_eq!( + s["error"]["disposition"], + serde_json::json!("needs_config"), + "{s:?}" + ); + + client.cancel().await?; + server_handle.await??; + drop(env); + drop(td); + Ok(()) +} + +/// A malformed ref is the caller's input problem, and it must stay +/// distinguishable from the store miss above — same tool, two situations, two +/// codes. ADR-0055 exists so an agent can branch here. +#[tokio::test] +#[serial_test::serial] +async fn tag_on_a_malformed_ref_is_invalid_ref_not_a_store_problem() -> anyhow::Result<()> { + let td = tempfile::TempDir::new().expect("tempdir"); + let root = camino::Utf8Path::from_path(td.path()).expect("utf-8 tempdir"); + + let env = EnvGuard::new(ENV_KEYS); + env.set("DOIGET_STORE_ROOT", root.join("papers").as_str()); + env.set("DOIGET_LOG_PATH", root.join("log.jsonl").as_str()); + + let (client, server_handle) = boot_in_memory_server().await?; + let result = client + .peer() + .call_tool( + CallToolRequestParams::new("doiget_tag").with_arguments(args(&[ + ("ref", serde_json::json!("not-a-doi")), + ("add", serde_json::json!(["x"])), + ])), + ) + .await?; + let s = result.structured_content.as_ref().expect("structured"); + + assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); + assert_eq!( + s["error"]["code"], + serde_json::json!("INVALID_REF"), + "{s:?}" + ); + assert_eq!( + s["error"]["disposition"], + serde_json::json!("terminal"), + "{s:?}" + ); + + client.cancel().await?; + server_handle.await??; + drop(env); + drop(td); + Ok(()) +} + +/// `doiget_annotate` with neither `text` nor `clear` is a request-shape +/// failure. Before this release it answered with a bare string in `error`, so +/// a caller had no code to branch on at all. +#[tokio::test] +#[serial_test::serial] +async fn annotate_with_no_text_and_no_clear_says_so_in_a_structured_error() -> anyhow::Result<()> { + let td = tempfile::TempDir::new().expect("tempdir"); + let root = camino::Utf8Path::from_path(td.path()).expect("utf-8 tempdir"); + + let env = EnvGuard::new(ENV_KEYS); + env.set("DOIGET_STORE_ROOT", root.join("papers").as_str()); + env.set("DOIGET_LOG_PATH", root.join("log.jsonl").as_str()); + + let (client, server_handle) = boot_in_memory_server().await?; + let result = client + .peer() + .call_tool( + CallToolRequestParams::new("doiget_annotate") + .with_arguments(args(&[("ref", serde_json::json!("10.1234/never-fetched"))])), + ) + .await?; + let s = result.structured_content.as_ref().expect("structured"); + + assert_eq!(s["ok"], serde_json::json!(false), "envelope: {s:?}"); + assert!( + s["error"].is_object(), + "an ok:false envelope carries an error OBJECT, not a bare string (ADR-0055): {s:?}" + ); + assert!( + s["error"]["disposition"].is_string(), + "every failure envelope carries a disposition: {s:?}" + ); + + client.cancel().await?; + server_handle.await??; + drop(env); + drop(td); + Ok(()) +}