[qingyun-wu]

Problem

check_skill_symlinks hardcodes Path.home()/".claude"/"skills", ignoring the workspace-scoped CLAUDE_CONFIG_DIR — the same bug class #1454 already fixed for _default_memory_dir() in this same file. On a migrated install the check scans a stale ~/.claude/skills/, producing both false positives and false negatives.

Before/after (live run on a migrated host, parent commit vs this head)

Before — scans stale ~/.claude/skills:

⚠ skill-symlinks  warn  1 unlinked: meeting-scheduler; 14 dangling not in this repo: calendar-prep, catchup-after-startup, ...

meeting-scheduler is a FALSE positive — its symlink exists and loads under the workspace claude-home ($CLAUDE_CONFIG_DIR/skills/meeting-scheduler → <repo>/skills/meeting-scheduler, verified). The 14 "dangling" entries are leftovers in a directory Claude Code no longer reads on this host.

After — scans the real claude-home via claude_home_path("skills"):

⚠ skill-symlinks  warn  5 unlinked: gws-gmail-voice, learned-skills, plugin-patches, pointer-teacher-tracer...; 1 dangling not in this repo: skill-usage-report

The false positive is gone, and the check now surfaces skills that are genuinely unlinked in the directory Claude Code actually loads from — findings the stale path was hiding.

Change

• skills_dst = claude_home_path("skills") (helper already imported at line 43; resolution order CLAUDE_CONFIG_DIR → CLAUDE_HOME → ~/.claude preserves ad-hoc-launch behavior).
• Docstring documents the parallel to the feat(workspace-revamp): staging→main rollup (M0+M1+M2) #1454 memory-dir fix.
• tests/health-check-dangling-symlinks.test.py: harness redirects claude_home_path instead of monkeypatching Path.home — all 7 cases pass, assertions unchanged.

Single concern; no behavior change on installs without CLAUDE_CONFIG_DIR set.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F4UsWQjcFqNQmaTHzdEibX

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 65%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 4 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Reviewed current head 5f7f78db56eadbc424b818d2435613ab9fb87cb6.

No blocking findings. check_skill_symlinks() now resolves the active Claude home through the existing claude_home_path("skills") helper, preserving the fallback behavior while fixing migrated/workspace-scoped installs. The focused test harness now redirects that same resolver, so its seven existing dangling/unlinked/fix cases exercise the production path.

Validation: all remote CI checks are green. Locally, git diff --check, the 7-case dangling-symlink suite, Python compilation, scripts/gen-src-map.py --check, and an explicit CLAUDE_CONFIG_DIR skills-path resolution check passed.

Approval signal: looks good to merge. Posted as a comment review because the authenticated account is the PR author.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed — clean, LGTM. Same bug class + fix pattern as the merged #1454 (_default_memory_dir in this same file): Path.home()/'.claude' ignored CLAUDE_CONFIG_DIR, so a migrated install scanned a stale ~/.claude/skills → false warn/ok. claude_home_path('skills') resolves it correctly, and the docstring + not-found detail now reflect the resolved path. CI green, MERGEABLE, live before/after on a migrated host. No blockers from me (shared gh identity → comment-LGTM, not a formal approve; needs a formal reviewer + owner merge).

[qingyun-wu]

Pushed a second commit (same concern — probe accuracy): the check now applies skills/install.sh's own SKILL.md filter, so manifest-loaded / scripts-only skills (no SKILL.md, deliberately never symlinked) stop being reported as 'unlinked'. The earlier 'after' output's 5 'unlinked' entries were exactly this class — verified on-host: all lack SKILL.md and install.sh skips them by design. Live output with both commits is now precisely the real findings: warn — 1 dangling not in this repo: skill-usage-report (a genuinely dead symlink). 8/8 tests pass (new case: SKILL.md-less dir ignored, real skill still warned).

[qingyun-wu]

Blocking finding on current head 82e458a (formal self-request-changes is unavailable): the implementation change now mirrors skills/install.sh by requiring SKILL.md, but the shipped live coverage in tests/health-check-skill-symlinks-live.test.py still creates foo/bar/zap as bare directories. As a result, CI now fails both tsc + tests (clean install) and diff coverage >= 95% (python): the live test expects bar and zap to be reported/fixed as unlinked skills, but check_skill_symlinks correctly skips them because they have no SKILL.md.

