config append manifest lock and nimble skills structure

docs/bmad_output/implementation-artifacts/sprint-status.yaml

@@ -95,7 +95,7 @@ development_status:
   6-2-permissions-display-and-install-confirmation: done
   6-3-per-skill-venv-creation-and-dependency-installation: done
   6-4-dependency-conflict-detection: done
-  6-5-config-append-manifest-lock-and-nimble-skills-structure: backlog
+  6-5-config-append-manifest-lock-and-nimble-skills-structure: done
   epic-6-retrospective: optional
 
   # Epic 7: Launch-Ready — Developer Experience & Ecosystem Artifacts

nimble/cli/commands.py

@@ -335,16 +335,65 @@ def add(
         typer.echo("Installation cancelled.")
         raise typer.Exit(0)
 
-    from nimble.manifest.installer import InstallError, install_skill_venv
+    import shutil
+
+    from nimble.manifest.installer import (
+        InstallError,
+        clone_skill_repo,
+        install_skill_venv,
+    )
+    from nimble.manifest.lock import write_lock_entry
+    from nimble.manifest.parser import (
+        ConfigError,
+        append_skill_to_config,
+        remove_skill_entry_from_config,
+    )
 
     typer.echo(f"Installing '{spec.name}'...")
+    repo_root = _repo_root()
+    skill_dir = repo_root / ".nimble" / "skills" / spec.name
+
+    try:
+        clone_skill_repo(repo_url, skill_dir)
+    except InstallError as exc:
+        shutil.rmtree(skill_dir, ignore_errors=True)
+        typer.echo(str(exc), err=True)
+        raise typer.Exit(1)
+
     try:
-        install_skill_venv(spec, _repo_root())
+        install_skill_venv(spec, repo_root)
     except InstallError as exc:
         typer.echo(str(exc), err=True)
         raise typer.Exit(1)
 
-    typer.echo(f"Dependencies installed for '{spec.name}'.")
+    config_path = repo_root / "config.yaml"
+    try:
+        append_skill_to_config(config_path, spec, shortcut, repo_url, repo_root)
+    except ConfigError as exc:
+        typer.echo(str(exc), err=True)
+        raise typer.Exit(1)
+
+    lock_path = repo_root / ".nimble" / "manifest.lock"
+    try:
+        write_lock_entry(lock_path, spec.name, repo_url, spec.version)
+    except Exception as exc:
+        try:
+            remove_skill_entry_from_config(config_path, spec.name)
+        except ConfigError as rb_exc:
+            typer.echo(
+                f"Failed to update manifest.lock ({exc}). "
+                f"Could not roll back config.yaml: {rb_exc}",
+                err=True,
+            )
+            raise typer.Exit(1)
+        typer.echo(
+            f"Failed to update manifest.lock ({exc}). "
+            "The new skill entry was removed from config.yaml.",
+            err=True,
+        )
+        raise typer.Exit(1)
+
+    typer.echo(f"Skill '{spec.name}' installed and bound to {shortcut}.")
 
 
 if __name__ == "__main__":

nimble/manifest/installer.py

@@ -19,6 +19,19 @@ def _venv_pip(venv_path: Path) -> Path:
     return venv_path / "bin" / "pip"
 
 
+def clone_skill_repo(repo_url: str, skill_dir: Path) -> None:
+    try:
+        result = subprocess.run(
+            ["git", "clone", "--depth=1", repo_url, str(skill_dir)],
+            capture_output=True,
+            text=True,
+        )
+    except FileNotFoundError:
+        raise InstallError("'git' not found — install git to use 'nimble add'")
+    if result.returncode != 0:
+        raise InstallError(f"Failed to clone {repo_url}:\n{result.stderr.strip()}")
+
+
 def check_dependency_conflicts(spec: ManifestSpec, repo_root: Path) -> None:
     """Pre-flight dry-run: detect conflicts in an existing venv."""
     venv_path = repo_root / ".nimble" / "skills" / spec.name / ".venv"

nimble/manifest/lock.py

@@ -0,0 +1,35 @@
+from __future__ import annotations
+
+from pathlib import Path
+
+import yaml
+
+from nimble.manifest.parser import atomic_write
+
+
+def read_lock(lock_path: Path) -> dict[str, dict[str, str]]:
+    if not lock_path.exists():
+        return {}
+    try:
+        with lock_path.open(encoding="utf-8") as f:
+            data = yaml.safe_load(f)
+        if not isinstance(data, dict):
+            return {}
+        skills = data.get("skills", {})
+        if not isinstance(skills, dict):
+            return {}
+        return {k: v for k, v in skills.items() if isinstance(v, dict)}
+    except Exception:
+        return {}
+
+
+def write_lock_entry(
+    lock_path: Path, name: str, installed_from: str, version: str
+) -> None:
+    skills = read_lock(lock_path)
+    skills[name] = {"installed_from": installed_from, "version": version}
+    lock_path.parent.mkdir(parents=True, exist_ok=True)
+    content = yaml.dump(
+        {"skills": skills}, default_flow_style=False, allow_unicode=True
+    )
+    atomic_write(lock_path, content)

nimble/manifest/parser.py

@@ -140,6 +140,77 @@ def disable_skill_in_config(config_path: Path, skill_name: str) -> None:
     raise ValueError(f"No skill named '{skill_name}' found in config.yaml")
 
 
+def append_skill_to_config(
+    config_path: Path,
+    spec: ManifestSpec,
+    binding: str,
+    repo_url: str,
+    repo_root: Path,
+) -> None:
+    if not (spec.class_name or "").strip():
+        raise ConfigError(
+            "manifest.yaml must declare 'class_name' for community skill installation"
+        )
+    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:
+        raw = {}
+
+    skills = raw.get("skills", [])
+    if not isinstance(skills, list):
+        raise ConfigError("config.yaml 'skills' must be a list")
+
+    rel_path = str(Path(".nimble") / "skills" / spec.name / spec.entrypoint)
+    entry: dict[str, Any] = {
+        "name": spec.name,
+        "source": "community",
+        "path": rel_path,
+        "class_name": spec.class_name,
+        "binding": binding,
+        "installed_from": repo_url,
+        "version": spec.version,
+    }
+    skills.append(entry)
+    raw["skills"] = skills
+    atomic_write(
+        config_path, yaml.dump(raw, default_flow_style=False, allow_unicode=True)
+    )
+
+
+def remove_skill_entry_from_config(config_path: Path, skill_name: str) -> None:
+    """Remove the last skills[] entry whose name matches (rollback helper)."""
+    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("Rollback failed: config.yaml is empty")
+
+    skills = raw.get("skills", [])
+    if not isinstance(skills, list):
+        raise ConfigError("Rollback failed: config.yaml 'skills' is not a list")
+
+    for i in range(len(skills) - 1, -1, -1):
+        entry = skills[i]
+        if isinstance(entry, dict) and entry.get("name") == skill_name:
+            del skills[i]
+            atomic_write(
+                config_path,
+                yaml.dump(raw, default_flow_style=False, allow_unicode=True),
+            )
+            return
+
+    raise ConfigError(
+        f"Rollback failed: no skill named {skill_name!r} found in config.yaml"
+    )
+
+
 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

@@ -414,7 +414,10 @@ def test_add_confirms_and_proceeds() -> None:
             return_value=spec,
         ),
         patch("nimble.cli.commands._repo_root", return_value=fake_root),
