fix(skills): include plugin skill roots safely

[AaronZ345]

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]

结论: Approve

总体判断: Plugin-provided skill roots are now discoverable for both Codex and Claude Code without regressing depth-1 SkillRegistry semantics, and symlink loops are bounded. Safe to merge.

Review 范围:

• agent/internal/skillroots/{skillroots.go,skillroots_test.go} (NEW)
• agent/claudecode/claudecode.go (SkillDirs refactor + plugin scan)
• agent/codex/codex.go (codexHome/plugins, superpowers/skills, .claude/skills, workDir .claude/skills)
• agent/claudecode/skilldirs_test.go, agent/codex/skilldirs_test.go (3 new tests)
• CHANGELOG.md entry
• 重点关注 correctness、symlink safety、test coverage、regression risk on existing skill discovery.

✅ 做得好的地方:

• Shared skillroots.Find cleanly factored into an internal package; both codex and claudecode consume the same scanner instead of duplicating the walk.
• Loop protection via realDir (EvalSymlinks) + seenDirs is exactly the right primitive for plugin caches that frequently symlink back to a shared content store; the new TestFindDoesNotLoopOnSymlink regression test pins it.
• seenRoots map dedupes identical paths and the deterministic top-down walk order makes the returned slice order stable enough to assert in tests (the TestSkillDirs_* test compares index-by-index).
• "Root is itself named skills" short-circuit in Find is a nice touch — avoids redundant walk when a caller already has the leaf path.
• Negative test: TestSkillDirs_*PluginSkillRoots explicitly asserts that a skills directory nested inside another skill's references/template/ is NOT returned, which is exactly the leak that motivated this change.
• New Codex path codexHome/superpowers/skills and homeDir/.claude/skills cover real cross-agent plugin layouts without changing existing path precedence.
• CHANGELOG entry is concise and points at the new plugin-root behavior, not at internal refactor.
• Session test flakiness fix (codexRuntimeConfigTimeout = 5*time.Second + idempotent-read wait loop) is a good orthogonal cleanup included with the change.

🚨/🔴 必须处理:

• 未发现必须阻塞合并的问题。

🟠 建议改进:

• agent/codex/codex.go codexSkillDirs constructs the slice with make([]string, 0, 4) but then appends up to 6+ entries from skillroots.Find plus a few hard-coded joins; minor: bumping capacity to ~8 would avoid a couple of regrows in plugin-rich homes. Not a blocker.
• agent/claudecode/claudecode.go claudeSkillDirs still always calls walkUpClaudeSkillDirs(workDir, home) even when workDir == "" is unlikely; the function already handles that path fine, but a short-circuit for workDir == "" would mirror the symmetry the function had before. Not a blocker.
• The new test TestSkillDirs_FollowsClaudePluginSymlink symlinks configHome/plugins to home/.claude/plugins; on a developer box where home/.claude/plugins already exists, the symlink will still succeed (replace behavior depends on OS). Consider os.RemoveAll first or use a unique subdir under home to make the test hermetic. Low risk — t.TempDir() is unique per test run, so this is more of a defense-in-depth note.

🔵 可选优化:

• The new CHANGELOG bullet is placed in the Unreleased section but doesn't name an issue or PR; the closing-issues list (refs #624, #1317, #1325) shows up on the PR, so this is fine. If you want symmetry with other entries, you can append (#1415) to the bullet.
• realDir falls back to filepath.Clean(path) when EvalSymlinks fails; on broken symlinks this means the loop guard loses protection because the cleaned path is not the resolved real path. For the current call sites (existing directories), EvalSymlinks should always succeed, so this is not urgent.

❓ 需要确认:

• 无。Plugin root precedence vs. hard-coded codexHome/skills: confirm that the order codexHome/skills → codexHome/superpowers/skills → discovered plugin roots is the intended precedence (it is what the test asserts and what the diff implements, just want a sanity check from the author since plugin vs. first-class ordering can be controversial).

Testing / Risk:

• 已看到的验证: 5/5 CI green (lint, unit-test, smoke-test, regression-test, performance-test). New unit tests cover nested plugin roots, symlink loop protection, depth-1 leaf acceptance, and the negative "no nested skills inside skill assets" case for both codex and claudecode.
• 未覆盖风险: Windows symlink path is t.Skip-guarded which is correct given admin requirements; manual smoke test on a real Codex/Claude install with a populated plugin cache would be ideal before release but is not required for this PR.

Next step:

• Merge. No blockers; remaining nits can be follow-up PRs if desired.

[chenhg5]

结论: Approve (re-approve after rebase)

总体判断: 之前在 commit 1bb4312 (2026-06-30) 已 approve。当前 head 46f0b86 与之前 commit 1bb4312 不在同一条 lineage (commit chain 只有 1 commit "fix(skills): include plugin skill roots safely" by AaronZ345),属于 force-push / rebase 后只保留最终实现版。按 rebase-vs-approve memory,旧 APPROVE 在 commit hash 变化后失效,需要基于新 head 重新 approve。本次按当前 head diff 重新评审后通过,无新发现 blocker。

