Marketplace: refuse an update to a version that is not installable (PR review #1)
Install and update now share ops::installable_at_head: the reviewed head must still be the head and the item's catalog entry there must be valid. The old update check (item_files) never parsed hook.json, so an upstream hook with an unknown event could be pinned and then held by every sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
@@ -8,9 +8,7 @@ use tauri::{AppHandle, Emitter, State};
|
||||
use tokio::sync::oneshot;
|
||||
|
||||
use crate::docker::container::is_container_running;
|
||||
use crate::marketplace::{
|
||||
self as mk, auth, catalog, diff, gh_login, git, tree::GitTree, MarketplaceManager,
|
||||
};
|
||||
use crate::marketplace::{self as mk, auth, diff, gh_login, git, MarketplaceManager};
|
||||
use crate::models::marketplace::{
|
||||
is_valid_commit, is_valid_item_key, AccountMethod, FileDiff, InstallScope, ItemUpdate,
|
||||
Marketplace, MarketplaceAccount, MarketplaceInstall, MarketplaceItemRef, MarketplaceSnapshot,
|
||||
@@ -24,7 +22,7 @@ use crate::AppState;
|
||||
/// testable without a Tauri runtime.
|
||||
pub(crate) mod ops {
|
||||
use crate::marketplace::{auth, git};
|
||||
use crate::models::marketplace::{MarketplaceInstall, MarketplaceItemRef};
|
||||
use crate::models::marketplace::{MarketplaceInstall, MarketplaceItemRef, MarketplaceSnapshot};
|
||||
|
||||
/// Insert, or replace the install of the same item (a re-install re-pins).
|
||||
pub fn upsert_install(list: &mut Vec<MarketplaceInstall>, inst: MarketplaceInstall) {
|
||||
@@ -82,6 +80,39 @@ pub(crate) mod ops {
|
||||
Ok(head.to_string())
|
||||
}
|
||||
|
||||
/// The one rule install and update share (PR review #1): pin
|
||||
/// `expected` only if it is still the head (see [`reviewed_head`]) and
|
||||
/// the item, as the catalog reads it at that head, is valid. `snap`'s
|
||||
/// items are always parsed at `snap.head_commit`, so for a hook this
|
||||
/// means its `hook.json` parses and names only known events — exactly
|
||||
/// what the payload later requires. `action` is "installed"/"updated".
|
||||
pub fn installable_at_head(
|
||||
snap: &MarketplaceSnapshot,
|
||||
item: &MarketplaceItemRef,
|
||||
expected: &str,
|
||||
marketplace_name: &str,
|
||||
action: &str,
|
||||
) -> Result<String, String> {
|
||||
let head = reviewed_head(snap.head_commit.as_deref(), expected, marketplace_name)?;
|
||||
let entry = snap
|
||||
.items
|
||||
.iter()
|
||||
.find(|i| i.kind == item.kind && i.key == item.key)
|
||||
.ok_or_else(|| {
|
||||
format!(
|
||||
"\"{}\" is no longer in \"{}\" — refresh the marketplace.",
|
||||
item.key, marketplace_name
|
||||
)
|
||||
})?;
|
||||
if let Some(reason) = &entry.invalid {
|
||||
return Err(format!(
|
||||
"\"{}\" cannot be {} at this version: {}",
|
||||
entry.name, action, reason
|
||||
));
|
||||
}
|
||||
Ok(head)
|
||||
}
|
||||
|
||||
/// 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.
|
||||
@@ -263,6 +294,75 @@ pub(crate) mod ops {
|
||||
);
|
||||
}
|
||||
|
||||
/// PR review #1: install and update share one rule — the item as the
|
||||
/// catalog reads it at the reviewed head must be valid. `item_files`
|
||||
/// alone (the old update check) accepts a hook whose `hook.json` names
|
||||
/// an unknown event, which every sync would then hold back.
|
||||
#[tokio::test]
|
||||
async fn an_item_invalid_at_the_reviewed_head_is_neither_installed_nor_updated() {
|
||||
use crate::marketplace::test_support::GitFixture;
|
||||
use crate::marketplace::{catalog, tree::GitTree, MarketplaceManager};
|
||||
use crate::models::marketplace::Marketplace;
|
||||
use crate::models::AppSettings;
|
||||
|
||||
let Some(fx) = GitFixture::new() else { return };
|
||||
fx.with_all_kinds();
|
||||
fx.write(
|
||||
"hooks/notify-on-stop/hook.json",
|
||||
r#"{"hooks":{"PreFoo":[{"hooks":[{"type":"command","command":"x"}]}]}}"#,
|
||||
);
|
||||
let head = fx.commit("bad hook event");
|
||||
let data = tempfile::tempdir().unwrap();
|
||||
let mgr = MarketplaceManager::new(data.path().to_path_buf());
|
||||
let mut settings = AppSettings::default();
|
||||
settings.marketplaces.push(Marketplace {
|
||||
id: "m1".into(),
|
||||
name: "Team".into(),
|
||||
url: fx.url(),
|
||||
branch: None,
|
||||
account_id: None,
|
||||
});
|
||||
let snap = crate::marketplace::refresh_marketplace(&mgr, &settings, "m1").await;
|
||||
|
||||
let repo = crate::marketplace::git::cache_path(data.path(), "m1");
|
||||
let tree = GitTree::open(&repo, &head).unwrap();
|
||||
assert!(
|
||||
catalog::item_files(&tree, ItemKind::Hook, "notify-on-stop").is_ok(),
|
||||
"the old update check let this through"
|
||||
);
|
||||
|
||||
let hook = MarketplaceItemRef {
|
||||
marketplace_id: "m1".into(),
|
||||
kind: ItemKind::Hook,
|
||||
key: "notify-on-stop".into(),
|
||||
};
|
||||
for action in ["installed", "updated"] {
|
||||
let e = installable_at_head(&snap, &hook, &head, "Team", action).unwrap_err();
|
||||
assert!(e.contains(&format!("cannot be {action}")), "{e}");
|
||||
assert!(e.contains("PreFoo"), "{e}");
|
||||
}
|
||||
let agent = MarketplaceItemRef {
|
||||
kind: ItemKind::Agent,
|
||||
key: "code-reviewer".into(),
|
||||
..hook.clone()
|
||||
};
|
||||
assert_eq!(
|
||||
installable_at_head(&snap, &agent, &head, "Team", "updated").unwrap(),
|
||||
head
|
||||
);
|
||||
let gone = MarketplaceItemRef {
|
||||
key: "no-such-agent".into(),
|
||||
..agent
|
||||
};
|
||||
let e = installable_at_head(&snap, &gone, &head, "Team", "updated").unwrap_err();
|
||||
assert!(e.contains("no longer in"), "{e}");
|
||||
assert!(
|
||||
installable_at_head(&snap, &hook, &"b".repeat(40), "Team", "installed")
|
||||
.unwrap_err()
|
||||
.contains("changed since you reviewed")
|
||||
);
|
||||
}
|
||||
|
||||
/// 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]
|
||||
@@ -693,23 +793,7 @@ pub async fn install_marketplace_item(
|
||||
let settings = state.settings_store.get();
|
||||
let m = find_marketplace(&settings, &item.marketplace_id)?;
|
||||
let snap = snapshot_blocking(&state, &m).await?;
|
||||
let head = ops::reviewed_head(snap.head_commit.as_deref(), &expected_commit, &m.name)?;
|
||||
let entry = snap
|
||||
.items
|
||||
.iter()
|
||||
.find(|i| i.kind == item.kind && i.key == item.key)
|
||||
.ok_or_else(|| {
|
||||
format!(
|
||||
"\"{}\" is no longer in \"{}\" — refresh the marketplace.",
|
||||
item.key, m.name
|
||||
)
|
||||
})?;
|
||||
if let Some(reason) = &entry.invalid {
|
||||
return Err(format!(
|
||||
"\"{}\" cannot be installed: {}",
|
||||
entry.name, reason
|
||||
));
|
||||
}
|
||||
let head = ops::installable_at_head(&snap, &item, &expected_commit, &m.name, "installed")?;
|
||||
let inst = MarketplaceInstall {
|
||||
marketplace_id: item.marketplace_id.clone(),
|
||||
kind: item.kind,
|
||||
@@ -829,17 +913,7 @@ pub async fn update_marketplace_item(
|
||||
let settings = state.settings_store.get();
|
||||
let m = find_marketplace(&settings, &item.marketplace_id)?;
|
||||
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());
|
||||
tokio::task::spawn_blocking(move || -> Result<(), String> {
|
||||
let tree = GitTree::open(&repo, &at)?;
|
||||
catalog::item_files(&tree, kind, &key).map(|_| ())
|
||||
})
|
||||
.await
|
||||
.map_err(|e| format!("Checking the new version failed: {e}"))?
|
||||
.map_err(|e| format!("\"{}\" cannot be updated: {e}", item.key))?;
|
||||
let head = ops::installable_at_head(&snap, &item, &expected_commit, &m.name, "updated")?;
|
||||
|
||||
match scope {
|
||||
InstallScope::Global => {
|
||||
|
||||
Reference in New Issue
Block a user