# Review: PR #47 round 2 @ 150b493

- Role: Reviewer (grok)
- Identity: base `e1ed2708d835d712f5f3a9dfd51b8f66ca1ac447`, head `150b4937f816c6407be16fc4c37ab88e478927e7`, tree `afce44d2e0f91720d81e8d7d484f0e1358dee680`, 1 commit, clean worktree
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261004T044643Z-1b1440/1`, except temporary `.claude/skills` layouts created there and removed afterward. Main stayed at `6af4a79`. The control-plane `.claude/skills` was only read.
- `git diff --stat e1ed270..HEAD`: `tests/test_issue42_skill_blocks.py`, +59/−12. `git diff --check` is clean. No product file and no version line.

Round 1 on `e1ed270` was PASS from this reviewer. The report noted that a dangling `.claude/skills` symlink skipped, because `Path.exists()` follows the link and is false before `is_symlink()` was considered. Codex R1 is that note. This commit closes it.

| Round 1 | `150b493` |
|---|---|
| `exists()` false meant skip, including a dangling top-level symlink | Absence is `not exists() and not is_symlink()` (`tests/test_issue42_skill_blocks.py:248-249`). A symlink is then checked (`250-252`): it must exist and resolve to `.agents/skills`, otherwise the function returns a problem. |
| The live test skipped in a worktree, so the dangling case never ran | `test_claude_skills_layouts` builds six layouts under `tmp_path` and calls the same function (`294`). `broken_top` expects a non-empty problem list (`282-283`, `299-300`). `None` (the old skip) and `[]` both fail that assertion. |

## Same function, deterministic layouts

`claude_skills_problems` (`240-260`) is the only checker. The live test calls it with `REPO / ".claude" / "skills"`, `SKILLS`, and `SKILL_NAMES` (`263-267`). The synthetic test calls it with the temp link, the temp skills directory, and two fixed names (`274-275`, `294`). Success layouts assert `problems == []`. The absent layout asserts `None`. The three failure layouts assert the list is non-empty. They do not pin the message text. The live test does: a present layout must be exactly `[]`.

The six layouts are built with relative links inside `tmp_path` (`../.agents/skills`, `../../.agents/skills/<name>`). Nothing in the repo tree is required, so the cases run in a worktree. `broken_top` points at `../.agents/no-such-dir`. `broken_entry` replaces the second skill with a link to `gone`. `wrong_skill` points `multi-agent-mux-b` at `multi-agent-mux-a`.

## Probes

Temporary layouts in the snapshot, then removed (`git status` clean, `.claude` gone):

- No `.claude/skills`: `37 passed, 1 skipped in 0.07s`. The skip is the live test. The six synthetic layouts are among the 37.
- Dangling `ln -s ../.agents/skills-missing .claude/skills`: the live test failed at `:267` with a non-empty problem list. It did not skip. `Path.resolve()` on that dangling link returned the missing path and did not raise.
- Directory of the eight skill links plus `lib.sh` and `lib_py`: the live test passed.

The control-plane directory is not a symlink. `claude_skills_problems` on that path, the control-plane `.agents/skills`, and this tree's eight `SKILL_NAMES` returned `[]`.

[VERDICT: PASS]