fix: third review round — the release pipeline, the rate limiter, the audit log, and a fetch that ate your tags

.github/workflows/posture-lint.yml

@@ -395,8 +395,12 @@ jobs:
           # One `insert("disposition"` per `insert("code"`. Not a proof, but it
           # fails on exactly the mistake that was made.
           f=crates/doiget-mcp/src/lib.rs
-          codes="$(grep -c 'insert("code"' "$f")"
-          disps="$(grep -c 'insert("disposition"' "$f")"
+          # `|| true`: `grep -c` exits 1 on a zero count, and under
+          # `set -euo pipefail` that kills the step at the assignment, before
+          # the `::error::` written below to explain it. Same guard as the
+          # release-sync step and `capture()`.
+          codes="$(grep -c 'insert("code"' "$f" || true)"
+          disps="$(grep -c 'insert("disposition"' "$f" || true)"
           if [ "$codes" != "$disps" ]; then
             echo "::error::posture-lint: $codes hand-built error objects but $disps dispositions in $f - a failure envelope is missing error.disposition (#506)"
             grep -n 'insert("code"' "$f"

.github/workflows/release-plz.yml

@@ -805,7 +805,33 @@ jobs:
           # Reading the MAP cannot drift the same way. It lists platform
           # packages only, so the wrapper is excluded by construction rather
           # than by a name test somebody has to remember to update.
-          for pkg in $(grep -oE '^doiget-[a-z0-9-]+:' scripts/stage-npm.sh | tr -d ':' | sort -u); do
+          #
+          # The empty case is guarded, and that is not defensive noise. The
+          # pipeline ends in `sort -u`, which exits 0 on empty input, so
+          # `set -euo pipefail` gives NOTHING here: a grep that matches
+          # nothing (a reformat of the MAP block, a delimiter change) yields
+          # an empty word list, the loop runs zero times, and the step falls
+          # through to publish the wrapper alone -- green, with every platform
+          # binary silently missing. `npm install doiget-cli` would then
+          # succeed while its optionalDependencies fail to resolve.
+          #
+          # The `doiget-*` glob this replaced failed LOUDLY on no-match (the
+          # unexpanded pattern reached `npm publish` as a path that does not
+          # exist). Trading that for silence in the step that performs the
+          # irreversible publish is the wrong direction. posture-lint's
+          # `capture()` already guards the identical grep for the identical
+          # reason; it was applied to the check and not to the publish.
+          pkgs="$(grep -oE '^doiget-[a-z0-9-]+:' scripts/stage-npm.sh | tr -d ':' | sort -u || true)"
+          if [ -z "$pkgs" ]; then
+            echo "::error::release: no platform packages found in scripts/stage-npm.sh -- refusing to publish the wrapper alone"
+            exit 1
+          fi
+          count=$(echo "$pkgs" | wc -l | tr -d " ")
+          if [ "$count" -ne 4 ]; then
+            echo "::error::release: expected 4 platform packages, found $count: $(echo $pkgs)"
+            exit 1
+          fi
+          for pkg in $pkgs; do
             npm publish "./npm-stage/$pkg" --provenance --access public --tag "$DIST_TAG"
           done
           npm publish ./npm-stage/doiget-cli --provenance --access public --tag "$DIST_TAG"

crates/doiget-cli/src/commands/tag.rs

@@ -122,7 +122,7 @@ pub fn run(
     }
 
     store
-        .write(&safekey, &metadata, None)
+        .write_user_authored(&safekey, &metadata, None)
         .with_context(|| format!("failed to write updated metadata for {ref_str}"))?;
 
     Ok(())
@@ -174,7 +174,7 @@ pub fn run_annotate(ref_str: String, text: Option<String>, clear: bool) -> Resul
     }
 
     store
-        .write(&safekey, &metadata, None)
+        .write_user_authored(&safekey, &metadata, None)
         .with_context(|| format!("failed to write updated metadata for {ref_str}"))?;
 
     Ok(())

