feat: show per-agent workspace skill roots in Skills Hub#413
feat: show per-agent workspace skill roots in Skills Hub#413Brixyy wants to merge 2 commits intobuilderz-labs:mainfrom
Conversation
0xNyk
left a comment
There was a problem hiding this comment.
Well-structured feature PR. Reviewed all three files:
route.ts — Dynamic workspace-* directory discovery via readdirSync on ~/.openclaw/. Scoped scan, no recursion, wrapped in try/catch. Clean.
skill-sync.ts — Adds the same dynamic discovery + replaces hardcoded localSources with getSkillRoots().map(r => r.source). This is smarter than #411's approach and forward-compatible — nice improvement.
skills-panel.tsx — Clean separation of global vs agent-workspace groups, avatar cards with deterministic coloring, proper source badges for workspace-* sources.
One minor note: readdirSync in getSkillRoots() runs synchronously on the API route handler. For the current use case (scanning one directory for workspace-* entries) this is fine — but worth noting if the number of workspace directories ever grows significantly.
LGTM — merging.
|
Approved and ready to merge, but there's a merge conflict now that #411 and #409 have been merged into The conflict is in git fetch upstream
git rebase upstream/main
# resolve conflict in skill-sync.ts by keeping your version
git push --force-with-leaseWill merge as soon as the branch is updated. |
0xNyk
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Summary
Good feature concept — per-agent workspace skill roots with deterministic avatars. However, there are security concerns and code duplication with PR #415 that need addressing.
Issues Found
-
Duplicate
getSkillRoots()— Same implementation as in PR #415. Since both PRs are yours, please coordinate: extract the shared function tosrc/lib/skill-roots.tsand import from there in both PRs (or merge them). -
No agent name validation — Directory names from
workspace-*are used directly without sanitization. Add validation:if (!/^[a-z0-9_-]+$/i.test(agentName)) continue;
-
No symlink detection — A
workspace-evilsymlink pointing to/etc/would be trusted. Uselstat()to verify directories are real, not symlinks:const stat = await fs.lstat(fullPath); if (!stat.isDirectory() || stat.isSymbolicLink()) continue;
-
Silent error handling — All catches swallow errors without logging. At minimum, add
console.warn()so operators can diagnose issues in production. -
No tests — The filesystem scanning logic is testable and should have unit tests covering: normal discovery, invalid names, symlinks, empty directories, and env var overrides.
Requested Changes
- Rebase on main
- Validate agent names with regex allowlist
- Use
lstat()to reject symlinks - Add warn-level logging in catch blocks
- Consolidate
getSkillRoots()with PR #415 - Add unit tests for discovery logic
|
Addressed all review points:
Branch rebased on main (fast-forward, no conflicts). |
…bs#412) - Dynamically scan ~/.openclaw/workspace-<name>/skills/ dirs at runtime - Add "Agent Workspaces" section with avatar cards (deterministic colors) - Add "Global" section header above shared source cards - Move main agent workspace card into Agent Workspaces section - Update source badge in skill list for workspace-* sources - localSources in skill-sync derived from getSkillRoots() — stays in sync automatically
…y hardening - Create src/lib/skill-roots.ts: single source of truth for skill root discovery, used by both route.ts and skill-sync.ts - Validate workspace-* agent names with ^[a-z0-9_-]+$i to prevent path injection via crafted directory names - Use lstatSync() to detect and reject symlinked workspace directories - Log console.warn when openclawState scan fails so operators can diagnose - Add 18 unit tests covering discovery, validation, symlinks, env overrides
a70dd51 to
7f4a73e
Compare
|
CI failure is pre-existing, not caused by this PR.
Main branch has the same failure: runs
|
|
Hey @Brixyy — friendly ping on the requested changes from yesterday's review. Quick summary:
Let me know if you have questions — happy to pair on this. |
|
Thanks for the workspace skill roots work! We've implemented this in #426 which supersedes this PR with the same core feature: dynamic workspace-* directory scanning in both skill-sync.ts and the API route. |
|
Superseded by #426 |
Summary
src/app/api/skills/route.ts,src/lib/skill-sync.ts):getSkillRoots()now scans~/.openclaw/at runtime and appends a root for everyworkspace-<name>directory found. No configuration required — new agent workspaces appear automatically. Env overrideMC_SKILLS_WORKSPACE_<NAME>_DIRis supported for custom paths.localSourcesderived fromgetSkillRoots()(src/lib/skill-sync.ts): removed the hardcoded array — sync engine stays in sync with discovered roots automatically.src/components/panels/skills-panel.tsx): compact card grid (2–4 per row) below the global sources. Each card shows the agent name, a deterministic avatar color, and skill count. Cards act as root filters just like the global cards.workspaceroot is treated as belonging to themainagent and displayed inside the Agent Workspaces section with a teal avatar.<agent> workspace(violet) for workspace-* sources instead of the raw source string.Closes #412
Test plan
pnpm build— production build succeedspnpm typecheck— no errorspnpm lint— 0 new errorsworkspace-*dirs present (section hidden)workspace-*dirs with skills existworkspace-*directory added at runtime appears after next sync/refresh🤖 Generated with Claude Code