Marketplace: explain, don't offer, updates that would be refused (PR re-review)

ItemUpdate gains invalid_at_head: compute_updates records why the item cannot be installed at head (catalog invalid, or gone) — the rule update_marketplace_item applies — read once per marketplace from the snapshot at head, else from the catalog parsed at head. The Installed row shows that reason instead of a Review button, and the update counts in the Marketplace tab and Settings count only applicable updates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-09-27 13:22:01 -07:00
parent 3dfdafc9ca
commit 60c03baf62
9 changed files with 158 additions and 10 deletions
@@ -23,7 +23,8 @@ use tauri::Emitter;
 use tokio::sync::oneshot;

 use crate::models::marketplace::{
-    effective_installs, CatalogItem, ItemUpdate, Marketplace, MarketplaceInstall, MarketplaceSnapshot, SyncReport,
+    effective_installs, CatalogItem, ItemKind, ItemUpdate, Marketplace, MarketplaceInstall, MarketplaceSnapshot,
+    SyncReport,
 };
 use crate::models::{AppSettings, Project};
 use catalog::{item_fingerprint, parse_catalog};
@@ -103,6 +104,18 @@ impl MarketplaceManager {
            .and_then(|s| s.head_commit.clone())
    }

+    /// Each item's `invalid` reason in the in-memory snapshot, if that
+    /// snapshot is at `head` — without copying the items' previews.
+    fn invalid_reasons_at(
+        &self,
+        marketplace_id: &str,
+        head: &str,
+    ) -> Option<HashMap<(ItemKind, String), Option<String>>> {
+        let snapshots = self.snapshots.lock().unwrap();
+        let snap = snapshots.get(marketplace_id)?;
+        (snap.head_commit.as_deref() == Some(head)).then(|| invalid_reasons(&snap.items))
+    }
+
    pub fn snapshot(&self, marketplace_id: &str) -> Option<MarketplaceSnapshot> {
        self.snapshots.lock().unwrap().get(marketplace_id).cloned()
    }
@@ -350,12 +363,21 @@ pub async fn remove_marketplace_cache(mgr: &MarketplaceManager, marketplace_id:
    .await;
 }

+fn invalid_reasons(items: &[CatalogItem]) -> HashMap<(ItemKind, String), Option<String>> {
+    items
+        .iter()
+        .map(|i| ((i.kind, i.key.clone()), i.invalid.clone()))
+        .collect()
+}
+
 /// One marketplace's side of an update check: its head, read once, and its
 /// cache, opened once, with trees shared across installs (PR review #10).
 struct UpdateCheck {
    head: String,
    repo: Option<gix::Repository>,
    trees: HashMap<String, Result<GitTree, String>>,
+    /// Catalog `invalid` per item at head, read once when first needed.
+    invalid_at_head: Option<HashMap<(ItemKind, String), Option<String>>>,
 }

 impl UpdateCheck {
@@ -365,9 +387,40 @@ impl UpdateCheck {
            head,
            repo: None,
            trees: HashMap::new(),
+            invalid_at_head: None,
        })
    }