crates/doiget-core/src/resolver_cache.rs

@@ -45,7 +45,20 @@ struct CacheEntry {
 /// The on-disk path for a ref's cache entry:
 /// `<cache_root>/resolver/<safekey>.toml`.
 #[must_use]
-pub fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf {
+// `pub(crate)`, not `pub`. Nothing outside `doiget-core` calls this module --
+// the orchestrator is the only consumer -- and it is absent from
+// `docs/PUBLIC_API.md`, so every one of these was an accidental semver
+// commitment, including the on-disk cache layout they encode. This cycle
+// added the `_with_options` half and doubled that surface.
+//
+// `#[cfg(test)]` on the remaining plain wrappers is not tidying: making them
+// `pub(crate)` is what revealed that production calls none of them. They
+// default the options for this module's own tests and nothing else, and `pub`
+// had been keeping the dead-code lint quiet about it. Two of the original
+// five, `read` and `write`, turned out to have no caller anywhere -- not even
+// a test -- and are gone.
+#[cfg(test)]
+pub(crate) fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf {
     cache_file_with_options(cache_root, ref_, MetadataOnlyOptions::default())
 }
 
@@ -69,7 +82,7 @@ pub fn cache_file(cache_root: &Utf8Path, ref_: &Ref) -> Utf8PathBuf {
 /// would both want `doi_10.1234_foo.oa.toml`. A safekey can never contain a
 /// path separator (`/` is replaced with `_`), so a subdirectory cannot.
 #[must_use]
-pub fn cache_file_with_options(
+pub(crate) fn cache_file_with_options(
     cache_root: &Utf8Path,
     ref_: &Ref,
     opts: MetadataOnlyOptions,
@@ -89,7 +102,8 @@ pub fn cache_file_with_options(
 /// expired, or a `response` blob that no longer deserializes. `now` is
 /// injected so tests can pin expiry without touching the clock.
 #[must_use]
-pub fn read_at(
+#[cfg(test)]
+pub(crate) fn read_at(
     cache_root: &Utf8Path,
     ref_: &Ref,
     now: DateTime<Utc>,
@@ -100,7 +114,7 @@ pub fn read_at(
 /// [`read_at`], reading the entry keyed by `opts`. See
 /// [`cache_file_with_options`] for why the options are part of the key.
 #[must_use]
-pub fn read_at_with_options(
+pub(crate) fn read_at_with_options(
     cache_root: &Utf8Path,
     ref_: &Ref,
     now: DateTime<Utc>,
@@ -119,15 +133,9 @@ pub fn read_at_with_options(
     serde_json::from_str(&entry.response).ok()
 }
 
-/// Read using the current wall clock. See [`read_at`].
-#[must_use]
-pub fn read(cache_root: &Utf8Path, ref_: &Ref) -> Option<MetadataOnlyOutcome> {
-    read_at(cache_root, ref_, Utc::now())
-}
-
 /// [`read`], reading the entry keyed by `opts`.
 #[must_use]
-pub fn read_with_options(
+pub(crate) fn read_with_options(
     cache_root: &Utf8Path,
     ref_: &Ref,
     opts: MetadataOnlyOptions,
@@ -138,7 +146,8 @@ pub fn read_with_options(
 /// Write `outcome` to the cache for `ref_`. Best-effort: returns `false`
 /// (after a `tracing::debug!`) on any I/O or serialization failure rather
 /// than propagating, since a cache write must never fail a resolve.
-pub fn write_at(
+#[cfg(test)]
+pub(crate) fn write_at(
     cache_root: &Utf8Path,
     ref_: &Ref,
     outcome: &MetadataOnlyOutcome,
@@ -154,7 +163,7 @@ pub fn write_at(
 }
 
 /// [`write_at`], writing the entry keyed by `opts`.
-pub fn write_at_with_options(
+pub(crate) fn write_at_with_options(
     cache_root: &Utf8Path,
     ref_: &Ref,
     outcome: &MetadataOnlyOutcome,
@@ -189,20 +198,20 @@ pub fn write_at_with_options(
             return false;
         }
     }
-    if let Err(e) = std::fs::write(&path, toml_text) {
+    // tmp + rename, not a plain write. A reader racing a plain write sees a
+    // half-written file, `toml::from_str` fails, and the entry degrades to a
+    // miss -- safe, per this module's best-effort contract, but it is a
+    // re-fetch nobody asked for and a `debug!` line that looks like
+    // corruption. The store next door already had the helper.
+    if let Err(e) = crate::store::atomic_write(&path, toml_text.as_bytes()) {
         tracing::debug!(error = %e, path = %path, "resolver cache: write failed");
         return false;
     }
     true
 }
 
-/// Write using the current wall clock. See [`write_at`].
-pub fn write(cache_root: &Utf8Path, ref_: &Ref, outcome: &MetadataOnlyOutcome) -> bool {
-    write_at(cache_root, ref_, outcome, Utc::now())
-}
-
 /// [`write()`], writing the entry keyed by `opts`.
-pub fn write_with_options(
+pub(crate) fn write_with_options(
     cache_root: &Utf8Path,
     ref_: &Ref,
     outcome: &MetadataOnlyOutcome,

crates/doiget-core/src/source.rs

@@ -260,7 +260,38 @@ impl From<&FetchError> for crate::ErrorCode {
             FetchError::Http(HttpError::HttpStatus {
                 status: 401 | 403, ..
             }) => crate::ErrorCode::CapabilityDenied,
-            FetchError::Http(_) => crate::ErrorCode::NetworkError,
+            // Exhaustive over `HttpError`, not `Http(_)`. The wildcard sent
+            // six deterministic outcomes to `NETWORK_ERROR`, whose disposition
+            // is `retry_after` -- so an agent was told to back off and retry an
+            // allowlist refusal, an http:// downgrade, a size cap, a
+            // wrong content type, an unregistered source key and a malformed
+            // header, none of which a retry can change. That is the defect
+            // ADR-0055 exists to remove, in the mapping every surface routes
+            // through. The `DenialContext` impl 100 lines down already matches
+            // all eight variants; this one opted out of the same protection.
+            FetchError::Http(e) => match e {
+                // Policy decisions. Settled until the configuration changes,
+                // which is what `needs_config` means -- and each of these
+                // carries a `DenialContext` naming the fix.
+                HttpError::RedirectDenied { .. } | HttpError::InsecureRedirect { .. } => {
+                    crate::ErrorCode::CapabilityDenied
+                }
+                // The response arrived and was not what was asked for.
+                // Re-requesting returns the same bytes.
+                HttpError::OversizedBody { .. } | HttpError::NotAPdf { .. } => {
+                    crate::ErrorCode::NoOaAvailable
+                }
+                // The caller asked for a source the client was never given.
+                // A build/wiring fault, not the network (#454, #462).
+                HttpError::UnknownSource { .. } | HttpError::InvalidHeader { .. } => {
+                    crate::ErrorCode::InternalError
+                }
+                // Genuinely transient: transport failures, and the statuses
+                // the arms above did not claim.
+                HttpError::Network(_) | HttpError::HttpStatus { .. } => {
+                    crate::ErrorCode::NetworkError
+                }
+            },
             FetchError::Log(_) => crate::ErrorCode::LogError,
             FetchError::InvalidRef(_) => crate::ErrorCode::InvalidRef,
             // An access refusal is not an internal error. Before #538 it
@@ -536,6 +567,60 @@ mod tests {
         assert!(res.metadata_json.is_none());
     }
 
+    /// A deterministic HTTP outcome must not be advertised as retriable.
+    ///
+    /// `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, so the mapping
+    /// every surface routes through was telling agents to back off and try
+    /// again on an allowlist refusal, a size cap and an unregistered source
+    /// key -- the exact advice ADR-0055 exists to stop giving.
+    #[test]
+    fn a_deterministic_http_failure_is_not_advertised_as_retriable() {
+        let cases: Vec<(HttpError, crate::Disposition)> = vec![
+            (
+                HttpError::RedirectDenied {
+                    source_key: "oa-publisher".into(),
+                    host: "evil.example.com".into(),
+                    expected_hosts: vec!["*.wiley.com".to_string()],
+                },
+                crate::Disposition::NeedsConfig,
+            ),
+            (
+                HttpError::UnknownSource {
+                    source_key: "tdm-aps".into(),
+                },
+                crate::Disposition::Terminal,
+            ),
+        ];
+        for (he, want) in cases {
+            let code: ErrorCode = FetchError::Http(he).into();
+            assert_ne!(
+                code,
+                ErrorCode::NetworkError,
+                "a policy/wiring outcome is not a network error: {code:?}"
+            );
+            assert_eq!(
+                code.disposition(),
+                want,
+                "and its disposition must not say retry_after: {code:?}"
+            );
+        }
+    }
+
+    /// The transient ones keep saying retry, so the fix did not overshoot.
+    #[test]
+    fn a_transient_http_failure_still_says_retry() {
+        let code: ErrorCode = FetchError::Http(HttpError::HttpStatus {
+            status: 503,
+            retry_after_ms: None,
+            url: "https://api.crossref.org/works/10.5555/x".into(),
+        })
+        .into();
+        assert_eq!(code, ErrorCode::NetworkError);
+        assert_eq!(code.disposition(), crate::Disposition::RetryAfter);
+    }
+
     #[test]
     fn fetch_error_collapses_to_error_code() {
         // Mirrors `docs/PUBLIC_API.md` §4 / PR #55 boundary collapse.
@@ -549,11 +634,20 @@ mod tests {
         let e: ErrorCode = FetchError::NoOaAvailable.into();
         assert_eq!(e, ErrorCode::NoOaAvailable);
 
+        // `UnknownSource` is "the caller asked HttpClient to fetch for a
+        // source it was never given" -- a wiring fault. This asserted
+        // `NetworkError` because the mapping used to be `Http(_) =>
+        // NetworkError`, i.e. it pinned the wildcard rather than a decision:
+        // retrying cannot register a missing source, and `NetworkError`'s
+        // `retry_after` disposition told an agent to try anyway. It is the
+        // error #462's TDM reproduction actually hit, and calling it a network
+        // problem is part of why it read as one.
         let e: ErrorCode = FetchError::Http(HttpError::UnknownSource {
             source_key: "mock".into(),
         })
         .into();
-        assert_eq!(e, ErrorCode::NetworkError);
+        assert_eq!(e, ErrorCode::InternalError);
+        assert_eq!(e.disposition(), crate::Disposition::Terminal);
 
         // 404 / 410 / 451 from a metadata source are authoritative "id does
         // not exist" → NotFound (network-independent), NOT NetworkError.

crates/doiget-core/src/sources/openalex.rs

@@ -270,7 +270,10 @@ pub fn open_access_pdf_url(record: &serde_json::Value) -> Option<&str> {
 /// the reader at the repository. "no OA PDF available" points them at giving
 /// up. Returns `None` when there is nothing to say.
 #[must_use]
-pub fn describe_locations(record: &serde_json::Value) -> Option<(usize, String)> {
+// `pub(crate)`, not `pub`: one caller, `orchestrator::describe_optional_source_locations`,
+// and the signature takes a raw `&serde_json::Value` -- publishing it would put
+// OpenAlex's wire shape under this crate's semver guarantee for no consumer.
+pub(crate) fn describe_locations(record: &serde_json::Value) -> Option<(usize, String)> {
     let locations = record
         .get("locations")
         .and_then(serde_json::Value::as_array)?;