diff --git a/docs/bmad_output/implementation-artifacts/deferred-work.md b/docs/bmad_output/implementation-artifacts/deferred-work.md index 3f115c9..aa5d530 100644 --- a/docs/bmad_output/implementation-artifacts/deferred-work.md +++ b/docs/bmad_output/implementation-artifacts/deferred-work.md @@ -1,5 +1,10 @@ # Deferred Work +## Deferred from: add-nimble-remove-command (2026-05-07) + +- `nimble/cli/commands.py:remove` + `nimble/manifest/installer.py`: skill directory path is constructed from an unvalidated name (`repo_root / ".nimble" / "skills" / skill_name`) — a malicious manifest with `name: "../../etc"` could cause `nimble add` to clone and `nimble remove` to delete outside `.nimble/skills/`. The `remove` command is gated by `remove_skill_from_config` (name must be in config.yaml), but the underlying path is not asserted to resolve within the expected subtree. Harden by resolving `skill_dir` and asserting it is relative to `repo_root / ".nimble" / "skills"` in both `add` and `remove`. +- `nimble/manifest/parser.py:remove_skill_from_config` + `remove_skill_entry_from_config`: both assume `yaml.safe_load` returns a `dict` (call `.get(...)` directly after a `None` check), but a valid YAML file containing a top-level list or scalar raises `AttributeError` instead of a clean `ConfigError`. Pre-existing pattern; fix with `if not isinstance(raw, dict): raise ConfigError(...)` in a parser-hardening pass. + ## Deferred from: nimble start config auto-create (2026-05-07) - `nimble/cli/commands.py:_repo_root()`: derives repo root from `Path(__file__).resolve().parent.parent.parent` (the installed package location), not from CWD or a git root. On a system-wide or pipx install, `config.yaml` is created inside the read-only package directory rather than the user's project. Pre-existing across all commands; resolve with a proper CWD-or-git-root detection strategy. diff --git a/docs/bmad_output/implementation-artifacts/spec-add-nimble-remove-command.md b/docs/bmad_output/implementation-artifacts/spec-add-nimble-remove-command.md new file mode 100644 index 0000000..3c34329 --- /dev/null +++ b/docs/bmad_output/implementation-artifacts/spec-add-nimble-remove-command.md @@ -0,0 +1,110 @@ +--- +title: 'Add nimble remove command' +type: 'feature' +created: '2026-05-07' +status: 'done' +baseline_commit: '8634d55e93f04aa3ef305e7a09b2efff98ce6ba6' +context: [] +--- + + + +## Intent + +**Problem:** There is no way to uninstall a skill installed via `nimble add` — users must manually edit `config.yaml`, delete the `.nimble/skills//` directory, and remove the lock entry. + +**Approach:** Add a `nimble remove ` CLI command that removes the config entry, lock entry, and skill directory in a single step after a `y/N` confirmation prompt. + +## Boundaries & Constraints + +**Always:** +- Require `y/Y` confirmation before any state is modified. +- Remove the skill entry from `config.yaml` first; abort with exit 1 on failure. +- Remove the lock entry from `manifest.lock` after config update — warn on `OSError`, do not abort. +- Delete `.nimble/skills//` after config update — warn if absent, do not abort. +- Print a restart hint if the daemon is running at removal time. + +**Ask First:** None — the operation is clear enough to proceed without mid-task human decisions. + +**Never:** +- Add a `--yes`/`--force` flag — explicit confirmation is intentional UX. +- Stop the daemon before removing — let the user decide when to restart. +- Fail the removal if the skill directory or lock entry is absent (may have been cleaned up manually). + +## I/O & Edge-Case Matrix + +| Scenario | Input / State | Expected Output / Behavior | Error Handling | +|----------|--------------|---------------------------|----------------| +| Happy path | skill in config + lock + dir; user enters `y` | config entry removed, lock entry removed, dir deleted, "Skill 'name' removed." | N/A | +| Confirmation declined | user enters `n` or empty | "Removal cancelled.", exit 0, no state changed | N/A | +| Skill not in config | no matching entry in config.yaml | "Skill 'name' not found in config.yaml", exit 1 | ConfigError propagated | +| Dir missing | config entry present, dir absent | config + lock updated, warn "Skill directory not found — skipping.", exit 0 | warn only | +| Lock entry missing | config entry present, no lock entry for name | config updated, no warning needed (silent no-op), exit 0 | silent | +| Daemon running | removal succeeds while daemon is running | success + "Nimble is running — restart with 'nimble restart' to apply changes." | N/A | + + + +## Code Map + +- `nimble/manifest/lock.py` — add `remove_lock_entry(lock_path, skill_name)` here; `read_lock` / `write_lock_entry` already present +- `nimble/manifest/parser.py:277` — existing `remove_skill_entry_from_config` (rollback helper, "Rollback failed:" prefix); add new `remove_skill_from_config` with user-friendly messages +- `nimble/cli/commands.py` — all CLI commands live here; `state`, `_repo_root`, `_do_stop` already imported +- `tests/unit/manifest/test_lock.py` — existing lock tests use `read_lock` / `write_lock_entry`; append tests for `remove_lock_entry` +- `tests/unit/cli/test_commands.py` — existing CLI tests; append tests for `remove` + +## Tasks & Acceptance + +**Execution:** +- [x] `nimble/manifest/lock.py` -- add `remove_lock_entry(lock_path: Path, skill_name: str) -> None` -- reads the lock, removes the named key if present (silently no-ops if absent or file missing), rewrites atomically via `atomic_write` +- [x] `nimble/manifest/parser.py` -- add `remove_skill_from_config(config_path: Path, skill_name: str) -> None` -- iterates the `skills` list, deletes all entries whose `name` matches, rewrites atomically; raises `ConfigError(f"Skill {skill_name!r} not found in config.yaml")` if none found +- [x] `nimble/cli/commands.py` -- add `remove` Typer command: prompt `y/N`, call `remove_skill_from_config` (exit 1 on `ConfigError`), call `remove_lock_entry` (warn on `OSError`), `shutil.rmtree` the skill dir (warn if absent), print success, print restart hint if daemon running +- [x] `tests/unit/manifest/test_lock.py` -- add tests: existing key removed and file updated; missing key is a no-op; missing lock file is a no-op +- [x] `tests/unit/cli/test_commands.py` -- add tests: happy path removes all three; confirmation declined leaves state unchanged; skill absent from config exits 1; dir absent still succeeds; daemon running shows restart hint + +**Acceptance Criteria:** +- Given a skill exists in config, lock, and dir, when `nimble remove ` is confirmed with `y`, then the config entry is removed, the lock entry is removed, the skill dir is deleted, "Skill '' removed." is printed, and the process exits 0. +- Given the user enters `n` at the prompt, when `nimble remove ` is run, then nothing is modified, "Removal cancelled." is printed, and the process exits 0. +- Given the skill has no entry in config.yaml, when `nimble remove ` is confirmed, then "Skill '' not found in config.yaml" is printed on stderr and the process exits 1. +- Given the skill directory is absent, when `nimble remove ` is confirmed, then config and lock are still updated, a warning about the missing dir is printed, and the process exits 0. +- Given the daemon is running when removal succeeds, then "Nimble is running — restart with 'nimble restart' to apply changes." is printed. + +## Verification + +**Commands:** +- `pytest tests/unit/manifest/test_lock.py tests/unit/cli/test_commands.py -q` -- expected: all new tests pass, zero regressions +- `flake8 nimble/manifest/lock.py nimble/manifest/parser.py nimble/cli/commands.py` -- expected: exit 0 +- `mypy nimble/manifest/lock.py nimble/manifest/parser.py nimble/cli/commands.py` -- expected: no errors + +## Suggested Review Order + +**Uninstall operation flow** + +- Entry point: command signature and y/N confirmation gate before any state changes + [`commands.py:474`](../../../nimble/cli/commands.py#L474) + +- Ordered teardown: config first (abort on failure), then lock and dir (warn-only) + [`commands.py:497`](../../../nimble/cli/commands.py#L497) + +- Directory deletion with OSError guard; dir-absent is warn-not-abort + [`commands.py:510`](../../../nimble/cli/commands.py#L510) + +- Daemon-running hint appended only after all mutations succeed + [`commands.py:522`](../../../nimble/cli/commands.py#L522) + +**Config mutation** + +- New user-facing function: removes all matches; contrast with rollback helper's last-match + [`parser.py:307`](../../../nimble/manifest/parser.py#L307) + +**Lock mutation** + +- Idempotent removal: early-return on absent key avoids a needless atomic write + [`lock.py:38`](../../../nimble/manifest/lock.py#L38) + +**Tests** + +- Happy path verifies skill directory is actually deleted, not just output text + [`test_commands.py:759`](../../../tests/unit/cli/test_commands.py#L759) + +- Lock unit tests: key removed, missing key no-op, missing file no-op + [`test_lock.py:63`](../../../tests/unit/manifest/test_lock.py#L63) diff --git a/nimble/cli/commands.py b/nimble/cli/commands.py index cd58c09..d238062 100644 --- a/nimble/cli/commands.py +++ b/nimble/cli/commands.py @@ -470,5 +470,61 @@ def add( typer.echo(f"Skill '{spec.name}' installed and bound to {shortcut}.") +@app.command() +def remove( + skill_name: str = typer.Argument(..., help="Name of the skill to remove"), +) -> None: + """Remove a skill installed via nimble add.""" + from nimble.manifest.lock import remove_lock_entry + from nimble.manifest.parser import ConfigError, remove_skill_from_config + + typer.echo(f"Remove skill '{skill_name}'? [y/N]: ", nl=False) + try: + line = sys.stdin.readline() + except (OSError, UnicodeDecodeError): + typer.echo("Removal cancelled.") + raise typer.Exit(0) + if line.rstrip("\r\n") not in ("y", "Y"): + typer.echo("Removal cancelled.") + raise typer.Exit(0) + + repo_root = _repo_root() + config_path = repo_root / "config.yaml" + lock_path = repo_root / ".nimble" / "manifest.lock" + skill_dir = repo_root / ".nimble" / "skills" / skill_name + + try: + remove_skill_from_config(config_path, skill_name) + except ConfigError as exc: + typer.echo(str(exc), err=True) + raise typer.Exit(1) + except OSError as exc: + typer.echo(f"Failed to update config.yaml: {exc}", err=True) + raise typer.Exit(1) + + try: + remove_lock_entry(lock_path, skill_name) + except OSError as exc: + typer.echo(f"Warning: failed to update manifest.lock: {exc}", err=True) + + if skill_dir.exists(): + import shutil + + try: + shutil.rmtree(skill_dir) + except OSError as exc: + typer.echo(f"Warning: failed to delete skill directory: {exc}", err=True) + else: + typer.echo("Skill directory not found — skipping.") + + typer.echo(f"Skill '{skill_name}' removed.") + + pid = state.read_pid() + if pid is not None and state.is_running(pid): + typer.echo( + "Nimble is running — restart with 'nimble restart' to apply changes." + ) + + if __name__ == "__main__": app() diff --git a/nimble/manifest/lock.py b/nimble/manifest/lock.py index b191be2..621696e 100644 --- a/nimble/manifest/lock.py +++ b/nimble/manifest/lock.py @@ -33,3 +33,14 @@ def write_lock_entry( {"skills": skills}, default_flow_style=False, allow_unicode=True ) atomic_write(lock_path, content) + + +def remove_lock_entry(lock_path: Path, skill_name: str) -> None: + skills = read_lock(lock_path) + if skill_name not in skills: + return + del skills[skill_name] + atomic_write( + lock_path, + yaml.dump({"skills": skills}, default_flow_style=False, allow_unicode=True), + ) diff --git a/nimble/manifest/parser.py b/nimble/manifest/parser.py index 9888363..77462b7 100644 --- a/nimble/manifest/parser.py +++ b/nimble/manifest/parser.py @@ -304,6 +304,34 @@ def remove_skill_entry_from_config(config_path: Path, skill_name: str) -> None: ) +def remove_skill_from_config(config_path: Path, skill_name: str) -> None: + """Remove all skills[] entries whose name matches (user-facing uninstall).""" + try: + with config_path.open(encoding="utf-8") as f: + raw = yaml.safe_load(f) + except (OSError, yaml.YAMLError) as exc: + raise ConfigError(f"Failed to read config.yaml: {exc}") from exc + + if raw is None: + raise ConfigError(f"Skill {skill_name!r} not found in config.yaml") + + skills = raw.get("skills", []) + if not isinstance(skills, list): + raise ConfigError("config.yaml 'skills' is not a list") + + updated = [ + e for e in skills if not (isinstance(e, dict) and e.get("name") == skill_name) + ] + if len(updated) == len(skills): + raise ConfigError(f"Skill {skill_name!r} not found in config.yaml") + + raw["skills"] = updated + atomic_write( + config_path, + yaml.dump(raw, default_flow_style=False, allow_unicode=True), + ) + + def read_skill_manifest(config: SkillConfig, base_path: Path) -> dict[str, Any] | None: base_root = base_path.resolve() skill_path = Path(config.path) diff --git a/tests/unit/cli/test_commands.py b/tests/unit/cli/test_commands.py index 3084d42..3d48af1 100644 --- a/tests/unit/cli/test_commands.py +++ b/tests/unit/cli/test_commands.py @@ -756,6 +756,104 @@ def test_add_config_field_trims_allowed_value_whitespace() -> None: assert configuration == {"target_language": "es"} +def test_remove_happy_path(tmp_path: Path) -> None: + skill_dir = tmp_path / ".nimble" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch("nimble.manifest.parser.remove_skill_from_config"), + patch("nimble.manifest.lock.remove_lock_entry"), + patch("nimble.cli.commands.state.read_pid", return_value=None), + ): + result = runner.invoke(app, ["remove", "my-skill"], input="y\n") + assert result.exit_code == 0 + assert "removed" in result.output + assert not skill_dir.exists() + + +def test_remove_confirmation_declined(tmp_path: Path) -> None: + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch( + "nimble.manifest.parser.remove_skill_from_config" + ) as mock_rm, + ): + result = runner.invoke(app, ["remove", "my-skill"], input="n\n") + assert result.exit_code == 0 + assert "cancelled" in result.output + mock_rm.assert_not_called() + + +def test_remove_confirmation_empty_declines(tmp_path: Path) -> None: + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch( + "nimble.manifest.parser.remove_skill_from_config" + ) as mock_rm, + ): + result = runner.invoke(app, ["remove", "my-skill"], input="\n") + assert result.exit_code == 0 + assert "cancelled" in result.output + mock_rm.assert_not_called() + + +def test_remove_skill_not_in_config(tmp_path: Path) -> None: + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch( + "nimble.manifest.parser.remove_skill_from_config", + side_effect=ConfigError("Skill 'my-skill' not found in config.yaml"), + ), + ): + result = runner.invoke(app, ["remove", "my-skill"], input="y\n") + assert result.exit_code == 1 + assert "not found" in result.output + + +def test_remove_dir_absent_warns_and_succeeds(tmp_path: Path) -> None: + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch("nimble.manifest.parser.remove_skill_from_config"), + patch("nimble.manifest.lock.remove_lock_entry"), + patch("nimble.cli.commands.state.read_pid", return_value=None), + ): + result = runner.invoke(app, ["remove", "my-skill"], input="y\n") + assert result.exit_code == 0 + assert "directory not found" in result.output + assert "removed" in result.output + + +def test_remove_lock_oserror_warns_but_succeeds(tmp_path: Path) -> None: + skill_dir = tmp_path / ".nimble" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch("nimble.manifest.parser.remove_skill_from_config"), + patch( + "nimble.manifest.lock.remove_lock_entry", + side_effect=OSError("disk full"), + ), + patch("nimble.cli.commands.state.read_pid", return_value=None), + ): + result = runner.invoke(app, ["remove", "my-skill"], input="y\n") + assert result.exit_code == 0 + assert "manifest.lock" in result.output + assert "removed" in result.output + + +def test_remove_daemon_running_shows_restart_hint(tmp_path: Path) -> None: + with ( + patch("nimble.cli.commands._repo_root", return_value=tmp_path), + patch("nimble.manifest.parser.remove_skill_from_config"), + patch("nimble.manifest.lock.remove_lock_entry"), + patch("nimble.cli.commands.state.read_pid", return_value=12345), + patch("nimble.cli.commands.state.is_running", return_value=True), + ): + result = runner.invoke(app, ["remove", "my-skill"], input="y\n") + assert result.exit_code == 0 + assert "restart" in result.output + + def test_terminate_windows_openprocess_failure_raises() -> None: fake_kernel32 = MagicMock() fake_kernel32.OpenProcess.return_value = 0 diff --git a/tests/unit/manifest/test_lock.py b/tests/unit/manifest/test_lock.py index 871672f..ac788ea 100644 --- a/tests/unit/manifest/test_lock.py +++ b/tests/unit/manifest/test_lock.py @@ -4,7 +4,7 @@ import yaml -from nimble.manifest.lock import read_lock, write_lock_entry +from nimble.manifest.lock import read_lock, remove_lock_entry, write_lock_entry def test_read_lock_missing_file(tmp_path: Path) -> None: @@ -58,3 +58,28 @@ def test_write_lock_entry_creates_nimble_dir(tmp_path: Path) -> None: assert lock_path.exists() data = yaml.safe_load(lock_path.read_text(encoding="utf-8")) assert "my-skill" in data["skills"] + + +def test_remove_lock_entry_removes_named_key(tmp_path: Path) -> None: + lock_path = tmp_path / "manifest.lock" + write_lock_entry(lock_path, "skill-a", "https://github.com/u/a", "1.0.0") + write_lock_entry(lock_path, "skill-b", "https://github.com/u/b", "2.0.0") + remove_lock_entry(lock_path, "skill-a") + data = yaml.safe_load(lock_path.read_text(encoding="utf-8")) + assert "skill-a" not in data["skills"] + assert "skill-b" in data["skills"] + + +def test_remove_lock_entry_missing_key_is_noop(tmp_path: Path) -> None: + lock_path = tmp_path / "manifest.lock" + write_lock_entry(lock_path, "skill-b", "https://github.com/u/b", "2.0.0") + remove_lock_entry(lock_path, "skill-a") + data = yaml.safe_load(lock_path.read_text(encoding="utf-8")) + assert "skill-b" in data["skills"] + + +def test_remove_lock_entry_missing_file_is_noop(tmp_path: Path) -> None: + lock_path = tmp_path / "manifest.lock" + assert not lock_path.exists() + remove_lock_entry(lock_path, "skill-a") + assert not lock_path.exists()