fix: close the #580 review findings, and cut 0.8.13 - #588
Merged
Merged
Conversation
added 2 commits
September 1, 2026 12:23
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>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
added 3 commits
September 1, 2026 13:07
…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>
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.
Closes the review findings on #580 and cuts 0.8.13.
nextgoes to a clean0.8.13, so #580 becomes a valid promotion PR once thislands and its
version-bumpcheck turns green.Why one PR
The review found one defect class, repeated: a fix that landed on one surface
and not its sibling. Five of the fixes below are the missing half of work
already in this release, so splitting them from the cut would ship 0.8.13 with
the halves still uneven. The cut is here for the same reason:
version-bumprequires a clean
X.Y.Zon anext -> mainPR, and #580 cannot go green untilthis lands.
The one that could hurt a caller
doiget_batch_fetchanddoiget_batch_from_bibliographystill held verbatimcopies of the hand-rolled
FetchError -> ErrorCodematch thatfetch_paper_fetch_error_envelopeshed this cycle -- the one ending in_ => InternalError, one screen below the fix.FetchErrorNotRetrievable(new in #538)NO_OA_AVAILABLE/needs_configINTERNAL_ERROR/terminalNotFoundNOT_FOUND/terminalINTERNAL_ERRORAmbiguousAMBIGUOUS/terminalINTERNAL_ERRORHttp(404|410|451)NOT_FOUND/terminalNETWORK_ERROR/retry_afterdispositionis derived from the code, so the last row told an agent to keepretrying a DOI that will never resolve -- the exact failure ADR-0055 exists to
prevent. And "an access refusal is not an internal error", the headline of #538,
held on
doiget_fetch_paperand on no other fetch surface.Half-applied fixes, now on both surfaces
doiget batchstill answeredINVALID_REFfor a PMID-only entry.The verdict was flattened into a placeholder string and handed back to
Ref::parse, which has one thing to say.verifyand the MCP tool saidNOT_IMPLEMENTEDall along.doiget_paper_searchanswers an over-long query with{ok: true, total_results: 0}, which an agent reads as "no such paper exists" #534 --doiget search --mode jsonstill emitted{"ok": true, "total_results": 0}with no hint. The repro query from theissue reproduced the envelope byte for byte.
SessionEndrow saidresult: "ok"beside a non-nullerror_codefor a blocked leg. Only theerror_codehalf of the CLI rule wasported.
redirect_not_in_allowlistadjudicates a resolver hop as if it were a content host, so a gold-OA cc-by paper is refused unless the user allowlists doi.org itself #533 --config doctor --networkstill adjudicated withmatches, notpermits. Thepermitsdoc comment said "the four adjudication sites"; thiswas a fifth, and the one whose job is explaining allowlist refusals.
Zotero exports.
Claims that did not match the code
Formula/doiget.rband its generator said the release workflow regeneratesthe formula "so this file cannot describe a release that does not exist". No
workflow calls it, and
CONTRIBUTING.md-- added in the same release --says outright that it is manual.
version and checksums from the file under test, so a well-formed but wrong
checksum, and a stale version, both pass.
docs/ERRORS.mdnamedevery_bare_string_error_site_is_a_known_oneas theguard keeping its bare-string exemption list from growing. The guard did not
exist. It does now, the set it pins is empty (all thirteen sites build
error_object), and a second test fails if the document and the list disagree.#462 is not a release blocker
#580 raised the
#[ignore]d TDM reproduction as the shape of #454 reachableagain, and asked whether 0.8.13 should ship with it. The diagnosis was
wrong, in this release's own subject matter -- accurate about the code, false
about the world.
Both client builders have two branches. The production branch extends with
tier_3_allowlists()and was correct throughout. The test-override branch,taken whenever any
DOIGET_*_BASEis 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 makes the reproduction pass. It is no longer
#[ignore]d, it runs intest (tdm features), androute_coverage_e2ehas noremaining
Gap.Still true, and worth keeping:
fetch_contentis implemented by APS alone, sothree of the four Tier-3 sources cannot reach the route the tier exists for.
Read it as APS coverage, not Tier-3 coverage.
Breaking
Six wire-shape changes, all of them a wrong code becoming a right one; see the
(breaking)entries inCHANGELOG.md. The one to read first is the four toolsthat answered with a bare string in
errorand now answer with an object.Not in this PR: branch protection
Every gate this release added is advisory. Required checks on
nextandmainaretest (ubuntu-latest)andtest (windows-latest)only, soversion-bump,test (slow)-- which now holds the #304 regression test -- andall four new posture-lint pins can be red on a merge. The header of
version-bump.ymlsays it is "safe to mark as a required status check onnextand
main"; it is not marked. That is a repository setting, not a diff, and isthe maintainer's to apply.
Verification
Local, on the
x86_64-pc-windows-gnutoolchain:cargo test --workspace --all-targets --no-default-features --features oa-only-- 913 passed, 0 failed, 1 ignored... --features oa-only,citation-- 999 passed, 0 failed... --features oa-only,metadata,tdm-aps,tdm-elsevier,tdm-springer,tdm-ieee-- 1030 passed, 0 failed, including the un-ignored TDM route testcargo clippy -D warningsacross all five CI feature sets -- cleancargo fmt --all-- cleanscripts/update-homebrew-formula.test.sh,npm/doiget-cli/test/stage-npm.test.sh, and the new release-sync posture check -- passscripts/sync_docs_to_site.sh--site/regenerated and committedversion-bumpis red by design: this is the cut, and ADR-0025 D6.2 wantsthe clean
X.Y.Zto arrive onnextbefore the promotion PR.