fix: improve local path validation error messages (#431)

* 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>

* 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>

* 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>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: danielmeppiel

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/<name>/)
+        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:

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
 

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

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/<name>/)."""
+        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