+    /// Why `inst`'s item cannot be installed at head — the rule
+    /// `update_marketplace_item` applies (round 2) — from the snapshot when
+    /// it is at head, else from the catalog parsed at head.
+    fn invalid_reason(
+        &mut self,
+        mgr: &MarketplaceManager,
+        m: &Marketplace,
+        inst: &MarketplaceInstall,
+    ) -> Option<String> {
+        if self.invalid_at_head.is_none() {
+            let head = self.head.clone();
+            let reasons = match mgr.invalid_reasons_at(&m.id, &head) {
+                Some(r) => r,
+                None => match self.tree(&git::cache_path(mgr.data_root(), &m.id), &head) {
+                    Ok(tree) => invalid_reasons(&parse_catalog(tree)),
+                    Err(e) => return Some(e),
+                },
+            };
+            self.invalid_at_head = Some(reasons);
+        }
+        match self
+            .invalid_at_head
+            .as_ref()
+            .and_then(|r| r.get(&(inst.kind, inst.key.clone())))
+        {
+            Some(reason) => reason.clone(),
+            None => Some(format!("\"{}\" is no longer in \"{}\".", inst.key, m.name)),
+        }
+    }
+
    fn tree(&mut self, repo_path: &Path, commit: &str) -> Result<&GitTree, String> {
        if !self.trees.contains_key(commit) {
            if self.repo.is_none() {
@@ -428,6 +481,7 @@ pub fn compute_updates(
                item: inst.item_ref(),
                pinned: inst.commit.clone(),
                head: check.head.clone(),
+                invalid_at_head: check.invalid_reason(mgr, m, inst),
            }),
            Ok(false) => {}
            Err(e) => log::debug!("Update check skipped for {}: {}", inst.key, e),
@@ -876,6 +930,52 @@ mod tests {
        assert_eq!(updates[0].head, c2);
    }

+    /// Re-review round 2: an update to a head where the item is not
+    /// installable is listed with the reason, since update_marketplace_item
+    /// would always refuse it.
+    #[tokio::test]
+    async fn an_update_to_an_invalid_version_carries_the_reason() {
+        let Some(fx) = GitFixture::new() else { return };
+        let c1 = fx.with_all_kinds();
+        fx.write(
+            "hooks/notify-on-stop/hook.json",
+            r#"{"hooks":{"PreFoo":[{"hooks":[{"type":"command","command":"x"}]}]}}"#,
+        );
+        fx.write(
+            "agents/code-reviewer.md",
+            "---\nname: code-reviewer\ndescription: Reviews code\n---\nReview harder.\n",
+        );
+        std::fs::remove_file(fx.dir.path().join("commands/example-command.md")).unwrap();
+        let c2 = fx.commit("break the hook, tweak the agent, drop the command");
+        let data = tempfile::tempdir().unwrap();
+        let mut settings = settings_with(&fx.url());
+        settings.global_marketplace_installs = vec![
+            install(ItemKind::Hook, "notify-on-stop", &c1),
+            install(ItemKind::Agent, "code-reviewer", &c1),
+            install(ItemKind::Command, "example-command", &c1),
+        ];
+        {
+            let mgr = MarketplaceManager::new(data.path().to_path_buf());
+            refresh_marketplace(&mgr, &|| settings.clone(), "m1").await;
+            check_reasons(&compute_updates(&mgr, &settings, &[]), &c2);
+        }
+        // Same answer from the cache alone (no snapshot in memory).
+        let mgr = MarketplaceManager::new(data.path().to_path_buf());
+        check_reasons(&compute_updates(&mgr, &settings, &[]), &c2);
+    }
+
+    fn check_reasons(updates: &[ItemUpdate], head: &str) {
+        let reason = |key: &str| {
+            let u = updates.iter().find(|u| u.item.key == key).unwrap();
+            assert_eq!(u.head, head);
+            u.invalid_at_head.clone()
+        };
+        assert_eq!(updates.len(), 3, "{updates:?}");
+        assert!(reason("notify-on-stop").unwrap().contains("PreFoo"));
+        assert_eq!(reason("code-reviewer"), None);
+        assert!(reason("example-command").unwrap().contains("no longer in"));
+    }
+
    /// PR review #10: refreshing one marketplace sets only its pins, and so
    /// never waits on another marketplace's lock.
    #[tokio::test]
@@ -202,6 +202,10 @@ pub struct ItemUpdate {
    pub item: MarketplaceItemRef,
    pub pinned: String,
    pub head: String,
+    /// Why the item cannot be installed at `head` (invalid there, or gone),
+    /// so the update would be refused; `None` when it can be applied.
+    #[serde(default)]
+    pub invalid_at_head: Option<String>,
 }

 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
@@ -73,7 +73,7 @@ describe("InstalledPane", () => {

  it("badges and accepts an update for the matching install", async () => {
    const mp = api({
-      updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B }],
+      updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B, invalid_at_head: null }],
    });
    render(<InstalledPane mp={mp} />);
    fireEvent.click(screen.getByRole("button", { name: "Review update for rev" }));
@@ -99,7 +99,7 @@ describe("InstalledPane", () => {
      ] as unknown as Project[],
    });
    const mp = api({
-      updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B }],
+      updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B, invalid_at_head: null }],
    });
    render(<InstalledPane mp={mp} />);
    const global = screen.getByTestId("installed-global");
@@ -108,13 +108,30 @@ describe("InstalledPane", () => {
    expect(within(proj).queryByRole("button", { name: "Review update for rev" })).not.toBeInTheDocument();
  });

+  it("re-review round 2: an update that would be refused shows why instead of a Review button", () => {
+    const mp = api({
+      updates: [
+        {
+          item: { marketplace_id: "m1", kind: "agent", key: "rev" },
+          pinned: A,
+          head: B,
+          invalid_at_head: 'unknown hook event "PreFoo"',
+        },
+      ],
+    });
+    render(<InstalledPane mp={mp} />);
+    const global = screen.getByTestId("installed-global");
+    expect(within(global).queryByRole("button", { name: "Review update for rev" })).not.toBeInTheDocument();
+    expect(within(global).getByText(/Update to bbbbbbbb cannot be installed: unknown hook event "PreFoo"/)).toBeInTheDocument();
+  });
+
  it("I2: accepts the head that was reviewed even if the update list moves on", async () => {
    const C = "c".repeat(40);
    const item = { marketplace_id: "m1", kind: "agent" as const, key: "rev" };
-    const mp = api({ updates: [{ item, pinned: A, head: B }] });
+    const mp = api({ updates: [{ item, pinned: A, head: B, invalid_at_head: null }] });
    const { rerender } = render(<InstalledPane mp={mp} />);
    fireEvent.click(screen.getByRole("button", { name: "Review update for rev" }));
-    rerender(<InstalledPane mp={{ ...mp, updates: [{ item, pinned: A, head: C }] }} />);
+    rerender(<InstalledPane mp={{ ...mp, updates: [{ item, pinned: A, head: C, invalid_at_head: null }] }} />);
    fireEvent.click(screen.getByRole("button", { name: "accept diff" }));
    await waitFor(() => expect(mp.update).toHaveBeenCalledWith(item, { type: "global" }, B));
  });
