fix(cli): cap curator listing fanout and memoize repository fetches

Author: harlan-github-agent[bot]

crates/skilld-command/src/remote.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::fmt;
 use std::sync::Arc;
 #[cfg(not(target_os = "wasi"))]
@@ -28,6 +28,10 @@ const LISTING_PAGE: usize = 200;
 /// bounds the loop when the response lies; a partial or empty page also ends
 /// paging early.
 const MAX_LISTING_PAGES: usize = 25;
+/// Most server supplied listing rows skilld trusts per response, mirroring
+/// `MAX_LISTING_PAGES`: the curator collection list and each collection's
+/// entry list are capped here so a malformed response cannot fan out.
+const MAX_LISTING_ENTRIES: usize = MAX_LISTING_PAGES;
 const ARTIFACT_LIMIT: usize = 64 * 1024 * 1024;
 const DIRECT_BLOB_LIMIT: usize = 12 * 1024 * 1024;
 const MAX_REDIRECTS: usize = 3;
@@ -1611,18 +1615,26 @@ impl SkilldRemote {
         self.service_json(url)
     }
 
-    /// Every Skill one Repository carries, by name.
+    /// Every Skill one Repository carries, by name. The owner index fetch is
+    /// memoized per Repository in `memo` for one listing, so repeated
+    /// collection entries naming the same Repository cost one fetch.
     fn repository_skills(
         &self,
         owner: &str,
         repository: &str,
+        memo: &mut HashMap<(String, String), Vec<ListedSkill>>,
     ) -> Result<Vec<ListedSkill>, RemoteError> {
+        let key = (owner.to_ascii_lowercase(), repository.to_ascii_lowercase());
+        if let Some(items) = memo.get(&key) {
+            return Ok(items.clone());
+        }
         let mut items = self
             .owner_skills(owner)?
             .into_iter()
             .filter(|skill| skill.repository.eq_ignore_ascii_case(repository))
             .collect::<Vec<_>>();
         items.sort_by(|left, right| left.name.cmp(&right.name));
+        memo.insert(key, items.clone());
         Ok(items)
     }
 
@@ -1653,6 +1665,7 @@ impl SkilldRemote {
                 name: row.name,
                 reason: row.reason,
             })
+            .take(MAX_LISTING_ENTRIES)
             .collect())
     }
 
