Promotion: next → main (0.8.13-beta.17, no cut yet) - #580
Merged
Merged
Conversation
v0.8.12's release job ended red:
npm error You cannot publish over the previously published versions: 0.8.12
The loop globbed `./npm-stage/doiget-*`. That was safe while the wrapper
was named `doiget`, and stopped being safe the moment it was renamed to
`doiget-cli` -- which starts with `doiget-` too. So the wrapper was
published inside the loop and again on the explicit line after it.
It also sorts before `doiget-darwin-*`, so it went out ahead of the
packages its optionalDependencies pin. That is precisely the window the
comment directly above the loop warns about.
Everything had already shipped by the time it failed, which is the worst
shape a failure takes: a red job on a complete release. Anyone reading
the status would conclude npm did not publish. It did -- all five
packages are live at 0.8.12 and `npx doiget-cli --version` answers from
the real registry.
The loop now reads the platform list from `stage-npm.sh`'s MAP. That
list contains platform packages only, so the wrapper is excluded by
construction rather than by a name test somebody has to remember to
update.
`stage-npm.test.sh` bans the glob outright -- it is the thing that is
wrong, so it is checked as a shape rather than as a duplicate count --
and separately asserts no package appears twice. Mutation-checked both
ways: restoring the glob fails the check with the right message.
The same trap was caught in posture-lint's `find -name 'doiget-*'` and
excluded there, in the same PR that did the rename (#549). Two
`doiget-*` patterns needed the fix; one got it.
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
fix: the npm publish job published the wrapper twice
back-merge: main → next after 0.8.12
…d first `.mcp.json` ran a bare `doiget serve`, so installing the plugin did nothing for anyone who had not already installed doiget by some other route -- which is most people finding it through a plugin marketplace. It now runs `npx -y doiget-cli serve` and needs nothing beforehand: npm fetches the wrapper and the one matching platform binary on first run. That was the stated reason for keeping this as a self-hosted marketplace rather than submitting to the Anthropic plugin directory -- a listing whose first run fails for everyone without the binary on PATH is worse than no listing. The objection is gone, so the submission is viable. Verified by launching it rather than by reading it: `npx -y doiget-cli serve` from the real registry answers `initialize` as `doiget 0.8.12` and lists 22 tools, pure JSON-RPC on stdout, zero bytes on stderr. Two other things this turned up. The plugin manifests said 0.8.11 while the repository was two releases ahead. Nothing stamps them -- neither file is referenced by any script or workflow -- so they drift silently, and `/plugin marketplace add` reads the default branch, so that stale number is what users saw. Set to 0.8.12, the last released version. And two more references the `doiget` -> `doiget-cli` rename left behind. README's plugin paragraph promised `npx -y doiget serve`. The 0.8.11 release notes lead with `npx -y doiget serve` and `npm i -g doiget`, neither of which was ever valid: the wrapper is not called `doiget`, and that release's npm publish failed outright, so there was nothing on the registry to install either way. The 0.8.11 entry is annotated rather than rewritten -- that failure is the point of the surrounding text. That makes three places the rename missed. The same trap was caught and excluded in posture-lint's `find -name doiget-*` in the rename PR itself; the release workflow's publish glob and these two were not. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
feat: the Claude Code plugin no longer requires doiget to be installed first
… to the router `doiget_batch_from_bibliography`, `doiget_paper_tex_source`, `doiget_tag` and `doiget_annotate` are in every release and have no row in MCP_TOOLS.md. That document is what an integrator reads to decide what the server can do, and docs/INTEGRATION/*.md points at it, so a tool missing from it is discoverable only by calling `tools/list` and reading JSON Schemas -- the work the document exists to save. Two of the four came in with the tags work (#294) and one with the TeX source work. The pattern is "the tool landed, the reference page did not". Nothing compared the two, and the drift ran in BOTH directions at the same time: four tools with no row here, and one row with no tool (#552, the citation graph missing from every release binary). Neither is visible from inside the other file, which is why both survived. posture-lint now checks it. rmcp derives a tool's name from its `#[tool]` method name, so the method list is the router's own truth; the table rows are the document's. Mutation-checked both ways: deleting a row and inventing one each fail with the right message. Descriptions are taken from each tool's own `WHEN TO USE:` text rather than written fresh, so the row and the schema say the same thing. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
docs: four tools shipped undocumented, and nothing compared the table to the router
OpenAlex free-text matching degrades sharply past roughly eight terms and returns NOTHING 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 looking. In the session behind #534 that happened eleven times in a row. A known 1992 Am J Psychiatry paper was written off as unavailable and the search abandoned -- until an unrelated three-term query surfaced the same paper immediately. A zero-result search is a SUCCESS envelope, which is why neither open issue covered it: #506 is about error dispositions, #505 is scoped to the CLI fetch path. The gap was success-with-zero-results on the MCP search path. The envelope now carries a `hint` naming the submitted term count and what to retry with, at the exact point an agent would otherwise conclude absence. The tool description says the same thing, so the advice also reaches an agent that reads schemas and not envelopes. Short queries get nothing. A two-term search returning zero really may mean the work is not indexed, and a hint on every empty result would teach readers to skip the hint. Tested through the real tool over the wiremock harness, not only as a unit: the exact ten-term query from the report gets a hint naming "10 terms", and a two-term query gets none. Mutation-checked -- disabling the guard fails the first and leaves the second passing, which is the right shape. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
…-hint Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp> # Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml
fix(mcp): a zero-result search read as "this paper does not exist"
95% of the workspace suite's time was a single test. `batch_above_window_size_fetches_every_ref`: 627 s out of 663 s, with the next-slowest at 36 s and the other 45 suites under 10 s each. It is not waste. The test fetches MCP_BATCH_MAX_SIZE + 2 arXiv refs. `SOURCE_RATE_OVERRIDES` puts 3 s between arXiv requests because arXiv's Terms of Use do, and one attempt issues TWO requests -- the Atom feed then the PDF, both paced since #493. 102 * 2 * 3 s = 612 s, against 609 s measured. The rate limit is a legal safeguard; `RateLimits`' fields are `pub(crate)` precisely so tests cannot weaken it, and it is untouched here. What was wasteful was paying it FIVE times -- three operating systems and two feature sets -- for a dispatch-loop property that varies with neither. Roughly 50 minutes per CI run to learn the same thing five times. Now `#[ignore]`d, with a `test (slow)` job running it once via `--ignored`. Exactly one ignored test exists workspace-wide, so the split drops nothing and duplicates nothing -- verified with `--ignored --list`. It runs in parallel rather than lengthening any other job. The broad suite went from 627 s to 18 s locally. The test's own comment claimed "~20 s", counting only the global 5-per-second cap. That was true the day it was written (2026-06-16, 200 ms x 102). `2cc32ab` on 2026-08-25 -- "fetch arXiv at the rate its Terms of Use publish" -- made it 15x slower, and nobody noticed because the only symptom is CI minutes. Corrected in place, with the arithmetic and both things it had missed. Sharding was the obvious idea and would not have helped: one test is 95% of the suite and it is sleeping, not computing, so every shard but one would finish immediately and the last would still take ten minutes, having paid N compiles for it. `tokio::time::pause()` was the other candidate. Not attempted -- `http.rs` already records why: wiremock serves over real localhost IO and paused time auto-advances past reqwest's timeout. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
perf(ci): one test took ten minutes and was being run five times
`doiget_metadata_only` and `doiget_resolve_paper` advertised an `oa_url` "for the caller to act on separately". It is null for every DOI. The DOI path is Crossref-first because Crossref's `message.link[]` was believed to supply an OA URL without a second request. #517 measured twelve live entries and eight captured fixtures: not one was general-purpose. Every entry was scoped to a licensed programme -- Similarity Check, TDM, syndication -- and following one outside that programme would be taking a licensed route without the licence. So the extractor refuses all of them, correctly, and the field it was meant to fill stays empty while its documentation tells agents to act on it. An agent reading `oa_url: null` cannot tell it from "this work has no free copy". Now opt-in. `include_oa_location: true` consults Unpaywall. Off by default, so the default path stays one round-trip and nobody pays for a location they will not use. No guarantee is weakened: Unpaywall is a metadata source and the URL is still reported, never followed. `oa_status` carries which answer you got. "closed" means the lookup completed and there is no OA location; null means it did not complete. A failed Unpaywall call therefore leaves BOTH fields null rather than inventing a status -- asserting "closed" on the strength of a 500 would recreate the exact ambiguity being fixed. The Crossref metadata is still returned; an optional extra failing does not sink the resolve. This mirrors the `oa_status` + `pdf.status` pairing `doiget_fetch_paper` already uses. The resolver cache keys on the options, not just the ref, so a default entry is never served to a caller that asked for the location. By SUBDIRECTORY, not a `.oa` filename suffix: `Ref::safekey` keeps `.`, so a suffix collides the DOI `10.1234/foo.oa` resolved by default with `10.1234/foo` resolved with the flag, on one file. I wrote that suffix first, with a comment asserting it was safe because safekey percent-encodes `.`. It does not -- lib.rs:623 lists `.` in the allowed set. The test now guards it: reverting to the suffix fails that test and nothing else. `doiget-core` gains `MetadataOnlyOptions` plus `*_with_options` variants of `metadata_only`, `resolve_only` and `metadata_only_to_store`. The existing three delegate with defaults, so nothing downstream breaks. The options type is `#[non_exhaustive]` with a `with_oa_location` builder, so a future knob is not a breaking change either. Tested at the MCP boundary over wiremock with a REALISTIC Crossref fixture. The existing one carries `intended-application: "unspecified"`, which the #517 measurement found in zero of twenty entries; the new one is a Similarity Check link, correctly refused. The default-path test mounts Unpaywall with `.expect(0)`, so a regression that starts paying for a second round-trip fails on server drop rather than silently doubling everyone's cost. Mutation-checked: ignoring the flag fails the two opt-in tests and leaves the default one passing. Also folds a duplicate `### Changed` heading inside the Unreleased section, a merge artefact. Closes #539 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
crate-ci/typos reads it as 'option'. Renamed to 'mcp-resolve-opt-in.jsonl'. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
`[workspace.dependencies]` pins doiget-core / doiget-cli / doiget-mcp by version as well as by path. Bumping only `[workspace.package].version` left them requiring `^0.8.13-beta.5`, which no longer exists, so `cargo metadata` against the published registry could not resolve. Local builds did not notice, because the path entries satisfy the dependency without consulting the version. `version-check` did, which is what it is for. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
`doiget fetch 10.1002/pcn5.205` -- Harada & Kato 2024, gold OA, cc-by -- was refused with `redirect target doi.org not in allowlist`, and offered `doi.org` / `*.doi.org` as hosts to add. This is not a missing entry. It is a gate at the wrong layer. Unpaywall reports `best_oa_location.url` for this work as literally `https://doi.org/10.1002/pcn5.205`, with no `url_for_pdf` (verified against the live API, 2026-08-30). That is the normal shape for publisher-hosted gold OA, not an edge case. So the first host the fetch leg touches is the DOI resolver, and it was being adjudicated as though it were where the bytes come from -- refusing the chain one hop before `onlinelibrary.wiley.com`, whose `*.wiley.com` was already on the list. The remediation was worse than the refusal. Adding `doi.org` does not widen the trusted surface toward one publisher; it removes the bound entirely, because every DOI in existence resolves through it. An agent following that advice gets its PDF and silently loses the invariant ADR-0027 exists to hold. Now: a closed set -- `doi.org`, `dx.doi.org`, `hdl.handle.net`, each measured issuing a single 302 straight to the publisher -- is transparent. Followed, but never allowlisted, never named as remediation, never counted as the source of the content. The host that actually serves the response is adjudicated exactly as before, so this is transparent to the invariant rather than an exception to it. Exact match, no wildcards. `*.doi.org` would sweep in `www.doi.org`, which is the DOI Foundation's website and not a resolver; `evil-doi.org` and `doi.org.evil.test` are what an attacker registers. Rejected the issue's first suggestion -- adjudicate only the terminal host -- as a larger change than it looks: a chain could then traverse any host at all so long as it ended somewhere allowed, and every hop still sees the request. A named, closed set keeps the bound. This does NOT manufacture access. The paper now reaches Wiley and meets Wiley's own cookie wall; it fails honestly at the publisher instead of dishonestly at the addressing layer. ADR-0039 already decided the adjacent case -- do not add publisher hosts -- and the same principle decides this one the other way, because a resolver is not a content host at all. There were FIVE gates asking this question, and only one of them had ever been walked end to end, which is #462's point exactly: "every 'unreachable source' bug passed its unit tests." The fifth was a test-local copy of the production redirect policy, which had already drifted into testing something that does not ship. They now share one predicate, `SourceAllowlist::permits`; `matches` stays the narrower "is this host on the list" used to build and assert the lists, so a resolver is never reported inside any source's `expected_hosts`. A posture-lint step fails any gate that calls `matches` on a host variable. Tested against the real production allowlists rather than fixtures: `doi.org` is permitted but not matched and not present in `redirect_hosts`; `onlinelibrary.wiley.com` was trusted all along; `evil.example.com` is still refused; look-alikes are not resolvers; and every Tier-1 source treats a resolver hop as addressing. ADR-0053 records the decision. Closes #533 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
fix(mcp): oa_url was documented as actionable and was always null (#539)
`fetched ... (metadata-only: no OA PDF available)` was byte-identical whether the six optional sources were on and had nothing, or off and never asked. It also exits 0, so unlike the blocked path there was no `error[...]` block for the #413 trace to hang on -- which is why this, the one outcome that reads as a RESULT rather than a failure, was the one path with no diagnostics at all. That is where the silence misleads most. With the default profile the sentence means "three of eleven sources had nothing", and it does not say so. A narrow default is the right design; a narrow default that does not say it is narrow is a different thing. Now it prints the trace the blocked and NOT_FOUND paths already print, plus the command that widens the search -- the line to paste, not prose about it: = suggest: to widen the search: DOIGET_ENABLE_HAL=1 doiget fetch 10.1137/0117004 The variables come from the `Disabled` rows themselves, not a second registry, so the advice cannot drift into claiming a source was skipped when it was consulted, or naming a switch the chain does not read. Those rows have carried their own env vars since #470; nothing needed inventing. A switch that is ALREADY SET is reported as a build problem instead. `resolve_metadata_flag` returns false when the variable is set but the Cargo feature was not compiled in -- it warns through `tracing`, which nobody sees, and the source reports `Disabled` naming a variable the user set an hour ago. Printing "set DOIGET_ENABLE_X" at them would be the same species of unhelpful as the bare `no OA PDF available` this change is about, so it names the real blocker: the build. Part 3 of #505 -- ranking which source is most likely to hold the paper -- is deliberately NOT here. The issue argues a wrong ranking is worse than none because it makes people stop early, and that it must be an ordering of the full list that names the signal it ranked on. That is its own change, and #505 stays open for it. One test caught during review: `widening_env_is_deduped_and_in_chain_order` first landed in `mod attempt_denial_tests`, which is feature-gated, so it compiled and silently did not run. Moved into `mod tests`, where it does. Refs #505 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
Two CI failures, both mine and both avoidable: - `is_transparent_resolver` is public and linked the private `TRANSPARENT_RESOLVER_HOSTS`. Same class as the one caught on #564; I ran clippy on this branch but not `cargo doc -D warnings`. - crate-ci/typos reads `lookalikes` as `alike`/`likes`. Renamed the test to `resolver_impostor_hosts_are_not_transparent`. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
…sing fix(fetch): a DOI resolver is addressing, not a content host (#533)
feat(cli): the found-nothing fetch says what it consulted (#505 parts 1-2)
reclassified it
A source saying "I found it and cannot give it to you" returned
`FetchError::SourceSchema` with an explanatory hint, and
`classify_attempt` decided what the trace row said by reading that hint
back:
hint.contains("not open access")
|| hint.contains("openAccess")
|| hint.contains("no retrievable PDF")
Match and the row read "consulted: found, not open access". Miss and it
read "consulted: failed" -- which tells an operator the source BROKE,
rather than that the paper is not free there. Only one of those is a bug
to chase.
It had already fired. #503 reworded Europe PMC's refusal from "is
indexed but not open access" to "advertises no retrievable PDF", because
the gate moved from OA-subset membership to per-entry retrievability.
Nothing about the reword was wrong; the hint fell out of the predicate
and every Europe PMC refusal silently became `Failed`. Nothing in
`europepmc.rs` said the wording was load-bearing. `hal` matched on
`openAccess` -- a JSON FIELD NAME, not prose anyone chose. And it was
latent for any source added later, whose author had no way to learn that
the phrasing of an error message decides how the row renders.
Now `FetchError::NotRetrievable { source_key, detail }`, matched as a
variant. `is_access_refusal` is deleted. `detail` is still carried
verbatim into the row, because the reason is for a reader -- it is just
no longer parsed.
The compiler immediately found a second exhaustive match over
`FetchError` that the substring approach had no way to flag. That is the
argument for the change in one line.
It collapses to the EXISTING `NO_OA_AVAILABLE`, not a new code. #538
asked whether the closed set had to widen; it does not. "Found it, no
free copy" is what that code already means, and adding one would have
split a single situation across two wire values for an internal
refactor. `docs/ERRORS.md` §2's DESCRIPTION widens to match -- it said
"Tier 1 sources reported no OA URL" -- which is the NORMATIVE change
ADR-0014 requires an ADR for. ADR-0054 records it.
Underneath was a second defect. `SourceSchema` collapses to
`INTERNAL_ERROR`, so a paper simply not being free at one repository
could be reported as a bug in doiget. It no longer is.
Not a `DenialContext`: ADR-0023's channel is for policy refusals, where
there is a capability to grant or an allowlist to widen. No
configuration makes a closed paper open, and offering a denial_context
would send a reader after a fix that does not exist.
Three tests asserted `matches!(err, FetchError::SourceSchema { .. })`
and now assert the category plus the collapsed `ErrorCode`, which is a
stronger claim. A fourth asserted the PHRASE "no retrievable PDF"; it
now asserts the criterion the detail names (`documentStyle = pdf`,
`availabilityCode`), because asserting the phrase would have rebuilt
inside the test exactly the coupling this removes from the classifier.
Closes #538
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
fix(core): an access refusal was decided by substring, so rewording one reclassified it (#538)
`docs/ERRORS.md` §2 has carried per-code retry guidance since Phase 0 and
it is good guidance. None of it reached the wire:
grep -riE 'retryable|do_not_retry|permanent|transient' crates/doiget-mcp -> 0
grep -riE 'retry' the tool descriptions -> 0
The failure envelope was `{ok:false, error:{code, message,
denial_context?}}`, so an agent's only signal was the NAME of the code --
and several names point the wrong way. `NO_OA_AVAILABLE` is the most
common failure there is, and both its name and its ERRORS.md row ("Try
later, or enable opt-in source") read to a machine as WAIT when it is
nearly always CONFIGURE. That invites an unbounded retry loop over
something that will not change on its own.
Now `error.disposition`, in three states:
terminal the answer will not change; do not retry
retry_after it may change on its own; retry with backoff
needs_config it will not change by itself, but a named change
makes it -- surface that, do not loop
Two states cannot express the third, and the third is the one that
matters most here. `terminal` also covers failures a caller can act on
with a DIFFERENT request (`INVALID_REF`, `AMBIGUOUS`,
`TEXT_UNAVAILABLE`): this call is settled, which is what a disposition
is about. `STORE_ERROR` / `LOG_ERROR` are `needs_config` -- a machine
cannot name the fix for a full disk, but it must not loop on one, and
"surface this to a human" is exactly that state.
Derived in one place: `ErrorCode::disposition`, an exhaustive match with
no wildcard, so a new code must decide. Built in one place:
`error_object`, because a field present on some failures and absent on
others is worse than no field -- it teaches the reader to fall back to
guessing from the code's name, which is the habit this replaces. Ten
literal error objects now route through it.
`docs/ERRORS.md` §2 gains a Disposition column and a test parses the
SHIPPED document and asserts every row against the function. #506 asked
for exactly this ("generated from it or asserted against it in a test --
otherwise the doc and the wire drift, which is the #493 pattern");
generating would have cost the per-code prose, which is the useful part.
The test also asserts it saw exactly 15 rows, because a parser that
silently matches nothing passes every time. Mutation-checked both ways.
Stated in the MCP server `instructions`, delivered on `initialize` to
every client, so an agent that has never opened ERRORS.md still meets
the contract -- and it names the specific trap rather than the general
rule.
ADR-0055 records it.
Three parts of #506 are deliberately NOT here, and it stays open:
* `error.retry_after_ms`. `Retry-After` is parsed today but consumed
INSIDE the retry loop and discarded; by the time an error surfaces
the retries are exhausted and no honest number remains. A plausible
default would be indistinguishable from a measured one.
* `remediation` on `ok:false`. ADR-0043's channel keys on
`DenialContext`, present on only some failures -- carrying it for
those alone would reproduce the sometimes-present problem this
change rejects for `disposition`.
* `rate_limit_budget` on live responses, which is independent of the
retry contract.
Refs #506
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
ERRORS.md conflicted where both changes touched the same table: #538 widened NO_OA_AVAILABLE's Meaning, this branch added the Disposition column. Both kept. The DECISIONS index keeps ADR-0054 and ADR-0055. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
feat(mcp): a failure says what to do about it, not just what happened (#506)
coincidence
Two calls from one session, minutes apart, arriving in the same shape:
score 1.0 Coryell 1998 -> the paper asked for
score 0.5 Latalova 2010 -> a different paper, different journal,
different author
`quality`, `life`, `bipolar` and `2010` were enough to clear the bar.
Both came back as a single-element `candidates` array with identical
structure, and nothing in the envelope said which was which.
A bare float is not judgement material. For `score` to work as a gate,
the consumer has to already know that the scorer is token overlap and
not semantic similarity; that 0.5 is the FLOOR, so the worst candidate
the tool can ever emit still looks like a positive number; and that for
a citation carrying author + title + journal + volume + year, 0.5 means
most of it did not match. None of that was on the wire, and an agent
consuming a ranked list takes the head of it.
Candidates now carry:
confidence exact every query token was found
probable at least four query tokens in five
weak cleared the floor and no more
matched which of the query's tokens were found
`matched` is the evidence, and in the reported case it is the whole
story: not the author, not the journal.
This is the half of #372 that was never specified. #372's remedy --
return the top candidate with its score rather than an empty list, "so
the calling agent can judge" -- is in place and is correct. What the
agent judges WITH was missing. #242's floor stays exactly where it is;
this changes nothing about which candidates are returned.
The bands are over token overlap, not a semantic verdict, and the docs
say so: `exact` means every token matched, which is a strong signal and
still not proof. Both citation tool descriptions now tell an agent to
branch on `confidence`, not `score`, and to verify a `weak` candidate
with `doiget_resolve_paper` before citing -- because the tool
description is what an agent reads when it has never opened
MCP_TOOLS.md.
`Exact` compares the score against 0.999 rather than 1.0: the score is a
division, and banding an all-tokens match as `probable` on a rounding
accident would be this same defect in miniature.
doiget-core API: `ResolvedCandidate` gains two fields and becomes
`#[non_exhaustive]`, matching `MetadataOnlyOutcome` / `AttemptOutcome` /
`FetchError`, so the next field is not another break.
Closes #536
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
fix(ci): restore the citation-feature fix that auto-merge orphaned (#580)
ADR-0033's strict beta.N+1 means every PR that lands forces a re-bump and a fresh CI cycle on every other open PR. Three separate dependency PRs would have cost three of those rounds for work that is one kind of work, so #578, #579 and #581 are done together here: flate2 1.1.9 -> 1.1.10, uuid 1.25.0 -> 1.26.0 (#578) quick-xml 0.41.0 -> 0.42.0 (#579) codeql-action, install-action, typos CI actions (#581) quick-xml 0.42 does not build as a bump. The reader is UTF-8 throughout, so `QName::as_ref()` and `Attribute::key` yield `&str` instead of `&[u8]`, and `BytesText::decode()` is gone because `BytesText` derefs to `str` -- 13 compile errors across the two XML parsers. Both are migrated: `local_name` takes and returns `&str` (`rfind(':')` for `rposition`), XML-name comparisons drop their `b` prefixes, and the decode-then- unescape dance collapses to `quick_xml::escape::unescape(&t)`. The PDF magic-byte literals in the tests stay bytes; they are not XML names. `name_bytes` became `name`, because it has not been bytes since `e.name()` changed type. The existing parser tests cover the changed paths rather than just the compile: `parse_atom_feed_captures_published_doi_and_journal_ref` feeds an `<arxiv:doi>` element through the prefix stripping, and `parse_extracts_title_sections_and_inline_math` goes through the `alttext` attribute lookup. supply-chain: flate2 and uuid roll forward, quick-xml 0.42.0 replaces 0.41.0 which nothing uses any more, and miniz_oxide 0.9.1 and zlib-rs 0.6.7 are new entries. zlib-rs is worth naming rather than waving through -- it is a dependency this tree did not have, arriving because flate2 1.1.10's `rust_backend` now routes to it. Exempted at safe-to-deploy like every other unaudited crate here, which says something about our process and nothing about the crate. Verified locally at oa-only and oa-only,citation: clippy -D warnings clean, doiget-core suite green. Closes #578 Closes #579 Closes #581 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
…ntifier Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp> # Conflicts: # CHANGELOG.md # Cargo.lock # Cargo.toml
`cargo vet` refused the store outright -- "a file in the store is not correctly formatted" -- because I filed zlib-rs after zmij. It sorts before it: 'l' < 'm'. Reordered by `cargo vet fmt` rather than by hand, and `cargo vet --locked` now succeeds (82 audited, 239 exempted). Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
chore(deps): the three pending Dependabot bumps, in one pass
…ntifier Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp> # Conflicts: # Cargo.lock # Cargo.toml
STORE.md §6 lets a re-fetch rewrite the [doiget] table, and says why: "This is intentional, not silent: ... the operator always learns the entry was downgraded and why." The permission is conditional on the report. Since #539 there is a path with no report -- `metadata_only` without `include_oa_location` never runs the OA lookup, so it carries `oa_status: None` and `license: "unknown"`, those won, and nothing said so. No note: line, no pdf.status, no log row. Both are markers, not readings. The schema says as much: oa_status is "omitted when not determined" (#281), license falls back to "unknown". A paper that genuinely stops being open access reports Some("closed"); a license that changes reports the new string. So a call carrying the marker did not look, and preferring the stored value is not a guess about which is newer -- only one of the two is a value. merge_metadata's [doiget] arm now keeps the stored oa_status when the incoming one is None, and the stored license when the incoming one is LICENSE_UNDETERMINED. Everything else in the table still follows §6. "unknown" got a name so the check does not hang off a bare literal matching the others scattered through the orchestrator. Two tests: the preserve, and the other half -- a re-write that actually determined gold -> closed still wins, because a guard that suppressed real news would be worse than the bug. Mutation-checked: with the arm reverted the first fails on `left: None, right: Some("gold")`. The issue as filed said `oa_url` was overwritten with null. It is not. I wrote that from `build_metadata_only_metadata` and stopped before `FsStore::write`, which reads the existing file and merges; `merge_opt!(url)` already protected it. Probed before writing any of this, and #583 is corrected. Only the two [doiget] fields were ever affected, so the fix is a merge rule, not the store partition the issue proposed. Nothing BiblioFetch.jl reads changes shape: no field added, removed or renamed, schema_version unmoved, reserved top-level fields untouched. Refs #583 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
…s lost
codecov/patch was red on this PR and it was right: nothing drove the
three arms this change adds. `refs.rs` tests the parser; the JSONL
placeholder, the verify row and the MCP envelope -- the things a user
actually reads -- were asserted nowhere.
Looking at them found the reason to care. The same sentence had been
hand-copied to `verify.rs` and `doiget-mcp`, and both copies had been
wrapped across source lines and re-joined with the indentation still in
them:
"...which doiget cannot resolve yet..."
That is the defect the #580 review fixed elsewhere, reintroduced twice
by me in the same cycle, and it shipped because no test read the string.
So the fix is the cause, not the symptom: `unsupported_identifier_claim`
in `doiget_core::refs`, called by both sites, tested once.
`ParseError`'s own `#[error]` already carried the claim -- the copies
existed only to drop the `entry {entry_key:?}` prefix, which each caller
puts in a field of its own.
Three more of the same class turned up, all pre-existing on next and all
user-facing: two `= note:` lines in `fetch`'s widening advice, and the
`config.toml could not be read` warning in `commands/mod.rs`. Fixed.
A workspace-wide lint for the pattern is deliberately NOT here. Written,
measured, dropped: it flags `config doctor`'s aligned two-column output
and test assertion messages at every space threshold from four to ten
(12 hits, 3 of them real), and the aligned output uses the same 8+ space
runs the defect does. There is no structural discriminator, and a lint
that cries wolf is a lint someone deletes. Removing the duplication is
the half that lasts.
Refs #500
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
The shared-helper call fits on one line; I had left it wrapped the way the `format!` it replaced was. Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
fix(bib): a PubMed entry was reported as having no identifier; it has a PMID (#500)
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp> # Conflicts: # Cargo.lock # Cargo.toml
fix(store): a not-determined marker no longer overwrites a determination
Review of the 0.8.13 promotion found one defect class repeated across the release: a fix that landed on one surface and not its sibling. This closes those, corrects the claims that were wrong about the world, and strips the beta. The one that could hurt a caller: `doiget_batch_fetch` and `doiget_batch_from_bibliography` still held verbatim copies of the hand-rolled `FetchError -> ErrorCode` match that `fetch_paper_fetch_error_envelope` shed this cycle, ending in `_ => InternalError`. `disposition` is derived from that code, so a 404 on a batch entry arrived as `retry_after` and an agent was told to keep retrying a DOI that will never resolve. Half-applied fixes, now applied to both surfaces: * #500 `doiget batch` still said INVALID_REF for a PMID-only entry * #534 `doiget search --mode json` still emitted the silent zero envelope * #507 the MCP SessionEnd row said result "ok" beside an error_code * #533 `config doctor --network` still adjudicated with matches, not permits * #500 CSL-JSON had no PMID check; only BibTeX did Claims that did not match the code: * `Formula/doiget.rb` and its generator said the release workflow regenerates the formula. No workflow calls it, and CONTRIBUTING.md says so. * The Homebrew posture check said it fails on a bad checksum. It re-reads the checksums from the file under test. * docs/ERRORS.md named `every_bare_string_error_site_is_a_known_one` as the guard on its bare-string exemptions. The guard did not exist. It does now, and the set it pins is empty. * #462 was filed as the shape of #454 reachable again, and raised as a possible release blocker. It was not: the production branch of both client builders extends with `tier_3_allowlists()` and always did. The test-override branch had no Tier-3 entry, so no wiremock e2e could reach the route on either surface. Registering those keys makes the reproduction pass, so it is no longer #[ignore]d and `route_coverage_e2e` has no remaining Gap. Also: a 404 carrying Retry-After was classified NETWORK_ERROR, because #506 added `retry_after_ms: None` to the arm mapping authoritative absences. The arXiv-only text tools answered a DOI with NO_OA_AVAILABLE, whose disposition sends an agent looking for a config knob that does not exist. `.mcp.json` is pinned rather than resolving npm latest on every start, `just mcp-dev` exists so the pin does not cost dogfooding, and a posture check holds the four release-tracking files to one version. Cut: 0.8.13-beta.23 to 0.8.13. The version-bump gate is red by design on a cut (ADR-0025); it goes green on the next to main promotion this enables. Refs #500, #506, #507, #533, #534, #538, #462, #580 Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
The cut changed [workspace.package].version and left [workspace.dependencies] declaring ^0.8.13-beta.23, so cargo could not resolve doiget-core and the release gate G2 failed. Local builds hid it: path dependencies win over the version requirement. scripts/release-version-gate.sh v0.8.13 now passes G0-G6 (G7 skips pre-tag). Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
…ry prerelease Two defects in the check added one commit ago, both found by reviewing it. An empty extraction killed the step at the assignment, under set -euo pipefail, before the ::error:: written to explain it -- so deleting the doiget-cli@<version> pin from .mcp.json failed closed and silently. That is the shape fixed one file over in stage-npm.test.sh in the same PR. The prerelease guard enumerated spellings (*-beta.*|*-rc.*) and let 0.9.0-alpha.2 through. Any - suffix is a prerelease per SemVer. Probed both: an empty extraction now names the file, and beta/rc/alpha/pre/ rc1 are all rejected while 0.8.12 passes. Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
A six-agent review of #588 found the PR reproducing the defect it exists to fix. Every item below is mine, from the previous two commits. Fixes that landed on one surface and not its sibling -- the exact criticism this release makes of the code it changes: * doiget_paper_text / doiget_paper_tex_source were re-coded to NOT_IMPLEMENTED and the three CLI siblings (text, source, tex-source) were left on NO_OA_AVAILABLE, so one binary answered the same question two ways. The tools' own description strings, their input-schema field docs and docs/MCP_TOOLS.md also still named the old code. * doiget_fetch_paper got FetchPaperOutcome::is_clean_success and the two batch tools kept .all(|r| r.outcome.is_ok()), calling a batch where every entry was refused a success. A fourth site in the CLI had hand-rolled the same predicate. Guards that did not guard: * error_envelope_shape scanned for the literal "error": and missed insert("error".into(), ..) -- which is how fetch_paper_error_envelope writes it, the builder behind the INVALID_REF / STORE_ERROR / LOG_ERROR / INTERNAL_ERROR arms of doiget_fetch_paper. Proven by injecting a bare string there and watching the guard stay green. Its is_structured also accepted any identifier starting with error_obj. * Its KNOWN_BARE_STRING_TOOLS exemption told a failing contributor to add their tool to a list the second assertion never consulted. Removed. * route_coverage_e2e claimed tdm_fetched was covered by a test that is cfg-gated on tdm-aps, so in the two REQUIRED CI jobs (--features oa-only) the function is not in the binary at all. Coverage::By now records the feature and the checker cross-checks it against the source. * Its ignore detector missed cfg_attr(<cond>, ignore). Codes that made a claim the docs contradict: * doiget_tag / doiget_annotate reported a store miss as NOT_FOUND, which docs/ERRORS.md defines as "a metadata source authoritatively reported the id does not exist ... a definite dead reference". Those sites had no code at all before this PR, so the wrong claim is new. STORE_ERROR now, whose needs_config disposition fits "fetch it first", and ERRORS.md says so. And, one function over from the CHANGELOG line about a doc comment drifting onto the wrong item, resolve_store_root's doc had drifted onto store_root_env_is_usable. Also: the CONTRIBUTING release runbook gave three of four edits as comments between two real commands, so copy-pasting it produced a commit whose message claimed four files were bumped when one was; just mcp-dev registered doiget-dev, and local scope only shadows a project-scoped server of the SAME name, so both stayed connected and the pinned release remained callable; the release-sync check overclaimed what it catches; and the CSL-JSON bullet was missing its (breaking) marker, leaving five where the PR body says six. Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
doiget_tag and doiget_annotate had no test of any kind -- not an error path,
not even a success path. That is how a wrong code got into them: this release
gave their failure arms a structured error object for the first time and chose
NOT_FOUND for a store miss, which docs/ERRORS.md defines as a metadata source
authoritatively reporting the id does not exist. Nothing in the suite could
notice, and the review had to find it by reading.
Four e2e tests over the real MCP transport: a store miss is not a dead
reference (and stays needs_config), a malformed ref stays INVALID_REF so the
two remain distinguishable, and a request-shape failure carries an object
rather than a bare string.
Also covered, both flagged by the same review as changed-but-unasserted:
* doiget_paper_tex_source given a DOI. Its sibling doiget_paper_text had a
test for the identical arm and this one had none, so the pair could drift
the moment either was edited.
* the zero-result hint on the CLI search surface (#534), through the real
external_envelope: present on a long empty result, absent on a short one,
absent when there were results.
922 tests on oa-only, 1039 with the tdm features.
Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
fix: close the #580 review findings, and cut 0.8.13
added 3 commits
September 1, 2026 15:36
…ting user tags Two findings from the third review round, both in code no earlier round had reached. release-plz.yml: the npm publish loop reads its package list from stage-npm.sh through a pipeline ending in sort -u, which exits 0 on empty input. set -euo pipefail therefore protects nothing: a grep that matches nothing (a reformat of the MAP block, a delimiter change) gives an empty word list, the loop runs zero times, and the step falls through to publish the wrapper alone -- green, with every platform binary missing. npm install doiget-cli would succeed while its optionalDependencies fail to resolve. The doiget-* glob this replaced failed LOUDLY on no-match. Trading that for silence in the step that performs the irreversible publish is the wrong direction, and posture-lint guards the identical grep for the identical reason one file over. Now refuses an empty list and a count that is not 4; probed all three cases. fs_store.rs: merge_metadata protected oa_status and license under ADR-0056 and left tags, collections and annotation to the incoming side, which for a fetch is always empty -- all three orchestrator construction sites hard-code Vec::new() / None. So doiget tag X --add priority followed by any doiget fetch X discarded the tag silently. Same loss the adjacent fix closed, on the fields where it is the user's own data. The policy is an explicit UserFields parameter rather than preserve-when-empty, because tag --remove and annotate --clear legitimately mean the empty value; collapsing the two would make removal a silent no-op. Store gains write_user_authored for those four call sites; write keeps the fetch semantics. Also: the two bare grep -c assignments in posture-lint could die before their own ::error::, the same guard applied twice already in that file. Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
…l call The MCP server built both fresh inside every tool handler. Consequences, all in code no earlier review round had reached. RateLimiter's pacing state -- the rolling global window and the per-source next-allowed instants -- lives in its own Arc<Mutex<..>> fields, so it paces only the calls that share the instance. Thirteen handlers each got an empty per_source_next and no memory of the last call, so arXiv's 3 s spacing, which docs/LEGAL.md treats as an obligation rather than politeness, was not enforced between MCP calls at all. The type calls itself "process-wide"; nothing made it so. ProvenanceLog::open documents the same contract in its own words: the session_id "MUST be a 26-char ULID generated once per process", and a long-lived handle reuses it. Opening per call broke that thirteen times over, and worse: open seeds (next_seq, last_hash) by reading the file, so two overlapping calls both read the same state and both append rows claiming the same ts_seq with a prev_hash that does not match the row actually before them -- which is what audit-log --verify reports as a broken chain. Server now owns an Arc<RateLimiter> and a OnceLock<Arc<ProvenanceLog>>, opened lazily because Server::new is infallible. RateLimiter::new, Ulid::generate and ProvenanceLog::open each appear exactly once. Asserted by pointer identity, because that is the property; injecting a per-call limiter makes the test fail. Also in this batch: FetchError::Http(_) => NetworkError was a wildcard over all eight HttpError variants, and NetworkError's disposition is retry_after. Six of them cannot change on a retry: an allowlist refusal, an http:// downgrade, a size cap, a wrong content type, an unregistered source key and a malformed header. The mapping every surface routes through was giving the advice ADR-0055 exists to stop giving, and the DenialContext impl 100 lines below already matched all eight. Now exhaustive. One existing assertion changed with it: UnknownSource had been pinned to NetworkError, which recorded the wildcard rather than a decision. It is a wiring fault -- the caller asked HttpClient to fetch for a source it was never given -- and it is the error the TDM reproduction in #462 actually hit, which is part of why that read as a transport problem. resolver_cache's ten functions and openalex::describe_locations were pub with no caller outside doiget-core and no entry in docs/PUBLIC_API.md, so they were accidental semver commitments -- including the on-disk cache layout they encode. This cycle had doubled that surface by adding the _with_options half. Narrowing to pub(crate) is what revealed production calls none of the five plain wrappers; read and write have no caller anywhere, not even a test, and are deleted. pub had been keeping the dead-code lint quiet. 927 tests on oa-only, 1044 with the tdm features. Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
Two advisories from the same review round. resolver_cache used a plain std::fs::write. A reader racing it sees a half-written file, toml::from_str fails, and the entry degrades to a miss -- safe, per the module's best-effort contract, but it is a re-fetch nobody asked for and a debug! line that reads like corruption. atomic_write (tmp, fsync, rename) already existed one module over; it is now pub(crate) and both write the same way. safekey_from_metadata_filename mints a Safekey straight from a directory listing. The safety argument is that the filesystem only holds names which already passed guard_safekey at write time -- true today, and a claim about the world rather than something the type enforces, which is the shape this release keeps finding. A debug_assert catches a future write path that skips the guard in the test suite instead of in whatever reads the store next. Signed-off-by: Sota Shimozono <souta.shimozono@gmail.com>
fix: third review round — the release pipeline, the rate limiter, the audit log, and a fetch that ate your tags
github-actions Bot
added a commit
that referenced
this pull request
Sep 1, 2026
Promotion: next → main (0.8.13-beta.17, no cut yet) 505c695
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotion of everything on
nextsince 0.8.12, for review.No version cut in this PR.
nextis still0.8.13-beta.17, deliberately — this is a review surface for what has accumulated, not the release itself. The cut (strip-beta.N, curate the## [0.8.13]section, sign the tag) is a separate step per ADR-0025.Two PRs are still in flight and will be included if they land first: #575 (
retry_after_ms) and #576 (PMID reporting). Dependabot's #578/#579 are independent.The theme
Almost everything here is one defect class: a message that was accurate about what the code did and wrong about the world. Each one was found by measurement rather than by reading, and several were found by testing the thing I had just built and discovering it did nothing.
oa_url— "act on this"nullfor every DOI; Crossref'slink[]is programme-scopedno OA PDF available, exit 0, no traceok: true, total_results: 0Distribution
doiget-cli, five packages, Trusted Publishing).Formula/doiget.rb, generated from the release's own.sha256assets, never hand-edited.npx -y doiget-cli serve, so it needs nothing installed first — which is what made directory submission viable.New ADRs
NO_OA_AVAILABLECI
test (slow)split out — one test was 95% of the suite's runtime and was being run five times for a property that varies with neither OS nor feature. Roughly 50 minutes per run recovered, with the rate limit untouched (it is a legal safeguard).Four new posture-lint pins, each shaped to fail on a mistake that was actually made rather than on a hypothesis: host adjudication must use
permits; every hand-built error object carries adisposition; the Homebrew formula must be generator output; MCP_TOOLS.md must list exactly the router's tools.Things deliberately not done, with reasons on the issues
reffailure rows carryNETWORK_ERROR(retry_after, correctly let through), and the terminal outcomes write no row at all. Step 1 (record the call's outcome) is in; step 2 needs that first.Ref::Pmid—docs/SAFEKEY.mdis a shared spec with BiblioFetch.jl whose reference implementation discriminates classes asstartswith("10.") ? doi : arxiv. A PMID is neither, so doiget and BiblioFetch.jl would key the same paper differently. Needs a coordinated ADR and a change in another repository.rate_limit_budgeton live responses — what is available isRateLimits::HARD_CODED, a constant. Emitting it under the name "budget" would be bug(mcp): after #517oa_urlis effectively always null on the DOI path, while MCP_TOOLS.md still advertises it as the field to act on #539 again.One open defect this cycle surfaced
#462's route-coverage work found that
tdm_fetchedfails over MCP withno allowlist registered for source tdm-aps— #454's shape, reachable again — and that onlytdm-apsimplementsfetch_contentat all, so three of the four Tier-3 sources cannot produce the route the tier exists for. Left as an#[ignore]d reproduction rather than muted. Worth deciding before the cut whether 0.8.13 ships with it.Full detail in
CHANGELOG.mdunder## [Unreleased].