Please update the live test fixtures to write SKILL.md for the directories that are meant to behave as installable slash skills (foo/bar/zap), and keep/add a separate no-SKILL.md fixture for the manifest/scripts-only skip behavior. Until that lands, the PR is red and the shipped behavioral coverage is inconsistent with the new contract.

Validation in an isolated /private/tmp checkout:

• git diff --check origin/main...HEAD passed
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260725-1913/pycache python3 -m py_compile src/health-check.py tests/health-check-dangling-symlinks.test.py passed
• python3 tests/health-check-dangling-symlinks.test.py passed (8 tests)
• python3 tests/health-check-skill-symlinks-live.test.py fails locally with the same five failed checks reported by CI

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed current head 25bc73e9b642660afd766452b82b39190e69be38.

No remaining blocking findings. The new commit resolves the prior blocker by giving the live suite's installable foo/bar/zap fixtures SKILL.md, and it also closes the more serious isolation gap by clearing CLAUDE_CONFIG_DIR and CLAUDE_HOME before redirecting HOME; the shipped test can no longer create symlinks in a developer's active Claude home.

Focused validation passed in a detached checkout: git diff --check, Python compilation, the 8-case dangling/unlinked suite, the shipped live symlink suite, and scripts/gen-src-map.py --check. All completed remote checks are green; the refreshed clean-install and diff-coverage jobs are still running.

Approval signal: the code is ready once those two refreshed CI jobs pass. Posted as a comment review because the authenticated account is the PR author.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed current head 25bc73e. No blocking findings. The follow-up fixes the previous CI blocker: the live skill-symlink fixtures now mark installable test skills with SKILL.md, while the no-SKILL.md case remains covered by the structural suite. I also like the added env isolation in the live test — clearing CLAUDE_CONFIG_DIR and CLAUDE_HOME before exercising check_skill_symlinks() keeps the test from writing into a real workspace Claude-home when run on a migrated dev host.

Validation in an isolated /private/tmp checkout:

• git diff --check origin/main...HEAD passed
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260725-1913/pycache-new python3 -m py_compile src/health-check.py tests/health-check-dangling-symlinks.test.py tests/health-check-skill-symlinks-live.test.py passed
• python3 tests/health-check-dangling-symlinks.test.py passed (8 tests)
• CLAUDE_CONFIG_DIR=/private/tmp/pr-monitor-20260725-1913/real-ccd-guard CLAUDE_HOME=/private/tmp/pr-monitor-20260725-1913/real-chome-guard python3 tests/health-check-skill-symlinks-live.test.py passed
• Remote CI is green, including diff coverage and tsc + tests (clean install)

Approval signal: looks good to merge from my review. Posted as a regular review comment because the authenticated account is the PR author.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Approved current head 25bc73e9.

No blocking findings. The production path now uses the existing claude_home_path("skills") resolver, the focused harness redirects the same seam, and the live symlink suite now isolates both CLAUDE_CONFIG_DIR and CLAUDE_HOME, so it cannot mutate the developer’s active Claude home.

Exact-head verification:

• python3 tests/health-check-dangling-symlinks.test.py — 8/8 pass.
• python3 tests/health-check-skill-symlinks-live.test.py — pass.
• Python compile for all touched Python files — pass.
• python3 scripts/gen-src-map.py --check — pass.
• git diff --check origin/main...HEAD — pass.
• Added-line host-path scan found no production host-specific path.
• Current CI and CLA are green.

Reviewed by John’s Codex.

[bassilkhilo-ag2]

Requesting changes — the destination this PR switches to is wrong on at least one real host, and the PR's premise is inverted there.

The change swaps Path.home()/".claude"/"skills" for claude_home_path("skills") (i.e. $CLAUDE_CONFIG_DIR/skills), on the stated grounds that "on a migrated install this check scanned a stale ~/.claude/skills/ and warned 'unlinked' about skills whose symlinks exist … under the workspace claude-home."

On the host I'm on right now, it is exactly the other way around. Evidence:

$CLAUDE_CONFIG_DIR = …/space.ag2.app/workspace/.claude-sutando
$CLAUDE_CONFIG_DIR/skills/                → exists, 50 entries, NO campaign-stats
~/.claude/skills/campaign-stats           → present

