fix(skills): include plugin skill roots safely

[AaronZ345]

Replaces #1415 with the same approved patch on a fresh branch.

Summary

• add a small shared scanner that discovers plugin-provided skills root directories without registering nested SKILL.md files directly
• include Codex plugin skill roots (CODEX_HOME/plugins/**/skills) plus Codex superpowers and .claude/skills compatibility roots
• include Claude Code plugin skill roots from CLAUDE_CONFIG_DIR/plugins/**/skills and configured plugin_dir trees

This supersedes the approach in #624. That PR is now stale/dirty after #1317, and it reintroduced recursive core skill registration. This keeps #1317's depth-1 registry behavior intact, so nested reference/template SKILL.md files still stay hidden while plugin skill bundles become discoverable.

It also complements #1325: plugin_dir still loads Claude plugins at runtime, and this PR makes plugin-provided skill roots visible to cc-connect's static skill/command registry.

Tests

• git diff --check
• go test ./agent/internal/skillroots ./agent/codex ./agent/claudecode ./core
• go test -tags no_web ./cmd/cc-connect
• GOOS=linux go test -tags no_web -c -o /private/tmp/cc-connect-plugin-skill-roots-cmd.test ./cmd/cc-connect

[chenhg5]

Conclusion: Comment

Overall assessment:

• The implementation is fundamentally sound — agent/internal/skillroots/skillroots.go provides a correct, symlink-loop-safe scanner, and the Claude Code / Codex SkillDirs() extensions preserve the depth-1 contract enforced by core.SkillRegistry after #1317. No blocker found; leaving this as Comment to confirm a couple of design decisions (.claude/skills cross-discovery in Codex, superpowers/skills directory choice) and to surface one minor dedup edge case worth a 5-second author comment rather than a code change.

Review scope:

