diff --git a/.github/workflows/posture-lint.yml b/.github/workflows/posture-lint.yml index 8769ec19b..017c84276 100644 --- a/.github/workflows/posture-lint.yml +++ b/.github/workflows/posture-lint.yml @@ -287,10 +287,78 @@ 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. + # + # 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 + # 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 + 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. 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 + *-*) + echo "::error::posture-lint release-sync: .mcp.json pins a prerelease ($pinned); the plugin must point at a published stable" + exit 1 + ;; + esac + # 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 # 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..ece17a414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,197 @@ 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. + +- **[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: 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. + 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] (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 + 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`, `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 + +- **[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..1920928bb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -216,28 +216,65 @@ 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 +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 Formula/doiget.rb -m "chore(homebrew): formula for 0.8.12" +git commit -s -m "chore(release): tracking files for $V" -- Formula/doiget.rb .claude-plugin .mcp.json ``` -Only for **stable** releases. Beta tags are not published to the tap. +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. 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 of the **same name** shadows the project-scoped one: + +```sh +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 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..489652f8d 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" @@ -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 = [ 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..8aa0d39b3 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 @@ -680,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/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..1d1aa6240 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 @@ -361,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-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-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/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/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..765f9d174 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,80 @@ 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:?}"), + } + } + + /// 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. + #[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..86be2518c 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, .. } => { @@ -844,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, @@ -1124,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, @@ -1478,7 +1492,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); } } @@ -1498,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\ @@ -1506,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, @@ -1519,13 +1533,22 @@ 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(_)) => { 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)", ))); @@ -1627,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, @@ -1644,7 +1667,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 +2207,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 +2296,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 +2309,7 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": e, + "error": error_object(ErrorCode::CapabilityDenied, e), }))); } }; @@ -2307,7 +2325,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 +2352,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 +2364,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 +2377,7 @@ impl Server { Err(e) => { return Ok(CallToolResult::structured(serde_json::json!({ "ok": false, - "error": e, + "error": error_object(ErrorCode::CapabilityDenied, e), }))); } }; @@ -2370,7 +2394,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 +2471,26 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("no store entry for {}; fetch the paper first", input.ref_), + // 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::StoreError, + format!("no store entry for {}; fetch the paper first", input.ref_), + ), }))); } Err(e) => { @@ -2461,7 +2507,13 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("entry {} has no [doiget] table; fetch it first", input.ref_), + // 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::StoreError, + format!("entry {} has no [doiget] table; fetch it first", input.ref_), + ), }))); } }; @@ -2562,7 +2614,26 @@ impl Server { Ok(None) => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("no store entry for {}; fetch the paper first", input.ref_), + // 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::StoreError, + format!("no store entry for {}; fetch the paper first", input.ref_), + ), }))); } Err(e) => { @@ -2579,7 +2650,13 @@ impl Server { None => { return Ok(CallToolResult::structured(json!({ "ok": false, - "error": format!("entry {} has no [doiget] table; fetch it first", input.ref_), + // 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::StoreError, + format!("entry {} has no [doiget] table; fetch it first", input.ref_), + ), }))); } }; @@ -2590,14 +2667,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", + ), }))); } @@ -2828,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")] @@ -2845,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, @@ -3231,14 +3314,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 +3775,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 +3883,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 +4111,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 +4210,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()), @@ -4232,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): /// @@ -4249,43 +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. -/// 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("${") -} - fn resolve_store_root() -> Option { if let Ok(s) = std::env::var("DOIGET_STORE_ROOT") { if store_root_env_is_usable(&s) { @@ -4460,38 +4542,111 @@ 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", + ), + // 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", + ), + ]; - /// 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..601c42e77 --- /dev/null +++ b/crates/doiget-mcp/tests/error_envelope_shape.rs @@ -0,0 +1,122 @@ +//! 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; + +fn router_src() -> Utf8PathBuf { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/lib.rs") +} + +/// 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 hand-assembled envelopes fill in and insert. Both are objects. +fn is_structured(value: &str) -> bool { + let v = value.trim_start(); + 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] +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) = error_field_value(trimmed) 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)); + } + } + + // 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(), + "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 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_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"); + 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/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..8d378e8c6 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")); @@ -249,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/route_coverage_e2e.rs b/crates/doiget-mcp/tests/route_coverage_e2e.rs index df1ece2d2..8ee8d9898 100644 --- a/crates/doiget-mcp/tests/route_coverage_e2e.rs +++ b/crates/doiget-mcp/tests/route_coverage_e2e.rs @@ -36,11 +36,27 @@ 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 { 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. @@ -57,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, }, ), ( @@ -64,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, }, ), ( @@ -71,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, }, ), ( @@ -78,12 +97,20 @@ const ROUTES: &[(&str, Coverage)] = &[ Coverage::By { file: "fetch_paper_e2e.rs", test_fn: "fetch_paper_doi_falls_back_to_the_arxiv_preprint", + feature: None, }, ), ( "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", + // Only the `test (tdm features)` CI job compiles this. + feature: Some("tdm-aps"), }, ), ]; @@ -111,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); @@ -131,7 +163,41 @@ 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("#[")) + // `#[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" )); @@ -247,7 +313,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/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(()) +} diff --git a/docs/ERRORS.md b/docs/ERRORS.md index 55aa21977..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. | @@ -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/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 ad4a52e8a..55265dde8 100644 --- a/justfile +++ b/justfile @@ -25,6 +25,38 @@ 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. +# +# 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 --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`; the +# project-scoped `.mcp.json` entry becomes visible again. +mcp-dev-off: + claude mcp remove doiget --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..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. | @@ -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`). | 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.** |