diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index dcbf772a..100e4586 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -155,9 +155,11 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo validated_packages.append(canonical) existing_identities.add(identity) # prevent duplicates within batch else: - reason = "not accessible or doesn't exist" - if not verbose: - reason += " -- run with --verbose for auth details" + reason = _local_path_failure_reason(dep_ref) + if not reason: + reason = "not accessible or doesn't exist" + if not verbose: + reason += " -- run with --verbose for auth details" invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) @@ -212,6 +214,50 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo return validated_packages, outcome +def _local_path_failure_reason(dep_ref): + """Return a specific failure reason for local path deps, or None for remote.""" + if not (dep_ref.is_local and dep_ref.local_path): + return None + local = Path(dep_ref.local_path).expanduser() + if not local.is_absolute(): + local = Path.cwd() / local + local = local.resolve() + if not local.exists(): + return "path does not exist" + if not local.is_dir(): + return "path is not a directory" + # Directory exists but has no package markers + return "no apm.yml, SKILL.md, or plugin.json found" + + +def _local_path_no_markers_hint(local_dir, verbose_log=None): + """Scan two levels for sub-packages and print a hint if any are found.""" + from apm_cli.utils.helpers import find_plugin_json + + markers = ("apm.yml", "SKILL.md") + found = [] + for child in sorted(local_dir.iterdir()): + if not child.is_dir(): + continue + if any((child / m).exists() for m in markers) or find_plugin_json(child) is not None: + found.append(child) + # Also check one more level (e.g. skills//) + for grandchild in sorted(child.iterdir()) if child.is_dir() else []: + if not grandchild.is_dir(): + continue + if any((grandchild / m).exists() for m in markers) or find_plugin_json(grandchild) is not None: + found.append(grandchild) + + if not found: + return + + _rich_info(" [i] Found installable package(s) inside this directory:") + for p in found[:5]: + _rich_echo(f" apm install {p}", color="dim") + if len(found) > 5: + _rich_echo(f" ... and {len(found) - 5} more", color="dim") + + def _validate_package_exists(package, verbose=False): """Validate that a package exists and is accessible on GitHub, Azure DevOps, or locally.""" import os @@ -241,7 +287,11 @@ def _validate_package_exists(package, verbose=False): if (local / "apm.yml").exists() or (local / "SKILL.md").exists(): return True from apm_cli.utils.helpers import find_plugin_json - return find_plugin_json(local) is not None + if find_plugin_json(local) is not None: + return True + # Directory exists but lacks package markers -- surface a hint + _local_path_no_markers_hint(local, verbose_log) + return False # For virtual packages, use the downloader's validation method if dep_ref.is_virtual: diff --git a/tests/unit/compilation/test_agents_compiler_coverage.py b/tests/unit/compilation/test_agents_compiler_coverage.py index e60e81ec..9274fe6e 100644 --- a/tests/unit/compilation/test_agents_compiler_coverage.py +++ b/tests/unit/compilation/test_agents_compiler_coverage.py @@ -256,8 +256,9 @@ def test_validate_primitives_outside_base_dir_uses_absolute_path(self): compiler.validate_primitives(primitives) self.assertEqual(len(compiler.warnings), 1) - # Should use absolute path (contains tmp2 path, not relative). - self.assertIn(str(tmp2), compiler.warnings[0]) + # portable_relpath resolves and returns POSIX paths + resolved_tmp2 = Path(tmp2).resolve().as_posix() + self.assertIn(resolved_tmp2, compiler.warnings[0]) finally: import shutil @@ -448,7 +449,9 @@ def test_generate_placement_summary_outside_base_uses_absolute(self): result = self._make_distributed_result([(outside_path, 2)]) summary = compiler._generate_placement_summary(result) - self.assertIn(outside_path, summary) + # portable_relpath resolves and returns POSIX paths + resolved_path = (Path(other_tmp) / "AGENTS.md").resolve().as_posix() + self.assertIn(resolved_path, summary) finally: import shutil @@ -477,7 +480,9 @@ def test_generate_distributed_summary_outside_base(self): summary = compiler._generate_distributed_summary( result, CompilationConfig() ) - self.assertIn(outside_path, summary) + # portable_relpath resolves and returns POSIX paths + resolved_path = (Path(other_tmp) / "AGENTS.md").resolve().as_posix() + self.assertIn(resolved_path, summary) finally: import shutil diff --git a/tests/unit/policy/test_discovery.py b/tests/unit/policy/test_discovery.py index 1f7f9f34..25059900 100644 --- a/tests/unit/policy/test_discovery.py +++ b/tests/unit/policy/test_discovery.py @@ -133,34 +133,24 @@ class TestLoadFromFile(unittest.TestCase): """Test _load_from_file with real filesystem.""" def test_valid_policy_file(self): - with tempfile.NamedTemporaryFile( - mode="w", suffix=".yml", delete=False - ) as f: - f.write(VALID_POLICY_YAML) - f.flush() - try: - result = _load_from_file(Path(f.name)) - self.assertTrue(result.found) - self.assertIsInstance(result.policy, ApmPolicy) - self.assertEqual(result.policy.name, "test-policy") - self.assertIn("file:", result.source) - self.assertIsNone(result.error) - finally: - os.unlink(f.name) + with tempfile.TemporaryDirectory() as tmpdir: + p = Path(tmpdir) / "policy.yml" + p.write_text(VALID_POLICY_YAML, encoding="utf-8") + result = _load_from_file(p) + self.assertTrue(result.found) + self.assertIsInstance(result.policy, ApmPolicy) + self.assertEqual(result.policy.name, "test-policy") + self.assertIn("file:", result.source) + self.assertIsNone(result.error) def test_invalid_yaml(self): - with tempfile.NamedTemporaryFile( - mode="w", suffix=".yml", delete=False - ) as f: - f.write("enforcement: invalid-value\n") - f.flush() - try: - result = _load_from_file(Path(f.name)) - self.assertFalse(result.found) - self.assertIsNotNone(result.error) - self.assertIn("Invalid policy file", result.error) - finally: - os.unlink(f.name) + with tempfile.TemporaryDirectory() as tmpdir: + p = Path(tmpdir) / "bad-policy.yml" + p.write_text("enforcement: invalid-value\n", encoding="utf-8") + result = _load_from_file(p) + self.assertFalse(result.found) + self.assertIsNotNone(result.error) + self.assertIn("Invalid policy file", result.error) def test_unreadable_file(self): result = _load_from_file(Path("/nonexistent/file.yml")) @@ -494,19 +484,14 @@ class TestDiscoverPolicy(unittest.TestCase): """Integration-level tests for discover_policy.""" def test_override_local_file(self): - with tempfile.NamedTemporaryFile( - mode="w", suffix=".yml", delete=False - ) as f: - f.write(VALID_POLICY_YAML) - f.flush() - try: - result = discover_policy( - Path("/fake"), policy_override=f.name - ) - self.assertTrue(result.found) - self.assertIn("file:", result.source) - finally: - os.unlink(f.name) + with tempfile.TemporaryDirectory() as tmpdir: + p = Path(tmpdir) / "override-policy.yml" + p.write_text(VALID_POLICY_YAML, encoding="utf-8") + result = discover_policy( + Path("/fake"), policy_override=str(p) + ) + self.assertTrue(result.found) + self.assertIn("file:", result.source) @patch("apm_cli.policy.discovery.requests") def test_override_url(self, mock_requests): diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index e5cb15d5..52ab9d55 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -489,3 +489,114 @@ def tracking_callback(dep_ref, mods_dir, parent_chain=""): assert ">" in chain, ( f"Expected '>' separator in chain, got: '{chain}'" ) + + +class TestLocalPathValidationMessages: + """Tests for improved local path validation error messages.""" + + def test_local_path_failure_reason_nonexistent(self, tmp_path): + """Non-existent path returns 'path does not exist'.""" + from apm_cli.commands.install import _local_path_failure_reason + from apm_cli.models.apm_package import DependencyReference + + dep_ref = DependencyReference.parse(str(tmp_path / "does-not-exist-xyz-9999")) + reason = _local_path_failure_reason(dep_ref) + assert reason == "path does not exist" + + def test_local_path_failure_reason_file_not_dir(self, tmp_path): + """A file (not directory) returns 'path is not a directory'.""" + from apm_cli.commands.install import _local_path_failure_reason + from apm_cli.models.apm_package import DependencyReference + + f = tmp_path / "somefile.txt" + f.write_text("hello") + dep_ref = DependencyReference.parse(str(f)) + reason = _local_path_failure_reason(dep_ref) + assert reason == "path is not a directory" + + def test_local_path_failure_reason_no_markers(self, tmp_path): + """Directory without markers returns specific message.""" + from apm_cli.commands.install import _local_path_failure_reason + from apm_cli.models.apm_package import DependencyReference + + empty_dir = tmp_path / "empty-pkg" + empty_dir.mkdir() + dep_ref = DependencyReference.parse(str(empty_dir)) + reason = _local_path_failure_reason(dep_ref) + assert reason == "no apm.yml, SKILL.md, or plugin.json found" + + def test_local_path_failure_reason_valid_apm_yml(self, tmp_path): + """Directory with apm.yml still returns 'no markers' message. + + _local_path_failure_reason is only called when _validate_package_exists + already returned False, so it doesn't re-check markers. We verify it + returns a string (not None) and doesn't crash. + """ + from apm_cli.commands.install import _local_path_failure_reason + from apm_cli.models.apm_package import DependencyReference + + pkg = tmp_path / "valid-pkg" + pkg.mkdir() + (pkg / "apm.yml").write_text("name: test\nversion: 1.0.0\n") + dep_ref = DependencyReference.parse(str(pkg)) + reason = _local_path_failure_reason(dep_ref) + assert reason == "no apm.yml, SKILL.md, or plugin.json found" + + def test_local_path_failure_reason_remote_ref(self): + """Remote refs return None (not a local path).""" + from apm_cli.commands.install import _local_path_failure_reason + from apm_cli.models.apm_package import DependencyReference + + dep_ref = DependencyReference.parse("owner/repo") + reason = _local_path_failure_reason(dep_ref) + assert reason is None + + def test_hint_finds_skill_in_subdirectory(self, tmp_path, capsys): + """Hint discovers SKILL.md in a child directory.""" + from apm_cli.commands.install import _local_path_no_markers_hint + + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\n") + + _local_path_no_markers_hint(tmp_path) + captured = capsys.readouterr() + # Rich may wrap long paths across lines; collapse before asserting + flat = captured.out.replace("\n", "") + assert "my-skill" in flat + + def test_hint_finds_nested_skill(self, tmp_path, capsys): + """Hint discovers SKILL.md two levels deep (skills//).""" + from apm_cli.commands.install import _local_path_no_markers_hint + + nested = tmp_path / "skills" / "deep-skill" + nested.mkdir(parents=True) + (nested / "SKILL.md").write_text("---\nname: deep-skill\n---\n") + + _local_path_no_markers_hint(tmp_path) + captured = capsys.readouterr() + flat = captured.out.replace("\n", "") + assert "deep-skill" in flat + + def test_hint_silent_when_no_packages(self, tmp_path, capsys): + """Hint produces no output when no sub-packages found.""" + from apm_cli.commands.install import _local_path_no_markers_hint + + (tmp_path / "random-file.txt").write_text("nothing here") + _local_path_no_markers_hint(tmp_path) + captured = capsys.readouterr() + assert captured.out == "" + + def test_hint_caps_at_five(self, tmp_path, capsys): + """Hint shows at most 5 packages then a '... and N more' line.""" + from apm_cli.commands.install import _local_path_no_markers_hint + + for i in range(8): + d = tmp_path / f"skill-{i:02d}" + d.mkdir() + (d / "SKILL.md").write_text(f"---\nname: skill-{i:02d}\n---\n") + + _local_path_no_markers_hint(tmp_path) + captured = capsys.readouterr() + assert "apm install" in captured.out + assert "... and 3 more" in captured.out