Marketplace sync script: review fixes (round 1)

- Validate manifest structure up front; malformed items are skipped with a reason instead of aborting extraction; an unreadable manifest changes nothing (no removals). - Empty/whitespace settings.json reads as {}; non-object settings are left untouched; hook installs/updates/removals are reported and recorded only once their entries are actually merged; mv failures are checked. - Dangling symlinks at user paths count as occupied. - Removal paths are derived from kind+key, never taken from state.json. - A symlinked settings.json is written through, not replaced. - mktemp failure emits a JSON report instead of exiting silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-09-27 09:24:16 -07:00
parent b73067019f
commit 5cbb4591fe
2 changed files with 408 additions and 60 deletions
@@ -93,9 +93,15 @@ fn run(env: &Env) -> SyncReport {

 /// Run the script under a specific shell (`sh` is dash on Ubuntu).
 fn run_with(env: &Env, shell: &str) -> SyncReport {
+    run_full(env, shell, &[])
+}
+
+/// Run the script with extra environment variables.
+fn run_full(env: &Env, shell: &str, extra: &[(&str, &str)]) -> SyncReport {
    let out = Command::new(shell)
        .arg(&env.script)
        .env_clear()
+        .envs(extra.iter().copied())
        .env("HOME", &env.home)
        .env(
            "PATH",
@@ -588,3 +594,260 @@ fn sync_skips_user_owned_skill_and_command() {
    assert_eq!(fs::read_to_string(&skill).unwrap(), "mine\n");
    assert_eq!(fs::read_to_string(&command).unwrap(), "mine\n");
 }
+
+// ── Review fix round 1 ──────────────────────────────────────────────────────
+
+fn install_all(env: &Env) {
+    let (files, manifest) = all_kinds(C1);
+    payload(env, &files, manifest);
+    let r = run(env);
+    assert_eq!(r.installed.len(), 4, "{r:?}");
+}
+
+fn assert_all_installed(env: &Env) {
+    let claude = env.home.join(".claude");
+    assert!(claude.join("agents/code-reviewer.md").is_file());
+    assert!(claude.join("skills/example-skill/SKILL.md").is_file());
+    assert!(claude.join("commands/example-command.md").is_file());
+    assert!(claude
+        .join("triple-c/hooks/notify-on-stop/notify.sh")
+        .is_file());
+}
+
+#[test]
+fn sync_malformed_item_neither_aborts_nor_removes() {
+    let Some(env) = env() else { return };
+    install_all(&env);
+
+    // Malformed entries first, then the valid ones; the still-selected agent
+    // has a non-string commit.
+    let (files, mut manifest) = all_kinds(C1);
+    manifest["items"][0]["commit"] = json!(7);
+    let items = manifest["items"].as_array().unwrap().clone();
+    let mut all = vec![
+        json!({ "kind": "agent", "key": { "x": 1 }, "marketplace": "m1", "commit": C1 }),
+        json!(5),
+        json!({ "kind": ["agent"], "key": "k", "marketplace": "m1", "commit": C1 }),
+        json!({ "kind": "plugin", "key": { "y": 1 }, "marketplace": "m1", "commit": C1, "slug": SLUG }),
+    ];
+    all.extend(items);
+    manifest["items"] = Value::Array(all);
+    payload(&env, &files, manifest);
+
+    let r = run(&env);
+
+    assert!(r.removed.is_empty(), "{r:?}");
+    assert!(r.errors.is_empty(), "{r:?}");
+    assert_all_installed(&env);
+    let skipped = r
+        .skipped
+        .iter()
+        .map(|s| s.item.as_str())
+        .collect::<Vec<_>>();
+    assert!(skipped.contains(&"agent:code-reviewer"), "{r:?}");
+    assert_eq!(r.skipped.len(), 5, "{r:?}");
+
+    // The carried-forward record is still owned: deselecting removes it.
+    payload(&env, &[], empty_manifest());
+    let r = run(&env);
+    assert_eq!(r.removed.len(), 4, "{r:?}");
+}
+
+#[test]
+fn sync_structurally_bad_manifest_removes_nothing() {
+    let Some(env) = env() else { return };
+    install_all(&env);
+    for bad in [
+        json!({ "version": 1 }),
+        json!({ "version": 1, "items": {}, "plugin_marketplaces": [] }),
+        json!({ "version": 1, "items": [], "plugin_marketplaces": "x" }),
+    ] {
+        payload(&env, &[], bad.clone());
+        let r = run(&env);
+        assert!(r.removed.is_empty(), "{bad}: {r:?}");
+        assert_eq!(r.errors.len(), 1, "{bad}: {r:?}");
+        assert_all_installed(&env);
+    }
+    // State survived: a real deselection still removes everything.
+    payload(&env, &[], empty_manifest());
+    assert_eq!(run(&env).removed.len(), 4);
+}
+
+#[test]
+fn sync_treats_blank_settings_as_empty() {
+    let Some(env) = env() else { return };
+    let settings_path = env.home.join(".claude/settings.json");
+    fs::create_dir_all(settings_path.parent().unwrap()).unwrap();
+    fs::write(&settings_path, "  \n\t\n").unwrap();
+    let (files, manifest) = all_kinds(C1);
+    payload(&env, &files, manifest);
+
+    let r = run(&env);
+
+    assert!(r.errors.is_empty(), "{r:?}");
+    assert!(
+        r.installed.contains(&"hook:notify-on-stop".to_string()),
+        "{r:?}"
+    );
+    assert_eq!(read_json(&settings_path)["hooks"], hook_settings());
+}
+
+#[test]
+fn sync_does_not_report_hooks_it_could_not_wire() {
+    let Some(env) = env() else { return };
+    let settings_path = env.home.join(".claude/settings.json");
+    fs::create_dir_all(settings_path.parent().unwrap()).unwrap();
+    for bad in ["[1]", "{\"a\":", "{} {}"] {
+        fs::write(&settings_path, bad).unwrap();
+        let (files, manifest) = all_kinds(C1);
+        payload(&env, &files, manifest);
+        let r = run(&env);
+        assert!(
+            !r.installed.contains(&"hook:notify-on-stop".to_string()),
+            "{bad}: {r:?}"
+        );
+        assert_eq!(r.errors.len(), 1, "{bad}: {r:?}");
+        assert_eq!(
+            fs::read_to_string(&settings_path).unwrap(),
+            bad,
+            "left untouched"
+        );
+    }
+    // Once settings.json is fixed the hook is installed for real.
+    fs::write(&settings_path, "{}").unwrap();
+    let (files, manifest) = all_kinds(C1);
+    payload(&env, &files, manifest);
+    let r = run(&env);
+    assert_eq!(r.installed, vec!["hook:notify-on-stop"], "{r:?}");
+    assert_eq!(read_json(&settings_path)["hooks"], hook_settings());
+}
+
+#[cfg(unix)]
+#[test]
+fn sync_skips_dangling_user_symlinks() {
+    use std::os::unix::fs::symlink;
+    let Some(env) = env() else { return };
+    let claude = env.home.join(".claude");
+    let agent = claude.join("agents/code-reviewer.md");
+    let skill = claude.join("skills/example-skill");
+    for p in [&agent, &skill] {
+        fs::create_dir_all(p.parent().unwrap()).unwrap();
+        symlink("/nonexistent/dotfiles/target", p).unwrap();
+    }
+    let (files, manifest) = all_kinds(C1);
+    payload(&env, &files, manifest);
+
+    let r = run(&env);
+
+    let skipped = sorted(r.skipped.iter().map(|s| s.item.clone()).collect());
+    assert_eq!(
+        skipped,
+        vec!["agent:code-reviewer", "skill:example-skill"],
+        "{r:?}"
+    );
+    for p in [&agent, &skill] {
+        assert_eq!(
+            fs::read_link(p).unwrap(),
+            Path::new("/nonexistent/dotfiles/target")
+        );
+    }
+}
+
+#[test]
+fn sync_removal_never_uses_paths_from_state() {
+    let Some(env) = env() else { return };
+    install_all(&env);
+    let claude = env.home.join(".claude");
+    let keep = claude.join("keep.txt");
+    fs::write(&keep, "keep").unwrap();
+    let state_path = claude.join("triple-c/marketplace/state.json");
+    let mut state = read_json(&state_path);
+    state["items"]["agent:code-reviewer"]["path"] = json!(format!("{}/", claude.display()));
+    state["items"]["skill:example-skill"]["path"] =
+        json!(format!("{}/.claude", env.home.display()));
+    state["items"]["command:example-command"]["path"] = json!(keep.display().to_string());
+    fs::write(&state_path, state.to_string()).unwrap();
+
+    payload(&env, &[], empty_manifest());
+    let r = run(&env);
+
+    assert_eq!(r.removed.len(), 4, "{r:?}");
+    assert_eq!(fs::read_to_string(&keep).unwrap(), "keep");
+    assert!(!claude.join("agents/code-reviewer.md").exists());
+    assert!(!claude.join("skills/example-skill").exists());
+    assert!(!claude.join("commands/example-command.md").exists());
+}
+
+#[cfg(unix)]
+#[test]
+fn sync_writes_through_a_symlinked_settings_json() {
+    use std::os::unix::fs::symlink;
+    let Some(env) = env() else { return };
+    let dotfiles = env.home.join("dotfiles/settings.json");
+    fs::create_dir_all(dotfiles.parent().unwrap()).unwrap();
+    fs::write(&dotfiles, r#"{"model":"opus"}"#).unwrap();
+    let settings_path = env.home.join(".claude/settings.json");
+    fs::create_dir_all(settings_path.parent().unwrap()).unwrap();
+    symlink(&dotfiles, &settings_path).unwrap();
+    let (files, manifest) = all_kinds(C1);
+    payload(&env, &files, manifest);
+
+    let r = run(&env);
+
+    assert!(r.errors.is_empty(), "{r:?}");
+    assert!(
+        fs::symlink_metadata(&settings_path)
+            .unwrap()
+            .file_type()
+            .is_symlink(),
+        "link kept"
+    );
+    let merged = read_json(&dotfiles);
+    assert_eq!(merged["model"], "opus");
+    assert_eq!(merged["hooks"], hook_settings());
+}
+
+#[test]
+fn sync_reports_when_mktemp_fails() {
+    let Some(env) = env() else { return };
+    let r = run_full(&env, "sh", &[("TMPDIR", "/nonexistent/triple-c-tmp")]);
+    assert_eq!(r.errors.len(), 1, "{r:?}");
+}
+
+#[cfg(unix)]
+#[test]
+fn sync_settings_move_failure_is_reported_and_not_recorded() {
+    use std::os::unix::fs::PermissionsExt;
+    let Some(env) = env() else { return };
+    // An `mv` that refuses to replace settings.json and delegates otherwise.
+    let mv = env.stub_dir.join("mv");
+    fs::write(
+        &mv,
+        "#!/bin/sh\nfor a; do last=$a; done\ncase \"$last\" in */settings.json) exit 1 ;; esac\nexec /bin/mv \"$@\"\n",
+    )
+    .unwrap();
+    fs::set_permissions(&mv, fs::Permissions::from_mode(0o755)).unwrap();
+    let (files, manifest) = all_kinds(C1);
+    payload(&env, &files, manifest);
+
+    let r = run(&env);
+
+    assert_eq!(r.errors.len(), 1, "{r:?}");
+    assert!(
+        !r.installed.contains(&"hook:notify-on-stop".to_string()),
+        "{r:?}"
+    );
+    let claude = env.home.join(".claude");
+    assert!(!claude.join("settings.json").exists());
+    let leftovers: Vec<_> = fs::read_dir(&claude)
+        .unwrap()
+        .filter_map(|e| e.ok())
+        .filter(|e| e.file_name().to_string_lossy().contains(".tmp."))
+        .collect();
+    assert!(leftovers.is_empty(), "temp file cleaned up");
+    let state = read_json(&claude.join("triple-c/marketplace/state.json"));
+    assert!(
+        state["items"].get("hook:notify-on-stop").is_none(),
+        "{state}"
+    );
+}