• Read agent/internal/skillroots/skillroots.go (+68), agent/claudecode/claudecode.go (+46/-40, SkillDirs path), agent/codex/codex.go (+12/-5, codexSkillDirs + walkUpCodexProjectSkillDirs), and the matching test files (skillroots_test.go +66, claudecode/skilldirs_test.go +80, codex/skilldirs_test.go +80, codex/session_test.go +11).
• Focused on correctness of the recursive Find, symlink/loop safety, integration with the existing depth-1 discoverSkillsInDir contract (#1317), and boundary handling for empty pluginDirs, empty codexHome, and the CLAUDE_CONFIG_DIR="" case.
• Ran locally: go test -race ./agent/internal/skillroots ./agent/codex ./agent/claudecode -count=1 -timeout 90s → ALL PASS (3/3 packages). go test ./agent/internal/skillroots ./agent/codex ./agent/claudecode ./core → ALL PASS. go vet ./agent/... clean. go test -tags no_web ./cmd/cc-connect → PASS.
• CI per gh pr checks: lint, unit-test, smoke-test, regression-test, performance-test all SUCCESS.

✅ What looks good:

• Symlink-loop safety by realpath tracking: walk keys both seenDirs and seenRoots by the EvalSymlinks-resolved path (realDir), not the lexical path. This is the same defense core.SkillRegistry.ListAll uses (TestSkillRegistryListAll_DoesNotLoopOnDirectorySymlinks), and the new TestFindDoesNotLoopOnSymlink pins the contract locally. A symlinked plugin/loop pointing back at the walk root cannot produce infinite recursion.
• Honoring the depth-1 boundary at the right layer: Find only returns <root>/<something>/skills directories; it does not recurse into the discovered skills/ directory or walk into <skill>/references/template/skills/. Combined with the existing depth-1 discoverSkillsInDir (which only reads <root>/<skill>/SKILL.md), the phantom-slash-command regression fixed by #1317 cannot reappear. The notion/references/template/skills negative-assertion in both TestSkillDirs_IncludesClaudePluginSkillRoots and TestSkillDirs_IncludesCodexPluginSkillRoots proves this end-to-end.
• Layering is clean: the scanner lives in agent/internal/skillroots, not in core, which is correct — it is only consumed by the two agent adapters that own a plugin ecosystem. core.SkillRegistry stays generic and unchanged.
• Claude Code plugin_dir coverage: claudeSkillDirs now iterates a.pluginDirs (the resolved list from opts["plugin_dir"] at claudecode.go:155-164) and feeds each entry through skillroots.Find. This composes correctly with #1325's runtime --plugin-dir loader as the PR description claims: --plugin-dir is for runtime, this PR is for static discovery — both finally agree on the same plugin roots.
• a.pluginDirs snapshot under mutex: SkillDirs() copies a.pluginDirs via append([]string(nil), a.pluginDirs...) while holding a.mu.RLock, matching the pattern already used for workDir and codexHome. No data race introduced against setPluginDirs-style mutators (none exist today, but the existing TestSkillDirs_RaceFreeAgainstSetWorkDir pattern is the right model).
• waitForArgsFile settle window: the new text != last debounce in agent/codex/session_test.go (+6 lines) is a real test-only fix for the partial-write flake. It sleeps 20ms after the first non-empty read and only returns on the second consecutive identical read — this matches the actual partial-write race (writer hasn't flushed yet) and doesn't slow the happy path beyond one extra 20ms tick. Not strictly part of the headline feature, but nice to see bundled in.
• codexRuntimeConfigTimeout extension to 5s in TestGetModelAndReasoningEffort_FromRuntimeConfigWhenUnset — the old 1500ms default is too tight for the spawned codex shim under parallel CI load, so the test would intermittently fail with "context deadline exceeded". The override + t.Cleanup restoration is the standard pattern; safe to ship.

🟠 Should improve (P2, non-blocking):

• .claude/skills is silently added to Codex project and home scanning (codex.go:620, codex.go:639). This is a deliberate cross-discovery — a user who writes a ~/.claude/skills skill now sees it surface under Codex too. Two questions:
  1. Is this intentional cross-runtime sharing? Worth a one-line comment in codexSkillDirs explaining "Codex can consume Claude-format skill directories for parity with Claude Code skill authors". The current code reads as accidental.
  2. Should the reverse also be true (Codex .codex/skills discoverable by Claude)? Today the answer is no (claudecode only reads <configHome>/skills and plugin_dir roots). If the project wants symmetric skill sharing, both adapters should agree on the same set; otherwise users will be confused about why a skill works in one runtime and not the other. Worth deciding.
• codexHome/superpowers/skills directory (codex.go:613). superpowers is a third-party skill orchestration tool (not Codex, not Claude), and this path will silently fail to register anything for the 99% of users without it. Risk is low (the directory just doesn't exist, uniqueCodexSkillDirs skips empty results, SkillRegistry.SetDirs accepts non-existent dirs without error), but two improvements would be worth it:
  1. A brief comment at codex.go:613 noting this is the superpowers skill layout, so future readers understand why a hardcoded superpowers/skills is there.
  2. A test that asserts SkillDirs() does not include a <random>/superpowers/skills for users who happen to have a superpowers directory in their Codex home that is not the official layout. The existing TestSkillDirs_IncludesCodexPluginSkillRoots doesn't cover this.
• Find() accepts the root if and only if its base name is skills, but does not walk into siblings of that root (skillroots.go:13). That's fine for the call sites, but if a future caller passes ~/.codex/plugins/<plugin>/skills directly (i.e. a plugin root that is a skills directory), the early-return at line 13 returns [root] without dedup-checking against the caller's seenDirs. Today there are no such callers, so this is theoretical — but worth a one-line uniqueSkillDirs-style pass at the end of Find to keep the contract tight.
• TestFindAcceptsRootNamedSkills only exercises the early-return branch (skillroots_test.go:37). The recursive walk is exercised by TestFindDiscoversNestedSkillRoots and TestFindDoesNotLoopOnSymlink, but no test asserts the recursive walk honors the depth-1 contract on a 3-deep nested chain (e.g. root/a/b/c/d/e/skills → only e/skills returned, never c/skills or d/skills). Adding one would make the contract self-documenting.

🔵 Optional (P3, do not block):

• Find() does not log or surface a warning when the root itself is missing (os.Stat → error in isDir → empty result). Callers silently treat an unset CODEX_HOME and a missing ~/.claude/plugins identically. This matches today's behavior and is fine, but a single slog.Debug in Find when root is empty/missing would help users diagnose "why aren't my plugin skills showing up" without code spelunking. (Moot if you consider this a "config issue, not a runtime issue" — reasonable either way.)
• CHANGELOG.md does not reference the related PRs (#1317 depth-1, #1325 plugin_dir runtime). A "follows #1317 depth-1 contract; complements #1325 plugin_dir loader" parenthetical would help future archeology, but is not required.
• agent/codex/codex.go:613-615 could collapse the two appends into one (userDirs = append(userDirs, filepath.Join(codexHome, "skills"), filepath.Join(codexHome, "superpowers", "skills"))) — purely cosmetic; the current shape documents the plugin-vs-static split. Leave as-is.

❓ Questions:

• For AaronZ345: the .claude/skills discovery under Codex (codex.go:620, codex.go:639) — is this intentional cross-runtime skill sharing, or a deliberate fallback because Codex's own skill format is still evolving? A short comment in the source would answer this for the next reader without a round-trip.
• For maintainer: PR description says "Replaces #1415 with the same approved patch on a fresh branch." Was #1415 reviewed and approved? If so, the QA pass should match the prior verdict. If not, this is effectively a fresh review and the questions above apply.

Testing / Risk:

• Verified evidence: 3 new skillroots unit tests + 2 new claudecode SkillDirs tests + 2 new codex SkillDirs tests all pass under -race. TestSkillDirs_RaceFreeAgainstSetWorkDir (pre-existing in codex) still passes — the new codexSkillDirs body does not introduce new racy reads of a.workDir or a.codexHome. go vet ./agent/... clean. CI 5/5 SUCCESS.
• Remaining risk: the new .claude/skills and superpowers/skills entries will silently scan directories that may not exist on most users' systems — core.SkillRegistry tolerates missing roots (no error), but if any future code starts asserting "every configured skill dir must exist at startup", this PR would need a rebase. Worth a sanity grep before merge: grep -rn "os.Stat.*skill" core/ to confirm SkillRegistry.SetDirs is still tolerant of empty/missing dirs.
• The unrelated agent/acp/TestCursorCLI_ACPHandshake failure I observed locally is a pre-existing integration-test credential issue (acp: authenticate (cursor_login): json-rpc -32602: Invalid params), not caused by this PR. Confirmed by checking out origin/main and re-running the same test — fails identically.

Next step:

• Author: drop a 1-2 line comment in codex.go near the .claude/skills and superpowers/skills entries explaining the intent. Optionally add the 3-deep TestFindAcceptsNestedChain test. After that, this is mergeable.
• Maintainer: once the comment question is resolved, ready to merge.