fix(agent-setup): address review feedback

Author: hassiebp

.agents/AGENTS.md

@@ -156,7 +156,8 @@ Security/config notes:
 - Keep credentials and machine-specific secrets in environment variables or
   local untracked files, never in committed agent config.
 - The shared Claude settings intentionally deny reading `./.env` and
-  `./.env.*`. If a task genuinely requires inspecting local env overrides, get
+  `./.env.*`, and they do not auto-approve Bash commands by default. If a task
+  genuinely requires inspecting local env overrides or shell access, get
   explicit user approval first instead of weakening the default policy.
 - For authenticated MCP servers or provider-specific config additions, prefer
   secret injection via environment variables rather than committed inline

.agents/config.json

@@ -13,15 +13,7 @@
   "claude": {
     "settings": {
       "permissions": {
-        "allow": [
-          "Bash(find:*)",
-          "Bash(rg:*)",
-          "Bash(grep:*)",
-          "Bash(ls:*)",
-          "Bash(cat:*)",
-          "Bash(head:*)",
-          "Bash(tail:*)"
-        ],
+        "allow": [],
         "deny": [
           "Read(./.env)",
           "Read(./.env.*)"

scripts/agents/sync-agent-shims.py

@@ -158,10 +158,22 @@ def remove_path(path: Path) -> None:
         path.unlink()
 
 
+def expected_symlink_target(path: Path, target: Path) -> Path:
+    return Path(os.path.relpath(target, start=path.parent))
+
+
 def is_matching_symlink(path: Path, target: Path) -> bool:
     if not path.is_symlink():
         return False
-    return path.resolve() == target.resolve()
+    return Path(os.readlink(path)) == expected_symlink_target(path, target)
+
+
+def is_within_repo(path: Path, repo_root: Path) -> bool:
+    try:
+        path.resolve().relative_to(repo_root.resolve())
+        return True
+    except ValueError:
+        return False
 
 
 def find_unexpected_children(path: Path, expected_children: set[str]) -> list[Path]:
@@ -238,21 +250,41 @@ def sync_symlink_outputs(
             continue
 
         remove_path(path)
-        relative_target = Path(os.path.relpath(target, start=path.parent))
-        path.symlink_to(relative_target, target_is_directory=target.is_dir())
+        path.symlink_to(
+            expected_symlink_target(path, target), target_is_directory=target.is_dir()
+        )
         print(f"Linked {path}")
 
     return has_mismatch
 
 
 def sync_managed_directories(
-    managed_directories: list[dict[str, Any]], check_mode: bool
+    managed_directories: list[dict[str, Any]], repo_root: Path, check_mode: bool
 ) -> bool:
     has_mismatch = False
 
     for directory in managed_directories:
         path: Path = directory["path"]
         expected_children: set[str] = directory["expected_children"]
+
+        if path.is_symlink():
+            message = f"Managed generated shim directory must not be a symlink: {path}"
+            if check_mode:
+                has_mismatch = True
+                print_error(message)
+                continue
+            raise RuntimeError(message)
+
+        if path.exists() and not is_within_repo(path, repo_root):
+            message = (
+                f"Managed generated shim directory resolves outside the repository: {path}"
+            )
+            if check_mode:
+                has_mismatch = True
+                print_error(message)
+                continue
+            raise RuntimeError(message)
+
         unexpected_children = find_unexpected_children(path, expected_children)
 
         if not unexpected_children:
@@ -319,7 +351,7 @@ def main() -> int:
     skills_root = repo_root / ".agents" / "skills"
     shared_skill_names = sorted(
         entry.name
-        for entry in skills_root.iterdir()
+        for entry in (skills_root.iterdir() if skills_root.exists() else [])
         if entry.is_dir() and (entry / "SKILL.md").exists()
     )
 
@@ -354,7 +386,8 @@ def main() -> int:
 
     has_mismatch = sync_symlink_outputs(symlink_outputs, check_mode) or has_mismatch
     has_mismatch = (
-        sync_managed_directories(managed_directories, check_mode) or has_mismatch
+        sync_managed_directories(managed_directories, repo_root, check_mode)
+        or has_mismatch
     )
 
     return 1 if check_mode and has_mismatch else 0

tests/test_sync_agent_shims.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import json
+import os
 import subprocess
 import sys
 from pathlib import Path
@@ -101,11 +102,9 @@ def test_sync_agent_shims_generates_expected_outputs(tmp_path: Path) -> None:
         ],
     }
     assert (repo_root / "AGENTS.md").is_symlink()
-    assert (repo_root / "AGENTS.md").resolve() == (
-        repo_root / ".agents" / "AGENTS.md"
-    ).resolve()
+    assert Path(os.readlink(repo_root / "AGENTS.md")) == Path(".agents/AGENTS.md")
     assert (repo_root / "CLAUDE.md").is_symlink()
-    assert (repo_root / "CLAUDE.md").resolve() == (repo_root / "AGENTS.md").resolve()
+    assert Path(os.readlink(repo_root / "CLAUDE.md")) == Path("AGENTS.md")
     assert (repo_root / ".claude" / "skills" / "example").is_symlink()
     assert not (repo_root / ".claude" / "skills" / "stale").exists()
     assert (repo_root / ".codex" / "config.toml").exists()
@@ -121,3 +120,54 @@ def test_sync_agent_shims_check_mode_detects_drift(tmp_path: Path) -> None:
 
     assert result.returncode == 1
     assert "Out of sync" in result.stderr
+
+
+def test_sync_agent_shims_check_mode_detects_symlink_target_drift(tmp_path: Path) -> None:
+    repo_root = make_fixture_repo(tmp_path)
+    run_script(repo_root)
+    (repo_root / "CLAUDE.md").unlink()
+    (repo_root / "CLAUDE.md").symlink_to(".agents/AGENTS.md")
+
+    result = run_script(repo_root, "--check")
+
+    assert result.returncode == 1
+    assert "Out of sync symlink" in result.stderr
+
+
+def test_sync_agent_shims_handles_missing_skills_directory(tmp_path: Path) -> None:
+    repo_root = make_fixture_repo(tmp_path)
+    readme_path = repo_root / ".agents" / "skills" / "README.md"
+    example_path = repo_root / ".agents" / "skills" / "example"
+    (example_path / "SKILL.md").unlink()
+    readme_path.unlink()
+    example_path.rmdir()
+    (repo_root / ".agents" / "skills").rmdir()
+
+    result = run_script(repo_root)
+
+    assert result.returncode == 0
+    managed_dir = repo_root / ".claude" / "skills"
+    assert managed_dir.exists()
+    assert list(managed_dir.iterdir()) == []
+
+
+def test_sync_agent_shims_rejects_symlinked_managed_directories(tmp_path: Path) -> None:
+    repo_root = make_fixture_repo(tmp_path)
+    run_script(repo_root)
+    external_dir = tmp_path / "external-claude-skills"
+    external_dir.mkdir()
+    (external_dir / "foreign").mkdir()
+    managed_dir = repo_root / ".claude" / "skills"
+    for child in managed_dir.iterdir():
+        child.unlink()
+    managed_dir.rmdir()
+    managed_dir.symlink_to(external_dir, target_is_directory=True)
+
+    check_result = run_script(repo_root, "--check")
+    sync_result = run_script(repo_root)
+
+    assert check_result.returncode == 1
+    assert "must not be a symlink" in check_result.stderr
+    assert sync_result.returncode == 1
+    assert "must not be a symlink" in sync_result.stderr
+    assert (external_dir / "foreign").exists()