@@ -100,9 +100,16 @@ export default function InstalledPane({ mp }: { mp: MarketplaceApi }) {
            {KIND_LABELS[i.kind].replace(/s$/, "").toLowerCase()} · {nameOf(i.marketplace_id)} · {i.commit.slice(0, 8)}
          </span>
          {gone && <span className="ml-2 text-[var(--warning)]">Source removed</span>}
+          {upd?.invalid_at_head && !gone && (
+            // The update would be refused (re-review round 2), so it is
+            // explained rather than offered.
+            <span className="block text-[var(--warning)]">
+              Update to {upd.head.slice(0, 8)} cannot be installed: {upd.invalid_at_head}
+            </span>
+          )}
        </div>
        <div className="flex gap-1 flex-shrink-0">
-          {upd && !gone && (
+          {upd && !upd.invalid_at_head && !gone && (
            <Button
              size="sm"
              variant="secondary"
@@ -1,5 +1,6 @@
 import { useEffect, useRef, useState } from "react";
 import { useMarketplace } from "../../hooks/useMarketplace";
+import { applicableUpdates } from "../../lib/marketplace";
 import BrowsePane from "./BrowsePane";
 import InstalledPane from "./InstalledPane";
 import AccountsPane from "./AccountsPane";
@@ -49,9 +50,9 @@ export default function MarketplaceView({ active }: Props) {
            }`}
          >
            {t.label}
-            {t.id === "installed" && mp.updates.length > 0 && (
+            {t.id === "installed" && applicableUpdates(mp.updates).length > 0 && (
              <span className="ml-1.5 px-1 rounded-[4px] text-[10px] bg-[var(--accent-muted)] text-[var(--accent)]">
-                {mp.updates.length}
+                {applicableUpdates(mp.updates).length}
              </span>
            )}
          </button>
@@ -25,7 +25,19 @@ describe("MarketplaceSettings", () => {
      } as unknown as AppSettings,
    });
    listMarketplaceUpdates.mockResolvedValue([
-      { item: { marketplace_id: "m1", kind: "agent", key: "a" }, pinned: "a".repeat(40), head: "b".repeat(40) },
+      {
+        item: { marketplace_id: "m1", kind: "agent", key: "a" },
+        pinned: "a".repeat(40),
+        head: "b".repeat(40),
+        invalid_at_head: null,
+      },
+      // Not applicable (re-review round 2): not counted as available.
+      {
+        item: { marketplace_id: "m1", kind: "hook", key: "h" },
+        pinned: "a".repeat(40),
+        head: "b".repeat(40),
+        invalid_at_head: 'unknown hook event "PreFoo"',
+      },
    ]);
  });

@@ -1,6 +1,7 @@
 import { useEffect, useState } from "react";
 import { useAppState } from "../../store/appState";
 import { listMarketplaceUpdates } from "../../lib/tauri-commands";
+import { applicableUpdates } from "../../lib/marketplace";
 import Button from "../ui/Button";

 const plural = (n: number, one: string, many: string) => `${n} ${n === 1 ? one : many}`;
@@ -14,7 +15,7 @@ export default function MarketplaceSettings() {
    let cancelled = false;
    listMarketplaceUpdates()
      .then((u) => {
-        if (!cancelled) setUpdateCount(u.length);
+        if (!cancelled) setUpdateCount(applicableUpdates(u).length);
      })
      .catch(() => {
        if (!cancelled) setUpdateCount(null);
@@ -1,5 +1,6 @@
 import type {
  ItemKind,
+  ItemUpdate,
  MarketplaceInstall,
  MarketplaceItemRef,
  MarketplaceSnapshot,
@@ -27,6 +28,9 @@ export const KIND_LABELS: Record<ItemKind, string> = {
 /** A marketplace is refreshed when its tab opens if the last fetch is older than this. */
 export const STALE_AFTER_MS = 15 * 60 * 1000;

+/** Updates that can actually be applied (not refused as invalid at head). */
+export const applicableUpdates = (updates: ItemUpdate[]) => updates.filter((u) => u.invalid_at_head === null);
+
 export const itemRefKey = (r: MarketplaceItemRef) => `${r.marketplace_id}/${r.kind}/${r.key}`;

 /** Same shape as the item strings in a `SyncReport`. */
@@ -357,6 +357,8 @@ export interface ItemUpdate {
  item: MarketplaceItemRef;
  pinned: string;
  head: string;
+  /** Why the item cannot be installed at `head`, so the update would be refused; null when it applies. */
+  invalid_at_head: string | null;
 }
 export type FileChange = "added" | "removed" | "modified";
 export interface FileDiff {