fix(flare-docs): caller-vs-service error mapping, search limit cap, non-blocking-fetch test

crates/flare-docs/src/fetch.rs

@@ -9,11 +9,14 @@ const MAX_DECOMPRESSED_BYTES: u64 = 512 * 1024 * 1024;
 
 #[derive(Debug, thiserror::Error)]
 pub enum FetchError {
-    /// Split out from [`Self::Http`] so callers can tell "the server said this
-    /// does not exist" from "the request never got an answer". Collapsing the
-    /// two turns a timeout into a confident, wrong claim of absence.
-    #[error("not found")]
-    NotFound,
+    /// A response arrived carrying a non-2xx status. Split out from
+    /// [`Self::Http`] so callers can tell "the server said this does not
+    /// exist" from "the request never got an answer" — collapsing the two
+    /// turns a timeout into a confident, wrong claim of absence — and so a
+    /// 4xx ("you asked for the wrong thing") is distinguishable from a 5xx
+    /// ("the registry is broken").
+    #[error("http status {0}")]
+    Status(u16),
     #[error("http error: {0}")]
     Http(String),
     #[error("decompression error: {0}")]
@@ -22,6 +25,43 @@ pub enum FetchError {
     TooLarge(String),
 }
 
+/// Whether a failure was the caller's fault rather than the service's.
+///
+/// Exists so the MCP layer can map a typo'd package name onto
+/// `invalid_params` instead of reporting `internal_error` for what is really
+/// a bad argument. Implemented per error type because only the error itself
+/// knows which of its variants are caller-caused.
+pub trait ClientError {
+    fn is_client_error(&self) -> bool;
+
+    /// Whether the registry reported the package itself as absent.
+    ///
+    /// Separate from [`Self::is_client_error`] because the two answer
+    /// different questions: a package that exists but ships no types is the
+    /// caller's problem yet is emphatically *not* missing. Only a genuine
+    /// absence justifies "try the other ecosystem" advice — appending it to
+    /// anything else states as fact something that is not true.
+    fn is_package_missing(&self) -> bool;
+}
+
+impl ClientError for FetchError {
+    fn is_client_error(&self) -> bool {
+        match self {
+            // 408 and 429 sit in the 4xx range but are retryable: the request
+            // was well-formed and the caller simply needs to wait. Reporting
+            // them as `invalid_params` sends an agent off to "fix" arguments
+            // that were never wrong, when the correct response is to back off.
+            FetchError::Status(408 | 429) => false,
+            FetchError::Status(400..=499) => true,
+            _ => false,
+        }
+    }
+
+    fn is_package_missing(&self) -> bool {
+        matches!(self, FetchError::Status(404))
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct FetchedBytes {
     pub bytes: Vec<u8>,
@@ -80,14 +120,19 @@ impl Fetcher for UreqFetcher {
             .get(url)
             .set("User-Agent", USER_AGENT)
             .call()
-            .map_err(|e| FetchError::Http(e.to_string()))?;
-
+            .map_err(|e| match e {
+                // ureq turns any status >= 400 into an error by default, so
+                // this is the arm a 404 for a misspelled package reaches.
+                ureq::Error::Status(code, _) => FetchError::Status(code),
+                other => FetchError::Http(other.to_string()),
+            })?;
+
+        // Not made dead by the `Status` arm above: ureq only auto-errors on
+        // >= 400, so a 1xx/3xx response (redirect budget exhausted, or an
+        // agent configured not to follow them) still arrives here as `Ok`.
         let status = resp.status();
-        if status == 404 {
-            return Err(FetchError::NotFound);
-        }
         if !(200..300).contains(&status) {
-            return Err(FetchError::Http(format!("status {status}")));
+            return Err(FetchError::Status(status));
         }
 
         let etag = resp.header("etag").map(|s| s.to_string());
@@ -136,6 +181,22 @@ mod tests {
         assert_eq!(read.len(), 100);
     }
 
+    #[test]
+    fn only_4xx_counts_as_a_caller_mistake() {
+        // 404 is a typo'd package name; 500 and a transport failure are the
+        // registry's problem and must stay `internal_error` at the MCP layer.
+        assert!(FetchError::Status(404).is_client_error());
+        assert!(FetchError::Status(400).is_client_error());
+        assert!(FetchError::Status(499).is_client_error());
+        assert!(!FetchError::Status(500).is_client_error());
+        assert!(!FetchError::Status(302).is_client_error());
+        // Retryable 4xx: the arguments were fine, the caller just has to wait.
+        assert!(!FetchError::Status(429).is_client_error());
+        assert!(!FetchError::Status(408).is_client_error());
+        assert!(!FetchError::Http("connection refused".into()).is_client_error());
+        assert!(!FetchError::TooLarge("too big".into()).is_client_error());
+    }
+
     #[test]
     fn decompress_zstd_rejects_output_over_the_limit() {
         // A payload whose decompressed size alone exceeds

crates/flare-docs/src/lib.rs

@@ -29,6 +29,6 @@ pub struct FetchOutcome {
     /// `items_indexed` is 0 in that case. `doc` is unaffected either way.
     pub items_error: Option<String>,
 }
-pub use fetch::{FetchError, FetchedBytes, Fetcher, UreqFetcher};
+pub use fetch::{ClientError, FetchError, FetchedBytes, Fetcher, UreqFetcher};
 pub use rustdoc::{RustdocError, docs_id_path, docs_rs_json_url, fetch_and_store, store_fetched};
-pub use store::{DocsStore, Error, PROJECT_ID};
+pub use store::{DocsStore, Error, MAX_SEARCH_LIMIT, PROJECT_ID};

crates/flare-docs/src/npm/mod.rs

@@ -11,7 +11,7 @@ pub mod extract;
 pub mod fetch;
 
 use crate::FetchOutcome;
-use crate::fetch::{FetchError, Fetcher};
+use crate::fetch::{ClientError, FetchError, Fetcher};
 use crate::store::{BatchItem, DocsStore, Error as StoreError};
 use agentflare_store::documents::DocUpsertOpts;
 pub use extract::{ApiItem, ExtractError, extract, relative_imports};
@@ -29,6 +29,31 @@ pub enum NpmError {
     Store(#[from] StoreError),
 }
 
+impl ClientError for NpmError {
+    fn is_client_error(&self) -> bool {
+        match self {
+            NpmError::Fetch(e) => e.is_client_error(),
+            // The package resolved fine; it just ships no types and has no
+            // @types counterpart. That is a fact about what the caller asked
+            // for, not a registry failure.
+            NpmError::Npm(NpmFetchError::NoTypes(_)) => true,
+            // A malformed manifest or unreadable tarball is the registry
+            // serving something broken, and an extract/store failure is ours.
+            _ => false,
+        }
+    }
+
+    fn is_package_missing(&self) -> bool {
+        match self {
+            NpmError::Fetch(e) => e.is_package_missing(),
+            // Deliberately not `NoTypes`: that package was found, it just
+            // ships no declarations. Calling it missing would contradict the
+            // error's own message.
+            _ => false,
+        }
+    }
+}
+
 /// The [`DocsStore`] path an npm package's docs are cached under. Distinct
 /// from the `docsrs/` prefix so both ecosystems coexist in one store and one
 /// search index.
@@ -76,7 +101,9 @@ pub fn fetch_package(
                 // surface as itself -- reporting a retryable blip as a
                 // permanent absence of types sends the caller to diagnose the
                 // wrong thing.
-                FetchError::NotFound => NpmError::Npm(NpmFetchError::NoTypes(package.to_string())),
+                FetchError::Status(404) => {
+                    NpmError::Npm(NpmFetchError::NoTypes(package.to_string()))
+                }
                 other => NpmError::Fetch(other),
             })?;
         let types_manifest = fetch::parse_manifest(&fetched.bytes)?;
@@ -234,7 +261,7 @@ mod tests {
             }
             // An unregistered URL models "the registry has no such package",
             // i.e. a 404 -- not a transport failure.
-            Err(FetchError::NotFound)
+            Err(FetchError::Status(404))
         }
     }
 

crates/flare-docs/src/store.rs

@@ -8,6 +8,14 @@ use std::path::{Path, PathBuf};
 /// reused by every project) rather than scoped per-project.
 pub const PROJECT_ID: &str = "global";
 
+/// Ceiling on results from a single [`DocsStore::search`] call.
+///
+/// Enforced here rather than at each caller so the MCP tool, the CLI, and any
+/// future caller inherit it — an uncapped `limit` lets one request pull the
+/// whole index into a response body. Mirrors the "max 50" the memory tool
+/// already documents.
+pub const MAX_SEARCH_LIMIT: usize = 50;
+
 #[derive(Debug, thiserror::Error)]
 pub enum Error {
     #[error(transparent)]
@@ -67,7 +75,11 @@ impl DocsStore {
         Ok(self.inner.doc_get_by_path(PROJECT_ID, path)?)
     }
 
+    /// `limit` is clamped to [`MAX_SEARCH_LIMIT`]; a larger request is
+    /// silently capped rather than rejected, since asking for "everything" is
+    /// a reasonable thing to want and a truncated answer still serves it.
     pub fn search(&self, query: &str, limit: usize) -> Result<Vec<DocMatch>, Error> {
+        let limit = limit.min(MAX_SEARCH_LIMIT);
         Ok(self.inner.doc_search(PROJECT_ID, query, limit)?)
     }
 
@@ -345,6 +357,29 @@ mod tests {
         assert_eq!(listed[0].id, doc.id);
     }
 
+    #[test]
+    fn search_clamps_an_oversized_limit() {
+        // Every caller (MCP tool, CLI) routes through here, so an unbounded
+        // `--limit`/`limit:` can never pull more than the cap.
+        let store = DocsStore::open_memory().unwrap();
+        for i in 0..(MAX_SEARCH_LIMIT + 10) {
+            store
+                .upsert(
+                    &format!("docsrs/crate{i}"),
+                    "serialization framework",
+                    DocUpsertOpts::default(),
+                )
+                .unwrap();
+        }
+
+        let hits = store.search("serialization", usize::MAX).unwrap();
+        assert_eq!(hits.len(), MAX_SEARCH_LIMIT);
+
+        // A limit under the cap is still honoured exactly.
+        let few = store.search("serialization", 3).unwrap();
+        assert_eq!(few.len(), 3);
+    }
+
     #[test]
     fn upsert_batch_inserts_multiple_documents_in_one_call() {
         let store = DocsStore::open_memory().unwrap();

src/cli/docs.rs

@@ -11,6 +11,7 @@ pub enum DocsCmd {
     /// Search cached third-party documentation.
     Search {
         query: String,
+        /// Max results to return; capped at 50.
         #[arg(long, default_value_t = 10)]
         limit: usize,
     },