Marketplace sync script: review fixes (round 2)
Removal only derives a path from an exact <kind>:<key> state id with a known kind; any other record is dropped with an error and nothing is deleted (an id like "skill" used to remove ~/.claude/skills/skill). Invalid plugin records are dropped too, and a failed chmod 600 on settings.json is reported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -271,9 +271,16 @@ while read -r id; do
|
|||||||
if grep -qxF "$id" "$R/new_ids"; then continue; fi
|
if grep -qxF "$id" "$R/new_ids"; then continue; fi
|
||||||
# Still selected but failed this run: keep the old files and record.
|
# Still selected but failed this run: keep the old files and record.
|
||||||
if in_manifest "$id"; then carry_forward "$id"; continue; fi
|
if in_manifest "$id"; then carry_forward "$id"; continue; fi
|
||||||
|
# Only an exact "<kind>:<key>" with a known kind names a path; anything
|
||||||
|
# else in state is dropped without deleting anything.
|
||||||
|
case "$id" in
|
||||||
|
agent:* | skill:* | command:* | hook:*)
|
||||||
kind=${id%%:*}
|
kind=${id%%:*}
|
||||||
key=${id#*:}
|
key=${id#*:}
|
||||||
if ! valid_key "$key" || ! path=$(item_path "$kind" "$key"); then
|
;;
|
||||||
|
*) kind="" key="" ;;
|
||||||
|
esac
|
||||||
|
if [ -z "$kind" ] || ! valid_key "$key" || ! path=$(item_path "$kind" "$key"); then
|
||||||
fail "$id: dropped an unrecognised record from the marketplace state"
|
fail "$id: dropped an unrecognised record from the marketplace state"
|
||||||
continue
|
continue
|
||||||
fi
|
fi
|
||||||
@@ -330,7 +337,8 @@ if [ "$OLD_HOOKS" != "{}" ] || [ "$NEW_HOOKS" != "{}" ]; then
|
|||||||
' "$R/current.json" >"$tmp" 2>/dev/null &&
|
' "$R/current.json" >"$tmp" 2>/dev/null &&
|
||||||
jq -e 'type == "object"' "$tmp" >/dev/null 2>&1 &&
|
jq -e 'type == "object"' "$tmp" >/dev/null 2>&1 &&
|
||||||
mv -f "$tmp" "$target"; then
|
mv -f "$tmp" "$target"; then
|
||||||
chmod 600 "$target"
|
chmod 600 "$target" ||
|
||||||
|
fail "~/.claude/settings.json was updated but could not be made private (chmod 600)"
|
||||||
else
|
else
|
||||||
rm -f "$tmp"
|
rm -f "$tmp"
|
||||||
HOOKS_FAILED=1
|
HOOKS_FAILED=1
|
||||||
@@ -389,7 +397,11 @@ while read -r id; do
|
|||||||
if in_manifest "$id"; then carry_forward "$id"; continue; fi
|
if in_manifest "$id"; then carry_forward "$id"; continue; fi
|
||||||
key=${id#plugin:}
|
key=${id#plugin:}
|
||||||
slug=$(jq -r --arg id "$id" '.items[$id].slug // ""' "$STATE")
|
slug=$(jq -r --arg id "$id" '.items[$id].slug // ""' "$STATE")
|
||||||
if valid_key "$key" && valid_slug "$slug" && claude_cmd plugin uninstall "$key@triple-c-$slug"; then
|
if ! valid_key "$key" || ! valid_slug "$slug"; then
|
||||||
|
fail "$id: dropped an unrecognised record from the marketplace state"
|
||||||
|
continue
|
||||||
|
fi
|
||||||
|
if claude_cmd plugin uninstall "$key@triple-c-$slug"; then
|
||||||
report removed "$id"
|
report removed "$id"
|
||||||
else
|
else
|
||||||
fail "$id: uninstall failed"
|
fail "$id: uninstall failed"
|
||||||
|
|||||||
@@ -851,3 +851,36 @@ fn sync_settings_move_failure_is_reported_and_not_recorded() {
|
|||||||
"{state}"
|
"{state}"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn sync_drops_state_records_without_a_kind_key_id() {
|
||||||
|
let Some(env) = env() else { return };
|
||||||
|
install_all(&env);
|
||||||
|
let claude = env.home.join(".claude");
|
||||||
|
let mine = [
|
||||||
|
claude.join("skills/skill/SKILL.md"),
|
||||||
|
claude.join("agents/agent.md"),
|
||||||
|
];
|
||||||
|
for p in &mine {
|
||||||
|
fs::create_dir_all(p.parent().unwrap()).unwrap();
|
||||||
|
fs::write(p, "mine\n").unwrap();
|
||||||
|
}
|
||||||
|
let state_path = claude.join("triple-c/marketplace/state.json");
|
||||||
|
let mut state = read_json(&state_path);
|
||||||
|
for bogus in ["skill", "agent", "widget:x", "agent:a:b", "plugin:a:b"] {
|
||||||
|
state["items"][bogus] = json!({ "commit": C1, "path": "/" });
|
||||||
|
}
|
||||||
|
fs::write(&state_path, state.to_string()).unwrap();
|
||||||
|
|
||||||
|
payload(&env, &[], empty_manifest());
|
||||||
|
let r = run(&env);
|
||||||
|
|
||||||
|
assert_eq!(sorted(r.removed.clone()).len(), 4, "{r:?}");
|
||||||
|
assert_eq!(r.errors.len(), 5, "{r:?}");
|
||||||
|
assert!(claude_log(&env).is_empty(), "{:?}", claude_log(&env));
|
||||||
|
for p in &mine {
|
||||||
|
assert_eq!(fs::read_to_string(p).unwrap(), "mine\n", "{}", p.display());
|
||||||
|
}
|
||||||
|
let state = read_json(&state_path);
|
||||||
|
assert_eq!(state["items"], json!({}), "bogus records dropped");
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user