Marketplace: warn on imported global plugins; tidy import follow-ups
- The import preview counts global plugin installs and warns on them:
a plugin can bring hooks and MCP servers into every container and an
imported install skips the confirm step, like a hook.
- Item keys, hosts and branches in errors are quoted with {:?} and
capped, since they can come from an import file.
- After an import, caches and snapshots of marketplaces the import
dropped are removed (under the repo lock) and pins are refreshed for
the imported installs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -62,6 +62,19 @@ pub(crate) mod ops {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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.
|
||||
pub fn shown(value: &str) -> String {
|
||||
const MAX: usize = 60;
|
||||
if value.chars().count() > MAX {
|
||||
let head: String = value.chars().take(MAX).collect();
|
||||
format!("{:?}…", head)
|
||||
} else {
|
||||
format!("{:?}", value)
|
||||
}
|
||||
}
|
||||
|
||||
pub fn validate_label(label: &str) -> Result<String, String> {
|
||||
let label = label.trim();
|
||||
if label.is_empty() {
|
||||
@@ -86,7 +99,7 @@ pub(crate) mod ops {
|
||||
if git::valid_branch(&b) {
|
||||
Ok(Some(b))
|
||||
} else {
|
||||
Err(format!("{b:?} is not a valid branch name."))
|
||||
Err(format!("{} is not a valid branch name.", shown(&b)))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -101,7 +114,7 @@ pub(crate) mod ops {
|
||||
if auth::valid_host(&host) && !host.starts_with('.') && !host.starts_with(':') && port_ok {
|
||||
Ok(host)
|
||||
} else {
|
||||
Err(format!("{host:?} is not a valid host name."))
|
||||
Err(format!("{} is not a valid host name.", shown(&host)))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -294,7 +307,10 @@ fn validate_item(item: &MarketplaceItemRef) -> Result<(), String> {
|
||||
if is_valid_item_key(&item.key) {
|
||||
Ok(())
|
||||
} else {
|
||||
Err(format!("\"{}\" is not a valid item name.", item.key))
|
||||
Err(format!(
|
||||
"{} is not a valid item name.",
|
||||
ops::shown(&item.key)
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -382,6 +398,17 @@ pub(crate) fn validate_imported_marketplace_state(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Marketplaces configured in `before` that `after` no longer has: an import
|
||||
/// that drops them leaves their caches and snapshots to be removed.
|
||||
pub(crate) fn dropped_marketplace_ids(before: &AppSettings, after: &AppSettings) -> Vec<String> {
|
||||
before
|
||||
.marketplaces
|
||||
.iter()
|
||||
.filter(|m| !after.marketplaces.iter().any(|a| a.id == m.id))
|
||||
.map(|m| m.id.clone())
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// The in-memory snapshot, else the cached one (which is then remembered).
|
||||
fn snapshot_or_cached(mgr: &MarketplaceManager, m: &Marketplace) -> MarketplaceSnapshot {
|
||||
if let Some(s) = mgr.snapshot(&m.id) {
|
||||
@@ -406,7 +433,7 @@ async fn snapshot_blocking(
|
||||
/// Make each cache's pin refs exactly the commits installs reference, so a
|
||||
/// pinned version can never be garbage-collected away. Under the repo lock
|
||||
/// (pre-flight F11): a concurrent fetch writes refs in the same repos.
|
||||
async fn refresh_pins(state: &AppState) {
|
||||
pub(crate) async fn refresh_pins(state: &AppState) {
|
||||
let settings = state.settings_store.get();
|
||||
let pins = mk::pins_by_marketplace(&settings, &state.projects_store.list());
|
||||
let root = state.marketplace.data_root().to_path_buf();
|
||||
@@ -432,7 +459,7 @@ async fn refresh_pins(state: &AppState) {
|
||||
}
|
||||
|
||||
/// Forget a marketplace's snapshot and delete its cache, under the repo lock.
|
||||
async fn remove_cache(state: &AppState, marketplace_id: &str) {
|
||||
pub(crate) async fn remove_cache(state: &AppState, marketplace_id: &str) {
|
||||
state.marketplace.remove_snapshot(marketplace_id);
|
||||
let path = git::cache_path(state.marketplace.data_root(), marketplace_id);
|
||||
let _repo_guard = state.marketplace.repo_lock().lock().await;
|
||||
@@ -1144,4 +1171,38 @@ mod tests {
|
||||
s.marketplaces.clear();
|
||||
validate_imported_marketplace_state(&mut s, &tokens()).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_invalid_item_key_is_quoted_and_capped_in_the_error() {
|
||||
use crate::models::marketplace::MarketplaceItemRef;
|
||||
let item = |key: String| MarketplaceItemRef {
|
||||
marketplace_id: MARKET.into(),
|
||||
kind: ItemKind::Agent,
|
||||
key,
|
||||
};
|
||||
let e = validate_item(&item("bad\nkey".into())).unwrap_err();
|
||||
assert!(!e.contains('\n'), "raw control character in {e:?}");
|
||||
assert!(e.contains("\"bad\\nkey\""), "{e}");
|
||||
let e = validate_item(&item(format!("{}/", "x".repeat(500)))).unwrap_err();
|
||||
assert!(
|
||||
e.chars().count() < 150,
|
||||
"not capped: {} chars",
|
||||
e.chars().count()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn marketplaces_an_import_drops_are_the_ones_whose_caches_go() {
|
||||
let mut before = imported();
|
||||
let mut kept = before.marketplaces[0].clone();
|
||||
kept.id = "kept-1".into();
|
||||
before.marketplaces.push(kept.clone());
|
||||
let mut after = AppSettings::default();
|
||||
after.marketplaces.push(kept);
|
||||
assert_eq!(
|
||||
dropped_marketplace_ids(&before, &after),
|
||||
vec![MARKET.to_string()]
|
||||
);
|
||||
assert!(dropped_marketplace_ids(&after, &before).is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -441,6 +441,13 @@ pub async fn apply_settings_import(
|
||||
) = imported_marketplace;
|
||||
state.settings_store.update(s)?
|
||||
};
|
||||
// Caches of marketplaces the import dropped are dead weight now, and
|
||||
// the pins must match the imported installs.
|
||||
use crate::commands::marketplace_commands as mc;
|
||||
for id in mc::dropped_marketplace_ids(¤t, &saved) {
|
||||
mc::remove_cache(&state, &id).await;
|
||||
}
|
||||
mc::refresh_pins(&state).await;
|
||||
|
||||
// `reconcile_gateway` (inside `update_settings`) only reacts to a changed
|
||||
// *shape* — port, provider, base URL, models — because that's what's
|
||||
|
||||
@@ -164,6 +164,10 @@ pub struct SettingsImportPreview {
|
||||
/// step an install from the Marketplace tab shows, so the preview warns.
|
||||
#[serde(default)]
|
||||
pub global_hook_install_count: usize,
|
||||
/// Plugins the import installs for every project. A plugin can bring
|
||||
/// its own hooks and MCP servers, and skips the same confirm step.
|
||||
#[serde(default)]
|
||||
pub global_plugin_install_count: usize,
|
||||
/// Non-blank marketplace account tokens the import restores.
|
||||
#[serde(default)]
|
||||
pub marketplace_account_token_count: usize,
|
||||
@@ -224,6 +228,12 @@ impl SettingsImportPreview {
|
||||
.iter()
|
||||
.filter(|i| i.kind == crate::models::marketplace::ItemKind::Hook)
|
||||
.count(),
|
||||
global_plugin_install_count: payload
|
||||
.settings
|
||||
.global_marketplace_installs
|
||||
.iter()
|
||||
.filter(|i| i.kind == crate::models::marketplace::ItemKind::Plugin)
|
||||
.count(),
|
||||
marketplace_account_token_count: payload
|
||||
.secrets
|
||||
.marketplace_account_tokens
|
||||
@@ -446,4 +456,25 @@ mod tests {
|
||||
};
|
||||
assert!(!secrets.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn global_plugin_installs_are_counted_apart_from_hooks() {
|
||||
use crate::models::marketplace::{ItemKind, MarketplaceInstall};
|
||||
let mut payload = payload_with(ExportedSecrets::default());
|
||||
let install = |kind, key: &str| MarketplaceInstall {
|
||||
marketplace_id: "m1".into(),
|
||||
kind,
|
||||
key: key.into(),
|
||||
commit: "a".repeat(40),
|
||||
};
|
||||
payload.settings.global_marketplace_installs = vec![
|
||||
install(ItemKind::Plugin, "p1"),
|
||||
install(ItemKind::Hook, "h1"),
|
||||
install(ItemKind::Plugin, "p2"),
|
||||
install(ItemKind::Skill, "s1"),
|
||||
];
|
||||
let preview = SettingsImportPreview::from_payload(&payload);
|
||||
assert_eq!(preview.global_plugin_install_count, 2);
|
||||
assert_eq!(preview.global_hook_install_count, 1);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user