From 10481f47e0046a365f9204527ada698c76fae774 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 24 Mar 2026 02:38:10 +0100 Subject: [PATCH 1/3] fix: improve local path validation error messages When a local path points to a directory that exists but lacks package markers (apm.yml, SKILL.md, or plugin.json), the error message now says 'no apm.yml, SKILL.md, or plugin.json found' instead of the misleading 'not accessible or doesn't exist'. Additionally, when the directory contains discoverable sub-packages (up to 2 levels deep), a hint is printed suggesting the correct apm install commands. Closes #427 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 58 +++++++++++++-- tests/unit/test_install_command.py | 111 +++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 4 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index e7a0ca73..618f1886 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -157,9 +157,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) @@ -214,6 +216,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 one level 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 @@ -243,7 +289,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/test_install_command.py b/tests/unit/test_install_command.py index e5cb15d5..f1594636 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): + """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("/tmp/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 returns None (valid, no failure).""" + 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)) + # _local_path_failure_reason returns None for valid packages + # (since _validate_package_exists returns True first) + # But it also returns None for remote refs + reason = _local_path_failure_reason(dep_ref) + # For a local path that exists and is a dir but has markers, + # the function still returns the "no markers" message because + # it doesn't re-check markers. That's OK — this function is + # only called when _validate_package_exists already returned False. + # We just verify it doesn't crash and returns a string. + assert reason is not None or reason is None # no crash + + 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() + assert "my-skill" in captured.out + + 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() + assert "deep-skill" in captured.out + + 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 From e54ec4062986d7cafd987f4b43264b6ee9cbfd3e Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 24 Mar 2026 06:01:54 +0100 Subject: [PATCH 2/3] fix: Windows CI failures and PR review comments - Fix 3 path assertion failures in test_agents_compiler_coverage.py: use Path.resolve().as_posix() to match portable_relpath output instead of str(Path()) which returns 8.3 short names on Windows - Fix 3 file-locking failures in test_discovery.py: replace NamedTemporaryFile(delete=False)+os.unlink with TemporaryDirectory pattern to avoid WinError 32 - Fix hardcoded /tmp path in test_install_command.py (use tmp_path) - Fix tautological assertion (reason is not None or reason is None) - Fix docstring 'one level' -> 'two levels' in _local_path_no_markers_hint Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 2 +- .../test_agents_compiler_coverage.py | 13 ++-- tests/unit/policy/test_discovery.py | 63 +++++++------------ tests/unit/test_install_command.py | 21 +++---- 4 files changed, 43 insertions(+), 56 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index d78bbaca..100e4586 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -231,7 +231,7 @@ def _local_path_failure_reason(dep_ref): def _local_path_no_markers_hint(local_dir, verbose_log=None): - """Scan one level for sub-packages and print a hint if any are found.""" + """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") 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 f1594636..debacf39 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -494,12 +494,12 @@ def tracking_callback(dep_ref, mods_dir, parent_chain=""): class TestLocalPathValidationMessages: """Tests for improved local path validation error messages.""" - def test_local_path_failure_reason_nonexistent(self): + 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("/tmp/does-not-exist-xyz-9999") + 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" @@ -526,7 +526,12 @@ def test_local_path_failure_reason_no_markers(self, tmp_path): 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 returns None (valid, no failure).""" + """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 @@ -534,16 +539,8 @@ def test_local_path_failure_reason_valid_apm_yml(self, tmp_path): pkg.mkdir() (pkg / "apm.yml").write_text("name: test\nversion: 1.0.0\n") dep_ref = DependencyReference.parse(str(pkg)) - # _local_path_failure_reason returns None for valid packages - # (since _validate_package_exists returns True first) - # But it also returns None for remote refs reason = _local_path_failure_reason(dep_ref) - # For a local path that exists and is a dir but has markers, - # the function still returns the "no markers" message because - # it doesn't re-check markers. That's OK — this function is - # only called when _validate_package_exists already returned False. - # We just verify it doesn't crash and returns a string. - assert reason is not None or reason is None # no crash + 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 84cc2a744443e3ef176a73c5e73d6aa2edd71b67 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 24 Mar 2026 06:05:36 +0100 Subject: [PATCH 3/3] fix: handle Rich line-wrapping in hint tests Rich wraps long paths at 80 columns in CI, splitting substrings across lines. Collapse newlines before asserting substring presence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unit/test_install_command.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index debacf39..52ab9d55 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -561,7 +561,9 @@ def test_hint_finds_skill_in_subdirectory(self, tmp_path, capsys): _local_path_no_markers_hint(tmp_path) captured = capsys.readouterr() - assert "my-skill" in captured.out + # 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//).""" @@ -573,7 +575,8 @@ def test_hint_finds_nested_skill(self, tmp_path, capsys): _local_path_no_markers_hint(tmp_path) captured = capsys.readouterr() - assert "deep-skill" in captured.out + 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."""