feat(cli): saasaloy remove — undo applied files via manifest - #37
Open
mimukit wants to merge 4 commits into
Open
feat(cli): saasaloy remove — undo applied files via manifest#37mimukit wants to merge 4 commits into
mimukit wants to merge 4 commits into
Conversation
Lays the ledger groundwork for `remove` (issue #27) — a structural config patch mutates a file another module owns, so it can't be a clean managed copy, but its application still needs to be recorded so `remove` can warn which files it can't undo on its own. - add `ManifestPatch` (module/file/patch) and `Manifest.patches[]` - `executePlan` appends an entry whenever a patch actually changes a file - dedupe on structural equality so a `--force` re-apply landing the same op again doesn't duplicate the entry - extend `manifest.schema.json` with the `patches` array
Closes #27 — an offline undo for a module's applied files, mirroring `add`'s plan/execute split and clack UX so the two commands feel like a pair. - `remover.ts`: `buildRemovePlan`/`executeRemovePlan` classify each manifest-owned file as delete/drift/missing against current disk state, handle owned `.claude/skills` links, prune emptied directories, drop dangling aliases, and report (never reverse) this module's config patches - `commands/remove.ts`: CLI surface with `--dry-run`/`--diff`/`--yes`/ `--force`; drifted (hand-edited) files always need per-file confirmation and are never silently clobbered under `--yes`; refuses to remove a module other installed modules still depend on unless `--force`d - wire `remove` into `index.ts`'s command table
Two review nits on the just-added remove command surfaced misleading output around drifted files and skill-link conflicts. - drop the pre-execute conflict skill-link warning now that the post-execute warning is the sole, accurate report of what happened - label a drifted file "drift -> kept (untracked)" under --yes instead of implying a confirmation prompt that never runs
Issue #27 adds the offline undo path (add -> remove); document the human-only test cases that automated coverage can't reach. - cover round-trip add/remove, dependents-refusal, and patch-warning flows using the repo's real api/database module pair - focus on interactive confirm prompts, the picker, Ctrl-C handling, and clack TUI legibility, since deterministic behavior is already agent-verified via --yes/--force
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
saasaloy remove <module>is the offline undo foradd: it deletes exactly the files.saasaloy/manifest.jsonattributes to a module, protects hand-edited (drifted) content, reconcilessaasaloy.json/saasaloy-lock.json/the manifest, prunes emptied directories, drops dangling aliases, and warns (never reverses) about config patches the module applied elsewhere. To make that last part possible, this PR also teaches theaddside to record every applied config patch in the manifest soremovehas something to warn about.Closes #27.
Changes
lib/manifest.ts/schemas/manifest.schema.json: addManifest.patches[]({module, file, patch}), loaded with an empty-array default for backward compatibility with manifests written before this change.lib/applier.ts:executePlanappends aManifestPatchentry whenever a config patch actually changes a file, deduped by structural equality so a--forcere-apply landing the same op doesn't duplicate the entry. Repoints the "clean reverse-patching is a follow-up" comment from#27to#36.lib/remover.ts(new):buildRemovePlan/executeRemovePlan— classifies each manifest-owned file asdelete/drift/missingagainst current disk state, handles owned.claude/.agentsskill links, prunes emptied directories, drops dangling aliases, computes cross-module dependents, and surfaces this module's recorded config patches for warning (never reversal).commands/remove.ts(new): CLI surface mirroringadd's clack UX —--dry-run,--diff(renders deletions as an all-red unified diff),--yes,--force; a bareremoveshows a picker over installed modules. Drifted files always require a per-file confirm and are never silently clobbered under--yes(labeled "drift → kept (untracked)" instead). Refuses to remove a module other installed modules still depend on unless--forced.index.ts: wiresremoveinto the command table.docs/qa/qa-remove-command-2026-07-25.md(new): manual QA plan (see below).Follow-up nit commit (
e4e8ed8) fixed two review findings on the first pass: dropped a misleading pre-execute skill-link-conflict warning in favor of the accurate post-execute one, and corrected the drift label shown under--yes.Assumptions
Mechanical choices made during implementation, called out for the reviewer:
patchesentry shape is{module, file, patch: RegistryPatch}; schema validation mirrorsregistry-item.schema.json's loose pattern (requiremodule+file+patch, don't deep-validate the patch payload itself).{module, file, patch}— an identical entry is never pushed twice (keeps a--forcere-apply idempotent).installed[]cases error/exit-0 via clackcancel/note, mirroringadd.ts's existing error UX rather than inventing a new pattern.remove.ts) unit tests — matches repo convention: onlylib/is unit-tested, command behavior is covered by the manual QA plan instead.--diffrenders a deletion as an all-red diff of(oldContent → ""), reusingadd's diff renderer and line cap.applier.ts's patch-reversal comment was repointed from#27to#36(the reverse-patching follow-up, confirmed open: "reverse config patches on remove...").Known behavior / follow-up (plan-sanctioned, not a defect)
dropDanglingAliasesscans all aliases, so removing module A can also drop a pre-existing dangling alias that belongs to module B. This is plan-sanctioned ("drop any alias whose prefix directory no longer exists") — noted here for reviewer awareness, not treated as a bug.removedrops the patch's manifest entry and warns per affected file, but does not revert the patch's content. Reversal is tracked in follow-up issue feat(cli): reverse config patches on remove via tracked manifest patches #36.Test plan
pnpm --filter saasaloy-cli test→ 9 test files, 100 tests, all passing (includes newremover.test.ts— 24 tests — and the patch-tracking additions toapplier.test.ts— 31 tests).pnpm --filter saasaloy-cli typecheck→ 0 errors.pnpm --filter saasaloy build→ tsup build succeeds.add→removeround trip exercised manually end-to-end in.dev/playgroundagainst the repo's realapi/databasemodules: install, dependents-refusal +--forceoverride,--dry-run/--diffpreviews, config-patch record/warn (never reverse), unmanaged-file and prune-boundary handling, hand-edit drift survival, unknown-module/unknown-flag errors, and backward-compat load of a pre-existing manifest with nopatcheskey.docs/qa/qa-remove-command-2026-07-25.md— 9 manual test cases covering the interactive-only surface (confirm prompts, the picker, Ctrl-C handling, clack TUI legibility) that the automated suite above can't reach.