From db8e789435b67448456214e222e324e055ceb740 Mon Sep 17 00:00:00 2001 From: Lucas Machado Date: Mon, 10 Aug 2026 17:50:13 +0200 Subject: [PATCH] =?UTF-8?q?feat:=20loadout=20rm=20=E2=80=94=20artifact=20r?= =?UTF-8?q?emoval=20with=20clean=20uninstall?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the missing delete verb (spec 009): one confirmation-gated pass uninstalls an artifact from every agent home and deletes it from the store. Fills the MergeJSON removal hole with jsonRemove — the surgical inverse of jsonMerge — so removing an MCP or hook artifact now cleans the merged settings.json/.claude.json/.mcp.json entry instead of leaking it (closes the ROADMAP MCP-uninstall open question). - Drift safety: targets edited since install are skipped with a warning (--force removes, snapshot first); their install records are retained so a later --force still works. loadouts switch inherits this. - rm snapshots (trigger "rm") also capture the store manifest + content under the shared 'store' pseudo-agent (same layout as spec 008), so 'loadout restore' resurrects a regretted rm even without git. - Store.RemoveArtifact: manifest first, then content, path-safe. Co-Authored-By: Claude Fable 5 --- README.md | 5 +- ROADMAP.md | 2 +- docs/specs/009-artifact-removal.md | 204 +++++++++++++++ docs/specs/009-artifact-removal.plan.md | 41 +++ integration/loadouts_test.go | 20 +- integration/rm_test.go | 206 +++++++++++++++ internal/cli/completion.go | 6 +- internal/cli/loadouts.go | 2 +- internal/cli/rm.go | 145 +++++++++++ internal/cli/root.go | 1 + internal/engine/merge.go | 154 +++++++++++ internal/engine/merge_test.go | 169 ++++++++++++ internal/engine/removal.go | 296 +++++++++++++++++++-- internal/engine/removal_test.go | 330 +++++++++++++++++++++--- internal/engine/snapshot.go | 8 +- internal/engine/snapshot_test.go | 2 +- internal/store/store.go | 27 ++ internal/store/store_test.go | 73 ++++++ 18 files changed, 1622 insertions(+), 69 deletions(-) create mode 100644 docs/specs/009-artifact-removal.md create mode 100644 docs/specs/009-artifact-removal.plan.md create mode 100644 integration/rm_test.go create mode 100644 internal/cli/rm.go diff --git a/README.md b/README.md index 297e5b7..22c3a87 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,7 @@ loadout sync # install into detected agents (asks first) | `loadout restore [snapshot-id]` | List pre-write snapshots, or restore one (`--yes`) — recover files an overwrite replaced, including store files a `source-update` or `bundle-import` rewrote | | `loadout new ` | Scaffold a `skill`, `instruction`, `command`, `hook` or `mcp` — or draft a skill with `--ai "description"` via your logged-in agent | | `loadout add ` | Adopt an existing native artifact (skill/hook/mcp dir, command/instruction file) into the store | +| `loadout rm ...` | Remove artifacts: uninstall from every agent (files deleted, managed blocks stripped, merged JSON entries removed) and delete from the store — always asks first (`--dry-run`, `--yes`, `--force` for targets you edited since install, `--keep-store` to only uninstall) | | `loadout source add\|list\|remove\|import` | Track skills from external git repos (pinned in `sources.lock.yaml`); `add --alias =` and `import --as ` import under a different local name that still tracks the real upstream skill; `list` shows an offline STATE column (`ok`/`edited`/`legacy`) | | `loadout check` | Check sources for updates — `git ls-remote` only, nothing fetched or applied | | `loadout update [source]` | Fetch updates and classify every skill three-way (upstream-changed / locally-edited / conflict), apply after confirmation — your local edits are never discarded unless you pass `--force-discard` (`--yes` alone never does) | @@ -159,7 +160,7 @@ Where it lands (always deep-merged — only the `mcpServers.` entry loadou Project scope resolves against `--project `, or the current directory when it contains a `.git` (no walk-up, no guessing — without a project root the artifact reports `n/a`). -**Secrets never enter the store.** Env and header values under secret-looking keys (`*_KEY`, `*_TOKEN`, `*_SECRET`, `*password*`, `Authorization`) must be `${VAR}` placeholders — they are written to agent configs verbatim and resolve from your environment at agent runtime. Literal values are rejected at `new`/`add`, by `loadout doctor`, at bundle import, and again at sync render time. Removing an MCP artifact clears its install record but leaves the merged JSON entry in place (same as hook settings fragments) — clean it up manually for now. +**Secrets never enter the store.** Env and header values under secret-looking keys (`*_KEY`, `*_TOKEN`, `*_SECRET`, `*password*`, `Authorization`) must be `${VAR}` placeholders — they are written to agent configs verbatim and resolve from your environment at agent runtime. Literal values are rejected at `new`/`add`, by `loadout doctor`, at bundle import, and again at sync render time. `loadout rm` removes the merged `mcpServers.` entry (and hook settings fragments) surgically — your other servers and settings keep their exact bytes, and values you edited by hand are left in place unless you pass `--force`. ## Safety: conflicts and snapshots @@ -167,7 +168,7 @@ loadout will never silently destroy config you already had. Two guards protect a - **Conflicts are skipped by default.** A whole-file artifact (skill, command, hook script) whose target already exists on disk but was *never installed by loadout* is reported as a `conflict` and left untouched. `sync` tells you what it skipped and stops there. Re-run with `--force` to overwrite them on purpose — for example when a store's `plan` command collides with a `~/.claude/commands/plan.md` you wrote yourself. Files loadout provably installed (`drifted-local`) still re-converge on a normal sync; only genuinely-unowned files need `--force`. -- **A snapshot is taken before any overwrite or delete.** Right before `sync`, a loadout `switch`, or a `--force` write touches an existing file, loadout copies the originals into a timestamped snapshot under the machine-local data dir (`/loadout/backups//`). Recover them any time: +- **A snapshot is taken before any overwrite or delete.** Right before `sync`, a loadout `switch`, an `rm`, or a `--force` write touches an existing file, loadout copies the originals into a timestamped snapshot under the machine-local data dir (`/loadout/backups//`). An `rm` snapshot also captures the artifact's store files (manifest entry + content), so restoring it resurrects the artifact even in a store that isn't git-tracked. Recover them any time: ``` loadout restore # list snapshots, newest first diff --git a/ROADMAP.md b/ROADMAP.md index 120a1e5..96a87a0 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -101,7 +101,7 @@ loadout is built in shippable milestones — every milestone leaves `main` relea - Codex `config.toml` settings **and MCP** merge without destroying user comments (comment-preserving TOML editor or managed `# loadout:begin/end` line block — the block helpers currently hardcode HTML-comment sentinels end-to-end, so this needs a marker-style-aware refactor; spec 006 dropped codex MCP for exactly this reason) - Antigravity MCP config: exact file location unverified — verify against a real install, then wire `Supports(KindMCP)` (spec 006 marked it SHOULD) -- MCP uninstall: `json-merge` removal is still unsupported, so removing an MCP artifact leaves the merged entry behind (warned, state cleared) — a keyed "remove this JSON path" removal would fix hooks-in-settings too +- ~~MCP uninstall: `json-merge` removal~~ — ✅ resolved by spec 009 (`loadout rm`): keyed JSON removal cleans MCP entries and hooks-in-settings surgically, with drift-safe skips and store-inclusive snapshots - Local secret-overrides file for MCP env values (machine-local, data-dir) if the `${VAR}`-placeholder-only policy proves painful - Project-scoped artifacts (`.cursor/rules/*.mdc`, `.agents/{rules,workflows,skills}`, per-repo CLAUDE.md) — per-project loadout manifests; the engine-side root plumbing (`TargetFile.Root`, `--project`, root-qualified state keys) landed with spec 006 - `loadout spec` follow-ups (spec 007 shipped the CLI + gh backend): more backends (Jira/Linear), PR <-> spec linking, spec progress in web UI/TUI diff --git a/docs/specs/009-artifact-removal.md b/docs/specs/009-artifact-removal.md new file mode 100644 index 0000000..f3e5c2e --- /dev/null +++ b/docs/specs/009-artifact-removal.md @@ -0,0 +1,204 @@ +# 009 — `loadout rm`: artifact removal with clean uninstall + +**Status:** Approved (review waived) — implemented +**Owner:** Lucas Machado +**Related:** spec 003 (loadouts switch introduced `PlanRemoval`/`ApplySwitch`), spec 004 (sync safety: snapshots, conflict semantics), spec 006 (MCP configs — deferred uninstall), ROADMAP "Open questions" item on MCP uninstall + +## Context + +The store has no delete verb. `internal/store` exposes `AddArtifact` but no removal, so +dropping a hand-made artifact means editing `manifest.yaml` by hand — the single most +surprising gap in the CLI for a manager tool. A failed bulk import or an experiment can't be +cleanly undone. + +Half the machinery already exists: `loadouts switch` ships `engine.PlanRemoval` + +`ApplySwitch`, which correctly delete `MergeReplace` targets and strip `MergeBlock` managed +blocks. But `MergeJSON` targets fall through to a warning +(`internal/engine/removal.go:202`): removing an MCP artifact leaves the merged entry in +`~/.claude.json` / `.mcp.json`, and removing a hook leaves its entry in `settings.json` +forever. This is the parked ROADMAP item ("json-merge removal is still unsupported") and it +bites both MCP and hooks. + +Affected: every user who adopts, imports or generates an artifact and later wants it gone — +which is everyone, eventually. Also a prerequisite for the spec-kit adoption story (bulk +adopt is only safe when a bad import can be reversed). + +## Requirements + +- **R1.** A `loadout rm ...` command MUST exist accepting one or more artifact + IDs. Unknown IDs fail with a non-zero exit and a pointer to `loadout status`. Flags: + `--dry-run`, `--yes`, `--force`, `--keep-store`. +- **R2.** Default behavior is **full removal** in one confirmation-gated pass: uninstall + from every agent home where state records an install (files deleted, blocks stripped, + JSON keys removed) **and** delete the artifact from the store (manifest entry + content + files). `--keep-store` uninstalls from agent homes but keeps the store artifact. +- **R3.** The engine MUST support **keyed JSON removal**: for `MergeJSON` targets, + `PlanRemoval` produces an `OpUpdate` item whose desired content is the current file with + exactly the fragment's contribution removed — fragment object leaf keys deleted, array + items removed only when deep-equal to a fragment item, parent objects pruned when the + removal empties them. Bytes outside the removed keys are preserved byte-for-byte + (surgical `sjson` delete, same discipline as `jsonMerge`). A semantic no-op (nothing of + the fragment present) produces no write. The shared file itself is NEVER deleted, even if + removal empties it. +- **R4.** **Drift safety**: when the on-disk value no longer matches what loadout installed + — a `MergeJSON` value at a fragment path differing from the fragment, a `MergeReplace` + file whose hash differs from the state record, a `MergeBlock` block whose content hash + differs — the target is skipped with a warning naming the path and reason. `--force` + removes it anyway. *(Amended during implementation.)* A drift-skipped target KEEPS its + install record (the state group shrinks to the retained subset instead of vanishing) — + clearing it would make a later `rm --force` treat the file as never-installed and + refuse. Fully removed targets clear their records as normal. (Mirrors sync's + conflict/`--force` semantics.) +- **R5.** Removing an artifact with `origin: source` is allowed, but the plan output MUST + warn: next `loadout update` of that source will re-add it, and `loadout source remove` + drops the whole source. No exclusion mechanism in this spec (non-goal). +- **R6.** **Ordering & crash safety**: agent-home uninstall happens first (while the + artifact still renders, so `PlanRemoval` can compute merge strategies), store deletion + last, state saved once per phase. A crash mid-way leaves the artifact in the store and + re-running `loadout rm` is safe (plans re-diff from disk + state, all steps idempotent). +- **R7.** Removing a hook artifact MUST remove both its script file + (`hooks/loadout-.sh`) and its `settings.json` hook entries (via R3 array removal). + Removing an MCP artifact MUST remove its server entry from the user- or project-scope + config per its `scope`. Closes the ROADMAP MCP-uninstall item for claude-code, cursor + and gemini-cli. +- **R8.** All four safety invariants hold: only the engine writes; `rm` writes nothing + without a confirm callback returning true (covered by the permanent invariant suite); a + pre-write snapshot with trigger `rm` captures every file about to be modified or deleted; + content outside managed keys/blocks is untouched. +- **R9.** The pre-write snapshot SHOULD also capture the artifact's **store content** + (manifest entry + content files) so `loadout restore` can resurrect a regretted `rm` + even in a store that isn't git-tracked. If store capture is infeasible in the snapshot + format, `rm` MUST print where the store lives and recommend git — never silently + unrecoverable. +- **R10.** UX: the plan output lists per-agent actions, warnings and skips before the + prompt; success prints artifact/agent counts and the next step (`loadout status`). + README section, shell completions and `.structlint.yaml` updated in the same PR. + +## Non-goals + +- Per-agent uninstall (`rm --agent X` / a separate `loadout uninstall` verb) — full + removal or `--keep-store` only; per-agent granularity stays with `loadouts switch`. +- Source exclusion lists (rm'd artifact re-appearing on `update` is warned, not prevented). +- Codex `config.toml` removal — codex has no MCP/settings support yet (parked separately). +- `loadout undo` / `restore --last` ergonomics — next spec. +- Web UI / TUI removal surfaces — CLI first; the web `DELETE /api/artifacts/` route + can reuse the same engine path in a follow-up. +- Removing profiles / library entries / loadouts (they have their own `rm` verbs already). + +## Design + +### Inverse merge (`jsonRemove`) + +Mirror of `applyFragment` in `internal/engine/merge.go`, walking the fragment against the +current bytes: + +- **Object keys**: recurse when both sides are objects; at leaf paths, compare the current + value to the fragment value. Equal → `sjson.DeleteBytes` at that path. Different → + drift: record a warning (or delete under force). After deletions, prune now-empty + ancestor objects that the fragment introduced (`mcpServers: {}` left behind is noise, + but never prune the root document). +- **Arrays**: inverse of the union in `setLeaf` — remove elements deep-equal to fragment + items; user-added or user-edited elements are untouched (an edited element simply no + longer deep-equals, which *is* the drift case: warn, force-remove removes nothing here + because we cannot identify the edited element — documented behavior). +- **Scalars set wholesale** (fragment set the whole subtree on install): deep-equal → + delete subtree; different → drift. + +Returns `(newBytes, changed, warnings)`; `changed == false` keeps existing bytes so user +formatting never causes phantom writes (same guarantee as `jsonMerge`). + +### Engine changes + +`PlanRemoval` gains an options struct (`Force bool`) and fills the `MergeJSON` case with +`jsonRemove`, emitting `OpUpdate` items like the `MergeBlock` case does today. The +`MergeReplace`/`MergeBlock` cases gain the R4 drift check (hash vs state record). +`loadouts switch` inherits the drift-skip behavior — a deliberate improvement (today +switch deletes drifted files without asking); its `--force` flag threads through. + +A new `Engine.ApplyRemoval(ctx, plan, confirm)` wraps the existing write pass (same shape +as `ApplySwitch` with a nil install plan) so `rm` doesn't fake an empty install plan. + +### Store changes + +`Store.RemoveArtifact(id)`: drop the manifest entry, save, then delete the artifact's +content path (`skills//`, `commands/.md`, …). Refuses unknown IDs. Pure store +concern — no engine or adapter knowledge. + +### CLI + +`internal/cli/rm.go` with `rmCmd()`, registered in `root.go`. Sequence: resolve IDs → +`PlanRemoval` → print plan + warnings (incl. R5 source-origin warning) → confirm → +`ApplyRemoval` → unless `--keep-store`, `RemoveArtifact` per ID → summary. + +### Alternatives considered + +- **Track installed JSON keys in `state.yaml`** (key list per target instead of re-render): + rejected — schema migration for existing users, and redundant while the artifact is + still in the store (render is deterministic). The one case it would help — artifact + already gone from the manifest — already has a defined warning fallback in `PlanRemoval`. +- **Remove the fragment's top-level keys wholesale, ignoring drift**: rejected — deletes + user edits inside the subtree (e.g. a hand-added `"disabled": true` on an MCP entry), + violating invariant 3 and the team's newcomer-safety bar. +- **Sentinel comments in JSON files**: rejected — comments are invalid JSON; agents parse + these files. + +## Data & API changes + +- No manifest, config, or `state.yaml` schema changes. +- Snapshot meta: entries MAY gain a `root: store` variant for R9 store-content capture + (backward-compatible addition; old snapshots restore unchanged). +- New: `Store.RemoveArtifact`, `engine.ApplyRemoval`, `jsonRemove` in merge.go, + `internal/cli/rm.go`. +- No HTTP API changes (web parity is a non-goal). + +## Test plan + +Per team policy: TDD for the ROADMAP:104 bug class (write the failing MCP/hook-leak test +first), behavior-named tests, integration tests drive the **built binary** against a temp +HOME. + +- **R3 (unit, `merge_test.go` style tables)**: removing an object leaf preserves sibling + bytes exactly; array item removed only on deep-equality; user-added array elements + survive; emptied `mcpServers` pruned; root never pruned; fragment absent → no-op, bytes + identical; invalid existing JSON → loud error. +- **R4 (unit, `removal_test.go`)**: drifted JSON value / edited replace-file / edited + block → skipped with warning, state cleared; same plan with `Force` → removed; snapshot + taken before the forced write. +- **R1/R2/R6/R7 (integration)**: build the binary; init store; add a skill + hook + MCP + artifact; sync into a fake claude-code home; `loadout rm` each: files gone, `settings.json` + hook entry and `.claude.json` server entry removed while user-authored sibling keys keep + their exact bytes; manifest entry and store content gone; state has no record; + `--keep-store` leaves the store entry; second `rm` of the same ID errors; `rm` with no + confirmation (pipe `n`) writes nothing (invariant suite extension). +- **R5 (integration)**: source-origin artifact rm prints the re-add warning; subsequent + `update` re-adds it (documents the behavior). +- **R8 (invariant suite)**: extend the permanent no-confirm-no-write test to cover `rm`. +- **R9 (integration)**: `rm` then `loadout restore ` brings the agent-home files back; + store content restored (or, if capture was descoped, the printed guidance is asserted). +- **R10**: README/completions/structlint checked by existing CI gates. + +## Rollout + +- No feature flags, no migrations. Ships in one PR: `feat: loadout rm — artifact removal + with clean uninstall (json-merge removal)`. +- Behavior change to note in the PR body: `loadouts switch` now skips drifted targets + instead of deleting them (strictly safer; `--force` restores old behavior). +- ROADMAP: mark the MCP-uninstall open question closed for claude-code/cursor/gemini-cli; + codex TOML remains parked. +- Rollback: revert the PR — no persisted formats change, old binaries read the same + state/store. + +## Checklist + +Tracked via `loadout spec check 009 `: + +- [x] failing TDD tests reproducing the MCP + hook JSON-leak (ROADMAP:104) +- [x] `jsonRemove` inverse-merge with drift detection + pruning +- [x] `PlanRemoval` MergeJSON case + drift checks + `Force` option +- [x] `engine.ApplyRemoval` (confirm-gated, snapshot trigger `rm`) +- [x] store-content capture in snapshots (R9) or explicit descope note +- [x] `Store.RemoveArtifact` +- [x] `loadout rm` CLI + source-origin warning + completions +- [x] invariant suite extended to `rm` +- [x] integration scenarios (full removal, keep-store, force, restore) +- [x] README + ROADMAP + structlint updated diff --git a/docs/specs/009-artifact-removal.plan.md b/docs/specs/009-artifact-removal.plan.md new file mode 100644 index 0000000..0447d7f --- /dev/null +++ b/docs/specs/009-artifact-removal.plan.md @@ -0,0 +1,41 @@ +# Plan — 009 `loadout rm`: artifact removal with clean uninstall + +Derived from [009-artifact-removal.md](009-artifact-removal.md). Checked off as work lands; +this file, not the conversation, carries progress. + +## Approach + +Fill the `MergeJSON` hole in the existing removal machinery (`engine.PlanRemoval` / +`ApplySwitch`, shipped with loadouts switch) with an inverse of `jsonMerge`, add the R4 +drift gate to all three merge strategies, then wrap it in a new `ApplyRemoval` + store +delete + `loadout rm` command. TDD: the ROADMAP:104 leak (MCP + hook JSON entries left +behind) gets failing engine tests before any implementation. + +Key semantics decided during design (see spec Design): + +- `jsonRemove` walks the fragment against current bytes: deep-equal subtree → delete key; + both objects → recurse (+ prune emptied objects, never the root); arrays remove + deep-equal items only (edited items are unidentifiable — drift warning, `--force` + cannot help there); value differs → drift warning, `--force` deletes the subtree. + User-added sibling keys always survive (invariant 3); shared files are never deleted. +- Drift checks compare disk against the **install-time state hash** (not the current + fragment) for replace/block targets, so a store-side update never blocks removal. +- `PlanRemoval(ctx, ids, RemovalOptions{Force, RemoveStore})`; `RemoveStore` adds the + manifest + artifact content files to the plan (`StoreFiles`) so the pre-write snapshot + captures them (R9) — restore can resurrect a regretted rm even without git. +- Order: `ApplyRemoval` (confirm → snapshot incl. store files → agent writes → state + save) then `Store.RemoveArtifact` (manifest first, then content). Crash between the + two leaves the artifact in the store; re-running `rm` converges (R6). + +## Checklist + +- [x] failing engine tests: hook + MCP JSON entries must be removed (currently warn/leak) +- [x] `jsonRemove` in engine/merge.go + unit tables (byte preservation, arrays, pruning, drift, no-op) +- [x] `PlanRemoval` options struct, MergeJSON case, drift checks for replace/block/json, StoreFiles +- [x] `engine.ApplyRemoval` — confirm gate, snapshot trigger `rm`, single state save +- [x] snapshot capture of store manifest + content (pseudo-agent `store`), restore round-trip +- [x] `Store.RemoveArtifact` (manifest entry + content, path-safe) + unit tests +- [x] `internal/cli/rm.go` + root.go registration + completion scripts + source-origin warning +- [x] invariant suite: rm writes nothing on nil/declined confirm +- [x] integration: full removal, keep-store, force/drift, restore-after-rm, unknown ID +- [x] README rm section, ROADMAP:104 closure note, `make ci` + `make test-integration` green diff --git a/integration/loadouts_test.go b/integration/loadouts_test.go index 5812ce3..91e66f6 100644 --- a/integration/loadouts_test.go +++ b/integration/loadouts_test.go @@ -197,7 +197,7 @@ func TestLoadouts_SwitchAgentNotInstalled_BehavesLikeSyncSkip(t *testing.T) { } } -func TestLoadouts_HandEditedBlockStillGetsRemovedOnSwitch(t *testing.T) { +func TestLoadouts_HandEditedBlockSkippedOnSwitchRemovedWithForce(t *testing.T) { e := newEnv(t) e.mustRun("init") e.mustRun("new", "instruction", "style") @@ -214,16 +214,28 @@ func TestLoadouts_HandEditedBlockStillGetsRemovedOnSwitch(t *testing.T) { t.Fatal(err) } - // "other" excludes "style" — switching to it must remove style's block. + // "other" excludes "style". Since spec 009, a hand-edited block is drift: + // a plain switch leaves it with a warning; --force removes it. e.mustRun("loadouts", "save", "other", "demo") - e.mustRun("loadouts", "switch", "other", "--yes") + out := e.mustRun("loadouts", "switch", "other", "--yes") + if !strings.Contains(out, "--force") { + t.Fatalf("switch should warn about the edited block and point at --force:\n%s", out) + } + raw, err = os.ReadFile(claudeMD) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), "hand-edited line") { + t.Fatalf("edited block must be left in place without --force:\n%s", raw) + } + e.mustRun("loadouts", "switch", "other", "--force", "--yes") raw, err = os.ReadFile(claudeMD) if err != nil { t.Fatal(err) } if strings.Contains(string(raw), "loadout:begin style") { - t.Fatalf("drifted block should still be force-removed:\n%s", raw) + t.Fatalf("--force should remove the drifted block:\n%s", raw) } } diff --git a/integration/rm_test.go b/integration/rm_test.go new file mode 100644 index 0000000..390e8fb --- /dev/null +++ b/integration/rm_test.go @@ -0,0 +1,206 @@ +//go:build integration + +package integration + +import ( + "os" + "regexp" + "strings" + "testing" +) + +func TestRm_FullRemovalCleansAgentHomesAndStore(t *testing.T) { + e := newEnv(t) + // A user settings.json that predates loadout — its bytes must survive the + // hook's settings entry being merged in and later removed (ROADMAP:104). + userSettings := "{\n\t\"model\": \"opus\"\n}\n" + if err := os.WriteFile(e.claude("settings.json"), []byte(userSettings), 0o644); err != nil { + t.Fatal(err) + } + + e.mustRun("init") + e.mustRun("new", "skill", "demo") + e.mustRun("new", "hook", "lint") + e.mustRun("sync", "--yes") + + if raw, err := os.ReadFile(e.claude("settings.json")); err != nil || !strings.Contains(string(raw), "loadout-lint") { + t.Fatalf("precondition: hook entry should be merged into settings.json, got %q (%v)", raw, err) + } + + out := e.mustRun("rm", "demo", "lint", "--yes") + if !strings.Contains(out, "store artifact(s)") { + t.Fatalf("rm output: %s", out) + } + + if _, err := os.Stat(e.claude("skills", "demo")); !os.IsNotExist(err) { + t.Error("installed skill directory should be gone") + } + if _, err := os.Stat(e.claude("hooks", "loadout-lint.sh")); !os.IsNotExist(err) { + t.Error("hook script should be gone") + } + raw, err := os.ReadFile(e.claude("settings.json")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "hooks") || strings.Contains(string(raw), "loadout-lint") { + t.Errorf("hook entry leaked in settings.json:\n%s", raw) + } + if !strings.Contains(string(raw), "\t\"model\": \"opus\"") { + t.Errorf("user settings bytes not preserved:\n%s", raw) + } + + manifest, err := os.ReadFile(e.storePath("loadout.yaml")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(manifest), "demo") || strings.Contains(string(manifest), "lint") { + t.Errorf("manifest still lists removed artifacts:\n%s", manifest) + } + if _, err := os.Stat(e.storePath("skills", "demo")); !os.IsNotExist(err) { + t.Error("store content for demo should be deleted") + } + + status := e.mustRun("status") + if strings.Contains(status, "demo") || strings.Contains(status, "lint") { + t.Errorf("status still shows removed artifacts:\n%s", status) + } +} + +func TestRm_DeclinedConfirmationWritesNothing(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + e.mustRun("new", "skill", "demo") + e.mustRun("sync", "--yes") + + out, err := e.run("n\n", "rm", "demo") + if err != nil { + t.Fatalf("rm with declined prompt should exit 0: %v\n%s", err, out) + } + if !strings.Contains(out, "Aborted") { + t.Fatalf("rm output: %s", out) + } + if _, err := os.Stat(e.claude("skills", "demo", "SKILL.md")); err != nil { + t.Error("installed skill must be untouched after declining") + } + if _, err := os.Stat(e.storePath("skills", "demo", "SKILL.md")); err != nil { + t.Error("store content must be untouched after declining") + } + manifest, _ := os.ReadFile(e.storePath("loadout.yaml")) + if !strings.Contains(string(manifest), "demo") { + t.Error("manifest entry must be untouched after declining") + } +} + +func TestRm_KeepStoreOnlyUninstalls(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + e.mustRun("new", "skill", "demo") + e.mustRun("sync", "--yes") + + e.mustRun("rm", "demo", "--keep-store", "--yes") + + if _, err := os.Stat(e.claude("skills", "demo")); !os.IsNotExist(err) { + t.Error("installed skill should be gone") + } + manifest, _ := os.ReadFile(e.storePath("loadout.yaml")) + if !strings.Contains(string(manifest), "demo") { + t.Error("--keep-store must keep the manifest entry") + } + status := e.mustRun("status") + if !strings.Contains(status, "demo") || !strings.Contains(status, "missing") { + t.Errorf("status should show demo as missing again:\n%s", status) + } +} + +func TestRm_EditedFileSkippedWithoutForceRemovedWith(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + e.mustRun("new", "command", "plan") + e.mustRun("sync", "--yes") + + cmdPath := e.claude("commands", "plan.md") + if err := os.WriteFile(cmdPath, []byte("user rewrote this\n"), 0o644); err != nil { + t.Fatal(err) + } + + out := e.mustRun("rm", "plan", "--keep-store", "--yes") + if !strings.Contains(out, "--force") { + t.Fatalf("rm should warn about the edited file and point at --force:\n%s", out) + } + if _, err := os.Stat(cmdPath); err != nil { + t.Fatal("edited file must be left on disk without --force") + } + + e.mustRun("rm", "plan", "--keep-store", "--force", "--yes") + if _, err := os.Stat(cmdPath); !os.IsNotExist(err) { + t.Error("--force should remove the edited file") + } +} + +func TestRm_UnknownArtifactErrors(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + + out, err := e.run("", "rm", "nope") + if err == nil { + t.Fatalf("rm of an unknown artifact should fail, got:\n%s", out) + } + if !strings.Contains(out, "unknown artifact") || !strings.Contains(out, "loadout status") { + t.Errorf("error should teach the next step:\n%s", out) + } +} + +func TestRm_SourceOriginArtifactWarnsAboutReadd(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + e.mustRun("new", "skill", "demo") + + // Mark the artifact as source-origin the way `loadout source add` would. + manifestPath := e.storePath("loadout.yaml") + raw, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + patched := []byte(strings.Replace(string(raw), "origin: local", "origin: source:team-skills", 1)) + if string(patched) == string(raw) { + t.Fatalf("could not patch manifest:\n%s", raw) + } + if err := os.WriteFile(manifestPath, patched, 0o644); err != nil { + t.Fatal(err) + } + + out := e.mustRun("rm", "demo", "--dry-run") + if !strings.Contains(out, "loadout update") || !strings.Contains(out, "team-skills") { + t.Errorf("rm should warn that update re-adds source artifacts:\n%s", out) + } +} + +func TestRm_RestoreResurrectsStoreAndAgentFiles(t *testing.T) { + e := newEnv(t) + e.mustRun("init") + e.mustRun("new", "skill", "demo") + e.mustRun("sync", "--yes") + + e.mustRun("rm", "demo", "--yes") + if _, err := os.Stat(e.storePath("skills", "demo")); !os.IsNotExist(err) { + t.Fatal("precondition: store content should be gone after rm") + } + + list := e.mustRun("restore") + m := regexp.MustCompile(`(?m)^\s+(\S+)\s+.*\brm\b`).FindStringSubmatch(list) + if m == nil { + t.Fatalf("no rm snapshot listed:\n%s", list) + } + e.mustRun("restore", m[1], "--yes") + + if _, err := os.Stat(e.storePath("skills", "demo", "SKILL.md")); err != nil { + t.Error("restore should resurrect the store content") + } + manifest, _ := os.ReadFile(e.storePath("loadout.yaml")) + if !strings.Contains(string(manifest), "demo") { + t.Error("restore should resurrect the manifest entry") + } + if _, err := os.Stat(e.claude("skills", "demo", "SKILL.md")); err != nil { + t.Error("restore should resurrect the installed skill") + } +} diff --git a/internal/cli/completion.go b/internal/cli/completion.go index 2bc8edd..076dbbf 100644 --- a/internal/cli/completion.go +++ b/internal/cli/completion.go @@ -43,7 +43,7 @@ func completionCmd() *cli.Command { const bashCompletion = `# loadout bash completion _loadout() { local cur="${COMP_WORDS[COMP_CWORD]}" - local commands="init status diff sync install restore new add source check update bundle spec wizard profile tui serve agents doctor version completion" + local commands="init status diff sync install restore new add rm source check update bundle spec wizard profile loadouts tui serve agents doctor version completion" COMPREPLY=($(compgen -W "${commands}" -- "${cur}")) } complete -F _loadout loadout @@ -61,6 +61,7 @@ _loadout() { 'restore:List or restore pre-write snapshots' 'new:Scaffold a new artifact in the store' 'add:Adopt an existing native artifact into the store' + 'rm:Remove artifacts from the store and all agent homes' 'source:Track skills from external git repositories' 'check:Check sources for updates (non-intrusive)' 'update:Fetch and apply source updates (gated)' @@ -68,6 +69,7 @@ _loadout() { 'spec:Spec-driven development checklists (files or GitHub issues)' 'wizard:Guided builder for skills and conventions' 'profile:Save and reuse named wizard answer sets' + 'loadouts:Named, switchable sets of active artifacts' 'tui:Interactive dashboard' 'serve:Local web UI' 'agents:List supported agents' @@ -90,6 +92,7 @@ complete -c loadout -n '__fish_use_subcommand' -a install -d 'First-time ful complete -c loadout -n '__fish_use_subcommand' -a restore -d 'List or restore pre-write snapshots' complete -c loadout -n '__fish_use_subcommand' -a new -d 'Scaffold a new artifact in the store' complete -c loadout -n '__fish_use_subcommand' -a add -d 'Adopt an existing native artifact into the store' +complete -c loadout -n '__fish_use_subcommand' -a rm -d 'Remove artifacts from the store and all agent homes' complete -c loadout -n '__fish_use_subcommand' -a source -d 'Track skills from external git repositories' complete -c loadout -n '__fish_use_subcommand' -a check -d 'Check sources for updates (non-intrusive)' complete -c loadout -n '__fish_use_subcommand' -a update -d 'Fetch and apply source updates (gated)' @@ -97,6 +100,7 @@ complete -c loadout -n '__fish_use_subcommand' -a bundle -d 'Export/import complete -c loadout -n '__fish_use_subcommand' -a spec -d 'Spec-driven development checklists (files or GitHub issues)' complete -c loadout -n '__fish_use_subcommand' -a wizard -d 'Guided builder for skills and conventions' complete -c loadout -n '__fish_use_subcommand' -a profile -d 'Save and reuse named wizard answer sets' +complete -c loadout -n '__fish_use_subcommand' -a loadouts -d 'Named, switchable sets of active artifacts' complete -c loadout -n '__fish_use_subcommand' -a tui -d 'Interactive dashboard' complete -c loadout -n '__fish_use_subcommand' -a serve -d 'Local web UI' complete -c loadout -n '__fish_use_subcommand' -a agents -d 'List supported agents' diff --git a/internal/cli/loadouts.go b/internal/cli/loadouts.go index f390999..0b5ca7c 100644 --- a/internal/cli/loadouts.go +++ b/internal/cli/loadouts.go @@ -192,7 +192,7 @@ func runLoadoutSwitch(ctx context.Context, cmd *cli.Command, eng *engine.Engine, removeIDs = append(removeIDs, id) } } - removalPlan, err := eng.PlanRemoval(ctx, removeIDs) + removalPlan, err := eng.PlanRemoval(ctx, removeIDs, engine.RemovalOptions{Force: cmd.Bool("force")}) if err != nil { return err } diff --git a/internal/cli/rm.go b/internal/cli/rm.go new file mode 100644 index 0000000..8a014e6 --- /dev/null +++ b/internal/cli/rm.go @@ -0,0 +1,145 @@ +package cli + +import ( + "bufio" + "context" + "errors" + "fmt" + "os" + "strings" + + "github.com/AxeForging/loadout/internal/engine" + "github.com/urfave/cli/v3" +) + +func rmCmd() *cli.Command { + return &cli.Command{ + Name: "rm", + Usage: "Remove artifacts: uninstall from every agent and delete from the store (always asks first)", + ArgsUsage: "...", + Flags: []cli.Flag{ + &cli.BoolFlag{Name: "dry-run", Usage: "Show the plan and exit without writing"}, + &cli.BoolFlag{Name: "yes", Aliases: []string{"y"}, Usage: "Apply without the interactive confirmation prompt"}, + &cli.BoolFlag{Name: "force", Usage: "Also remove targets whose content changed since loadout installed them (a snapshot is taken first)"}, + &cli.BoolFlag{Name: "keep-store", Usage: "Only uninstall from agent homes; keep the artifact in the store"}, + projectFlag(), + }, + Action: func(ctx context.Context, cmd *cli.Command) error { + ids := cmd.Args().Slice() + if len(ids) == 0 { + return fmt.Errorf("usage: loadout rm ... (see 'loadout status' for what's in the store)") + } + eng, err := newEngine(cmd) + if err != nil { + return err + } + return runRemove(ctx, cmd, eng, ids) + }, + } +} + +// runRemove is the rm orchestration: validate IDs, plan the uninstall across +// agent homes (plus the store files about to go), show everything, and apply +// under one confirmation. Store deletion runs after the engine pass — a crash +// in between leaves the artifact in the store and re-running rm converges. +func runRemove(ctx context.Context, cmd *cli.Command, eng *engine.Engine, ids []string) error { + s := eng.Store + installedIDs, err := eng.InstalledArtifactIDs() + if err != nil { + return err + } + installed := make(map[string]bool, len(installedIDs)) + for _, id := range installedIDs { + installed[id] = true + } + for _, id := range ids { + if _, found := s.Artifact(id); !found && !installed[id] { + return fmt.Errorf("unknown artifact %q (see 'loadout status')", id) + } + } + + keepStore := cmd.Bool("keep-store") + rp, err := eng.PlanRemoval(ctx, ids, engine.RemovalOptions{ + Force: cmd.Bool("force"), + RemoveStore: !keepStore, + }) + if err != nil { + return err + } + + printRemovals(rp) + for _, sk := range rp.Skips { + fmt.Printf(" ~ %s skipped: %s\n", sk.Agent, sk.Reason) + } + for _, sf := range rp.StoreFiles { + if sf.Op == engine.OpDelete { + fmt.Printf(" delete store: %s\n", sf.RelPath) + } + } + for _, id := range ids { + if art, found := s.Artifact(id); found && strings.HasPrefix(art.Origin, "source:") { + name := strings.TrimPrefix(art.Origin, "source:") + fmt.Printf(" ! %s came from source %q — the next 'loadout update' will re-add it; 'loadout source remove %s' drops the whole source\n", id, name, name) + } + } + + if len(rp.Items) == 0 && len(rp.StoreFiles) == 0 && len(rp.ClearState) == 0 { + fmt.Println("Nothing to remove.") + return nil + } + if len(rp.Items) == 0 && len(rp.StoreFiles) == 0 { + fmt.Println(" (nothing on disk — only stale install records to clear)") + } + if cmd.Bool("dry-run") { + fmt.Println("\nDry run — nothing written.") + return nil + } + + removed, err := eng.ApplyRemoval(ctx, rp, confirmRemoval(cmd, rp)) + if errors.Is(err, engine.ErrNotConfirmed) { + fmt.Println("Aborted — nothing written.") + return nil + } + if err != nil { + return err + } + + storeRemoved := 0 + if !keepStore { + for _, id := range ids { + if _, found := s.Artifact(id); !found { + continue + } + if err := s.RemoveArtifact(id); err != nil { + return fmt.Errorf("store entry for %q: %w (agent homes already cleaned — re-run 'loadout rm %s' to retry)", id, err, id) + } + storeRemoved++ + } + } + + fmt.Printf("Removed: %d agent change(s), %d store artifact(s).\n", removed, storeRemoved) + if len(rp.Warnings) > 0 { + fmt.Printf("%d warning(s) above — those targets were left in place.\n", len(rp.Warnings)) + } + fmt.Println("Next: 'loadout status' to verify — 'loadout restore' can undo this.") + return nil +} + +// confirmRemoval prompts on stdin unless --yes was passed. CLI side of +// ApplyRemoval's confirm gate. +func confirmRemoval(cmd *cli.Command, rp *engine.RemovalPlan) func(*engine.RemovalPlan) bool { + return func(*engine.RemovalPlan) bool { + if cmd.Bool("yes") { + return true + } + total := len(rp.Items) + len(rp.StoreFiles) + fmt.Printf("\nRemove %d target(s)? [y/N]: ", total) + reader := bufio.NewReader(os.Stdin) + answer, err := reader.ReadString('\n') + if err != nil { + return false + } + answer = strings.ToLower(strings.TrimSpace(answer)) + return answer == "y" || answer == "yes" + } +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 7f925b0..cab2196 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -21,6 +21,7 @@ func Commands() []*cli.Command { restoreCmd(), newCmd(), addCmd(), + rmCmd(), sourceCmd(), checkCmd(), updateCmd(), diff --git a/internal/engine/merge.go b/internal/engine/merge.go index 779ac57..29b8f18 100644 --- a/internal/engine/merge.go +++ b/internal/engine/merge.go @@ -139,6 +139,160 @@ func setLeaf(existing []byte, path string, value any) ([]byte, bool, error) { return out, true, nil } +// jsonRemove computes the inverse of jsonMerge: the current bytes with exactly +// the fragment's contribution removed, surgically — bytes outside the removed +// keys are preserved, and the file itself is never emptied to nothing the +// caller wouldn't expect (the root object always survives). Returns the drifted +// paths: places where the on-disk value no longer matches the fragment, which +// are left in place unless force is set. +func jsonRemove(existing, fragment []byte, force bool) (out []byte, changed bool, drifted []string, err error) { + var frag any + if err := json.Unmarshal(fragment, &frag); err != nil { + return nil, false, nil, fmt.Errorf("invalid JSON fragment: %w", err) + } + if len(bytes.TrimSpace(existing)) == 0 { + return existing, false, nil, nil + } + // Reject a corrupt existing file loudly — same discipline as jsonMerge. + var probe any + if err := json.Unmarshal(existing, &probe); err != nil { + return nil, false, nil, fmt.Errorf("existing file is not valid JSON: %w", err) + } + fm, ok := frag.(map[string]any) + if !ok { + return nil, false, nil, fmt.Errorf("fragment root must be a JSON object") + } + result, changed, drifted, err := removeMapKeys(existing, "", fm, force) + if err != nil { + return nil, false, nil, err + } + if !changed { + return existing, false, drifted, nil + } + return result, true, drifted, nil +} + +// removeMapKeys deletes this map's fragment-defined keys from existing. +// Removal is atomic within the object that directly contains an edited value: +// if any of this map's own keys drifted (value differs and the shapes don't +// allow recursing), every deletion inside this map is rolled back and the +// drift reported — a partially-edited unit (e.g. one MCP server entry) is +// removed whole under force or left whole, never gutted. +func removeMapKeys(existing []byte, path string, frag map[string]any, force bool) ([]byte, bool, []string, error) { + pre := existing + result := existing + anyChange := false + var drifts []string + directDrift := false + + for _, k := range sortedKeys(frag) { + sub := joinPath(path, k) + existingAt := gjson.GetBytes(result, sub) + if !existingAt.Exists() { + continue // already absent — nothing of ours here + } + if jsonEqual(existingAt.Raw, frag[k]) { + next, err := sjson.DeleteBytes(result, sub) + if err != nil { + return nil, false, nil, fmt.Errorf("remove %q: %w", sub, err) + } + result = next + anyChange = true + continue + } + if subMap, ok := frag[k].(map[string]any); ok && existingAt.IsObject() { + next, subChanged, subDrifts, err := removeMapKeys(result, sub, subMap, force) + if err != nil { + return nil, false, nil, err + } + // Prune the object when our removals emptied it — an empty + // "mcpServers": {} left behind is noise, not user content. + if subChanged { + if at := gjson.GetBytes(next, sub); at.IsObject() && len(at.Map()) == 0 { + if next, err = sjson.DeleteBytes(next, sub); err != nil { + return nil, false, nil, fmt.Errorf("prune %q: %w", sub, err) + } + } + } + result = next + anyChange = anyChange || subChanged + drifts = append(drifts, subDrifts...) + continue + } + if arr, ok := frag[k].([]any); ok && existingAt.IsArray() { + next, subChanged, missing, err := removeArrayItems(result, sub, arr) + if err != nil { + return nil, false, nil, err + } + result = next + anyChange = anyChange || subChanged + if missing { + // An installed array item was edited or hand-removed; it can't + // be identified anymore, so it stays — even under force. + drifts = append(drifts, sub) + directDrift = true + } + continue + } + // Value differs and the shapes don't allow recursion: a user edit. + if force { + next, err := sjson.DeleteBytes(result, sub) + if err != nil { + return nil, false, nil, fmt.Errorf("remove %q: %w", sub, err) + } + result = next + anyChange = true + continue + } + drifts = append(drifts, sub) + directDrift = true + } + + if directDrift && !force { + return pre, false, drifts, nil + } + return result, anyChange, drifts, nil +} + +// removeArrayItems removes elements deep-equal to the fragment's items — the +// inverse of setLeaf's array union. User-added elements survive; an emptied +// array is deleted outright. missing reports fragment items with no deep-equal +// element left (edited or already removed by hand — unidentifiable). +func removeArrayItems(existing []byte, path string, fragItems []any) (out []byte, changed, missing bool, err error) { + existingAt := gjson.GetBytes(existing, path) + var current []any + if err := json.Unmarshal([]byte(existingAt.Raw), ¤t); err != nil { + return nil, false, false, err + } + for _, item := range fragItems { + if !containsDeep(current, item) { + missing = true + } + } + kept := make([]any, 0, len(current)) + for _, cur := range current { + if !containsDeep(fragItems, cur) { + kept = append(kept, cur) + } + } + if len(kept) == len(current) { + return existing, false, missing, nil + } + if len(kept) == 0 { + out, err := sjson.DeleteBytes(existing, path) + return out, err == nil, missing, err + } + raw, err := json.Marshal(kept) + if err != nil { + return nil, false, false, err + } + out, err = sjson.SetRawBytes(existing, path, raw) + if err != nil { + return nil, false, false, fmt.Errorf("remove from %q: %w", path, err) + } + return out, true, missing, nil +} + // jsonEqual reports whether raw and want represent the same JSON value — // tolerant of whitespace and key order. func jsonEqual(raw string, want any) bool { diff --git a/internal/engine/merge_test.go b/internal/engine/merge_test.go index f77a8cb..dc6217c 100644 --- a/internal/engine/merge_test.go +++ b/internal/engine/merge_test.go @@ -152,3 +152,172 @@ func TestJSONMerge_FailsOnCorruptExistingFile(t *testing.T) { t.Fatal("expected error for invalid existing JSON") } } + +func TestJSONRemove_DeletesExactMatchKeyPreservesSiblingBytes(t *testing.T) { + existing := []byte("{\n \"mine\": true,\n \"managed\": {\"a\": 1}\n}\n") + fragment := []byte(`{"managed": {"a": 1}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if !changed || len(drifted) != 0 { + t.Fatalf("changed=%v drifted=%v, want changed with no drift", changed, drifted) + } + if strings.Contains(string(out), "managed") { + t.Errorf("managed key not removed:\n%s", out) + } + if !strings.Contains(string(out), "\n \"mine\": true") { + t.Errorf("sibling bytes not preserved:\n%s", out) + } +} + +func TestJSONRemove_RemovesOneServerEntryKeepsUserServers(t *testing.T) { + existing := []byte(`{"mcpServers": {"user-own": {"command": "deno"}, "gh": {"command": "npx", "args": ["-y"]}}}`) + fragment := []byte(`{"mcpServers": {"gh": {"command": "npx", "args": ["-y"]}}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if !changed || len(drifted) != 0 { + t.Fatalf("changed=%v drifted=%v, want clean removal", changed, drifted) + } + var got map[string]any + if err := json.Unmarshal(out, &got); err != nil { + t.Fatalf("output not valid JSON: %v", err) + } + servers := got["mcpServers"].(map[string]any) + if _, gone := servers["gh"]; gone { + t.Errorf("gh entry survived: %v", servers) + } + if _, kept := servers["user-own"]; !kept { + t.Errorf("user's own server lost: %v", servers) + } +} + +func TestJSONRemove_PrunesEmptiedParentObjects(t *testing.T) { + existing := []byte(`{"model": "opus", "hooks": {"Stop": [{"matcher": "a"}]}}`) + fragment := []byte(`{"hooks": {"Stop": [{"matcher": "a"}]}}`) + + out, changed, _, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if !changed { + t.Fatal("want a change") + } + if strings.Contains(string(out), "hooks") { + t.Errorf("emptied hooks object not pruned:\n%s", out) + } + if !strings.Contains(string(out), `"model": "opus"`) { + t.Errorf("user key lost:\n%s", out) + } +} + +func TestJSONRemove_ArrayRemovesOnlyInstalledItemsKeepsUserItems(t *testing.T) { + existing := []byte(`{"hooks": {"Stop": [{"matcher": "user"}, {"matcher": "ours"}]}}`) + fragment := []byte(`{"hooks": {"Stop": [{"matcher": "ours"}]}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if !changed || len(drifted) != 0 { + t.Fatalf("changed=%v drifted=%v, want clean partial removal", changed, drifted) + } + var got map[string]any + json.Unmarshal(out, &got) + stop := got["hooks"].(map[string]any)["Stop"].([]any) + if len(stop) != 1 || stop[0].(map[string]any)["matcher"] != "user" { + t.Errorf("Stop = %v, want only the user's entry", stop) + } +} + +func TestJSONRemove_EditedArrayItemReportsDriftChangesNothing(t *testing.T) { + existing := []byte(`{"hooks": {"Stop": [{"matcher": "edited-by-user"}]}}`) + fragment := []byte(`{"hooks": {"Stop": [{"matcher": "ours"}]}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if changed { + t.Errorf("edited array item must not be touched:\n%s", out) + } + if len(drifted) != 1 || drifted[0] != "hooks.Stop" { + t.Errorf("drifted = %v, want [hooks.Stop]", drifted) + } + if string(out) != string(existing) { + t.Error("bytes must be identical when nothing changed") + } +} + +func TestJSONRemove_DriftRollsBackSiblingRemovalsInSameObject(t *testing.T) { + // User edited "command" inside our server entry: the whole entry must stay + // intact — not get gutted down to just the edited key. + existing := []byte(`{"mcpServers": {"gh": {"command": "my-custom-wrapper", "args": ["-y"]}}}`) + fragment := []byte(`{"mcpServers": {"gh": {"command": "npx", "args": ["-y"]}}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if changed { + t.Errorf("partially-edited entry must be left whole:\n%s", out) + } + if len(drifted) != 1 || drifted[0] != "mcpServers.gh.command" { + t.Errorf("drifted = %v, want [mcpServers.gh.command]", drifted) + } + if !strings.Contains(string(out), `"args"`) { + t.Errorf("sibling key was removed despite drift:\n%s", out) + } +} + +func TestJSONRemove_ForceRemovesDriftedValuesAndPrunes(t *testing.T) { + existing := []byte(`{"keep": 1, "mcpServers": {"gh": {"command": "my-custom-wrapper", "args": ["-y"]}}}`) + fragment := []byte(`{"mcpServers": {"gh": {"command": "npx", "args": ["-y"]}}}`) + + out, changed, _, err := jsonRemove(existing, fragment, true) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if !changed { + t.Fatal("force must remove the drifted entry") + } + if strings.Contains(string(out), "mcpServers") { + t.Errorf("drifted entry (and emptied parent) not removed under force:\n%s", out) + } + if !strings.Contains(string(out), `"keep": 1`) { + t.Errorf("user key lost under force:\n%s", out) + } +} + +func TestJSONRemove_FragmentAbsentIsNoopWithIdenticalBytes(t *testing.T) { + existing := []byte("{\n \"only\": \"user stuff\"\n}\n") + fragment := []byte(`{"mcpServers": {"gh": {"command": "npx"}}}`) + + out, changed, drifted, err := jsonRemove(existing, fragment, false) + if err != nil { + t.Fatalf("jsonRemove() error = %v", err) + } + if changed || len(drifted) != 0 { + t.Errorf("changed=%v drifted=%v, want pure no-op", changed, drifted) + } + if string(out) != string(existing) { + t.Error("no-op must return the input bytes untouched") + } +} + +func TestJSONRemove_InvalidExistingFileErrorsLoudly(t *testing.T) { + if _, _, _, err := jsonRemove([]byte("{broken"), []byte(`{"a": 1}`), false); err == nil { + t.Fatal("want an error for a corrupt existing file") + } +} + +func TestJSONRemove_EmptyExistingIsNoop(t *testing.T) { + out, changed, _, err := jsonRemove(nil, []byte(`{"a": 1}`), false) + if err != nil || changed || len(out) != 0 { + t.Fatalf("out=%q changed=%v err=%v, want empty no-op", out, changed, err) + } +} diff --git a/internal/engine/removal.go b/internal/engine/removal.go index 79b8f74..8a7e545 100644 --- a/internal/engine/removal.go +++ b/internal/engine/removal.go @@ -1,8 +1,10 @@ package engine import ( + "bytes" "context" "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -10,6 +12,7 @@ import ( "github.com/AxeForging/loadout/internal/adapter" "github.com/AxeForging/loadout/internal/domain" + "github.com/AxeForging/loadout/internal/store" ) // RemovalWarning documents an artifact × agent × relPath the engine could @@ -28,6 +31,29 @@ type StateKey struct { Agent domain.AgentID } +// RemovalOptions tunes PlanRemoval. +type RemovalOptions struct { + // Force also removes targets whose on-disk content no longer matches what + // loadout installed (drifted) — mirrors sync's --force conflict semantics. + // Without it, drifted targets are left in place with a warning. + Force bool + // RemoveStore adds the artifact's store files (manifest + content) to the + // plan's StoreFiles so the pre-write snapshot captures them; the actual + // store deletion stays with store.RemoveArtifact — the engine never writes + // into the store. + RemoveStore bool +} + +// StoreFile is a store-side path a removal will delete or rewrite (the +// manifest). Captured in the pre-write snapshot so `loadout restore` can +// resurrect a removed artifact even in a store that isn't git-tracked. +type StoreFile struct { + RelPath string + AbsPath string + IsDir bool + Op Op +} + // RemovalPlan is what removing a set of previously-installed artifacts // would do. Performs no writes. type RemovalPlan struct { @@ -39,8 +65,17 @@ type RemovalPlan struct { // be dropped once ApplySwitch runs — including pairs with zero Items // (MergeJSON targets left in place; blocks already absent). ClearState []StateKey - Warnings []RemovalWarning - Skips []Skip + // StoreFiles lists store-side files the caller intends to delete after + // ApplyRemoval (populated by RemovalOptions.RemoveStore). They are + // snapshotted before any write but never written by the engine. + StoreFiles []StoreFile + // RetainState maps artifact×agent groups to the install records that must + // SURVIVE the state clear: targets skipped because they drifted. Keeping + // them is what lets a later `rm --force` still recognize the file as + // loadout-installed instead of having forgotten it. + RetainState map[StateKey]map[string]string + Warnings []RemovalWarning + Skips []Skip } // PlanRemoval computes what removing a set of previously-installed artifact @@ -50,15 +85,16 @@ type RemovalPlan struct { // just can't determine a merge strategy for those, and falls back to a // Warning (leave the file, clear the state record) rather than guessing. // Performs no writes. -func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string) (*RemovalPlan, error) { +func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string, opts RemovalOptions) (*RemovalPlan, error) { st, err := loadState(e.StatePath) if err != nil { return nil, fmt.Errorf("load state: %w", err) } - rp := &RemovalPlan{} + rp := &RemovalPlan{RetainState: map[StateKey]map[string]string{}} skippedAgents := map[domain.AgentID]bool{} dirQueued := map[string]bool{} + storeTouched := false for _, artID := range artifactIDs { agents := st.Artifacts[artID] @@ -71,7 +107,18 @@ func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string) (*Remova for _, agentStr := range agentIDs { agentID := domain.AgentID(agentStr) relPaths := agents[agentStr] - rp.ClearState = append(rp.ClearState, StateKey{Artifact: artID, Agent: agentID}) + skey := StateKey{Artifact: artID, Agent: agentID} + rp.ClearState = append(rp.ClearState, skey) + // retain keeps a drift-skipped target's install record alive so a + // later `rm --force` still recognizes it as loadout-installed. + retain := func(rels ...string) { + if rp.RetainState[skey] == nil { + rp.RetainState[skey] = map[string]string{} + } + for _, r := range rels { + rp.RetainState[skey][r] = relPaths[r] + } + } ad, ok := e.Registry.Get(agentID) if !ok { @@ -158,12 +205,37 @@ func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string) (*Remova continue } dirQueued[key] = true + if !opts.Force { + if drifted, why := skillDirDrifted(dirAbs, det.Home, relPaths); drifted { + rp.Warnings = append(rp.Warnings, RemovalWarning{ + Artifact: artID, Agent: agentID, RelPath: filepath.ToSlash(filepath.Join("skills", art.ID)), + Reason: fmt.Sprintf("%s — left on disk (re-run with --force to remove; a snapshot is taken first)", why), + }) + prefix := filepath.ToSlash(filepath.Join("skills", art.ID)) + "/" + for rel := range relPaths { + if strings.HasPrefix(rel, prefix) { + retain(rel) + } + } + continue + } + } rp.Items = append(rp.Items, Item{ Agent: agentID, Artifact: art, RelPath: filepath.ToSlash(filepath.Join("skills", art.ID)), AbsPath: dirAbs, Op: OpDelete, IsDir: true, Merge: adapter.MergeReplace, }) continue } + if !opts.Force { + if current, err := os.ReadFile(abs); err == nil && store.HashBytes(current) != relPaths[relPath] { + rp.Warnings = append(rp.Warnings, RemovalWarning{ + Artifact: artID, Agent: agentID, RelPath: relPath, + Reason: "content changed since loadout installed it — left on disk (re-run with --force to remove; a snapshot is taken first)", + }) + retain(relPath) + continue + } + } rp.Items = append(rp.Items, Item{ Agent: agentID, Artifact: art, RelPath: relPath, AbsPath: abs, Op: OpDelete, IsDir: false, Merge: adapter.MergeReplace, @@ -189,6 +261,17 @@ func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string) (*Remova if !blockFound { continue // already absent } + if !opts.Force { + block := adapter.ExtractBlock(current, art.ID) + if block != nil && store.HashBytes(bytes.TrimRight(block, "\n")) != relPaths[relPath] { + rp.Warnings = append(rp.Warnings, RemovalWarning{ + Artifact: artID, Agent: agentID, RelPath: relPath, + Reason: "managed block content changed since loadout installed it — left in place (re-run with --force to remove; a snapshot is taken first)", + }) + retain(relPath) + continue + } + } mode := tf.Mode if info, statErr := os.Stat(abs); statErr == nil { mode = info.Mode() @@ -200,18 +283,114 @@ func (e *Engine) PlanRemoval(ctx context.Context, artifactIDs []string) (*Remova }) case adapter.MergeJSON: - rp.Warnings = append(rp.Warnings, RemovalWarning{ - Artifact: artID, Agent: agentID, RelPath: relPath, - Reason: "json-merge fragment left in place — automatic removal isn't supported yet (state record cleared, artifact stays installed until manually cleaned up)", + current, err := os.ReadFile(abs) + if err != nil { + if os.IsNotExist(err) { + continue // file gone — nothing of ours left + } + rp.Warnings = append(rp.Warnings, RemovalWarning{Artifact: artID, Agent: agentID, RelPath: relPath, Reason: fmt.Sprintf("could not read file: %v", err)}) + continue + } + newContent, changed, drifted, err := jsonRemove(current, tf.Content, opts.Force) + if err != nil { + rp.Warnings = append(rp.Warnings, RemovalWarning{ + Artifact: artID, Agent: agentID, RelPath: relPath, + Reason: fmt.Sprintf("could not compute JSON removal: %v — left untouched", err), + }) + continue + } + for _, p := range drifted { + reason := fmt.Sprintf("value at %q differs from what loadout installed — left in place (re-run with --force to remove; a snapshot is taken first)", p) + if opts.Force { + reason = fmt.Sprintf("installed entry at %q was edited or already removed by hand — cannot be identified anymore, left in place", p) + } + rp.Warnings = append(rp.Warnings, RemovalWarning{Artifact: artID, Agent: agentID, RelPath: relPath, Reason: reason}) + } + if !changed { + if len(drifted) > 0 && !opts.Force { + retain(relPath) + } + continue // nothing of ours present (or drift kept it whole) + } + mode := tf.Mode + if info, statErr := os.Stat(abs); statErr == nil { + mode = info.Mode() + } + rp.Items = append(rp.Items, Item{ + Agent: agentID, Artifact: art, RelPath: tf.RelPath, AbsPath: abs, + Mode: mode, Merge: adapter.MergeJSON, Current: current, Exists: true, + Desired: newContent, Op: OpUpdate, Root: tf.Root, StateKey: relPath, + }) + } + } + } + + if opts.RemoveStore { + if art, found := e.Store.Artifact(artID); found { + storeTouched = true + abs := filepath.Join(e.Store.Root, art.Path) + if info, err := os.Stat(abs); err == nil { + rp.StoreFiles = append(rp.StoreFiles, StoreFile{ + RelPath: filepath.ToSlash(art.Path), AbsPath: abs, + IsDir: info.IsDir(), Op: OpDelete, }) } } } } + // Removing any manifest entry rewrites loadout.yaml — capture it too so a + // restore brings the entry back, not just the content files. + if storeTouched { + rp.StoreFiles = append(rp.StoreFiles, StoreFile{ + RelPath: store.ManifestFile, + AbsPath: filepath.Join(e.Store.Root, store.ManifestFile), + Op: OpUpdate, + }) + } + return rp, nil } +// skillDirDrifted reports whether a skill's installed directory differs from +// what loadout recorded at install time: any edited file, or any file loadout +// did not put there. A missing directory is not drift — there is nothing left +// to protect. +func skillDirDrifted(dirAbs, home string, recorded map[string]string) (bool, string) { + var reason string + err := filepath.WalkDir(dirAbs, func(p string, d fs.DirEntry, walkErr error) error { + if walkErr != nil || d.IsDir() { + return walkErr + } + rel, err := filepath.Rel(home, p) + if err != nil { + return err + } + rel = filepath.ToSlash(rel) + want, ok := recorded[rel] + if !ok { + reason = fmt.Sprintf("%s was not installed by loadout", rel) + return fs.SkipAll + } + raw, err := os.ReadFile(p) + if err != nil { + return err + } + if store.HashBytes(raw) != want { + reason = fmt.Sprintf("%s changed since loadout installed it", rel) + return fs.SkipAll + } + return nil + }) + if err != nil { + if os.IsNotExist(err) { + return false, "" + } + return true, fmt.Sprintf("could not verify directory contents: %v", err) + } + return reason != "", reason +} + func ownsPath(owned []string, relPath string) bool { relPath = filepath.ToSlash(relPath) for _, prefix := range owned { @@ -257,19 +436,8 @@ func (e *Engine) ApplySwitch(ctx context.Context, install *Plan, removal *Remova } } for _, it := range remChanges { - switch it.Op { - case OpDelete: - if it.IsDir { - if err := os.RemoveAll(it.AbsPath); err != nil { - return 0, 0, err - } - } else if err := os.Remove(it.AbsPath); err != nil && !os.IsNotExist(err) { - return 0, 0, err - } - default: // OpUpdate: block-strip rewrite - if err := writeItem(it); err != nil { - return 0, 0, err - } + if err := removeItem(it); err != nil { + return 0, 0, err } } @@ -280,15 +448,95 @@ func (e *Engine) ApplySwitch(ctx context.Context, install *Plan, removal *Remova for key, files := range groupFragments(install.stateItems()) { st.setGroup(key.artifact, key.agent, files) } - for _, k := range removal.ClearState { - st.removeGroup(k.Artifact, string(k.Agent)) - } + clearState(st, removal) if err := st.save(e.StatePath); err != nil { return 0, 0, fmt.Errorf("save state: %w", err) } return len(instChanges), len(remChanges), nil } +// removeItem executes one removal item: OpDelete unlinks (idempotently — an +// already-absent path is a safe no-op), anything else is a strip rewrite. +func removeItem(it Item) error { + switch it.Op { + case OpDelete: + if it.IsDir { + return os.RemoveAll(it.AbsPath) + } + if err := os.Remove(it.AbsPath); err != nil && !os.IsNotExist(err) { + return err + } + return nil + default: // OpUpdate: block-strip or JSON-removal rewrite + return writeItem(it) + } +} + +// ApplyRemoval performs a RemovalPlan as one confirmed write pass: snapshot +// (trigger "rm", including the store files the caller is about to delete), +// remove/rewrite every item, clear the state records, save state once. It +// honors the same confirmation invariant as Apply: nil or false confirm +// writes nothing. Store deletion itself stays with store.RemoveArtifact, +// called by the CLI afterwards — a crash in between leaves the artifact in +// the store, and re-running rm converges. +func (e *Engine) ApplyRemoval(ctx context.Context, rp *RemovalPlan, confirm func(*RemovalPlan) bool) (int, error) { + if len(rp.Items) == 0 && len(rp.ClearState) == 0 && len(rp.StoreFiles) == 0 { + return 0, nil + } + if confirm == nil || !confirm(rp) { + return 0, ErrNotConfirmed + } + + if e.BackupDir != "" { + // Store files ride under the same 'store' pseudo-agent that source + // updates and bundle imports use (spec 008), so restore round-trips + // through one consistent snapshot layout. + targets := itemSnapTargets(rp.Items) + for _, sf := range rp.StoreFiles { + targets = append(targets, snapTarget{ + agent: SnapshotAgent, relPath: sf.RelPath, absPath: sf.AbsPath, + op: sf.Op, mode: 0o644, isDir: sf.IsDir, root: adapter.RootAgentHome, + }) + } + if _, err := captureSnapshot(e.BackupDir, "rm", targets, e.now()); err != nil { + return 0, fmt.Errorf("snapshot before removal: %w", err) + } + if err := pruneSnapshots(e.BackupDir, e.KeepBackups); err != nil { + return 0, err + } + } + + for _, it := range rp.Items { + if err := removeItem(it); err != nil { + return 0, err + } + } + + st, err := loadState(e.StatePath) + if err != nil { + return 0, err + } + clearState(st, rp) + if err := st.save(e.StatePath); err != nil { + return 0, fmt.Errorf("save state: %w", err) + } + return len(rp.Items), nil +} + +// clearState drops the plan's artifact×agent install records, except for +// drift-skipped targets, whose records shrink to the retained subset instead +// of vanishing — forgetting them would make a later --force removal treat the +// file as never-installed. +func clearState(st state, rp *RemovalPlan) { + for _, k := range rp.ClearState { + if kept := rp.RetainState[k]; len(kept) > 0 { + st.setGroup(k.Artifact, string(k.Agent), kept) + continue + } + st.removeGroup(k.Artifact, string(k.Agent)) + } +} + // writeItem is the MkdirAll+WriteFile+Chmod sequence Apply and ApplySwitch // both use for non-delete items. func writeItem(it Item) error { diff --git a/internal/engine/removal_test.go b/internal/engine/removal_test.go index 5c92488..8992309 100644 --- a/internal/engine/removal_test.go +++ b/internal/engine/removal_test.go @@ -24,7 +24,7 @@ func TestPlanRemoval_MergeReplaceFile_QueuesDelete(t *testing.T) { if err != nil { t.Fatal(err) } - rp, err := env.eng.PlanRemoval(context.Background(), installed) + rp, err := env.eng.PlanRemoval(context.Background(), installed, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -56,7 +56,7 @@ func TestPlanRemoval_SkillArtifact_CollapsesToSingleDirectoryDelete(t *testing.T if err != nil { t.Fatal(err) } - rp, err := env.eng.PlanRemoval(context.Background(), installed) + rp, err := env.eng.PlanRemoval(context.Background(), installed, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -88,7 +88,7 @@ func TestPlanRemoval_MergeBlock_StripsBlockKeepsRestOfFile(t *testing.T) { if err != nil { t.Fatal(err) } - rp, err := env.eng.PlanRemoval(context.Background(), installed) + rp, err := env.eng.PlanRemoval(context.Background(), installed, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -116,7 +116,7 @@ func TestPlanRemoval_MergeBlock_AlreadyAbsent_NoopButClearsState(t *testing.T) { t.Fatal(err) } - rp, err := env.eng.PlanRemoval(context.Background(), []string{"style"}) + rp, err := env.eng.PlanRemoval(context.Background(), []string{"style"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -141,7 +141,7 @@ func TestPlanRemoval_MergeBlock_BrokenMarkers_WarnsDoesNotTouchFile(t *testing.T t.Fatal(err) } - rp, err := env.eng.PlanRemoval(context.Background(), []string{"style"}) + rp, err := env.eng.PlanRemoval(context.Background(), []string{"style"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -157,46 +157,82 @@ func TestPlanRemoval_MergeBlock_BrokenMarkers_WarnsDoesNotTouchFile(t *testing.T } } -func TestPlanRemoval_MergeJSON_WarnsLeavesFileClearsState(t *testing.T) { +func TestPlanRemoval_Hook_RemovesSettingsEntryKeepsUserBytes(t *testing.T) { env := newTestEnv(t) - if _, err := env.store.New(domain.KindHook, "lint"); err != nil { + userSettings := "{\n\t\"model\": \"opus\",\n\t\"env\": {\"FOO\": \"bar\"}\n}\n" + settingsPath := filepath.Join(env.home, "settings.json") + if err := os.WriteFile(settingsPath, []byte(userSettings), 0o644); err != nil { t.Fatal(err) } - env.applyAll(t) - - settingsPath := filepath.Join(env.home, "settings.json") - before, err := os.ReadFile(settingsPath) - if err != nil { + if _, err := env.store.New(domain.KindHook, "lint"); err != nil { t.Fatal(err) } + env.applyAll(t) - rp, err := env.eng.PlanRemoval(context.Background(), []string{"lint"}) + rp, err := env.eng.PlanRemoval(context.Background(), []string{"lint"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } - // One warning for the JSON fragment; the hook script (MergeReplace) is a - // real delete Item. - var jsonWarned bool - for _, w := range rp.Warnings { - if w.RelPath == "settings.json" { - jsonWarned = true + // The hook script (MergeReplace) is a delete Item; settings.json must be an + // OpUpdate item whose Desired has the merged hook entry surgically removed + // (ROADMAP:104 — this used to leak the entry forever). + var settings *Item + for i := range rp.Items { + if rp.Items[i].RelPath == "settings.json" { + settings = &rp.Items[i] } } - if !jsonWarned { - t.Errorf("Warnings = %v, want a warning for settings.json", rp.Warnings) + if settings == nil { + t.Fatalf("Items = %+v, want an OpUpdate item for settings.json", rp.Items) } - for _, it := range rp.Items { - if it.RelPath == "settings.json" { - t.Errorf("settings.json should never be a removal Item, got %+v", it) - } + if settings.Op != OpUpdate { + t.Errorf("settings.json Op = %s, want %s", settings.Op, OpUpdate) + } + desired := string(settings.Desired) + if strings.Contains(desired, "loadout-lint") || strings.Contains(desired, "hooks") { + t.Errorf("hook entry not removed from Desired:\n%s", desired) + } + if !strings.Contains(desired, "\t\"model\": \"opus\"") || !strings.Contains(desired, "{\"FOO\": \"bar\"}") { + t.Errorf("user settings bytes not preserved:\n%s", desired) } if len(rp.ClearState) != 1 || rp.ClearState[0].Artifact != "lint" { t.Errorf("ClearState = %v, want [lint]", rp.ClearState) } - after, err := os.ReadFile(settingsPath) - if err != nil || string(after) != string(before) { - t.Error("settings.json must be left byte-identical") +} + +func TestPlanRemoval_MCP_RemovesUserScopeServerEntryKeepsOthers(t *testing.T) { + fakeUserHome := t.TempDir() + t.Setenv("HOME", fakeUserHome) + env := newTestEnv(t) + + userConfig := "{\n\t\"numStartups\": 42,\n\t\"mcpServers\": {\n\t\t\"user-own\": {\"command\": \"deno\"}\n\t}\n}\n" + configPath := filepath.Join(fakeUserHome, ".claude.json") + if err := os.WriteFile(configPath, []byte(userConfig), 0o644); err != nil { + t.Fatal(err) + } + if _, err := env.store.New(domain.KindMCP, "gh"); err != nil { + t.Fatal(err) + } + env.applyAll(t) + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"gh"}, RemovalOptions{}) + if err != nil { + t.Fatalf("PlanRemoval() error = %v", err) + } + if len(rp.Items) != 1 { + t.Fatalf("Items = %+v, want one OpUpdate for .claude.json", rp.Items) + } + it := rp.Items[0] + if it.Op != OpUpdate || it.RelPath != ".claude.json" { + t.Errorf("item = %+v, want OpUpdate on .claude.json", it) + } + desired := string(it.Desired) + if strings.Contains(desired, "\"gh\"") { + t.Errorf("server entry not removed from Desired:\n%s", desired) + } + if !strings.Contains(desired, "\t\"numStartups\": 42") || !strings.Contains(desired, "\"user-own\": {\"command\": \"deno\"}") { + t.Errorf("user config bytes not preserved:\n%s", desired) } } @@ -214,7 +250,7 @@ func TestPlanRemoval_UnknownArtifactInManifest_WarnsDoesNotCrash(t *testing.T) { } env.store.Manifest.Artifacts = kept - rp, err := env.eng.PlanRemoval(context.Background(), []string{"review"}) + rp, err := env.eng.PlanRemoval(context.Background(), []string{"review"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v (must not error/crash)", err) } @@ -259,7 +295,7 @@ func TestPlanRemoval_AgentNotDetected_SkipsButClearsState(t *testing.T) { t.Fatal(err) } - rp, err := eng.PlanRemoval(context.Background(), []string{"review"}) + rp, err := eng.PlanRemoval(context.Background(), []string{"review"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -302,7 +338,7 @@ func TestPlanRemoval_RejectsDeleteOutsideOwnedPrefix(t *testing.T) { t.Fatal(err) } - rp, err := eng.PlanRemoval(context.Background(), []string{"plan"}) + rp, err := eng.PlanRemoval(context.Background(), []string{"plan"}, RemovalOptions{}) if err != nil { t.Fatalf("PlanRemoval() error = %v", err) } @@ -324,7 +360,7 @@ func TestApplySwitch_WithoutConfirmationWritesNothing(t *testing.T) { if err != nil { t.Fatal(err) } - removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}) + removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}, RemovalOptions{}) if err != nil { t.Fatal(err) } @@ -365,7 +401,7 @@ func TestApplySwitch_InstallAndRemovalInOneStateSave(t *testing.T) { if err != nil { t.Fatal(err) } - removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"old"}) + removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"old"}, RemovalOptions{}) if err != nil { t.Fatal(err) } @@ -404,7 +440,7 @@ func TestApplySwitch_PartialWriteThenRerunConverges(t *testing.T) { t.Fatal(err) } - removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}) + removalPlan, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}, RemovalOptions{}) if err != nil { t.Fatal(err) } @@ -424,3 +460,229 @@ func TestApplySwitch_PartialWriteThenRerunConverges(t *testing.T) { t.Errorf("InstalledArtifactIDs() = %v, want empty — state now converged", ids) } } + +func TestPlanRemoval_EditedReplaceFile_SkippedWithoutForceRemovedWith(t *testing.T) { + env := newTestEnv(t) + if _, err := env.store.New(domain.KindCommand, "plan"); err != nil { + t.Fatal(err) + } + env.applyAll(t) + + cmdPath := filepath.Join(env.home, "commands", "plan.md") + if err := os.WriteFile(cmdPath, []byte("user rewrote this\n"), 0o644); err != nil { + t.Fatal(err) + } + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"plan"}, RemovalOptions{}) + if err != nil { + t.Fatalf("PlanRemoval() error = %v", err) + } + if len(rp.Items) != 0 { + t.Errorf("Items = %+v, want none — edited file must be skipped", rp.Items) + } + if len(rp.Warnings) != 1 || !strings.Contains(rp.Warnings[0].Reason, "--force") { + t.Errorf("Warnings = %v, want one pointing at --force", rp.Warnings) + } + + forced, err := env.eng.PlanRemoval(context.Background(), []string{"plan"}, RemovalOptions{Force: true}) + if err != nil { + t.Fatalf("PlanRemoval(force) error = %v", err) + } + if len(forced.Items) != 1 || forced.Items[0].Op != OpDelete { + t.Errorf("forced Items = %+v, want the delete", forced.Items) + } +} + +func TestPlanRemoval_SkillDirWithUserAddedFile_SkippedWithoutForce(t *testing.T) { + env := newTestEnv(t) + env.addSkill(t, "review") + env.applyAll(t) + + if err := os.WriteFile(filepath.Join(env.home, "skills", "review", "my-notes.md"), []byte("mine\n"), 0o644); err != nil { + t.Fatal(err) + } + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"review"}, RemovalOptions{}) + if err != nil { + t.Fatalf("PlanRemoval() error = %v", err) + } + if len(rp.Items) != 0 { + t.Errorf("Items = %+v, want none — dir with a user file must be skipped", rp.Items) + } + if len(rp.Warnings) != 1 || !strings.Contains(rp.Warnings[0].Reason, "not installed by loadout") { + t.Errorf("Warnings = %v, want one naming the foreign file", rp.Warnings) + } +} + +func TestPlanRemoval_EditedBlock_SkippedWithoutForce(t *testing.T) { + env := newTestEnv(t) + if _, err := env.store.New(domain.KindInstruction, "style"); err != nil { + t.Fatal(err) + } + env.applyAll(t) + + claudeMD := filepath.Join(env.home, "CLAUDE.md") + raw, err := os.ReadFile(claudeMD) + if err != nil { + t.Fatal(err) + } + edited := strings.Replace(string(raw), "", "user edit inside the block\n", 1) + if err := os.WriteFile(claudeMD, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"style"}, RemovalOptions{}) + if err != nil { + t.Fatalf("PlanRemoval() error = %v", err) + } + if len(rp.Items) != 0 { + t.Errorf("Items = %+v, want none — edited block must be skipped", rp.Items) + } + if len(rp.Warnings) != 1 || !strings.Contains(rp.Warnings[0].Reason, "--force") { + t.Errorf("Warnings = %v, want one pointing at --force", rp.Warnings) + } +} + +func TestApplyRemoval_WithoutConfirmationWritesNothing(t *testing.T) { + env := newTestEnv(t) + env.addSkill(t, "gone") + env.applyAll(t) + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}, RemovalOptions{}) + if err != nil { + t.Fatal(err) + } + if _, err := env.eng.ApplyRemoval(context.Background(), rp, nil); err != ErrNotConfirmed { + t.Fatalf("ApplyRemoval(nil confirm) error = %v, want ErrNotConfirmed", err) + } + if _, err := env.eng.ApplyRemoval(context.Background(), rp, func(*RemovalPlan) bool { return false }); err != ErrNotConfirmed { + t.Fatalf("ApplyRemoval(decline) error = %v, want ErrNotConfirmed", err) + } + if _, err := os.Stat(filepath.Join(env.home, "skills", "gone", "SKILL.md")); err != nil { + t.Fatal("nothing may be removed without confirmation") + } +} + +func TestApplyRemoval_RemovesFilesAndClearsState(t *testing.T) { + env := newTestEnv(t) + env.addSkill(t, "gone") + env.applyAll(t) + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}, RemovalOptions{}) + if err != nil { + t.Fatal(err) + } + n, err := env.eng.ApplyRemoval(context.Background(), rp, func(*RemovalPlan) bool { return true }) + if err != nil { + t.Fatalf("ApplyRemoval() error = %v", err) + } + if n != 1 { + t.Errorf("removed = %d, want 1", n) + } + if _, err := os.Stat(filepath.Join(env.home, "skills", "gone")); !os.IsNotExist(err) { + t.Error("skill directory should be gone") + } + ids, err := env.eng.InstalledArtifactIDs() + if err != nil { + t.Fatal(err) + } + if len(ids) != 0 { + t.Errorf("InstalledArtifactIDs() = %v, want empty", ids) + } +} + +func TestApplyRemoval_SnapshotsStoreFilesAndRestoreResurrects(t *testing.T) { + env := newTestEnv(t) + backupDir := filepath.Join(env.store.Root, "..", "backups") + env.eng.BackupDir = backupDir + env.addSkill(t, "gone") + env.applyAll(t) + + rp, err := env.eng.PlanRemoval(context.Background(), []string{"gone"}, RemovalOptions{RemoveStore: true}) + if err != nil { + t.Fatal(err) + } + if len(rp.StoreFiles) != 2 { + t.Fatalf("StoreFiles = %+v, want content dir + manifest", rp.StoreFiles) + } + if _, err := env.eng.ApplyRemoval(context.Background(), rp, func(*RemovalPlan) bool { return true }); err != nil { + t.Fatal(err) + } + // Simulate the CLI's follow-up store deletion. + if err := env.store.RemoveArtifact("gone"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(env.store.Root, "skills", "gone")); !os.IsNotExist(err) { + t.Fatal("store content should be deleted") + } + + snaps, err := ListSnapshots(backupDir) + if err != nil || len(snaps) == 0 { + t.Fatalf("ListSnapshots() = %v, %v — want the rm snapshot", snaps, err) + } + snap := snaps[0] + if snap.Trigger != "rm" { + t.Errorf("Trigger = %q, want rm", snap.Trigger) + } + if _, err := env.eng.ApplyRestore(context.Background(), &snap, func(*Snapshot) bool { return true }); err != nil { + t.Fatalf("ApplyRestore() error = %v", err) + } + if _, err := os.Stat(filepath.Join(env.store.Root, "skills", "gone", "SKILL.md")); err != nil { + t.Error("restore should resurrect the store content") + } + raw, err := os.ReadFile(filepath.Join(env.store.Root, "loadout.yaml")) + if err != nil || !strings.Contains(string(raw), "gone") { + t.Error("restore should resurrect the manifest entry") + } + if _, err := os.Stat(filepath.Join(env.home, "skills", "gone", "SKILL.md")); err != nil { + t.Error("restore should resurrect the installed skill file") + } +} + +func TestApplyRemoval_DriftSkipRetainsStateSoForceStillWorks(t *testing.T) { + env := newTestEnv(t) + if _, err := env.store.New(domain.KindCommand, "plan"); err != nil { + t.Fatal(err) + } + env.applyAll(t) + + cmdPath := filepath.Join(env.home, "commands", "plan.md") + if err := os.WriteFile(cmdPath, []byte("user rewrote this\n"), 0o644); err != nil { + t.Fatal(err) + } + + // First rm: drift-skips the edited file but must NOT forget it was + // loadout-installed — otherwise --force afterwards has nothing to act on. + rp, err := env.eng.PlanRemoval(context.Background(), []string{"plan"}, RemovalOptions{}) + if err != nil { + t.Fatal(err) + } + if _, err := env.eng.ApplyRemoval(context.Background(), rp, func(*RemovalPlan) bool { return true }); err != nil { + t.Fatal(err) + } + ids, err := env.eng.InstalledArtifactIDs() + if err != nil { + t.Fatal(err) + } + if len(ids) != 1 || ids[0] != "plan" { + t.Fatalf("InstalledArtifactIDs() = %v, want the drift-skipped record retained", ids) + } + + forced, err := env.eng.PlanRemoval(context.Background(), []string{"plan"}, RemovalOptions{Force: true}) + if err != nil { + t.Fatal(err) + } + if len(forced.Items) != 1 { + t.Fatalf("forced Items = %+v, want the delete to still be plannable", forced.Items) + } + if _, err := env.eng.ApplyRemoval(context.Background(), forced, func(*RemovalPlan) bool { return true }); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(cmdPath); !os.IsNotExist(err) { + t.Error("--force should remove the edited file") + } + ids, _ = env.eng.InstalledArtifactIDs() + if len(ids) != 0 { + t.Errorf("state should be fully cleared after the forced removal, got %v", ids) + } +} diff --git a/internal/engine/snapshot.go b/internal/engine/snapshot.go index 80967f5..606d8a3 100644 --- a/internal/engine/snapshot.go +++ b/internal/engine/snapshot.go @@ -72,6 +72,12 @@ func snapshotRel(root adapter.TargetRoot, relPath string) string { // overwrite or delete existing content. Pure creations (nothing on disk yet) // are ignored. Returns (nil, nil) when there is nothing to lose. func takeSnapshot(backupDir, trigger string, items []Item, now time.Time) (*Snapshot, error) { + return captureSnapshot(backupDir, trigger, itemSnapTargets(items), now) +} + +// itemSnapTargets converts plan items into snapshot targets, keeping only +// those that would overwrite or delete existing content. +func itemSnapTargets(items []Item) []snapTarget { targets := make([]snapTarget, 0, len(items)) for _, it := range items { if it.Op != OpUpdate && it.Op != OpDelete { @@ -82,7 +88,7 @@ func takeSnapshot(backupDir, trigger string, items []Item, now time.Time) (*Snap op: it.Op, mode: it.Mode, isDir: it.IsDir, root: it.Root, }) } - return captureSnapshot(backupDir, trigger, targets, now) + return targets } // captureSnapshot copies each target's current content into a fresh snapshot diff --git a/internal/engine/snapshot_test.go b/internal/engine/snapshot_test.go index 87f43a4..31c4695 100644 --- a/internal/engine/snapshot_test.go +++ b/internal/engine/snapshot_test.go @@ -82,7 +82,7 @@ func TestApplySwitch_SnapshotsDeletedSkillDirectory(t *testing.T) { env.applyAll(t) // Remove "review" via a switch (empty install plan + removal of the skill). - removal, err := env.eng.PlanRemoval(context.Background(), []string{"review"}) + removal, err := env.eng.PlanRemoval(context.Background(), []string{"review"}, RemovalOptions{}) if err != nil { t.Fatal(err) } diff --git a/internal/store/store.go b/internal/store/store.go index 9556277..577cdd5 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -123,6 +123,33 @@ func (s *Store) AddArtifact(a domain.Artifact) error { return s.SaveManifest() } +// RemoveArtifact drops an artifact's manifest entry and deletes its content +// files. Manifest first: a crash in between leaves orphaned content files on +// disk (harmless — the manifest is the source of truth), never a manifest +// entry pointing at nothing. +func (s *Store) RemoveArtifact(id string) error { + idx := -1 + for i, a := range s.Manifest.Artifacts { + if a.ID == id { + idx = i + break + } + } + if idx < 0 { + return fmt.Errorf("artifact %q not found", id) + } + a := s.Manifest.Artifacts[idx] + s.Manifest.Artifacts = append(s.Manifest.Artifacts[:idx], s.Manifest.Artifacts[idx+1:]...) + if err := s.SaveManifest(); err != nil { + return err + } + abs := filepath.Join(s.Root, a.Path) + if rel, err := filepath.Rel(s.Root, abs); err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return fmt.Errorf("artifact %q: content path %q escapes the store — entry removed, files left untouched", id, a.Path) + } + return os.RemoveAll(abs) +} + // Content reads an artifact's file set. Directory artifacts return every // regular file keyed by relative path; file artifacts return one entry keyed // by base name. diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 19d3529..3e44309 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -202,3 +202,76 @@ func keys(c domain.Content) []string { } return out } + +func TestRemoveArtifact_DropsManifestEntryAndContent(t *testing.T) { + s, err := Init(t.TempDir(), "test") + if err != nil { + t.Fatal(err) + } + if _, err := s.New(domain.KindSkill, "gone"); err != nil { + t.Fatal(err) + } + if _, err := s.New(domain.KindCommand, "kept"); err != nil { + t.Fatal(err) + } + + if err := s.RemoveArtifact("gone"); err != nil { + t.Fatalf("RemoveArtifact() error = %v", err) + } + if _, found := s.Artifact("gone"); found { + t.Error("manifest entry should be gone") + } + if _, err := os.Stat(filepath.Join(s.Root, "skills", "gone")); !os.IsNotExist(err) { + t.Error("content directory should be deleted") + } + + // Persisted, not just in memory — and the other artifact survives intact. + reopened, err := Open(s.Root) + if err != nil { + t.Fatal(err) + } + if _, found := reopened.Artifact("gone"); found { + t.Error("removal not persisted to loadout.yaml") + } + if _, found := reopened.Artifact("kept"); !found { + t.Error("unrelated artifact lost") + } +} + +func TestRemoveArtifact_UnknownIDErrors(t *testing.T) { + s, err := Init(t.TempDir(), "test") + if err != nil { + t.Fatal(err) + } + if err := s.RemoveArtifact("nope"); err == nil { + t.Fatal("want an error for an unknown artifact") + } +} + +func TestRemoveArtifact_EscapingContentPathRefusesFileDeletion(t *testing.T) { + dir := t.TempDir() + s, err := Init(filepath.Join(dir, "store"), "test") + if err != nil { + t.Fatal(err) + } + outside := filepath.Join(dir, "precious.txt") + if err := os.WriteFile(outside, []byte("keep\n"), 0o644); err != nil { + t.Fatal(err) + } + s.Manifest.Artifacts = append(s.Manifest.Artifacts, domain.Artifact{ + ID: "evil", Kind: domain.KindCommand, Path: "../precious.txt", + }) + if err := s.SaveManifest(); err != nil { + t.Fatal(err) + } + + if err := s.RemoveArtifact("evil"); err == nil { + t.Fatal("want an error for a content path escaping the store") + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Error("file outside the store must never be deleted") + } + if _, found := s.Artifact("evil"); found { + t.Error("manifest entry should still be removed") + } +}