feat: add nimble remove command to uninstall community skills

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.

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: []
+---
+
+<frozen-after-approval reason="human-owned intent — do not modify unless human renegotiates">
+
+## Intent
+
+**Problem:** There is no way to uninstall a skill installed via `nimble add` — users must manually edit `config.yaml`, delete the `.nimble/skills/<name>/` directory, and remove the lock entry.
+
+**Approach:** Add a `nimble remove <skill-name>` 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/<skill-name>/` 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 |
+
+</frozen-after-approval>
+
+## 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 <name>` is confirmed with `y`, then the config entry is removed, the lock entry is removed, the skill dir is deleted, "Skill '<name>' removed." is printed, and the process exits 0.
+- Given the user enters `n` at the prompt, when `nimble remove <name>` 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 <name>` is confirmed, then "Skill '<name>' not found in config.yaml" is printed on stderr and the process exits 1.
+- Given the skill directory is absent, when `nimble remove <name>` 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)

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()

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),
+    )

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)

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

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()