+        patch("nimble.manifest.installer.clone_skill_repo"),
         patch("nimble.manifest.installer.install_skill_venv") as mock_install,
+        patch("nimble.manifest.parser.append_skill_to_config"),
+        patch("nimble.manifest.lock.write_lock_entry"),
     ):
         result = runner.invoke(
             app, ["add", "ctrl+shift+d", "github.com/user/skill"], input="y\n"
@@ -471,7 +474,10 @@ def test_add_uppercase_Y_confirms() -> None:
             return_value=spec,
         ),
         patch("nimble.cli.commands._repo_root", return_value=fake_root),
+        patch("nimble.manifest.installer.clone_skill_repo"),
         patch("nimble.manifest.installer.install_skill_venv") as mock_install,
+        patch("nimble.manifest.parser.append_skill_to_config"),
+        patch("nimble.manifest.lock.write_lock_entry"),
     ):
         result = runner.invoke(
             app, ["add", "ctrl+shift+d", "github.com/user/skill"], input="Y\n"
@@ -487,6 +493,7 @@ def test_add_install_error_exits_with_code_1() -> None:
             "nimble.manifest.parser.fetch_remote_manifest",
             return_value=_make_manifest_spec(),
         ),
+        patch("nimble.manifest.installer.clone_skill_repo"),
         patch(
             "nimble.manifest.installer.install_skill_venv",
             side_effect=InstallError("pip failed"),
@@ -499,6 +506,35 @@ def test_add_install_error_exits_with_code_1() -> None:
     assert "pip failed" in result.output
 
 
+def test_add_lock_write_failure_rolls_back_config(tmp_path: Path) -> None:
+    import yaml as _yaml
+
+    spec = _make_manifest_spec()
+    cfg = tmp_path / "config.yaml"
+    cfg.write_text("skills: []\n", encoding="utf-8")
+    with (
+        patch(
+            "nimble.manifest.parser.fetch_remote_manifest",
+            return_value=spec,
+        ),
+        patch("nimble.cli.commands._repo_root", return_value=tmp_path),
+        patch("nimble.manifest.installer.clone_skill_repo"),
+        patch("nimble.manifest.installer.install_skill_venv"),
+        patch(
+            "nimble.manifest.lock.write_lock_entry",
+            side_effect=OSError("disk full"),
+        ),
+    ):
+        result = runner.invoke(
+            app, ["add", "ctrl+shift+d", "github.com/user/skill"], input="y\n"
+        )
+    assert result.exit_code == 1
+    assert "manifest.lock" in result.output
+    assert "removed from config.yaml" in result.output
+    data = _yaml.safe_load(cfg.read_text(encoding="utf-8"))
+    assert data["skills"] == []
+
+
 def test_terminate_windows_openprocess_failure_raises() -> None:
     fake_kernel32 = MagicMock()
     fake_kernel32.OpenProcess.return_value = 0

tests/unit/manifest/test_installer.py

@@ -9,6 +9,7 @@
 from nimble.manifest.installer import (
     InstallError,
     check_dependency_conflicts,
+    clone_skill_repo,
     install_skill_venv,
 )
 from nimble.manifest.parser import ManifestSpec
@@ -176,3 +177,31 @@ def test_new_install_still_cleaned_up_on_failure(tmp_path: Path) -> None:
         with pytest.raises(InstallError):
             install_skill_venv(_make_spec(dependencies=[]), tmp_path)
     assert not skill_dir.exists()
+
+
+# ---------------------------------------------------------------------------
+# clone_skill_repo tests
+# ---------------------------------------------------------------------------
+
+
+def test_clone_skill_repo_success(tmp_path: Path) -> None:
+    skill_dir = tmp_path / "skill"
+    with patch(
+        "subprocess.run", return_value=MagicMock(returncode=0, stderr="")
+    ) as mock:
+        clone_skill_repo("https://github.com/u/skill", skill_dir)
+    args = mock.call_args[0][0]
+    assert args[0] == "git"
+    assert "--depth=1" in args
+    assert "https://github.com/u/skill" in args
+    assert str(skill_dir) in args
+
+
+def test_clone_skill_repo_failure(tmp_path: Path) -> None:
+    skill_dir = tmp_path / "skill"
+    with patch(
+        "subprocess.run",
+        return_value=MagicMock(returncode=1, stderr="fatal: repo not found"),
+    ):
+        with pytest.raises(InstallError, match="Failed to clone"):
+            clone_skill_repo("https://github.com/u/missing", skill_dir)

tests/unit/manifest/test_lock.py

@@ -0,0 +1,60 @@
+from __future__ import annotations
+
+from pathlib import Path
+
+import yaml
+
+from nimble.manifest.lock import read_lock, write_lock_entry
+
+
+def test_read_lock_missing_file(tmp_path: Path) -> None:
+    result = read_lock(tmp_path / ".nimble" / "manifest.lock")
+    assert result == {}
+
+
+def test_read_lock_empty_skills(tmp_path: Path) -> None:
+    lock_path = tmp_path / "manifest.lock"
+    lock_path.write_text("skills: {}\n", encoding="utf-8")
+    result = read_lock(lock_path)
+    assert result == {}
+
+
+def test_write_lock_entry_creates_file(tmp_path: Path) -> None:
+    lock_path = tmp_path / "manifest.lock"
+    write_lock_entry(
+        lock_path, "log-diagnosis", "https://github.com/u/log-diagnosis", "1.0.0"
+    )
+    data = yaml.safe_load(lock_path.read_text(encoding="utf-8"))
+    assert (
+        data["skills"]["log-diagnosis"]["installed_from"]
+        == "https://github.com/u/log-diagnosis"
+    )
+    assert data["skills"]["log-diagnosis"]["version"] == "1.0.0"
+
+
+def test_write_lock_entry_overwrites_existing(tmp_path: Path) -> None:
+    lock_path = tmp_path / "manifest.lock"
+    write_lock_entry(lock_path, "my-skill", "https://github.com/u/my-skill", "1.0.0")
+    write_lock_entry(lock_path, "my-skill", "https://github.com/u/my-skill", "2.0.0")
+    data = yaml.safe_load(lock_path.read_text(encoding="utf-8"))
+    assert data["skills"]["my-skill"]["version"] == "2.0.0"
+    assert len(data["skills"]) == 1
+
+
+def test_write_lock_entry_preserves_other_entries(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")
+    write_lock_entry(lock_path, "skill-a", "https://github.com/u/a", "1.1.0")
+    data = yaml.safe_load(lock_path.read_text(encoding="utf-8"))
+    assert data["skills"]["skill-a"]["version"] == "1.1.0"
+    assert data["skills"]["skill-b"]["version"] == "2.0.0"
+
+
+def test_write_lock_entry_creates_nimble_dir(tmp_path: Path) -> None:
+    lock_path = tmp_path / ".nimble" / "manifest.lock"
+    assert not lock_path.parent.exists()
+    write_lock_entry(lock_path, "my-skill", "https://github.com/u/s", "1.0.0")
+    assert lock_path.exists()
+    data = yaml.safe_load(lock_path.read_text(encoding="utf-8"))
+    assert "my-skill" in data["skills"]