And the decisive part: I invoked campaign-stats successfully today — it ran and produced output. So the runtime is loading from ~/.claude/skills, which is the directory this PR would stop checking. After this change, health-check on this host would scan a skills/ dir that lacks the cloud-skill symlinks the agent actually uses, and would report "unlinked" for skills that are linked and loaded. That is the same false-warning failure mode you're fixing, pointed the other way.

Root cause of the disagreement: which of the two trees is authoritative is host-dependent — it depends on how the install was migrated and what the launcher baked into CLAUDE_CONFIG_DIR. Swapping one hardcoded root for another moves the bug rather than removing it.

Suggested shape: treat the destination as a set. Check claude_home_path("skills") and Path.home()/".claude"/"skills", and consider a skill linked if it appears in either; warn only when it is missing from all candidate roots. That is correct on both layouts and needs no per-host configuration. If you'd rather have one answer, resolve the dir the runtime actually loads from rather than inferring it from the config var.

---

What is unambiguously good here, and worth keeping regardless

The SKILL.md filter is a genuine fix on its own:

if not (skill_dir / "SKILL.md").is_file():
    continue

Mirroring skills/install.sh's filter is right — manifest-loaded and scripts-only skills are correctly never symlinked, and warning about them was pure noise. I'd happily approve that half as a standalone PR today.

The test change is also an improvement in principle (patching claude_home_path instead of Path.home is less brittle), though note it currently encodes the same single-root assumption — lambda *sub: self._home.joinpath(".claude", *sub) — so it would pass under either layout and therefore doesn't cover the divergence above. A case with the symlink present in only the other root would have caught this.

What I did not verify

I did not run the health-check test suite. My finding is from the live filesystem state and from having successfully invoked a skill that exists in only one of the two roots.

[sonichi]

Approving per Chi's explicit go (one-by-one). Review-verified LGTM: claude_home_path() skills-dir resolution + SKILL.md install-filter; kills the false skill-unlinked warn.

[bassilkhilo-ag2]

Withdrawing my 20:12Z CHANGES_REQUESTED — I approve this. My blocking rationale was wrong, and I owe a correction on the record.

I claimed "the PR's premise is inverted" on this host. It is not. I independently hit this exact bug tonight, ~9h after that review and before re-reading this PR: health-check reported ⚠ skill-symlinks warn 1 unlinked: meeting-scheduler, and I traced it to skills_dst = Path.home()/".claude"/"skills" ignoring $CLAUDE_CONFIG_DIR, where the symlink is present and loads. That is your before-case, reproduced blind:

$CLAUDE_CONFIG_DIR/skills/meeting-scheduler -> <engine>/skills/meeting-scheduler/   LINKED
~/.claude/skills/meeting-scheduler                                                  absent

So the false positive is real and this PR removes it. My earlier framing was overstated: the destination is not wrong, it is incomplete.

The residual defect, which I do not think blocks merge but should not be lost. Your after-case lists 5 newly-surfaced entries as "findings the stale path was hiding":

⚠ skill-symlinks warn 5 unlinked: gws-gmail-voice, learned-skills, plugin-patches, pointer-teacher-tracer, secret-vault

On this host those 5 are loadable, so flagging them trades one false positive for five. Both directories are live skill roots here — evidence:

• campaign-runner loaded in my current session as projectSettings:campaign-runner and I have been executing ~/.claude/skills/campaign-runner/scripts/smartlead.py from it all evening.
• It exists only at ~/.claude/skills/campaign-runner. There is no <repo>/.claude/skills (verified absent), so projectSettings is resolving to ~/.claude/skills — that directory is not stale on this install.
• All 5 of the newly-flagged skills exist in ~/.claude/skills and not in $CLAUDE_CONFIG_DIR/skills. Exactly the mirror of meeting-scheduler.

Counts on this host: before = 1 false unlinked + 14 false dangling; after = 5 false unlinked + 1 dangling. Net noise drops 15 → 6, which is why this is an improvement worth shipping rather than a regression worth blocking.

Suggested follow-up (not a merge condition): treat a repo skill as linked if it resolves in either root — union claude_home_path("skills") with Path.home()/".claude"/"skills" when the two differ — and report per-root only when it resolves in neither. A single-root check cannot be correct on an install where both roots load; picking the other root just moves which skills get misreported.

Approving on the current head. Verified: the meeting-scheduler false positive is reproduced and removed; the union gap is a live but lesser defect I am happy to see handled separately.

[github-actions]

@cla-assistant check