From dd019cf2c03ca60def60681e77817dc78ce0a020 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 10:11:19 -0700 Subject: [PATCH] Marketplace: pin the commit the user reviewed (final review I2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Install and update pinned whatever the marketplace head was when the click landed, so a background refresh between review and click could pin content nobody saw (including a hook's shell commands). install_marketplace_item and update_marketplace_item now take expected_commit and refuse with "changed since you reviewed this item — review it again" unless it is still the head. The UI passes the head the selected item was read at (Browse), the head frozen with a pending hook confirm (whose commands are frozen too), and the head of the accepted diff (Installed). Co-Authored-By: Claude Opus 5.5 --- .../src/commands/marketplace_commands.rs | 67 +++++++++++++------ .../marketplace/BrowsePane.test.tsx | 14 +++- app/src/components/marketplace/BrowsePane.tsx | 21 ++++-- .../marketplace/HookConfirmModal.tsx | 8 ++- .../marketplace/InstallControls.test.tsx | 41 +++++++++--- .../marketplace/InstallControls.tsx | 29 ++++++-- .../marketplace/InstalledPane.test.tsx | 13 +++- .../components/marketplace/InstalledPane.tsx | 18 +++-- app/src/components/marketplace/ItemDetail.tsx | 6 +- app/src/hooks/useMarketplace.test.ts | 16 ++++- app/src/hooks/useMarketplace.ts | 14 ++-- app/src/lib/tauri-commands.ts | 10 +-- .../specs/2026-09-27-marketplace-design.md | 5 +- 13 files changed, 197 insertions(+), 65 deletions(-) diff --git a/app/src-tauri/src/commands/marketplace_commands.rs b/app/src-tauri/src/commands/marketplace_commands.rs index 873dc65..2d716ad 100644 --- a/app/src-tauri/src/commands/marketplace_commands.rs +++ b/app/src-tauri/src/commands/marketplace_commands.rs @@ -62,6 +62,26 @@ pub(crate) mod ops { } } + /// The commit to pin for an install or update: the marketplace's current + /// head, but only if it is the one the person reviewed (`expected`, the + /// head the UI showed or diffed against). A refresh that lands between + /// review and click must not pin content nobody saw (final review I2). + pub fn reviewed_head( + head: Option<&str>, + expected: &str, + marketplace_name: &str, + ) -> Result { + let head = head.ok_or_else(|| { + format!("\"{marketplace_name}\" has not been fetched yet — refresh it first.") + })?; + if head != expected { + return Err(format!( + "\"{marketplace_name}\" has changed since you reviewed this item — review it again." + )); + } + Ok(head.to_string()) + } + /// An unvalidated value as it may appear in an error: quoted and escaped /// (`{:?}`) and capped at 60 characters, since it can come from an /// import file rather than from what the person just typed. @@ -226,6 +246,23 @@ pub(crate) mod ops { } } + /// Final review I2: an install or update pins exactly the commit the + /// person reviewed, or nothing. + #[test] + fn only_the_reviewed_head_is_pinned() { + let h = "a".repeat(40); + assert_eq!(reviewed_head(Some(&h), &h, "Team").unwrap(), h); + let moved = reviewed_head(Some(&h), &"b".repeat(40), "Team").unwrap_err(); + assert!(moved.contains("changed since you reviewed"), "{moved}"); + assert!(moved.contains("review it again"), "{moved}"); + assert!(reviewed_head(Some(&h), "", "Team").is_err()); + let unfetched = reviewed_head(None, &h, "Team").unwrap_err(); + assert!( + unfetched.contains("has not been fetched yet"), + "{unfetched}" + ); + } + /// Pre-flight F13: the add form and the fetch agree on what a branch /// is, so a name the fetch would refuse is refused up front. #[test] @@ -642,24 +679,21 @@ pub async fn forget_marketplace_installs( // Installs // ───────────────────────────────────────────────────────────────────────────── -/// Pins the item at the marketplace's current head. Returns fresh settings; -/// for a project scope the caller reloads projects. +/// Pins the item at `expected_commit`, the head the person reviewed, which +/// must still be the marketplace's head. Returns fresh settings; for a +/// project scope the caller reloads projects. #[tauri::command] pub async fn install_marketplace_item( item: MarketplaceItemRef, scope: InstallScope, + expected_commit: String, state: State<'_, AppState>, ) -> Result { validate_item(&item)?; let settings = state.settings_store.get(); let m = find_marketplace(&settings, &item.marketplace_id)?; let snap = snapshot_blocking(&state, &m).await?; - let head = snap.head_commit.clone().ok_or_else(|| { - format!( - "\"{}\" has not been fetched yet — refresh it first.", - m.name - ) - })?; + let head = ops::reviewed_head(snap.head_commit.as_deref(), &expected_commit, &m.name)?; let entry = snap .items .iter() @@ -781,26 +815,21 @@ pub async fn marketplace_item_diff( .map_err(|e| format!("Computing the diff failed: {e}"))? } -/// Moves one install's pin to the marketplace's head, if the item is still -/// installable there. +/// Moves one install's pin to `expected_commit`, the head whose diff the +/// person accepted, if that is still the marketplace's head and the item is +/// still installable there. #[tauri::command] pub async fn update_marketplace_item( item: MarketplaceItemRef, scope: InstallScope, + expected_commit: String, state: State<'_, AppState>, ) -> Result<(), String> { validate_item(&item)?; let settings = state.settings_store.get(); let m = find_marketplace(&settings, &item.marketplace_id)?; - let head = snapshot_blocking(&state, &m) - .await? - .head_commit - .ok_or_else(|| { - format!( - "\"{}\" has not been fetched yet — refresh it first.", - m.name - ) - })?; + let snap = snapshot_blocking(&state, &m).await?; + let head = ops::reviewed_head(snap.head_commit.as_deref(), &expected_commit, &m.name)?; let repo = git::cache_path(state.marketplace.data_root(), &m.id); let (kind, key, at) = (item.kind, item.key.clone(), head.clone()); diff --git a/app/src/components/marketplace/BrowsePane.test.tsx b/app/src/components/marketplace/BrowsePane.test.tsx index 4c74e50..1db79b7 100644 --- a/app/src/components/marketplace/BrowsePane.test.tsx +++ b/app/src/components/marketplace/BrowsePane.test.tsx @@ -4,7 +4,9 @@ import { useAppState } from "../../store/appState"; import type { AppSettings, CatalogItem, MarketplaceSnapshot } from "../../lib/types"; import type { MarketplaceApi } from "../../hooks/useMarketplace"; -vi.mock("./InstallControls", () => ({ default: () =>
install controls
})); +vi.mock("./InstallControls", () => ({ + default: ({ headCommit }: { headCommit: string | null }) =>
install controls at {headCommit}
, +})); vi.mock("./AddMarketplaceModal", () => ({ default: () =>
add modal
})); import BrowsePane from "./BrowsePane"; @@ -76,7 +78,15 @@ describe("BrowsePane", () => { fireEvent.click(screen.getByRole("button", { name: /code-reviewer/ })); expect(screen.getByText("code-reviewer preview body")).toBeInTheDocument(); - expect(screen.getByText("install controls")).toBeInTheDocument(); + expect(screen.getByText(`install controls at ${"a".repeat(40)}`)).toBeInTheDocument(); + }); + + it("I2: installs pin the head the shown item was read at, not a later one", () => { + const mp = api(); + const { rerender } = render(); + fireEvent.click(screen.getByRole("button", { name: /code-reviewer/ })); + rerender(); + expect(screen.getByText(`install controls at ${"a".repeat(40)}`)).toBeInTheDocument(); }); it("shows why an item is invalid", () => { diff --git a/app/src/components/marketplace/BrowsePane.tsx b/app/src/components/marketplace/BrowsePane.tsx index 08bf9ec..3f1edef 100644 --- a/app/src/components/marketplace/BrowsePane.tsx +++ b/app/src/components/marketplace/BrowsePane.tsx @@ -24,7 +24,13 @@ export default function BrowsePane({ mp }: { mp: MarketplaceApi }) { const setFilterId = useAppState((s) => s.setMarketplaceFilterProjectId); const [kind, setKind] = useState("all"); const [query, setQuery] = useState(""); - const [selected, setSelected] = useState<{ marketplaceId: string; item: CatalogItem } | null>(null); + // The item is kept as it was read, with the head it was read at: an + // install pins exactly what the detail pane shows (final review I2). + const [selected, setSelected] = useState<{ + marketplaceId: string; + item: CatalogItem; + headCommit: string | null; + } | null>(null); const [adding, setAdding] = useState(false); const [removing, setRemoving] = useState(null); @@ -35,7 +41,7 @@ export default function BrowsePane({ mp }: { mp: MarketplaceApi }) { .filter((i) => kind === "all" || i.kind === kind) .filter((i) => q === "" || `${i.name} ${i.key} ${i.description}`.toLowerCase().includes(q)) .sort((a, b) => KIND_ORDER.indexOf(a.kind) - KIND_ORDER.indexOf(b.kind) || a.name.localeCompare(b.name)) - .map((item) => ({ marketplaceId: snap.marketplace_id, item })), + .map((item) => ({ marketplaceId: snap.marketplace_id, item, headCommit: snap.head_commit })), ); }, [mp.snapshots, kind, query]); @@ -161,7 +167,7 @@ export default function BrowsePane({ mp }: { mp: MarketplaceApi }) { className={inputClass} />
    - {rows.map(({ marketplaceId, item }) => { + {rows.map(({ marketplaceId, item, headCommit }) => { const key = itemRefKey({ marketplace_id: marketplaceId, kind: item.kind, key: item.key }); const isSel = selected?.marketplaceId === marketplaceId && @@ -171,7 +177,7 @@ export default function BrowsePane({ mp }: { mp: MarketplaceApi }) {
  • @@ -175,9 +182,10 @@ export default function InstalledPane({ mp }: { mp: MarketplaceApi }) { fromCommit={pending.install.commit} toCommit={pending.update.head} scopeLabel={pending.scopeLabel} - hookCommands={hookCommandsFor(pending.update.item)} + hookCommands={pending.hookCommands} onClose={() => setPending(null)} - onAccept={() => mp.update(pending.update.item, pending.scope)} + // Pin exactly the head whose diff is on screen (final review I2). + onAccept={() => mp.update(pending.update.item, pending.scope, pending.update.head)} /> )} diff --git a/app/src/components/marketplace/ItemDetail.tsx b/app/src/components/marketplace/ItemDetail.tsx index 3a4647e..659454d 100644 --- a/app/src/components/marketplace/ItemDetail.tsx +++ b/app/src/components/marketplace/ItemDetail.tsx @@ -8,9 +8,11 @@ interface Props { mp: MarketplaceApi; item: CatalogItem; marketplaceId: string; + /** The marketplace head `item` was read at. */ + headCommit: string | null; } -export default function ItemDetail({ mp, item, marketplaceId }: Props) { +export default function ItemDetail({ mp, item, marketplaceId, headCommit }: Props) { return (
    @@ -45,7 +47,7 @@ export default function ItemDetail({ mp, item, marketplaceId }: Props) { )}

    Install

    - +

    Running containers pick changes up on their next start or with “Apply now” on the Installed tab. Changes apply to new Claude sessions. diff --git a/app/src/hooks/useMarketplace.test.ts b/app/src/hooks/useMarketplace.test.ts index 7aa27b3..83a855c 100644 --- a/app/src/hooks/useMarketplace.test.ts +++ b/app/src/hooks/useMarketplace.test.ts @@ -9,6 +9,7 @@ const listMarketplaceUpdates = vi.fn(); const getSettings = vi.fn(); const listProjects = vi.fn(); const installMarketplaceItem = vi.fn(); +const updateMarketplaceItem = vi.fn(); vi.mock("../lib/tauri-commands", () => ({ listMarketplaceSnapshots: () => listMarketplaceSnapshots(), @@ -17,6 +18,7 @@ vi.mock("../lib/tauri-commands", () => ({ getSettings: () => getSettings(), listProjects: () => listProjects(), installMarketplaceItem: (...a: unknown[]) => installMarketplaceItem(...a), + updateMarketplaceItem: (...a: unknown[]) => updateMarketplaceItem(...a), })); let syncHandler: ((e: { payload: unknown }) => void) | null = null; @@ -66,11 +68,23 @@ describe("useMarketplace", () => { installMarketplaceItem.mockRejectedValue("boom"); const { result } = renderHook(() => useMarketplace()); const ok = await act(() => - result.current.install({ marketplace_id: "m1", kind: "agent", key: "a" }, { type: "global" }), + result.current.install({ marketplace_id: "m1", kind: "agent", key: "a" }, { type: "global" }, "c".repeat(40)), ); expect(ok).toBe(false); expect(useAppState.getState().toasts[0]).toMatchObject({ kind: "error", detail: "boom" }); }); + + it("I2: passes the reviewed commit to install and update", async () => { + listMarketplaceSnapshots.mockResolvedValue([]); + installMarketplaceItem.mockResolvedValue({}); + updateMarketplaceItem.mockResolvedValue(undefined); + const item = { marketplace_id: "m1", kind: "hook" as const, key: "h" }; + const { result } = renderHook(() => useMarketplace()); + await act(() => result.current.install(item, { type: "global" }, "c".repeat(40))); + expect(installMarketplaceItem).toHaveBeenCalledWith(item, { type: "global" }, "c".repeat(40)); + await act(() => result.current.update(item, { type: "project", project_id: "p1" }, "d".repeat(40))); + expect(updateMarketplaceItem).toHaveBeenCalledWith(item, { type: "project", project_id: "p1" }, "d".repeat(40)); + }); }); describe("useMarketplaceSyncToasts", () => { diff --git a/app/src/hooks/useMarketplace.ts b/app/src/hooks/useMarketplace.ts index e1f57ae..9e9e5a8 100644 --- a/app/src/hooks/useMarketplace.ts +++ b/app/src/hooks/useMarketplace.ts @@ -21,10 +21,12 @@ export interface MarketplaceApi { refresh: (marketplaceId?: string) => Promise; /** Reload settings, projects and the update list after a mutation. */ reloadState: () => Promise; - install: (item: MarketplaceItemRef, scope: InstallScope) => Promise; + /** `commit`: the marketplace head the user reviewed (see `install_marketplace_item`). */ + install: (item: MarketplaceItemRef, scope: InstallScope, commit: string) => Promise; uninstall: (item: MarketplaceItemRef, scope: InstallScope) => Promise; setDisabled: (projectId: string, item: MarketplaceItemRef, disabled: boolean) => Promise; - update: (item: MarketplaceItemRef, scope: InstallScope) => Promise; + /** `commit`: the head whose diff the user accepted. */ + update: (item: MarketplaceItemRef, scope: InstallScope, commit: string) => Promise; forget: (marketplaceId: string) => Promise; remove: (marketplaceId: string) => Promise; } @@ -146,16 +148,16 @@ export function useMarketplace(): MarketplaceApi { load, refresh, reloadState, - install: (item, scope) => - mutate(`Could not install ${item.key}`, () => commands.installMarketplaceItem(item, scope)), + install: (item, scope, commit) => + mutate(`Could not install ${item.key}`, () => commands.installMarketplaceItem(item, scope, commit)), uninstall: (item, scope) => mutate(`Could not remove ${item.key}`, () => commands.uninstallMarketplaceItem(item, scope)), setDisabled: (projectId, item, disabled) => mutate(`Could not change ${item.key} for this project`, () => commands.setGlobalItemDisabled(projectId, item, disabled), ), - update: (item, scope) => - mutate(`Could not update ${item.key}`, () => commands.updateMarketplaceItem(item, scope)), + update: (item, scope, commit) => + mutate(`Could not update ${item.key}`, () => commands.updateMarketplaceItem(item, scope, commit)), forget: (marketplaceId) => mutate("Could not forget those installs", () => commands.forgetMarketplaceInstalls(marketplaceId)), remove: async (marketplaceId) => { diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index c911dcb..e69ea2b 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -449,8 +449,9 @@ export const updateMarketplace = (marketplace: Marketplace) => invoke("update_marketplace", { marketplace }); export const removeMarketplace = (marketplaceId: string) => invoke("remove_marketplace", { marketplaceId }); -export const installMarketplaceItem = (item: MarketplaceItemRef, scope: InstallScope) => - invoke("install_marketplace_item", { item, scope }); +/** `expectedCommit`: the head the user reviewed; the backend refuses if it moved. */ +export const installMarketplaceItem = (item: MarketplaceItemRef, scope: InstallScope, expectedCommit: string) => + invoke("install_marketplace_item", { item, scope, expectedCommit }); export const uninstallMarketplaceItem = (item: MarketplaceItemRef, scope: InstallScope) => invoke("uninstall_marketplace_item", { item, scope }); export const setGlobalItemDisabled = ( @@ -466,8 +467,9 @@ export const marketplaceItemDiff = ( fromCommit: string, toCommit: string, ) => invoke("marketplace_item_diff", { item, fromCommit, toCommit }); -export const updateMarketplaceItem = (item: MarketplaceItemRef, scope: InstallScope) => - invoke("update_marketplace_item", { item, scope }); +/** `expectedCommit`: the head whose diff the user accepted; the backend refuses if it moved. */ +export const updateMarketplaceItem = (item: MarketplaceItemRef, scope: InstallScope, expectedCommit: string) => + invoke("update_marketplace_item", { item, scope, expectedCommit }); export const applyMarketplaceNow = (projectId?: string) => invoke("apply_marketplace_now", { projectId: projectId ?? null }); export const getMarketplaceSyncReport = (projectId: string) => diff --git a/docs/superpowers/specs/2026-09-27-marketplace-design.md b/docs/superpowers/specs/2026-09-27-marketplace-design.md index ab0abd3..d06684f 100644 --- a/docs/superpowers/specs/2026-09-27-marketplace-design.md +++ b/docs/superpowers/specs/2026-09-27-marketplace-design.md @@ -184,7 +184,10 @@ password = token. Shallow fetch is not used (pins need history for diff/ancestry **Update detection** compares each installed item's own tree (item folder / file blob id) at its pin vs. the branch head; only a changed item shows "update available". **Update** shows a text diff of the item's files (pinned → head) and, on accept, moves the pin. -Hooks' diffs always show the rendered commands. +Hooks' diffs always show the rendered commands. Install and update both carry the commit the user +reviewed (the head the item was read at, the head of the accepted diff); the backend pins exactly +that commit and refuses with "changed since you reviewed this item — review it again" if the +marketplace's head has moved since. **Accounts** (`marketplace/auth.rs`):