Review 范围:

• agent/internal/skillroots/skillroots.go (新增 68 行): 共享的 plugin skill 根扫描器。
• agent/claudecode/claudecode.go (+86/-45): SkillDirs() 接入 plugin roots + 修复注释列对齐噪音。
• agent/codex/codex.go (+17): codexSkillDirs 接入 plugin roots + superpowers + .claude/skills。
• agent/codex/session_test.go (+11): 引入 codexRuntimeConfigTimeout 5s 与 waitForArgsFile 20ms 稳定化(测试可靠性 patch)。
• 4 个 skilldirs_test.go 新增 + 1 个 skillroots_test.go 新增 + 既有 TestSkillDirs_UsesProjectAgentAndCodexHomes / TestSkillDirs_FallsBackToHomeClaudeDir / TestSkillDirs_FallsBackToEnvCodexHome 更新断言。
• CHANGELOG.md (v1.3.3 那行追加一行)。

✅ 做得好的地方:

• 把 plugin 根扫描抽到 agent/internal/skillroots 共享包,claudecode + codex 两端用同一份 Find(),避免重复逻辑 — 跟 agent/internal 已经存在的 internal 包风格一致。
• skillroots.Find 用 filepath.EvalSymlinks 解析真实路径 + seenDirs / seenRoots 双层去重(防 symlink 环 + 防重复添加),且 if filepath.Base(root) == "skills" 短路命中 — 既有 root 直接是 skills/ 时也走 dedup,细节完整。
• 测试覆盖很扎实: Claude plugin root / Claude plugin symlink / Codex plugin root / Codex plugin symlink / 嵌套 reference-template 不被错误展开(must not include nested skills directories inside plugin skill roots 显式断言)、TestSkillDirs_FollowsClaudePluginSymlink 跨 home+CLAUDE_CONFIG_DIR 双向链接场景。Linux/macOS 用 t.Skip("symlink creation requires administrator on Windows") 跨平台跳过。
• codex 端的 superpowers + .claude/skills 兼容 root(homeDir != "" 时追加)跟既有 Codex/Claude 双 agent 兼容的实际部署期望对齐。
• PR body 明确 supersedes #624(原本 PR 路径 stale/dirty),避免 main 重复引入 skill discovery 入口。

🟠 建议改进:

• 🟠 P2 CHANGELOG 行位置建议稍微下沉到 ## Unreleased / ### Added 而不是直接贴在 ## Unreleased 的 free-form list 里(目前 CHANGELOG.md line 23 是在 ## Unreleased 段落的中间,混在 fixed-styled 列表中,跟其他 PR 风格不完全一致)。不阻塞,后续 PR 顺手整理。
• 🟠 P2 agent/codex/session_test.go 引入了 codexRuntimeConfigTimeout 字段,read 一下确认这个字段是不是这个 PR 新加的 export,如果是新加的(没看到 diff 里有 var codexRuntimeConfigTimeout = ... 声明),需要补一行 default 声明或 confirm 已经存在于既有 codex 包内。
• 🟠 P2 没看到 plugins 路径下 **/skills 与既有 walkUpCodexProjectSkillDirs(workDir 向上找 .codex/skills / .agents/skills / .claude/skills)的优先级明确说明。建议在 docs 或 SkillDirs comment 里写清:walk-up project root 优先,plugins roots 其次,user-level 最后,避免使用者疑惑。

❓ 需要确认:

• 是否考虑过 plugin roots 数量爆炸(用户装 N 个 plugins)对 SkillRegistry 启动期 scan 性能的影响?skillroots.Find 递归 walk 整棵 plugins 树,虽然 seenDirs 防环,但用户装 50 个 plugin 时第一次冷启动可能多花几秒。可接受但值得在 docs 提示。

Testing / Risk:

• 已看到的验证: 本地 unit tests (agent/internal/skillroots/skillroots_test.go + 两侧 skilldirs_test.go 新增 4 case),CI 5/5 green。
• 仍可补: 真机 (Codex + Claude Code) 安装实际 plugin 后,/list skills 验证 plugin 提供 skill 真正出现在列表;不在本 PR 阻塞,可以让 reporter 在 PR description 的 "Testing" 段补一句。
• 风险面: SkillDirs 是冷启动期扫描,错误的"展开嵌套"会把 plugin 包内的 references/template/skills 也注册成真 skill 路径 — 测试已覆盖(must not include nested skills directories),所以代码 + 测试匹配良好。

Next step: 作者无需返工,owner 可以直接 merge。本 PR 当前可合并,风险可接受。

[AaronZ345]

Hi @chenhg5, I’ve rebased this PR onto the latest main and all five CI checks are green. Could you please take another look and merge it when you have a chance? Thanks!

[AaronZ345]

Replacement PR: #1573