diff --git a/docs/subsystems/library.md b/docs/subsystems/library.md index e559bc6c..f038b092 100644 --- a/docs/subsystems/library.md +++ b/docs/subsystems/library.md @@ -20,6 +20,7 @@ agents RTK, a CLI that cuts shell output down before the model reads it. | Skills | A skill is a folder with a `SKILL.md`. It comes from GitHub, from a folder on this machine (linked, so editing the folder edits the skill) or from the market. Each agent gets a link or a marked copy (#896, `SkillHow`). An agent in a WSL distro always gets a copy. A copy's mark keeps the hash of what was copied, so a sync knows a copy unchanged since then is current without reading its files (`copyIsFresh`, #1031); other copies are hashed. A skill an agent has of its own (`foundSkills`, the page's "In your agents") is brought in with `ImportSkill`, or taken out of every agent that has it with `RemoveFoundSkill` (#1303): its folders and copies of the very same files, its shared `~/.agents/skills` entry and a folder of it in the library's own folder go to the backups, its links are taken away, and a folder elsewhere it only links to, or another skill by that name, stays. | [`skills.go`](../../internal/library/skills.go), [`skill_how.go`](../../internal/library/skill_how.go), [`skill_edits.go`](../../internal/library/skill_edits.go), [`skill_from.go`](../../internal/library/skill_from.go) | | Skills market | `MarketSkills` lists skills.sh's most installed and what its search finds. magpie's own skills (`featuredSkills`) go first, marked official: `magpie-quota`, in [magpie-community/plugins](https://github.com/magpie-community/plugins/tree/main/skills/magpie-quota), lets an agent read `magpie quota --json` and wait with `magpie quota wait`. Until skills.sh lists one it has no count of installs and no skills.sh link (`featured`). `InstallMarketSkill` installs any of them from its repository. | [`market.go`](../../internal/library/market.go) | | Skills a repository added | A check for updates (`CheckSkills`) also asks, for each GitHub repository and ref the library has skills from, GitHub's list of its files (`repoTree`, one API request, free when the ETag still matches). Only when that list has a skill folder beside the installed ones (under `newBase`, their deepest common folder) that the library doesn't have and hasn't offered does `findNew` fetch the repository for the skills' names and descriptions. `SeenSkills` records per `repo@ref` the paths the install picker showed and the ones set aside, so they aren't offered again. The finds live in memory (`newFound`) and reach the page as `View.NewSkills`; `AddNewSkills` gives each to the agents that have its repository's other skills, and `IgnoreNewSkills` sets it aside (`/api/library/skills/{add-new,ignore-new}`). | [`skillnew.go`](../../internal/library/skillnew.go), [`skillcheck.go`](../../internal/library/skillcheck.go) | +| Skill updates | A skill from GitHub (`kind: github`, a market install among them) keeps the commit it was fetched at and `Hash`, the hash of its files as fetched. `CheckSkills` asks GitHub for each one's last commit and keeps the result in memory; `View.CheckDue` says none was checked since magpie started, or not for `checkEvery` (12 hours), and the page then checks by itself as the skills open (#1449). `editState` says a skill was changed here when the library's folder no longer hashes to `Hash` (`.DS_Store` aside), or an agent's copy was edited since its mark and not yet taken in; the view's `edited` says so. `UpdateSkill` refuses such a skill with `*EditedError` unless told to replace it (the page's own Update asks first; the CLI takes `--replace-edits`), and `UpdateSkills`/`UpdateSomeSkills` leave it as it is, in `Unupdated`. A replaced version that was changed, or that can't be told (no `Hash`), is moved to the backups (`keepReplaced`). Skills the skills CLI manages (`npx skills`, `~/.agents/.skill-lock.json`) are only grouped: the CLI updates them (`npx skills update`). A linked folder, a skill imported from an agent and a `From`-only skill have no remote to check; one CC Switch installed (`Origin`) can be updated by hand but isn't checked. | [`skill_changed.go`](../../internal/library/skill_changed.go), [`skillcheck.go`](../../internal/library/skillcheck.go), [`skills.go`](../../internal/library/skills.go) | | Backups | Before a change first writes an agent file, `backups` copies it to `BackupDir`, one folder per change. The last `keepBackups` (30) changes are kept. | [`backup.go`](../../internal/library/backup.go) | | RTK | `SetRTK` runs rtk's own installer (`rtk init -g …`) for an agent being switched on. For one being switched off, it removes exactly what the installer wrote, using magpie's own code, which works even with rtk gone. `ReadRTK` reads each agent's files to say which have the hook. `rtkSpec.blocked` stops the installer where it would break an agent. A detected agent RTK has no hook for at all is still listed, from `rtkNoHook`, with the reason and a switch that can't be turned on (dsh: its hooks can allow or deny a command but not rewrite it, and `rtk init` has no `--agent dsh`). | [`rtk.go`](../../internal/library/rtk.go) | @@ -34,6 +35,7 @@ agents RTK, a CLI that cuts shell output down before the model reads it. - magpie takes away only what `Applied` or its mark says it wrote. A folder of the user's with a skill's name is never touched. The user's own fields in an MCP entry are kept. - An edit never silently loses. When an agent's copy of a skill and the library both changed, the newest edit wins, and the losing one is kept with the backups. +- An update from GitHub never silently replaces a skill changed here. It is refused, or skipped in an update of several, until the user says to replace it, and the version replaced goes to the backups. - A failure for one agent is a `Problem` in the result. It doesn't stop the others. - A server an agent can't reach is not written there, and `mcpFile.supports` says why. For example, Claude Desktop takes no remote server, and Codex, Goose, dsh, Grok, Command Code and Zed take no SSE. - A server's headers (remote) or environment (command) can name a variable as `${NAME}` (#1250). `library.json` keeps the placeholder, and `refsOf` gives each MCP format the agent's own syntax, with the agent's source cited beside it: `${NAME}` for most, `{env:NAME}` for OpenCode and MiMo, `${env:NAME}` for Cursor, and Codex's own keys (`bearer_token_env_var`, `env_http_headers`, `env_vars`). Goose gets `${NAME}` plus `env_keys`. dsh (DeepSeek Harness) gets a `!!js` expression it evaluates as it loads the server (#1435): a whole `${NAME}` is `!!js process.env.NAME`, and a value mixing text and variables is the text as JSON string literals joined with `(process.env.NAME ?? "")`, so the user's text is never run as code and an unset variable inside it is empty. A whole variable that isn't set makes dsh leave the server out as invalid. magpie reads only those forms back as `${NAME}`; any other `!!js` keeps the entry the user's. An agent with no syntax (Claude Desktop, Antigravity, Kimi, Cline, ZCode, Zed, Devin, Alma, Pi with pi-mcp-extension) is never written the value: `refsProblem` makes `supports` refuse the server, a magpie entry already there is taken out, and `AgentView.NoEnvRefs` greys its chip with the reason. Codex and Goose's extra keys are magpie's only while the server names a variable there (`refKeys`), so the user's own `env_vars` or `env_keys` are kept. `CheckServers` expands the variable from magpie's own environment, and one that isn't set is `novar`, naming it. @@ -49,9 +51,9 @@ agents RTK, a CLI that cuts shell output down before the model reads it. ## Verification ```sh -go test -tags nogui ./internal/library -run 'TestCheckSkillsFindsNewOnes|TestNewBase|TestTargets|TestServer|TestImportServer|TestCheck|TestInstructions|TestInstructionSets|TestSkill|TestRemoveFoundSkill|TestInstallSet|TestInstallLists|TestEverySkillAgents|TestEveryServerAgents|TestSomeServersAgents|TestRemoveServers|TestRTK|TestEnvRefs|TestDshRefRoundTrip|TestDshImportKeepsJS|TestAgentMCPFiles|TestCodeBuddyMCPFile|TestZedMCP|TestOwnServerFromElsewhereRunsThisMagpie|TestUsersOwnServerStaysAsItIs|TestCarriedOwnServerIsEachComputersMagpie' +go test -tags nogui ./internal/library -run 'TestCheckSkillsFindsNewOnes|TestNewBase|TestUpdateKeeps|TestCheckDue|TestCheckSkillsMarksItDone|TestTargets|TestServer|TestImportServer|TestCheck|TestInstructions|TestInstructionSets|TestSkill|TestRemoveFoundSkill|TestInstallSet|TestInstallLists|TestEverySkillAgents|TestEveryServerAgents|TestSomeServersAgents|TestRemoveServers|TestRTK|TestEnvRefs|TestDshRefRoundTrip|TestDshImportKeepsJS|TestAgentMCPFiles|TestCodeBuddyMCPFile|TestZedMCP|TestOwnServerFromElsewhereRunsThisMagpie|TestUsersOwnServerStaysAsItIs|TestCarriedOwnServerIsEachComputersMagpie' go test -tags nogui ./internal/library -node --test internal/gui/tests/library-items.test.cjs internal/gui/tests/library-rtk-gain.test.cjs internal/gui/tests/library-rtk-nohook.test.cjs internal/gui/tests/library-servers-bulk.test.cjs internal/gui/tests/library-env-refs.test.cjs internal/gui/tests/library-zed-mcp.test.cjs internal/gui/tests/library-workbuddy-trust.test.cjs internal/gui/tests/skill-new-in-repo.test.cjs internal/gui/tests/remove-found-skill.test.cjs +node --test internal/gui/tests/library-items.test.cjs internal/gui/tests/library-rtk-gain.test.cjs internal/gui/tests/library-rtk-nohook.test.cjs internal/gui/tests/library-servers-bulk.test.cjs internal/gui/tests/library-env-refs.test.cjs internal/gui/tests/library-zed-mcp.test.cjs internal/gui/tests/library-workbuddy-trust.test.cjs internal/gui/tests/skill-new-in-repo.test.cjs internal/gui/tests/skill-update-edited.test.cjs internal/gui/tests/remove-found-skill.test.cjs ``` The package's `TestMain` runs in a home of its own (`testenv`), never the diff --git a/internal/gui/assets/i18n.js b/internal/gui/assets/i18n.js index e556ac13..297f07c0 100644 --- a/internal/gui/assets/i18n.js +++ b/internal/gui/assets/i18n.js @@ -3687,6 +3687,14 @@ const I18N = { "Changed on GitHub since it was installed": "安装后 GitHub 上已有改动", "Not checked": "未能检查", "Up to date with GitHub": "与 GitHub 一致", + "Changed here": "本地已改动", + "You changed it since it was fetched from GitHub. Updating asks before it replaces your changes.": "从 GitHub 获取后你在本地改动过它。更新前会先询问,不会直接覆盖你的改动。", + "Changed here: asks before it replaces your changes": "本地已改动:更新前会先询问,不会直接覆盖", + "Replace your changes to {name}?": "用 GitHub 的版本替换你对 {name} 的改动?", + "You changed {name} here since it was fetched from GitHub. Updating puts GitHub's version in its place. Yours is kept in magpie's backups (Backups, at the foot of the Library).": "从 GitHub 获取 {name} 后你在本地改动过它。更新会用 GitHub 的版本替换它,你的版本会保存在 magpie 的备份里(资源库底部的「备份」)。", + "Keep mine": "保留我的", + "{name} is up to date; your version is in the backups": "{name} 已是最新,你的版本已存入备份", + "Kept as you changed them: {names}. Update each from its row to replace your changes.": "已保留你改动过的:{names}。如要用 GitHub 的版本替换,请在各自的行里单独更新。", "Update from GitHub ({repo}, as CC Switch installed it)": "从 GitHub 更新({repo},CC Switch 安装时的来源)", "Read {name}'s SKILL.md": "查看 {name} 的 SKILL.md", "Remove {name}?": "移除 {name}?", @@ -7559,6 +7567,14 @@ const I18N = { "Changed on GitHub since it was installed": "安裝後 GitHub 上已有改動", "Not checked": "未能檢查", "Up to date with GitHub": "與 GitHub 一致", + "Changed here": "本機已改動", + "You changed it since it was fetched from GitHub. Updating asks before it replaces your changes.": "從 GitHub 取得後你在本機改動過它。更新前會先詢問,不會直接覆蓋你的改動。", + "Changed here: asks before it replaces your changes": "本機已改動:更新前會先詢問,不會直接覆蓋", + "Replace your changes to {name}?": "用 GitHub 的版本取代你對 {name} 的改動?", + "You changed {name} here since it was fetched from GitHub. Updating puts GitHub's version in its place. Yours is kept in magpie's backups (Backups, at the foot of the Library).": "從 GitHub 取得 {name} 後你在本機改動過它。更新會用 GitHub 的版本取代它,你的版本會保存在 magpie 的備份裡(資源庫底部的「備份」)。", + "Keep mine": "保留我的", + "{name} is up to date; your version is in the backups": "{name} 已是最新,你的版本已存入備份", + "Kept as you changed them: {names}. Update each from its row to replace your changes.": "已保留你改動過的:{names}。如要用 GitHub 的版本取代,請在各自的列中單獨更新。", "Update from GitHub ({repo}, as CC Switch installed it)": "從 GitHub 更新({repo},CC Switch 安裝時的來源)", "Read {name}'s SKILL.md": "檢視 {name} 的 SKILL.md", "Remove {name}?": "移除 {name}?", @@ -11417,6 +11433,14 @@ const I18N = { "Changed on GitHub since it was installed": "インストール後に GitHub で変更されています", "Not checked": "未確認", "Up to date with GitHub": "GitHub と同じ", + "Changed here": "ローカルで変更済み", + "You changed it since it was fetched from GitHub. Updating asks before it replaces your changes.": "GitHub から取得した後、ここで変更されています。更新は変更を置き換える前に確認します。", + "Changed here: asks before it replaces your changes": "ローカルで変更済み:変更を置き換える前に確認します", + "Replace your changes to {name}?": "{name} への変更を GitHub の版で置き換えますか?", + "You changed {name} here since it was fetched from GitHub. Updating puts GitHub's version in its place. Yours is kept in magpie's backups (Backups, at the foot of the Library).": "{name} は GitHub から取得した後、ここで変更されています。更新すると GitHub の版に置き換わります。あなたの版は magpie のバックアップ(ライブラリ下部の「バックアップ」)に残ります。", + "Keep mine": "自分の版を残す", + "{name} is up to date; your version is in the backups": "{name} は最新です。あなたの版はバックアップにあります", + "Kept as you changed them: {names}. Update each from its row to replace your changes.": "変更済みのため残しました:{names}。変更を置き換えるには、それぞれの行から更新してください。", "Update from GitHub ({repo}, as CC Switch installed it)": "GitHub からアップデート({repo}、CC Switch がインストールした取得元)", "Read {name}'s SKILL.md": "{name} の SKILL.md を読む", "Remove {name}?": "{name} を削除しますか?", @@ -15295,6 +15319,14 @@ const I18N = { "Changed on GitHub since it was installed": "Seit der Installation auf GitHub geändert", "Not checked": "Nicht geprüft", "Up to date with GitHub": "Auf dem Stand von GitHub", + "Changed here": "Hier geändert", + "You changed it since it was fetched from GitHub. Updating asks before it replaces your changes.": "Du hast ihn geändert, seit er von GitHub geholt wurde. Ein Update fragt, bevor es deine Änderungen ersetzt.", + "Changed here: asks before it replaces your changes": "Hier geändert: fragt, bevor es deine Änderungen ersetzt", + "Replace your changes to {name}?": "Deine Änderungen an {name} ersetzen?", + "You changed {name} here since it was fetched from GitHub. Updating puts GitHub's version in its place. Yours is kept in magpie's backups (Backups, at the foot of the Library).": "Du hast {name} hier geändert, seit er von GitHub geholt wurde. Das Update setzt die Version von GitHub an seine Stelle. Deine bleibt in den Backups von magpie (Backups, unten in der Bibliothek).", + "Keep mine": "Meine behalten", + "{name} is up to date; your version is in the backups": "{name} ist aktuell; deine Version liegt in den Backups", + "Kept as you changed them: {names}. Update each from its row to replace your changes.": "Behalten, wie du sie geändert hast: {names}. Aktualisiere jeden in seiner Zeile, um deine Änderungen zu ersetzen.", "Update from GitHub ({repo}, as CC Switch installed it)": "Von GitHub aktualisieren ({repo}, wie CC Switch es installiert hat)", "Read {name}'s SKILL.md": "SKILL.md von {name} lesen", "Remove {name}?": "{name} entfernen?", diff --git a/internal/gui/assets/library.js b/internal/gui/assets/library.js index 452ba8a6..f36cbd8c 100644 --- a/internal/gui/assets/library.js +++ b/internal/gui/assets/library.js @@ -21,6 +21,7 @@ let probe = null; // skills found at a source: { source, candidates, pick:Set, agents:Set } let probing = false; let checking = false; // asking GitHub which skills it has changed + let autoChecked = false; // the page checked by itself, as the server said it was due (#1449) let modal = null; // what the library has open in the dialog const GLYPH = { @@ -1796,7 +1797,14 @@ if (checking) c.classList.add("busy"); rh.append(c); } - const stale = lib.skills.filter((s) => s.check?.status === "update"); + // not checked since magpie started, or not for a while: the page + // checks by itself, once, after it is drawn (#1449) + if (lib.checkDue && !checking && !autoChecked) { + autoChecked = true; + setTimeout(() => checkSkills(true), 0); + } + // one changed here is updated from its own row, which asks first + const stale = lib.skills.filter((s) => s.check?.status === "update" && !s.edited); if (stale.length) { const n = stale.length; const u = button(t("Update {n}", { n }), "action lib-updall", async (e, b) => { @@ -2199,7 +2207,8 @@ who.append(el("div", "sub", filtering && hits.length !== n ? t("{n} of {total} skills", { n: hits.length, total: n }) : n === 1 ? t("1 skill") : t("{n} skills", { n }))); const tags = el("div", "lib-tags"); - const stale = g.skills.filter((s) => s.check?.status === "update"); + // one changed here is updated from its own row, which asks first (#1449) + const stale = g.skills.filter((s) => s.check?.status === "update" && !s.edited); if (stale.length) { const u = button(t("Update {n}", { n: stale.length }), "action lib-updall", async (e, b) => { b.classList.add("busy"); @@ -2917,9 +2926,19 @@ take(v); const res = v.result || {}; const up = res.updated?.length || 0, no = res.unupdated || []; + // one changed here is left as it is (#1449): said apart from those + // that couldn't be fetched + const isEdited = (p) => lib.skills.some((s) => s.edited && "skill:" + s.name === p.what); + const kept = no.filter(isEdited).map((p) => p.what.replace(/^skill:/, "")), failed = no.filter((p) => !isEdited(p)); if (no.length) { - const p = no[0]; - status(t("{name} wasn't updated: {error}", { name: p.what.replace(/^skill:/, ""), error: p.error }) + (no.length > 1 ? " " + t("(and {n} more)", { n: no.length - 1 }) : "") + (up ? " · " + t("{n} up to date", { n: up }) : ""), "warn", 8000); + const parts = []; + if (failed.length) { + const p = failed[0]; + parts.push(t("{name} wasn't updated: {error}", { name: p.what.replace(/^skill:/, ""), error: p.error }) + (failed.length > 1 ? " " + t("(and {n} more)", { n: failed.length - 1 }) : "")); + } + if (kept.length) parts.push(t("Kept as you changed them: {names}. Update each from its row to replace your changes.", { names: kept.join(", ") })); + if (up) parts.push(t("{n} up to date", { n: up })); + status(parts.join(" · "), "warn", 10000); } else report(res, t("{n} skills up to date", { n: up })); render(); } catch (e) { @@ -3226,7 +3245,9 @@ // Which skills GitHub changed since they were installed: each row says, // and the heading offers to update just those. - async function checkSkills() { + // quiet is the page's own check as it opens the skills: it says only + // what there is to update or add, and nothing when there is nothing. + async function checkSkills(quiet) { if (checking) return; checking = true; render(); @@ -3238,12 +3259,17 @@ let msg = n ? (n === 1 ? t("1 skill has an update") : t("{n} skills have updates", { n })) : t("Every skill is up to date"); const more = lib.newSkills?.length || 0; if (more) msg += " · " + (more === 1 ? t("1 more skill in their repositories") : t("{n} more skills in their repositories", { n: more })); - if (unknown.length) { + if (quiet) { + const said = []; + if (n) said.push(n === 1 ? t("1 skill has an update") : t("{n} skills have updates", { n })); + if (more) said.push(more === 1 ? t("1 more skill in their repositories") : t("{n} more skills in their repositories", { n: more })); + if (said.length) status(said.join(" · "), "ok"); + } else if (unknown.length) { msg += " · " + t("{n} couldn't be checked: {error}", { n: unknown.length, error: checkError(unknown[0].check) }); status(msg, "warn", 8000); } else status(msg, "ok"); } catch (e) { - status(e.message, "err", 6000); + if (!quiet) status(e.message, "err", 6000); } checking = false; // drawn after the await, a page that can't be drawn says why, as one @@ -3405,6 +3431,9 @@ } else if (c?.status === "unknown") { nm.append(tag(t("Not checked"), "lib-unchecked", checkError(c))); } + // changed here since it was fetched from GitHub (#1449): an update asks + // before it replaces that + if (s.edited) nm.append(tag(t("Changed here"), "warn lib-edited", t("You changed it since it was fetched from GitHub. Updating asks before it replaces your changes."))); // a copy in an agent that differs from the library's skill (#896): a // sync makes it again if (s.behind?.length) { @@ -3437,6 +3466,7 @@ const acts = el("div", "lib-rowacts"); if (s.kind === "github" || s.origin) { const u = button("", "lib-icon", async (e, b) => { + if (s.edited) return confirmUpdateEdited(s); b.classList.add("busy"); await change("skills/update", { name: s.name }, t("{name} is up to date", { name: s.name })); b.classList.remove("busy"); @@ -3445,6 +3475,7 @@ u.title = s.origin ? t("Update from GitHub ({repo}, as CC Switch installed it)", { repo: s.origin.replace(/^https:\/\/github\.com\//, "") }) : t("Update from GitHub"); if (c?.status === "current") u.title += "\n" + t("Up to date with GitHub") + "\n" + checkLine(c); else if (c?.status === "update") u.title += "\n" + checkLine(c); + if (s.edited) u.title += "\n" + t("Changed here: asks before it replaces your changes"); acts.append(u); } const rm = button("", "lib-icon danger", () => confirmRemoveSkill(s)); @@ -3458,6 +3489,26 @@ return row; } + // An update of a skill changed here replaces the change (#1449): asked in + // the page, and the version replaced is kept with the backups. + function confirmUpdateEdited(s) { + const ed = el("div", "editor lib-editor lib-update-edited"); + const head = el("div", "ehead"); + head.append(glyph(GLYPH.up), el("b", "", t("Replace your changes to {name}?", { name: s.name }))); + ed.append(head); + ed.append(el("p", "lib-confirm", t("You changed {name} here since it was fetched from GitHub. Updating puts GitHub's version in its place. Yours is kept in magpie's backups (Backups, at the foot of the Library).", { name: s.name }))); + const bar = el("div", "bar"); + const go = button(t("Update"), "primary", async () => { + go.disabled = true; + if (await change("skills/update", { name: s.name, replace: true }, t("{name} is up to date; your version is in the backups", { name: s.name }))) closeLibModal(true); + else go.disabled = false; + }); + bar.append(el("span", "grow"), button(t("Keep mine"), "", closeLibModal), go); + ed.append(bar); + modal = { save: () => go.click() }; + openLib(ed); + } + function confirmRemoveSkill(s) { const ed = el("div", "editor lib-editor"); const head = el("div", "ehead"); diff --git a/internal/gui/library.go b/internal/gui/library.go index 2a269993..c6d5df40 100644 --- a/internal/gui/library.go +++ b/internal/gui/library.go @@ -345,6 +345,9 @@ func libraryRoutes(mux *http.ServeMux, w Windows) { Keep bool // a project removed keeps what magpie put in it On bool // every skill or server given to the agents, or taken from them How string // link or copy: how skills are given to Agent, or to every agent with none (#896) + // Replace updates a skill changed here all the same, its + // changed version kept with the backups (#1449) + Replace bool library.InstructionsChange } if err := json.NewDecoder(r.Body).Decode(&in); err != nil { @@ -377,7 +380,7 @@ func libraryRoutes(mux *http.ServeMux, w Windows) { case "skills/install": res, err = library.InstallSkills(in.Source, in.Paths, in.Agents) case "skills/update": - res, err = library.UpdateSkill(in.Name) + res, err = library.UpdateSkill(in.Name, in.Replace) case "skills/update-all": res, err = library.UpdateSkills() case "skills/update-some": diff --git a/internal/gui/tests/skill-update-edited.test.cjs b/internal/gui/tests/skill-update-edited.test.cjs new file mode 100644 index 00000000..91a24674 --- /dev/null +++ b/internal/gui/tests/skill-update-edited.test.cjs @@ -0,0 +1,146 @@ +// Run with Node's test runner and Playwright on the module path; see README.md. +// #1449 (YangZhiqiang98): Library → Skills should find which skills have +// updates and update them. The page now checks by itself when the server +// says a check is due (none since magpie started, or not for 12 hours), +// once, and says only what there is to update. A skill changed here since it +// was fetched from GitHub carries "Changed here"; its row's Update asks in +// the page before it replaces the change (Keep mine sends nothing, Update +// sends replace), and "Update N" leaves it out. A click never scrolls. In +// English and Chinese, in Chromium and WebKit, wide and narrow. No backend: +// the API is faked here. +const assert = require("node:assert/strict"); +const fs = require("node:fs/promises"); +const path = require("node:path"); +const { test } = require("node:test"); +const { chromium, webkit } = require("playwright"); + +const assets = path.resolve(__dirname, "../assets"); +const HOME = "/Users/lionel"; +const REPO = "anthropics/skills"; +const agent = (id, name) => ({ id, name, icon: "", skills: `${HOME}/.${id}/skills` }); +const sk = (name, extra) => ({ name, description: name + " skill", kind: "github", source: `https://github.com/${REPO}/tree/HEAD/skills/${name}`, agents: ["claude"], ...extra }); +const mine = (name) => ({ name, description: name + " of mine", kind: "folder", source: `${HOME}/skills/${name}`, agents: ["claude"] }); +const lib = (st) => ({ + dir: `${HOME}/.magpie/library`, backups: `${HOME}/.magpie/backups`, home: HOME, + agents: [agent("claude", "Claude Code"), agent("codex", "Codex")], + instructions: { agents: [] }, servers: [], foundServers: [], projects: [], foundSkills: [], newSkills: [], + checkDue: !st.checked, + skills: [ + sk("pdf", { edited: true, check: st.checked ? { name: "pdf", status: "update", commit: "c2", message: "pdf: forms" } : undefined }), + sk("docx", { check: st.checked ? { name: "docx", status: "update", commit: "c2", message: "docx: tables" } : undefined }), + sk("xlsx", { check: st.checked ? { name: "xlsx", status: "current", commit: "c1" } : undefined }), + ...Array.from({ length: 10 }, (_, i) => mine("note-" + i)), + ], +}); + +function server(lang, posts, st) { + return async (route) => { + const req = route.request(), url = new URL(req.url()); + if (url.pathname === "/boot.js") return route.fulfill({ contentType: "text/javascript", body: `window.bootPrefs = {lang:"${lang}",theme:"light",web:true};` }); + if (url.pathname === "/wails/runtime.js") return route.fulfill({ contentType: "text/javascript", body: "export const Window = {};" }); + if (url.pathname === "/api/state") return route.fulfill({ json: { agents: [], profiles: [], settings: { lang, theme: "light" } } }); + if (url.pathname === "/api/library") return route.fulfill({ json: lib(st) }); + if (url.pathname.startsWith("/api/library/skills/")) { + const what = url.pathname.split("/").pop(); + posts.push({ what, body: req.postDataJSON() }); + if (what === "check") st.checked = true; + const result = what === "update-some" ? { updated: ["docx"], changed: [] } : { changed: ["claude"] }; + return route.fulfill({ json: { ...lib(st), result } }); + } + if (url.pathname === "/api/plugins") return route.fulfill({ json: { plugins: [] } }); + if (url.pathname === "/api/usage/quotas") return route.fulfill({ json: [] }); + if (url.pathname === "/api/groups") return route.fulfill({ json: { groups: [] } }); + if (url.pathname === "/api/providers") return route.fulfill({ json: { providers: [], gateway: { running: true } } }); + if (url.pathname.startsWith("/api/")) return route.fulfill({ json: {} }); + const file = path.join(assets, url.pathname === "/" ? "index.html" : url.pathname); + const contentType = { ".html": "text/html", ".js": "text/javascript", ".css": "text/css", ".svg": "image/svg+xml", ".png": "image/png" }[path.extname(file)]; + await route.fulfill({ body: await fs.readFile(file), contentType }); + }; +} + +const press = async (page, loc) => { + const b = await loc.boundingBox(); + await page.mouse.click(b.x + b.width / 2, b.y + b.height / 2); +}; + +// every scrolled box's offset, to tell a click moved none of them +const scrolls = (page) => page.evaluate(() => [...document.querySelectorAll("*")].filter((e) => e.scrollTop).map((e) => (e.id || e.className) + ":" + e.scrollTop).concat("doc:" + document.scrollingElement.scrollTop).join(" ")); + +const words = { + en: { said: "2 skills have updates", edited: "Changed here", updN: "Update 1", head: "Replace your changes to pdf?", keep: "Keep mine", update: "Update", done: "pdf is up to date; your version is in the backups", tip: "You changed it since it was fetched from GitHub. Updating asks before it replaces your changes." }, + zh: { said: "2 个技能有更新", edited: "本地已改动", updN: "更新 1 个", head: "用 GitHub 的版本替换你对 pdf 的改动?", keep: "保留我的", update: "更新", done: "pdf 已是最新,你的版本已存入备份", tip: "从 GitHub 获取后你在本地改动过它。更新前会先询问,不会直接覆盖你的改动。" }, +}; + +const shots = process.env.ARTIFACT_DIR; + +for (const engine of (process.env.BROWSER ? [process.env.BROWSER] : ["chromium", "webkit"])) { + test(engine + ": skills are checked as the page opens, and a skill changed here asks before an update", async (t) => { + assert(["chromium", "webkit"].includes(engine), "BROWSER must be chromium or webkit"); + const browser = await (engine === "webkit" ? webkit.launch() : chromium.launch({ channel: "chromium" })); + const errors = []; + t.after(async () => { + if (errors.length) console.log(errors); + await browser.close(); + }); + for (const width of [860, 400]) for (const lang of ["en", "zh"]) { + await t.test(`${lang} at ${width}px`, async () => { + const w = words[lang], posts = [], st = { checked: false }; + const ctx = await browser.newContext({ viewport: { width, height: 1100 } }); + await ctx.addInitScript(() => { try { localStorage.setItem("magpie.libTab", "skills"); } catch {} }); + const page = await ctx.newPage(); + page.setDefaultTimeout(5000); + page.on("pageerror", (e) => errors.push(e.message)); + await page.route("**/*", server(lang, posts, st)); + await page.goto("http://magpie.test/"); + await page.locator('button[data-view="library"]').click(); + const v = page.locator("#view-library"); + // checked by itself, once, with no click + await v.locator(".lib-skill .lib-tag.lib-new").first().waitFor(); + await page.waitForTimeout(300); + assert.deepEqual(posts.map((p) => p.what), ["check"], "one check as the page opened"); + assert.equal((await page.locator("#status").textContent()).trim(), w.said); + + const pdf = v.locator(".lib-skill", { has: page.locator(".name", { hasText: /^pdf/ }) }); + const tag = pdf.locator(".lib-tag.lib-edited"); + assert.equal((await tag.textContent()).trim(), w.edited); + assert.equal(await tag.getAttribute("title"), w.tip); + assert.equal(await v.locator(".lib-skill .lib-tag.lib-edited").count(), 1, "only pdf was changed here"); + // Update N is for docx alone: pdf asks from its own row + const updN = v.locator(".row-head button.lib-updall", { hasText: w.updN }); + assert.equal(await updN.count(), 1, "Update N counts pdf"); + assert.equal(await page.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth), true, "no sideways scroll"); + + if (shots) await page.screenshot({ path: path.join(shots, `skill-edited-${engine}-${lang}-${width}.png`) }); + const before = await scrolls(page); + await press(page, pdf.locator(".lib-rowacts button.lib-icon").first()); + const modal = page.locator(".lib-update-edited"); + await modal.waitFor(); + assert.equal((await modal.locator(".ehead b").textContent()).trim(), w.head); + for (const b of await modal.locator(".bar button").all()) { + const box = await b.boundingBox(); + assert(box && box.x >= 0 && box.x + box.width <= width, `a button out of view: ${JSON.stringify(box)}`); + } + await page.waitForTimeout(300); + if (shots) await page.screenshot({ path: path.join(shots, `skill-edited-ask-${engine}-${lang}-${width}.png`) }); + assert.deepEqual(posts.map((p) => p.what), ["check"], "nothing sent before the answer"); + await press(page, modal.locator(".bar button", { hasText: w.keep })); + await modal.waitFor({ state: "detached" }); + assert.deepEqual(posts.map((p) => p.what), ["check"], "Keep mine sends nothing"); + assert.equal(await scrolls(page), before, "asking moved the page"); + + await press(page, pdf.locator(".lib-rowacts button.lib-icon").first()); + await modal.waitFor(); + await press(page, modal.locator(".bar button.primary", { hasText: w.update })); + await modal.waitFor({ state: "detached" }); + assert.deepEqual(posts[1], { what: "update", body: { name: "pdf", replace: true } }); + assert.equal((await page.locator("#status").textContent()).trim(), w.done); + + await press(page, updN); + await page.waitForTimeout(300); + assert.deepEqual(posts[2], { what: "update-some", body: { names: ["docx"] } }); + await ctx.close(); + }); + } + assert.deepEqual(errors, []); + }); +} diff --git a/internal/library/ccswitch_test.go b/internal/library/ccswitch_test.go index ff26e962..c1d62fb1 100644 --- a/internal/library/ccswitch_test.go +++ b/internal/library/ccswitch_test.go @@ -58,10 +58,10 @@ func TestCCSwitchSkillUpdates(t *testing.T) { if origins["pdf"] != "https://github.com/owner/repo/tree/main" || origins["mine"] != "" { t.Fatalf("origins %v", origins) } - if _, err := UpdateSkill("mine"); err == nil { + if _, err := UpdateSkill("mine", false); err == nil { t.Error("one CC Switch has no repository for was updated") } - ok(t)(UpdateSkill("pdf")) + ok(t)(UpdateSkill("pdf", false)) if asked[len(asked)-1] != "/owner/repo/main" { t.Errorf("asked %v", asked) } @@ -78,5 +78,5 @@ func TestCCSwitchSkillUpdates(t *testing.T) { if s := l.skill("pdf"); s.Source == nil || s.Source.Kind != "github" || s.Source.Repo != "owner/repo" || s.Source.Path != "skills/pdf" { t.Errorf("source %+v", s.Source) } - ok(t)(UpdateSkill("pdf")) // from GitHub now, like any other + ok(t)(UpdateSkill("pdf", false)) // from GitHub now, like any other } diff --git a/internal/library/library_test.go b/internal/library/library_test.go index 35a40df1..6e843064 100644 --- a/internal/library/library_test.go +++ b/internal/library/library_test.go @@ -768,7 +768,7 @@ func TestSkillsFromGitHub(t *testing.T) { t.Error("the tarball wrote outside its folder") } version = "two" - ok(t)(UpdateSkill("pdf")) + ok(t)(UpdateSkill("pdf", false)) v, _ := Read(nil) if v.Skills[0].Description != "PDFs two" || v.Skills[0].Source != in+"/pdf" { t.Errorf("after update: %+v", v.Skills[0]) diff --git a/internal/library/projects_test.go b/internal/library/projects_test.go index de8e9e41..fa6a75a8 100644 --- a/internal/library/projects_test.go +++ b/internal/library/projects_test.go @@ -195,7 +195,7 @@ func TestUpdateSkillRefreshesProjects(t *testing.T) { ok(t)(ProjectSkill(p, "docx", []string{"codex"})) } version = "two" - ok(t)(UpdateSkill("pdf")) + ok(t)(UpdateSkill("pdf", false)) ok(t)(UpdateSomeSkills([]string{"docx"})) for _, p := range []string{linked, copied} { for _, e := range []string{".claude/skills/pdf", ".agents/skills/docx"} { diff --git a/internal/library/skill_changed.go b/internal/library/skill_changed.go new file mode 100644 index 00000000..ccef15cc --- /dev/null +++ b/internal/library/skill_changed.go @@ -0,0 +1,113 @@ +package library + +// A skill from GitHub can be changed on this machine after it was +// installed: the user edits it in the library's folder, an agent it is +// linked into edits it there, or an agent's copy is edited and taken in +// (skill_edits.go). An update from GitHub takes its files in place of the +// library's, so it would undo such a change without a word (#1449). So +// the library tells a changed skill by the hash of its files as fetched +// (Skill.Hash): the page says it was changed here, an update of it alone +// asks first, an update of many leaves it as it is, and the version an +// update replaces is kept with the backups unless it is known to be +// GitHub's own. + +import ( + "fmt" + "os" + "path/filepath" + "slices" + "sync" + "time" +) + +// EditedError is an update refused because the skill was changed here +// since it was fetched from GitHub. +type EditedError struct{ Name string } + +func (e *EditedError) Error() string { + return fmt.Sprintf("%s was changed here since it was fetched from GitHub; updating it would replace those changes", e.Name) +} + +// edits remembers, for each skill's library folder, its hash as of when +// anything in it last changed, so a page read doesn't read every skill's +// files again. +var edits = struct { + sync.Mutex + m map[string]editMemo +}{m: map[string]editMemo{}} + +type editMemo struct { + at time.Time + hash string +} + +// libraryHash is the hash of the library's folder of a skill, leaving +// aside what Finder keeps in it: "" when it can't be read. +func libraryHash(name string) string { + dir := skillDir(name) + at := changedAt(dir) + edits.Lock() + m, ok := edits.m[dir] + edits.Unlock() + if ok && !at.IsZero() && m.at.Equal(at) { + return m.hash + } + h := hashFiles(dir, true) + if h != "" && !at.IsZero() { + edits.Lock() + edits.m[dir] = editMemo{at: at, hash: h} + edits.Unlock() + } + return h +} + +// editState tells whether a skill from GitHub was changed here since it +// was fetched: its library folder no longer holds the files fetched, or +// an agent's copy of it was edited since magpie made it and isn't taken +// in yet. known is false when that can't be told: magpie didn't keep the +// hash of what it fetched (installed before it did), or the folder can't +// be read. +func (l *Library) editState(s *Skill, targets []*Target) (edited, known bool) { + if s.Source == nil || s.Source.Kind != "github" || s.Hash == "" { + return false, false + } + lib := skillDir(s.Name) + if linked(lib) { + return false, false + } + h := libraryHash(s.Name) + if h == "" { + return false, false + } + // the fetched files may hold a .DS_Store of their own + if h != s.Hash && hashDir(lib) != s.Hash { + return true, true + } + for _, t := range targets { + if t.Skills == "" || t.Desktop != nil || !slices.Contains(s.Agents, t.Agent.ID) { + continue + } + p := filepath.Join(t.Skills, s.Name) + if !ours(p, s.Name) || linked(p) { + continue + } + made, ok := copiedAt(p) + if !ok || !changedAt(p).After(made) { + continue + } + if !sameTree(p, lib) { + return true, true + } + } + return false, true +} + +// keepReplaced moves the library's version of a skill an update replaces +// to the backups, where it is found as an edit taken over is. +func keepReplaced(name, old string) (string, error) { + dst := filepath.Join(BackupDir(), time.Now().Format("2006-01-02_15-04-05.000"), "library", "skills", name) + if err := os.MkdirAll(filepath.Dir(dst), 0o700); err != nil { + return "", err + } + return dst, move(old, dst) +} diff --git a/internal/library/skill_changed_test.go b/internal/library/skill_changed_test.go new file mode 100644 index 00000000..6e25050a --- /dev/null +++ b/internal/library/skill_changed_test.go @@ -0,0 +1,226 @@ +package library + +import ( + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" +) + +// fakeSkillTarball serves a repository whose skills/pdf says version. +func fakeSkillTarball(t *testing.T, version *string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(tarball(t, map[string]string{ + "skills/pdf/SKILL.md": "---\nname: pdf\ndescription: PDFs " + *version + "\n---\n", + "skills/pdf/forms.md": "forms " + *version, + "skills/docx/SKILL.md": "---\nname: docx\ndescription: Word " + *version + "\n---\n", + })) + })) + t.Cleanup(srv.Close) + old := tarballURL + tarballURL = func(repo, ref string) string { return srv.URL + "/" + repo + "/" + ref } + t.Cleanup(func() { tarballURL = old }) +} + +func skillView(t *testing.T, name string) SkillView { + t.Helper() + v, err := Read(nil) + if err != nil { + t.Fatal(err) + } + for _, s := range v.Skills { + if s.Name == name { + return s + } + } + t.Fatalf("no %s on the page", name) + return SkillView{} +} + +// keptBackups is every copy of a skill the backups keep as the library's. +func keptBackups(t *testing.T, name string) []string { + t.Helper() + got, _ := filepath.Glob(filepath.Join(BackupDir(), "*", "library", "skills", name, "forms.md")) + return got +} + +// #1449: a skill from GitHub the user changed in the library (by hand, or +// through an agent it is linked into) is said to be changed, and an update +// asks before replacing it: one of it alone is refused unless told to +// replace, an update of every skill leaves it as it is, and once replaced +// the changed version is in the backups. An unchanged one updates as +// before, with no backup of GitHub's old files. +func TestUpdateKeepsASkillChangedHere(t *testing.T) { + h := sandbox(t) + version := "one" + fakeSkillTarball(t, &version) + ok(t)(InstallSkills("owner/repo", []string{"skills/pdf", "skills/docx"}, []string{"claude"})) + if s := skillView(t, "pdf"); s.Edited { + t.Fatal("pdf is said to be changed as installed") + } + // Finder showing the folder is no change + write(t, filepath.Join(skillDir("pdf"), ".DS_Store"), "finder") + if s := skillView(t, "pdf"); s.Edited { + t.Fatal("a .DS_Store made pdf changed") + } + + // edited through Claude Code's link, which is the library's folder + write(t, filepath.Join(h, ".claude/skills/pdf/forms.md"), "my forms") + if s := skillView(t, "pdf"); !s.Edited { + t.Fatal("pdf edited here isn't said to be") + } + if s := skillView(t, "docx"); s.Edited { + t.Fatal("docx is said to be changed") + } + + version = "two" + _, err := UpdateSkill("pdf", false) + var edited *EditedError + if !errors.As(err, &edited) || edited.Name != "pdf" { + t.Fatalf("updating the edited pdf: %v", err) + } + if s := read(t, filepath.Join(skillDir("pdf"), "forms.md")); s != "my forms" { + t.Fatalf("the refused update wrote over the edit: %q", s) + } + + res, err := UpdateSkills() + if err != nil { + t.Fatal(err) + } + if !slices.Equal(res.Updated, []string{"docx"}) { + t.Errorf("updated %v, want docx only", res.Updated) + } + if len(res.Unupdated) != 1 || res.Unupdated[0].What != "skill:pdf" || !strings.Contains(res.Unupdated[0].Error, "changed here") { + t.Errorf("unupdated %+v", res.Unupdated) + } + if s := read(t, filepath.Join(skillDir("pdf"), "forms.md")); s != "my forms" { + t.Fatalf("updating every skill wrote over the edit: %q", s) + } + if got := keptBackups(t, "docx"); len(got) != 0 { + t.Errorf("docx, unchanged, was kept with the backups: %v", got) + } + + ok(t)(UpdateSkill("pdf", true)) + if s := read(t, filepath.Join(skillDir("pdf"), "forms.md")); s != "forms two" { + t.Fatalf("pdf replaced: %q", s) + } + got := keptBackups(t, "pdf") + if len(got) != 1 || read(t, got[0]) != "my forms" { + t.Fatalf("the edit replaced isn't in the backups: %v", got) + } + if s := skillView(t, "pdf"); s.Edited || s.Description != "PDFs two" { + t.Fatalf("pdf after the update: %+v", s) + } + if !ours(filepath.Join(h, ".claude/skills/pdf"), "pdf") { + t.Error("claude's link went in the update") + } +} + +// An agent given its skills as copies edits one: until a sync takes the +// edit in, the library's folder is GitHub's, and the copy is what was +// changed. An update asks all the same. +func TestUpdateKeepsACopyEditedInAnAgent(t *testing.T) { + h := sandbox(t) + version := "one" + fakeSkillTarball(t, &version) + ok(t)(InstallSkills("owner/repo", []string{"skills/pdf"}, []string{"claude"})) + ok(t)(SetSkillHow("", HowCopy)) + p := filepath.Join(h, ".claude/skills/pdf") + if linked(p) { + t.Fatal("claude's pdf is still a link") + } + if s := skillView(t, "pdf"); s.Edited { + t.Fatal("the copy is said to be changed as made") + } + later := time.Now().Add(2 * time.Second) + write(t, filepath.Join(p, "forms.md"), "claude's forms") + os.Chtimes(filepath.Join(p, "forms.md"), later, later) + if s := skillView(t, "pdf"); !s.Edited { + t.Fatal("the copy edited in Claude Code isn't said to be") + } + version = "two" + var edited *EditedError + if _, err := UpdateSkill("pdf", false); !errors.As(err, &edited) { + t.Fatalf("updating pdf edited in Claude Code's copy: %v", err) + } + if s := read(t, filepath.Join(p, "forms.md")); s != "claude's forms" { + t.Fatalf("the refused update wrote over the copy: %q", s) + } +} + +// A skill installed before magpie kept the hash of what it fetched can't +// be told changed or not: it isn't said to be, and an update keeps the +// version it replaces, in case it was. +func TestUpdateKeepsWhatItCantTell(t *testing.T) { + sandbox(t) + version := "one" + fakeSkillTarball(t, &version) + ok(t)(InstallSkills("owner/repo", []string{"skills/pdf"}, nil)) + mu.Lock() + l, err := load() + if err == nil { + l.skill("pdf").Hash = "" + err = l.save() + } + mu.Unlock() + if err != nil { + t.Fatal(err) + } + write(t, filepath.Join(skillDir("pdf"), "forms.md"), "maybe mine") + if s := skillView(t, "pdf"); s.Edited { + t.Fatal("pdf with no hash is said to be changed") + } + version = "two" + ok(t)(UpdateSkill("pdf", false)) + got := keptBackups(t, "pdf") + if len(got) != 1 || read(t, got[0]) != "maybe mine" { + t.Fatalf("what the update replaced isn't in the backups: %v", got) + } +} + +// The page checks the skills from GitHub by itself when they weren't +// checked since magpie started, or not for checkEvery; a library with none +// from there has nothing to check. +func TestCheckDue(t *testing.T) { + sandbox(t) + checkedAt.Store(0) + t.Cleanup(func() { checkedAt.Store(0) }) + version := "one" + fakeSkillTarball(t, &version) + if v, _ := Read(nil); v.CheckDue { + t.Fatal("due with no skills") + } + ok(t)(InstallSkills("owner/repo", []string{"skills/pdf"}, nil)) + if v, _ := Read(nil); !v.CheckDue { + t.Fatal("not due before any check") + } + checkedAt.Store(time.Now().Add(-time.Hour).UnixNano()) + if v, _ := Read(nil); v.CheckDue { + t.Fatal("due an hour after a check") + } + checkedAt.Store(time.Now().Add(-checkEvery - time.Minute).UnixNano()) + if v, _ := Read(nil); !v.CheckDue { + t.Fatal("not due a day after a check") + } +} + +// A check says it was made: the page doesn't check again as it opens. +func TestCheckSkillsMarksItDone(t *testing.T) { + sandbox(t) + checkedAt.Store(0) + t.Cleanup(func() { checkedAt.Store(0) }) + fakeSkillRepo(t, map[string]string{"skills/pdf/SKILL.md": "---\nname: pdf\ndescription: PDFs\n---\n"}) + ok(t)(InstallSkills("owner/marks", []string{"skills/pdf"}, nil)) + if _, err := CheckSkills(); err != nil { + t.Fatal(err) + } + if v, _ := Read(nil); v.CheckDue { + t.Fatal("due right after a check") + } +} diff --git a/internal/library/skillcheck.go b/internal/library/skillcheck.go index dddf5bba..751671df 100644 --- a/internal/library/skillcheck.go +++ b/internal/library/skillcheck.go @@ -84,6 +84,25 @@ func forgetCheck(name string) { checks.Unlock() } +// checkedAt is when the skills were last checked for updates (unix +// nanoseconds), 0 before the first check since magpie started. +var checkedAt atomic.Int64 + +// checkEvery is how long the Library page goes before it checks for +// updates by itself, as it opens the skills (#1449). +const checkEvery = 12 * time.Hour + +// checkDue is whether the page should check the library's skills from +// GitHub for updates: none checked since magpie started, or not for +// checkEvery. +func checkDue(l *Library) bool { + if !slices.ContainsFunc(l.Skills, func(s *Skill) bool { return s.Source != nil && s.Source.Kind == "github" }) { + return false + } + at := checkedAt.Load() + return at == 0 || time.Since(time.Unix(0, at)) >= checkEvery +} + // commit is one commit as GitHub's API lists it. type commit struct { SHA string `json:"sha"` @@ -177,7 +196,11 @@ func apiGet(u string) ([]byte, int, bool, error) { // hashDir is a hash of a skill's files, their names and what's in them, // as copyDir copies them. -func hashDir(dir string) string { +func hashDir(dir string) string { return hashFiles(dir, false) } + +// hashFiles is hashDir, leaving aside the .DS_Store files Finder leaves in +// a folder it shows when noFinder is set. +func hashFiles(dir string, noFinder bool) string { h := sha256.New() err := filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { if err != nil { @@ -193,7 +216,7 @@ func hashDir(dir string) string { case d.Type()&fs.ModeSymlink != 0: t, _ := os.Readlink(p) fmt.Fprintf(h, "link %s %s\n", rel, filepath.ToSlash(t)) - case d.Type().IsRegular() && rel != marker: + case d.Type().IsRegular() && rel != marker && !(noFinder && d.Name() == ".DS_Store"): f, err := os.Open(p) if err != nil { return err @@ -304,6 +327,7 @@ func CheckSkills() ([]SkillCheck, error) { checks.m[c.Name] = c } checks.Unlock() + checkedAt.Store(time.Now().UnixNano()) if out == nil { out = []SkillCheck{} } diff --git a/internal/library/skills.go b/internal/library/skills.go index b6e68f40..c2ad81a8 100644 --- a/internal/library/skills.go +++ b/internal/library/skills.go @@ -1058,8 +1058,10 @@ func installFrom(l *Library, p *Probe, paths, agents []string, shown bool, in *i } // UpdateSkill fetches a skill from GitHub again, in place: the agents' -// links go on pointing at it. -func UpdateSkill(name string) (*Result, error) { +// links go on pointing at it. One changed here since it was fetched is +// an *EditedError unless replace is set, and then the changed version is +// kept with the backups (#1449). +func UpdateSkill(name string, replace bool) (*Result, error) { mu.Lock() l, err := load() mu.Unlock() @@ -1072,16 +1074,29 @@ func UpdateSkill(name string) (*Result, error) { } f := &fetcher{} defer f.clean() + if edited, _ := l.editState(s, Targets()); edited && !replace { + return nil, &EditedError{Name: name} + } up, err := f.prepare(s) if err != nil { return nil, err } - return change(func(l *Library) error { return up.apply(l) }) + return change(func(l *Library) error { + targets := Targets() + if s := l.skill(name); s != nil && !replace { + // changed while it was fetched + if edited, _ := l.editState(s, targets); edited { + return &EditedError{Name: name} + } + } + return up.apply(l, targets) + }) } // UpdateSkills fetches again every skill that came from GitHub, each // repository once, and writes the agents once. A skill that couldn't be -// fetched is said in Unupdated; the others are updated all the same. +// fetched is said in Unupdated, as is one changed here since it was +// fetched, which is left as it is; the others are updated all the same. func UpdateSkills() (*Result, error) { return updateSkills(nil) } // UpdateSomeSkills is UpdateSkills for the skills named only: those a @@ -1131,8 +1146,15 @@ func updateSkills(names []string) (*Result, error) { wg.Wait() sort.Slice(failed, func(i, j int) bool { return failed[i].What < failed[j].What }) res, err := change(func(l *Library) error { + targets := Targets() for _, up := range ups { - if err := up.apply(l); err != nil { + if s := l.skill(up.name); s != nil { + if edited, _ := l.editState(s, targets); edited { + failed = append(failed, Problem{What: "skill:" + up.name, Error: (&EditedError{Name: up.name}).Error()}) + continue + } + } + if err := up.apply(l, targets); err != nil { failed = append(failed, Problem{What: "skill:" + up.name, Error: err.Error()}) } } @@ -1251,9 +1273,15 @@ func (f *fetcher) prepare(s *Skill) (*skillUpdate, error) { } // apply puts the fetched skill in the old one's place; the agents' links -// go on pointing at it. -func (up *skillUpdate) apply(l *Library) error { +// go on pointing at it. The old one goes to the backups unless it is +// known to be the files fetched before, unchanged. +func (up *skillUpdate) apply(l *Library, targets []*Target) error { name := up.name + keep := true + if s := l.skill(name); s != nil { + edited, known := l.editState(s, targets) + keep = edited || !known + } next, old := skillDir("."+name+".next"), skillDir("."+name+".old") os.RemoveAll(next) os.RemoveAll(old) @@ -1291,6 +1319,13 @@ func (up *skillUpdate) apply(l *Library) error { if old == "" { return nil } + if keep { + var lb *leftBehind + if _, err := keepReplaced(name, old); err != nil && !errors.As(err, &lb) { + // the update is in; what it replaced stays beside it, not lost + return fmt.Errorf("%s is updated, but the version it replaced couldn't be kept with the backups and is at %s: %w", name, old, err) + } + } return os.RemoveAll(old) } diff --git a/internal/library/skills_copy_test.go b/internal/library/skills_copy_test.go index c5f27772..cabd805d 100644 --- a/internal/library/skills_copy_test.go +++ b/internal/library/skills_copy_test.go @@ -43,7 +43,7 @@ func TestSkillCopyFollowsTheLibrary(t *testing.T) { } version = "two" - ok(t)(UpdateSkill("pdf")) + ok(t)(UpdateSkill("pdf", false)) if s := read(t, filepath.Join(p, "SKILL.md")); !strings.Contains(s, "PDFs two") { t.Fatalf("claude's copy after the update:\n%s", s) } diff --git a/internal/library/sync.go b/internal/library/sync.go index da188654..0e56afad 100644 --- a/internal/library/sync.go +++ b/internal/library/sync.go @@ -230,6 +230,10 @@ type SkillView struct { // Behind are the agents given it as a copy whose copy differs from it, // till the next sync makes it again Behind []string `json:"behind,omitempty"` + // Edited is set for one from GitHub changed here since it was fetched, + // in the library or in an agent's copy: an update replaces that, so + // the page asks first (#1449) + Edited bool `json:"edited,omitempty"` } // View is the Library page. @@ -248,6 +252,10 @@ type View struct { Backups string `json:"backups"` SkillGroups []SkillGroup `json:"skillGroups"` CopySkills bool `json:"copySkills"` // the library gives skills as copies (#896) + // CheckDue is set when the skills from GitHub weren't checked for + // updates since magpie started, or not for a while: the page checks + // them as it shows them (#1449) + CheckDue bool `json:"checkDue,omitempty"` } // Read is the whole page: the library, and what's found in the agents. @@ -356,6 +364,7 @@ func Read(problems []Problem) (*View, error) { } sv.Check = lastCheck(s.Name) sv.Behind = behind[s.Name] + sv.Edited, _ = l.editState(s, targets) v.Skills = append(v.Skills, sv) } v.FoundServers = foundServers(l) @@ -366,6 +375,7 @@ func Read(problems []Problem) (*View, error) { v.NewSkills = newSkills(l) v.Projects = projectViews(l, problems) v.SkillGroups = l.skillGroups() + v.CheckDue = checkDue(l) return v, nil } diff --git a/internal/tui/library.go b/internal/tui/library.go index f255b3fa..35629b7f 100644 --- a/internal/tui/library.go +++ b/internal/tui/library.go @@ -158,7 +158,7 @@ func (m model) updateLibrary(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } m.flash, m.flashOK = "fetching "+r.name+" from GitHub again…", true - return m, libCmd(func() (*library.Result, error) { return library.UpdateSkill(r.name) }, "updated "+r.name) + return m, libCmd(func() (*library.Result, error) { return library.UpdateSkill(r.name, false) }, "updated "+r.name) case "i": if r.own { m.flash, m.flashOK = r.name+" is put there by the agent's app itself, each time it starts: it stays as it is", false diff --git a/library_cli.go b/library_cli.go index df2b2f88..a4036a58 100644 --- a/library_cli.go +++ b/library_cli.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "io" "os" @@ -25,6 +26,7 @@ const libraryUsage = `magpie library what the library gives magpie library skill rm (skills are installed from the app's Library page) magpie library skill rm --all [--yes] every skill out of the library and the agents (asks first; --yes doesn't) magpie library skill update [name] fetch a skill from GitHub again; with no name, every one from there + (one changed here is left as it is; update it by name with --replace-edits) magpie library skill how link|copy [agent] give skills as links to the library's or as copies: every agent's way, or one agent's magpie library skill how default the agent goes the library's way again magpie library skill use-library an agent's own skill by that name is in the way: set it aside, link the library's @@ -130,9 +132,14 @@ func libraryCmd(args []string) error { res, err = library.UseLibrarySkill(rest[1], rest[2]) case len(rest) == 3 && rest[0] == "keep-own": res, err = library.KeepAgentSkill(rest[1], rest[2]) - case len(rest) == 2 && rest[0] == "update": - if res, err = library.UpdateSkill(rest[1]); err == nil { + case (len(rest) == 2 || len(rest) == 3 && rest[2] == "--replace-edits") && rest[0] == "update" && !strings.HasPrefix(rest[1], "-"): + // a skill changed here is updated only when asked to be, its + // changed version kept with the backups (#1449) + var edited *library.EditedError + if res, err = library.UpdateSkill(rest[1], len(rest) == 3); err == nil { fmt.Println(green.Render("✓"), rest[1], "is up to date") + } else if errors.As(err, &edited) { + err = fmt.Errorf("%w\n magpie library skill update %s --replace-edits takes GitHub's in its place, keeping yours with the backups", err, rest[1]) } case len(rest) == 1 && rest[0] == "update": res, err = library.UpdateSkills()