diff --git a/agr.lock b/agr.lock index 8776257..70d89c4 100644 --- a/agr.lock +++ b/agr.lock @@ -16,6 +16,6 @@ installed-name = "code-review" [[skill]] handle = "microsoft/playwright-cli/playwright-cli" source = "github" -commit = "4e37405ffe6b40db68bb6a787cd1dadbe999e4a0" -content-hash = "sha256:50aa57a0172e6ed2544ac7c161a1c61f424dbdd57511ef0e0a6a3f35c509ed65" +commit = "a0d5bfd4d9658073029d33f979ac5a027568caec" +content-hash = "sha256:75e47183c30bc8651e76286680eddac88a3024a7ee5a7f1bc486d4d3fdee34ce" installed-name = "playwright-cli" diff --git a/agr/commands/remove.py b/agr/commands/remove.py index 9768c18..7c460ec 100644 --- a/agr/commands/remove.py +++ b/agr/commands/remove.py @@ -140,5 +140,6 @@ def _print_remove_result(result: CommandResult) -> None: if lockfile is not None: for candidates, is_ralph_lf in zip(removed_candidates, removed_ralph_flags): for identifier in candidates: - remove_lockfile_entry(lockfile, identifier, ralph=is_ralph_lf) + if remove_lockfile_entry(lockfile, identifier, ralph=is_ralph_lf): + break save_lockfile(lockfile, lockfile_path) diff --git a/agr/commands/sync.py b/agr/commands/sync.py index 1898340..6a7b633 100644 --- a/agr/commands/sync.py +++ b/agr/commands/sync.py @@ -431,6 +431,13 @@ def _run_global_sync() -> None: handle, source_name = dep.resolve( config.default_source, config.default_owner ) + if dep.type == DEPENDENCY_TYPE_RALPH: + # Ralphs are project-level only; skip in global mode. + console.print( + f"[yellow]Skipped:[/yellow] {dep.identifier} " + "(ralphs are not supported in global installs)" + ) + continue result = _sync_one_dependency( handle, source_name, diff --git a/agr/fetcher.py b/agr/fetcher.py index ee9790b..c744ff4 100644 --- a/agr/fetcher.py +++ b/agr/fetcher.py @@ -324,18 +324,20 @@ def _copy_skill_to_destination( return dest -def skill_not_found_message(name: str) -> str: - """Build a user-friendly message for a missing skill in a repository. - - Used by both fetcher and sync to produce consistent error text. - """ +def _dep_not_found_message(kind: str, name: str, marker: str, subdir: str) -> str: + """Build a user-friendly message for a missing dependency in a repository.""" return ( - f"Skill '{name}' not found in repository.\n" - f"No directory named '{name}' containing SKILL.md was found.\n" - f"Hint: Create a skill at 'skills/{name}/SKILL.md' or '{name}/SKILL.md'" + f"{kind} '{name}' not found in repository.\n" + f"No directory named '{name}' containing {marker} was found.\n" + f"Hint: Create a {kind.lower()} at '{subdir}/{name}/{marker}' or '{name}/{marker}'" ) +def skill_not_found_message(name: str) -> str: + """Build a user-friendly message for a missing skill in a repository.""" + return _dep_not_found_message("Skill", name, SKILL_MARKER, "skills") + + def install_skill_from_repo( repo_dir: Path, skill_name: str, @@ -998,11 +1000,7 @@ def _copy_ralph_to_destination( def ralph_not_found_message(name: str) -> str: """Build a user-friendly message for a missing ralph in a repository.""" - return ( - f"Ralph '{name}' not found in repository.\n" - f"No directory named '{name}' containing RALPH.md was found.\n" - f"Hint: Create a ralph at 'ralphs/{name}/RALPH.md' or '{name}/RALPH.md'" - ) + return _dep_not_found_message("Ralph", name, RALPH_MARKER, "ralphs") def prepare_repo_for_ralph(repo_dir: Path, ralph_name: str) -> Path | None: @@ -1070,6 +1068,41 @@ def install_ralph_from_repo( ) +def _find_local_ralph_name_conflicts( + handle: ParsedHandle, + ralphs_dir: Path, + repo_root: Path | None, + default_dest: Path, +) -> tuple[list[Path], bool]: + """Find conflicting local installs with the same ralph name. + + Returns a tuple of (conflict_paths, has_unknown_metadata). + """ + handle_id = build_handle_id(handle, repo_root) + conflicts: list[Path] = [] + has_unknown = False + + candidates = [ralphs_dir / handle.name, ralphs_dir / handle.to_installed_name()] + + for path in candidates: + if path.resolve() == default_dest.resolve(): + continue + if not is_valid_ralph_dir(path): + continue + meta = read_skill_metadata(path) + if meta: + if meta.get(METADATA_KEY_TYPE) != METADATA_TYPE_LOCAL: + continue + if meta.get(METADATA_KEY_ID) == handle_id: + continue + conflicts.append(path) + continue + has_unknown = True + conflicts.append(path) + + return conflicts, has_unknown + + def install_local_ralph( source_path: Path, ralphs_dir: Path, @@ -1104,6 +1137,25 @@ def install_local_ralph( stamp_ralph_metadata(default_dest, handle, repo_root, default_dest.name) return default_dest + conflicts, has_unknown = _find_local_ralph_name_conflicts( + handle, ralphs_dir, repo_root, default_dest + ) + if conflicts: + locations = ", ".join(str(path) for path in conflicts) + hint = "" + if has_unknown: + hint = ( + " If this is a remote ralph, run " + "`agr sync` or reinstall it to " + "add metadata." + ) + raise AgrError( + f"Local ralph name '{handle.name}' is already installed at {locations}. " + "agr allows only one local ralph with a given name. " + "Rename the ralph or remove the existing one." + f"{hint}" + ) + ralph_dest = _resolve_ralph_destination(handle, ralphs_dir, repo_root) return _copy_ralph_to_destination( diff --git a/agr/lockfile.py b/agr/lockfile.py index 72b2119..f4acbfa 100644 --- a/agr/lockfile.py +++ b/agr/lockfile.py @@ -153,8 +153,7 @@ def _build_aot(entries: list[LockedSkill]) -> tomlkit.items.AoT: return aot doc["skill"] = _build_aot(lockfile.skills) - if lockfile.ralphs: - doc["ralph"] = _build_aot(lockfile.ralphs) + doc["ralph"] = _build_aot(lockfile.ralphs) path.write_text(tomlkit.dumps(doc)) @@ -212,9 +211,16 @@ def update_lockfile_entry( def remove_lockfile_entry( lockfile: Lockfile, identifier: str, *, ralph: bool = False -) -> None: - """Remove an entry from the lockfile by identifier.""" +) -> bool: + """Remove an entry from the lockfile by identifier. + + Returns True if an entry was removed, False if no match was found. + """ if ralph: + before = len(lockfile.ralphs) lockfile.ralphs = [r for r in lockfile.ralphs if r.identifier != identifier] + return len(lockfile.ralphs) < before else: + before = len(lockfile.skills) lockfile.skills = [s for s in lockfile.skills if s.identifier != identifier] + return len(lockfile.skills) < before diff --git a/agr/ralph.py b/agr/ralph.py index d83dd9f..14cd3af 100644 --- a/agr/ralph.py +++ b/agr/ralph.py @@ -2,29 +2,21 @@ from pathlib import Path, PurePosixPath -from agr.skill import EXCLUDED_DIRS, _shallowest +from agr.skill import _is_excluded_skill_path, _shallowest # Marker file for ralphs RALPH_MARKER = "RALPH.md" - -def _is_excluded_ralph_path(parts: tuple[str, ...]) -> bool: - """Check if a relative RALPH.md path should be excluded from discovery. - - Same rules as skill discovery: - 1. Root-level RALPH.md is a repo marker, not a ralph directory. - 2. Any path component matching EXCLUDED_DIRS disqualifies the entry. - """ - if len(parts) == 1: - return True - return any(part in EXCLUDED_DIRS for part in parts) +# Reuse skill exclusion logic — same rules apply to ralphs: +# root-level marker is a repo marker, and EXCLUDED_DIRS are skipped. +_is_excluded_marker_path = _is_excluded_skill_path def _is_excluded_path(path: Path, repo_dir: Path) -> bool: """Check if a path should be excluded from ralph discovery.""" rel = path.relative_to(repo_dir) - return _is_excluded_ralph_path(rel.parts) + return _is_excluded_marker_path(rel.parts) def is_valid_ralph_dir(path: Path) -> bool: @@ -63,7 +55,7 @@ def _find_ralph_dirs_in_listing(paths: list[str]) -> list[PurePosixPath]: rel_path = PurePosixPath(rel) if rel_path.name != RALPH_MARKER: continue - if _is_excluded_ralph_path(rel_path.parts): + if _is_excluded_marker_path(rel_path.parts): continue results.append(rel_path.parent) return results diff --git a/tests/cli/agr/test_add.py b/tests/cli/agr/test_add.py index be9f7a2..f05a589 100644 --- a/tests/cli/agr/test_add.py +++ b/tests/cli/agr/test_add.py @@ -120,3 +120,14 @@ def test_add_local_ralph_not_installed_to_tools(self, agr, cli_project, cli_ralp agr("add", "./ralphs/test-ralph") tool_dir = cli_project / ".claude" / "skills" / "test-ralph" assert not tool_dir.exists() + + def test_add_both_markers_fails(self, agr, cli_project): + """agr add fails when directory has both SKILL.md and RALPH.md.""" + both_dir = cli_project / "both-type" + both_dir.mkdir() + (both_dir / "SKILL.md").write_text("# Skill") + (both_dir / "RALPH.md").write_text("# Ralph") + + result = agr("add", "./both-type") + + assert_cli(result).failed().stdout_contains("both SKILL.md and RALPH.md") diff --git a/tests/cli/agr/test_remove.py b/tests/cli/agr/test_remove.py index e15907c..3242996 100644 --- a/tests/cli/agr/test_remove.py +++ b/tests/cli/agr/test_remove.py @@ -40,3 +40,30 @@ def test_remove_not_installed_fails(self, agr, cli_skill): result = agr("remove", "./skills/test-skill") assert_cli(result).failed() + + +class TestAgrRemoveRalph: + """Tests for agr remove with ralph type.""" + + def test_remove_ralph_succeeds(self, agr, cli_ralph): + """agr remove removes installed ralph.""" + agr("add", "./ralphs/test-ralph") + result = agr("remove", "./ralphs/test-ralph") + assert_cli(result).succeeded().stdout_contains("Removed:") + + def test_remove_ralph_cleans_directory(self, agr, cli_project, cli_ralph): + """agr remove deletes installed ralph directory.""" + agr("add", "./ralphs/test-ralph") + installed = cli_project / ".agents" / "ralphs" / "test-ralph" + assert installed.exists() + + agr("remove", "./ralphs/test-ralph") + assert not installed.exists() + + def test_remove_ralph_updates_config(self, agr, cli_project, cli_ralph): + """agr remove updates agr.toml.""" + agr("add", "./ralphs/test-ralph") + agr("remove", "./ralphs/test-ralph") + + config = (cli_project / "agr.toml").read_text() + assert "ralphs/test-ralph" not in config diff --git a/tests/cli/agr/test_sync.py b/tests/cli/agr/test_sync.py index 44dd77e..924bdc6 100644 --- a/tests/cli/agr/test_sync.py +++ b/tests/cli/agr/test_sync.py @@ -86,3 +86,28 @@ def test_sync_instructions_creates_agents_for_cursor( assert_cli(result).succeeded() assert (cli_project / "AGENTS.md").exists() assert (cli_project / "AGENTS.md").read_text() == "Claude instructions\n" + + +class TestAgrSyncRalph: + """Tests for agr sync with ralph dependencies.""" + + def test_sync_ralph_installs_from_config( + self, agr, cli_project, cli_ralph, cli_config + ): + """agr sync installs ralph from agr.toml config.""" + cli_config('dependencies = [{path = "./ralphs/test-ralph", type = "ralph"}]') + + result = agr("sync") + + assert_cli(result).succeeded().stdout_contains("Installed:") + installed = cli_project / ".agents" / "ralphs" / "test-ralph" + assert installed.exists() + assert (installed / "RALPH.md").exists() + + def test_sync_ralph_reports_up_to_date(self, agr, cli_project, cli_ralph): + """agr sync reports already installed ralph as up to date.""" + agr("add", "./ralphs/test-ralph") + + result = agr("sync") + + assert_cli(result).succeeded().stdout_contains("up to date") diff --git a/tests/unit/test_lockfile.py b/tests/unit/test_lockfile.py index 5a7c63e..224abc5 100644 --- a/tests/unit/test_lockfile.py +++ b/tests/unit/test_lockfile.py @@ -394,3 +394,35 @@ def test_is_lockfile_current_missing_ralph(self): Dependency(type="ralph", handle="user/repo/ralph"), ] assert is_lockfile_current(lockfile, deps) is False + + +class TestRemoveLockfileEntryReturnValue: + """Tests for remove_lockfile_entry return value.""" + + def test_returns_true_on_match(self): + lockfile = Lockfile( + skills=[LockedSkill(handle="user/repo/a", installed_name="a")] + ) + assert remove_lockfile_entry(lockfile, "user/repo/a") is True + assert len(lockfile.skills) == 0 + + def test_returns_false_on_miss(self): + lockfile = Lockfile( + skills=[LockedSkill(handle="user/repo/a", installed_name="a")] + ) + assert remove_lockfile_entry(lockfile, "user/repo/unknown") is False + assert len(lockfile.skills) == 1 + + def test_returns_true_on_ralph_match(self): + lockfile = Lockfile( + ralphs=[LockedSkill(handle="user/repo/r", installed_name="r")] + ) + assert remove_lockfile_entry(lockfile, "user/repo/r", ralph=True) is True + assert len(lockfile.ralphs) == 0 + + def test_returns_false_on_ralph_miss(self): + lockfile = Lockfile( + ralphs=[LockedSkill(handle="user/repo/r", installed_name="r")] + ) + assert remove_lockfile_entry(lockfile, "user/repo/x", ralph=True) is False + assert len(lockfile.ralphs) == 1