From f2bb092586d437d588b08d25494338c7dc1cfc83 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 10:16:27 -0700 Subject: [PATCH] Marketplace sync: keep installs the host could not build (final review M3) An install the host skipped (pinned commit missing from the cache, cache unreadable, item failing a tightened validation rule) never reached the manifest, so sync.sh treated it as deselected and deleted it from the container. The manifest now carries `held`: the state ids of such installs ("plugin:/" for plugins). The script counts them as still selected and carries their records forward, as it already does for items that fail inside the container. A removed marketplace is the one skip that still removes; a malformed `held` list changes nothing. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/marketplace/payload.rs | 70 +++++++++++++-- app/src-tauri/src/marketplace/sync.sh | 6 +- .../src/marketplace/sync_script_tests.rs | 87 +++++++++++++++++++ .../specs/2026-09-27-marketplace-design.md | 3 +- 4 files changed, 155 insertions(+), 11 deletions(-) diff --git a/app/src-tauri/src/marketplace/payload.rs b/app/src-tauri/src/marketplace/payload.rs index 52f2262..fa22c3d 100644 --- a/app/src-tauri/src/marketplace/payload.rs +++ b/app/src-tauri/src/marketplace/payload.rs @@ -100,6 +100,10 @@ pub fn build_payload(input: &PayloadInput) -> Result { let mut tar = TarWriter::new(); let mut items: Vec = Vec::new(); let mut skipped: Vec = Vec::new(); + // State ids (see sync.sh) of installs the host could not build this time: + // the container keeps what it has for them instead of treating them as + // deselected (final review M3). Only a removed source really removes. + let mut held: BTreeSet = BTreeSet::new(); let mut plugin_groups: BTreeMap = BTreeMap::new(); // Non-plugin items share one namespace in ~/.claude; plugins are namespaced // by their per-marketplace catalog, so they never collide. @@ -122,10 +126,22 @@ pub fn build_payload(input: &PayloadInput) -> Result { skip("its marketplace has been removed".to_string()); continue; }; - if !is_valid_item_key(&inst.key) || !is_valid_commit(&inst.commit) { + let state_id = match inst.kind { + ItemKind::Plugin => format!("plugin:{}/{}", marketplace_slug(&m.id), inst.key), + _ => label.clone(), + }; + let mut hold = |reason: String| { + held.insert(state_id.clone()); + skip(reason) + }; + if !is_valid_item_key(&inst.key) { skip("the saved install entry is invalid".to_string()); continue; } + if !is_valid_commit(&inst.commit) { + hold("the saved install entry is invalid".to_string()); + continue; + } if inst.kind != ItemKind::Plugin && taken.contains(&(inst.kind, inst.key.clone())) { skip(format!( "another marketplace's {label} is already installed" @@ -134,7 +150,7 @@ pub fn build_payload(input: &PayloadInput) -> Result { } let repo = git::cache_path(input.data_root, &m.id); if !git::has_commit(&repo, &inst.commit) { - skip(format!( + hold(format!( "pinned commit {} is not in the local cache of \"{}\" — refresh the marketplace", &inst.commit[..8], m.name @@ -144,7 +160,7 @@ pub fn build_payload(input: &PayloadInput) -> Result { let (tree, files) = match install_files(&repo, inst) { Ok(v) => v, Err(e) => { - skip(e); + hold(e); continue; } }; @@ -164,7 +180,7 @@ pub fn build_payload(input: &PayloadInput) -> Result { "commands" }; let Some(f) = files.first() else { - skip("has no files".to_string()); + hold("has no files".to_string()); continue; }; let path = format!("{dir}/{key}.md"); @@ -181,7 +197,7 @@ pub fn build_payload(input: &PayloadInput) -> Result { match rendered_hook_settings(&tree, key) { Ok(settings) => item["settings"] = settings, Err(e) => { - skip(e); + hold(e); continue; } } @@ -195,7 +211,7 @@ pub fn build_payload(input: &PayloadInput) -> Result { let mut entry = match plugin_catalog_entry(&tree, key) { Ok(e) => e, Err(e) => { - skip(e); + hold(e); continue; } }; @@ -242,8 +258,12 @@ pub fn build_payload(input: &PayloadInput) -> Result { .push(json!({ "slug": slug, "dir": format!("plugins/{slug}"), "plugins": group.keys })); } - let manifest = - json!({ "version": 1, "items": items, "plugin_marketplaces": plugin_marketplaces }); + let manifest = json!({ + "version": 1, + "items": items, + "plugin_marketplaces": plugin_marketplaces, + "held": held, + }); let bytes = serde_json::to_vec_pretty(&manifest).map_err(|e| e.to_string())?; tar.file("manifest.json", &bytes, false)?; @@ -466,6 +486,37 @@ mod tests { p.skipped[1].reason ); assert_eq!(p.manifest["items"].as_array().unwrap().len(), 1); + // Final review M3: host-side failures are held (the container keeps + // what it has); only a removed source really removes. + assert_eq!( + p.manifest["held"], + json!(["agent:code-reviewer", "agent:does-not-exist"]) + ); + } + + #[test] + fn a_plugin_that_cannot_be_built_is_held_under_its_marketplace() { + let Some(fx) = GitFixture::new() else { return }; + fx.with_all_kinds(); + let data = tempfile::tempdir().unwrap(); + cache(&fx, data.path()); + let installs = vec![inst(ItemKind::Plugin, "example-plugin", &"0".repeat(40))]; + let marketplaces = vec![market("m1aaaaaaaa")]; + let p = build_payload(&PayloadInput { + installs: &installs, + marketplaces: &marketplaces, + data_root: data.path(), + }) + .unwrap(); + assert_eq!(p.skipped.len(), 1); + assert_eq!( + p.manifest["held"], + json!([format!( + "plugin:{}/example-plugin", + marketplace_slug("m1aaaaaaaa") + )]) + ); + assert_eq!(p.manifest["plugin_marketplaces"], json!([])); } #[test] @@ -489,6 +540,7 @@ mod tests { assert_eq!(p.manifest["items"].as_array().unwrap().len(), 1); assert_eq!(p.skipped.len(), 1); assert!(p.skipped[0].reason.contains("another marketplace")); + assert_eq!(p.manifest["held"], json!([])); } #[test] @@ -502,7 +554,7 @@ mod tests { .unwrap(); assert_eq!( p.manifest, - json!({ "version": 1, "items": [], "plugin_marketplaces": [] }) + json!({ "version": 1, "items": [], "plugin_marketplaces": [], "held": [] }) ); assert!(unpack(&p.tar).contains_key("manifest.json")); } diff --git a/app/src-tauri/src/marketplace/sync.sh b/app/src-tauri/src/marketplace/sync.sh index befe3fb..642a285 100644 --- a/app/src-tauri/src/marketplace/sync.sh +++ b/app/src-tauri/src/marketplace/sync.sh @@ -168,7 +168,10 @@ if ! jq -e '.version == 1' "$MANIFEST" >/dev/null 2>&1; then fi # Nothing is changed (and, above all, nothing removed) unless the manifest is # structurally sound and every extraction below succeeds. -if ! jq -e '(.items | type) == "array" and (.plugin_marketplaces | type) == "array"' \ +# `held` (optional): state ids of installs the host could not build this time; +# they are kept exactly like a selected item that failed here. +if ! jq -e '(.items | type) == "array" and (.plugin_marketplaces | type) == "array" + and ((.held // []) | type == "array" and all(.[]; type == "string"))' \ "$MANIFEST" >/dev/null 2>&1; then malformed "the payload manifest is malformed, so nothing was changed" fi @@ -190,6 +193,7 @@ if ! { | [if .kind == "plugin" and (.slug | type) == "string" then "plugin:" + .slug + "/" + .key else .kind + ":" + .key end] | @tsv' \ "$MANIFEST" >"$R/manifest_ids" && + jq -r '(.held // [])[] | [.] | @tsv' "$MANIFEST" >>"$R/manifest_ids" && jq -r '.plugin_marketplaces[] | if type == "object" and (.slug | type) == "string" then .slug else "" end | [.] | @tsv' "$MANIFEST" >"$R/new_slugs" diff --git a/app/src-tauri/src/marketplace/sync_script_tests.rs b/app/src-tauri/src/marketplace/sync_script_tests.rs index 8a30fa6..585564d 100644 --- a/app/src-tauri/src/marketplace/sync_script_tests.rs +++ b/app/src-tauri/src/marketplace/sync_script_tests.rs @@ -1057,3 +1057,90 @@ fn a_slug_change_reinstalls_under_the_new_name_and_retires_the_old() { let r = run(&env); assert!(nothing_reported(&r), "{r:?}"); } + +// ── Host-side holds (final review M3) ──────────────────────────────────────── + +/// Everything `install_all` installed, plus plugin `p` from SLUG_A. +fn install_all_and_a_plugin(env: &Env) { + let (base, mut manifest) = all_kinds(C1); + let mut files: Vec<(String, String, bool)> = base + .iter() + .map(|(p, t, e)| (p.to_string(), t.to_string(), *e)) + .collect(); + files.push(( + format!("plugins/{SLUG_A}/.claude-plugin/marketplace.json"), + format!( + r#"{{"name":"triple-c-{SLUG_A}","owner":{{"name":"Triple-C"}},"plugins":[{{"name":"p","source":"./p"}}]}}"# + ), + false, + )); + files.push(( + format!("plugins/{SLUG_A}/p/.claude-plugin/plugin.json"), + r#"{"name":"p"}"#.to_string(), + false, + )); + manifest["items"].as_array_mut().unwrap().push( + json!({ "kind": "plugin", "key": "p", "marketplace": "m1", "commit": C1, "slug": SLUG_A }), + ); + manifest["plugin_marketplaces"] = + json!([{ "slug": SLUG_A, "dir": format!("plugins/{SLUG_A}"), "plugins": ["p"] }]); + let refs: Vec<(&str, &str, bool)> = files + .iter() + .map(|(p, t, e)| (p.as_str(), t.as_str(), *e)) + .collect(); + payload(env, &refs, manifest); + let r = run(env); + assert_eq!(r.installed.len(), 5, "{r:?}"); +} + +#[test] +fn held_items_are_kept_not_removed() { + let Some(env) = env() else { return }; + install_all_and_a_plugin(&env); + let settings_path = env.home.join(".claude/settings.json"); + let state_path = env.home.join(".claude/triple-c/marketplace/state.json"); + let state_before = read_json(&state_path)["items"].clone(); + + // The host could build none of them this time (cache gone, say). + fs::remove_file(&env.log).unwrap(); + let mut manifest = empty_manifest(); + manifest["held"] = json!([ + "agent:code-reviewer", + "skill:example-skill", + "command:example-command", + "hook:notify-on-stop", + format!("plugin:{SLUG_A}/p"), + ]); + payload(&env, &[], manifest); + let r = run(&env); + + assert!(nothing_reported(&r), "{r:?}"); + assert_all_installed(&env); + assert!(claude_log(&env).is_empty(), "{:?}", claude_log(&env)); + assert_eq!(read_json(&settings_path)["hooks"], hook_settings()); + assert_eq!(read_json(&state_path)["items"], state_before); + assert!(env + .home + .join(format!(".claude/triple-c/plugins/{SLUG_A}")) + .is_dir()); + + // A real deselection afterwards still removes everything. + payload(&env, &[], empty_manifest()); + let r = run(&env); + assert_eq!(r.removed.len(), 5, "{r:?}"); +} + +#[test] +fn a_malformed_held_list_changes_nothing() { + let Some(env) = env() else { return }; + install_all(&env); + for bad in [json!("agent:code-reviewer"), json!([1]), json!({})] { + let mut manifest = empty_manifest(); + manifest["held"] = bad.clone(); + payload(&env, &[], manifest); + let r = run(&env); + assert!(r.removed.is_empty(), "{bad}: {r:?}"); + assert_eq!(r.errors.len(), 1, "{bad}: {r:?}"); + assert_all_installed(&env); + } +} diff --git a/docs/superpowers/specs/2026-09-27-marketplace-design.md b/docs/superpowers/specs/2026-09-27-marketplace-design.md index 2ffe42b..21e29ed 100644 --- a/docs/superpowers/specs/2026-09-27-marketplace-design.md +++ b/docs/superpowers/specs/2026-09-27-marketplace-design.md @@ -233,7 +233,8 @@ in existing projects. Plugin commands additionally run under `flock /tmp/.triple agents/ skills//… commands/ hooks//… plugins/.claude-plugin/marketplace.json # generated: only selected plugins plugins//… # each at its own pin -manifest.json # effective set: kind, key, marketplace, commit, hook JSON +manifest.json # effective set: kind, key, marketplace, commit, hook JSON; + # `held`: ids the host could not build this time (kept as is) ``` uploaded to `~/.claude/triple-c/marketplace/incoming/` together with the sync script itself and