@@ -1676,6 +1689,7 @@ impl SkilldRemote {
     fn expand_entries(
         &self,
         entries: Vec<CollectionEntry>,
+        memo: &mut HashMap<(String, String), Vec<ListedSkill>>,
     ) -> Result<Vec<ListedSkill>, RemoteError> {
         let mut seen = BTreeSet::new();
         let mut items = Vec::new();
@@ -1686,7 +1700,7 @@ impl SkilldRemote {
                         .into_iter()
                         .collect()
                 }
-                None => self.repository_skills(&entry.owner, &entry.repository)?,
+                None => self.repository_skills(&entry.owner, &entry.repository, memo)?,
             };
             for skill in expanded {
                 if seen.insert(skill.selector()) {
@@ -1732,19 +1746,24 @@ fn not_found_as_source(error: RemoteError, message: String) -> RemoteError {
 
 impl RemoteProvider for SkilldRemote {
     fn list_skills(&self, reference: &MultiSkillRef) -> Result<SkillListing, RemoteError> {
+        let mut memo = HashMap::new();
         let items = match reference {
             MultiSkillRef::Repository { owner, repository } => {
-                self.repository_skills(owner, repository)?
+                self.repository_skills(owner, repository, &mut memo)?
             }
             MultiSkillRef::Collection { login, slug } => {
-                self.expand_entries(self.collection_entries(login, slug)?)?
+                self.expand_entries(self.collection_entries(login, slug)?, &mut memo)?
             }
             MultiSkillRef::Curator { login } => {
                 let mut entries = Vec::new();
-                for slug in self.curator_slugs(login)? {
+                for slug in self
+                    .curator_slugs(login)?
+                    .into_iter()
+                    .take(MAX_LISTING_ENTRIES)
+                {
                     entries.extend(self.collection_entries(login, &slug)?);
                 }
-                self.expand_entries(entries)?
+                self.expand_entries(entries, &mut memo)?
             }
         };
         Ok(SkillListing {

crates/skilld-command/tests/remote.rs

@@ -681,6 +681,112 @@ fn a_curator_ref_lists_every_collection_once() {
     );
 }
 
+/// Serves a curator payload with 100 collections, answers every collection
+/// detail with one name-less Repository entry, and serves a runaway owner
+/// index to every owner, so only client side caps and memoization can end
+/// the listing.
+#[derive(Default)]
+struct RunawayCuratorHttp {
+    requests: Mutex<Vec<String>>,
+}
+
+const RUNAWAY_CURATOR_COLLECTIONS: usize = 100;
+const RUNAWAY_CURATOR_PAGE_CAP: usize = 25;
+
+/// One curator payload, the capped collection details, and one memoized
+/// owner index fetch for the repeated entry spanning the capped pages.
+const RUNAWAY_CURATOR_REQUEST_LIMIT: usize = 1 + 2 * RUNAWAY_CURATOR_PAGE_CAP;
+
+impl HttpAdapter for RunawayCuratorHttp {
+    fn send(
+        &self,
+        request: &HttpRequest,
+        _cancellation: &dyn Cancellation,
+        _timeout: Option<Duration>,
+    ) -> Result<HttpResponse, RemoteError> {
+        let mut requests = self.requests.lock().unwrap();
+        requests.push(request.url.clone());
+        assert!(
+            requests.len() <= RUNAWAY_CURATOR_REQUEST_LIMIT,
+            "the curator listing kept issuing requests: {} requests issued",
+            requests.len()
+        );
+        if request.url.contains("/api/curators/") {
+            return Ok(response(
+                200,
+                serde_json::to_vec(&json!({
+                    "login": "curator",
+                    "collections": (0..RUNAWAY_CURATOR_COLLECTIONS)
+                        .map(|index| {
+                            json!({
+                                "slug": format!("collection-{index}"),
+                                "name": format!("Collection {index}"),
+                                "itemCount": 1,
+                            })
+                        })
+                        .collect::<Vec<_>>(),
+                }))
+                .unwrap(),
+            ));
+        }
+        if request.url.contains("/api/collections/by-author/") {
+            return Ok(response(
+                200,
+                serde_json::to_vec(&json!({
+                    "skills": [{
+                        "position": 0,
+                        "owner": "big",
+                        "repo": "wanted",
+                        "name": null,
+                        "reason": null,
+                    }]
+                }))
+                .unwrap(),
+            ));
+        }
+        let page = json!({
+            "items": (0..RUNAWAY_PAGE_ROWS)
+                .map(|_| json!({
+                    "name": "wanted",
+                    "owner": "big",
+                    "repo": "wanted",
+                    "description": null,
+                    "stars": 1,
+                    "registryPath": "/gh/big/wanted/wanted",
+                }))
+                .collect::<Vec<_>>(),
+            "total": 20_000_000,
+            "pages": u64::MAX,
+        });
+        Ok(response(200, serde_json::to_vec(&page).unwrap()))
+    }
+}
+
+#[test]
+fn a_runaway_curator_stops_fanning_out_and_fetches_one_repository_once() {
+    let http = Arc::new(RunawayCuratorHttp::default());
+    let remote = SkilldRemote::new(
+        http.clone(),
+        Arc::new(NoTokenProvider),
+        NativeRemoteConfig::Unconfigured,
+    )
+    .with_endpoint("http://127.0.0.1:8787")
+    .unwrap()
+    .with_sleeper(Arc::new(NoSleep));
+
+    let listing = remote
+        .list_skills(&MultiSkillRef::Curator {
+            login: "curator".to_owned(),
+        })
+        .unwrap();
+
+    assert_eq!(listing.items, [listed("big", "wanted", "wanted", None)]);
+    assert_eq!(
+        http.requests.lock().unwrap().len(),
+        RUNAWAY_CURATOR_REQUEST_LIMIT
+    );
+}
+
 #[test]
 fn a_missing_collection_is_a_source_not_found_error() {
     let http = Arc::new(FakeHttp::with([response(