feat(web): add cross-harness skill manager - #4630
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:trusted
github-actions
Bot
added
the
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces a significant new feature: a cross-harness skill manager with new server endpoints, contracts, UI components, and client-side logic. New user-facing capabilities of this scope warrant human review regardless of code quality. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review of the new skills service, HTTP layer, contracts, and client-runtime helper. One finding: an aliased service-module import in the new server test. Everything else (namespaced effect/* subpath imports, environment-based dependency acquisition via yield* ServerSettingsService, runPromise confined to the React boundary, schema-only contracts) matches the conventions.
Posted via Macroscope — Effect Service Conventions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/architecture/skill-manager-plan.html (1)
817-826: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the touchpoint list with the paths shipped in this PR.
The list names
packages/contracts/src/environment.tsandapps/server/src/http.ts. This PR addspackages/contracts/src/skillInventory.ts,apps/server/src/skills/http.ts, and wires the route inapps/server/src/server.ts. Update the list so the plan matches the delivered Phase 1 code.📝 Proposed doc update
- <code>packages/contracts/src/environment.ts</code> · - <code>packages/contracts/src/environmentHttp.ts</code> · new inventory schemas · + <code>packages/contracts/src/skillInventory.ts</code> · + <code>packages/contracts/src/environmentHttp.ts</code> · <code>apps/server/src/provider/Drivers/ClaudeSkills.ts</code> · <code>apps/server/src/provider/Layers/CodexProvider.ts</code> · new <code>apps/server/src/skills/</code> service/adapters · - <code>apps/server/src/http.ts</code> · new client-runtime query helper · + <code>apps/server/src/skills/http.ts</code> · + <code>apps/server/src/server.ts</code> · + <code>packages/client-runtime/src/state/skillInventoryHttp.ts</code> · <code>apps/web/src/components/settings/SettingsSidebarNav.tsx</code> · new <code>apps/web/src/routes/settings.skills.tsx</code> and settings components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/skill-manager-plan.html` around lines 817 - 826, Update the touchpoint list in the architecture plan to replace environment.ts and server http.ts references with the shipped skillInventory.ts and skills/http.ts paths, and add the route wiring file server.ts. Preserve the other listed Phase 1 touchpoints.apps/server/src/skills/SkillInventory.ts (1)
25-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated skill-installation mapping and mark the fallback providers.
The "codex" and "claudeAgent" branches both read each skill file and map it to
SkillInventoryInstallation; moving this through a shared map helper prevents future drift. The sharedreturn [];fallback also leaves Cursor and Grok unexplained — add a comment or explicit branches for them to satisfy the provider adapter decision guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/skills/SkillInventory.ts` around lines 25 - 83, Extract the duplicated file-reading and SkillInventoryInstallation mapping from discoverInstanceSkills into a shared helper, reusing the branch-specific provider ID, harness display name, skill metadata, and enabled state for both codex and claudeAgent. Replace both Effect.forEach mapping blocks with that helper while preserving their existing skill discovery inputs. Document the fallback return [] by adding explicit Cursor and Grok branches or a comment identifying those unsupported providers.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/skills/SkillInventory.test.ts`:
- Line 14: Replace the aliased layerTest import in SkillInventory.test.ts with a
namespace import for serverSettings, then update both call sites to invoke
ServerSettings.layerTest(...), preserving the existing arguments and test
behavior.
In `@apps/web/src/components/settings/SkillsSettings.logic.ts`:
- Around line 1-102: Move the framework-agnostic helpers formatSkillPath,
filterSkillInventory, displayedSkillCount, setKeyCollapsed, skillKey,
groupSkillsByHarness, and skillContentForDisplay, along with the
SkillHarnessGroup type and their supporting constants/helpers, into
packages/client-runtime beside the skills HTTP client. Export them from the
package and update the web settings logic to import and reuse those shared
symbols, preserving all existing behavior.
---
Nitpick comments:
In `@apps/server/src/skills/SkillInventory.ts`:
- Around line 25-83: Extract the duplicated file-reading and
SkillInventoryInstallation mapping from discoverInstanceSkills into a shared
helper, reusing the branch-specific provider ID, harness display name, skill
metadata, and enabled state for both codex and claudeAgent. Replace both
Effect.forEach mapping blocks with that helper while preserving their existing
skill discovery inputs. Document the fallback return [] by adding explicit
Cursor and Grok branches or a comment identifying those unsupported providers.
In `@docs/architecture/skill-manager-plan.html`:
- Around line 817-826: Update the touchpoint list in the architecture plan to
replace environment.ts and server http.ts references with the shipped
skillInventory.ts and skills/http.ts paths, and add the route wiring file
server.ts. Preserve the other listed Phase 1 touchpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 106ee80a-4e78-4d75-912a-1f334194d3b0
📒 Files selected for processing (16)
apps/server/src/server.tsapps/server/src/skills/SkillInventory.test.tsapps/server/src/skills/SkillInventory.tsapps/server/src/skills/http.tsapps/web/src/components/settings/SettingsSidebarNav.tsxapps/web/src/components/settings/SkillsSettings.logic.tsapps/web/src/components/settings/SkillsSettings.test.tsapps/web/src/components/settings/SkillsSettings.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/settings.skills.tsxdocs/architecture/skill-manager-plan.htmlpackages/client-runtime/package.jsonpackages/client-runtime/src/state/skillInventoryHttp.tspackages/contracts/src/environmentHttp.tspackages/contracts/src/index.tspackages/contracts/src/skillInventory.ts
befd2fe to
a63d4e1
Compare
t3dotgg
changed the title
88eaab3 to
86bd726
Compare
| const fileSystem = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| const installations = yield* Effect.forEach(input.skills, (skill) => | ||
| fileSystem.readFileString(skill.path).pipe( |
There was a problem hiding this comment.
🔴 Critical skills/SkillInventory.ts:42
readSkillInstallations reads skill.path via fileSystem.readFileString without verifying the path is not a symlink that resolves outside the discovered skill directory. A SKILL.md symlink such as skills/leak/SKILL.md -> /etc/passwd passes discovery, and line 42 returns the target file's full contents to any client of the remote skills API, leaking arbitrary host files. Consider rejecting symlinks or verifying the resolved real path is contained within the expected skill root before reading.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/skills/SkillInventory.ts around line 42:
`readSkillInstallations` reads `skill.path` via `fileSystem.readFileString` without verifying the path is not a symlink that resolves outside the discovered skill directory. A `SKILL.md` symlink such as `skills/leak/SKILL.md -> /etc/passwd` passes discovery, and line 42 returns the target file's full contents to any client of the remote skills API, leaking arbitrary host files. Consider rejecting symlinks or verifying the resolved real path is contained within the expected skill root before reading.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 86bd726. Configure here.
| ) : visibleInventory?.installations.length === 0 ? ( | ||
| <StatusLine> | ||
| {query ? "No skills match this search." : "No global Codex or Claude skills found."} | ||
| </StatusLine> |
There was a problem hiding this comment.
Wrong empty-state for blank search
Low Severity
The empty inventory message branches on raw query, while filtering and the rest of this panel treat only a trimmed query as an active search via isSearching. Whitespace-only input still counts as a search here, so an actually empty inventory can show the “no matches” copy instead of the “no skills found” copy.
Reviewed by Cursor Bugbot for commit 86bd726. Configure here.
1 participant
Summary
SKILL.mdcontentWhy
Skills currently live in harness-specific directories on individual computers, which makes them difficult to discover or inspect. This introduces a unified read-only view so users can understand what is installed and where before adding mutation operations such as copy, install, or removal.
Screenshot
Validation
vp test run apps/server/src/skills/SkillInventory.test.ts apps/web/src/components/settings/SkillsSettings.test.tsvp run --filter @t3tools/web typecheckNote
Low Risk
Read-only filesystem scans behind existing environment auth; no skill mutation. Main exposure is returning full SKILL.md paths and contents to authorized clients.
Overview
Adds a read-only Skills settings page at
/settings/skillsso users can see global Codex and Claude skill installations on each connected computer, with search, refresh, collapsible grouping by environment and harness, copy-path, and inlineSKILL.mdviewing.Server: New
GET /api/skills(orchestration read scope) runsdiscoverGlobalSkillInventory, scanning configured provider instances via existing Claude/Codex discovery, readingSKILL.mdcontent and skipping unreadable files. Contracts addSkillInventorytypes; client-runtime adds@t3tools/client-runtime/state/skillsfor fetch, filter, and display helpers.Also wires sidebar/search routing and includes an architecture plan doc for future harness coverage and safe removal.
Reviewed by Cursor Bugbot for commit 86bd726. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add cross-harness skill manager settings page with server-side inventory API
/settings/skillspage in the web app that lists skills across all connected environments, with search, refresh, and collapsible per-harness groupsGET /api/skillsendpoint on the server that scans global skills for all configured provider instances (Codex and Claude drivers), returning a typedSkillInventorypayloadSkillInventoryandSkillInventoryInstallationschemas to the contracts package, and exposes a new@t3tools/client-runtime/state/skillssubpath with filtering, grouping, and path-formatting helpersSKILL.mdfiles from provider home directories; unreadable files are silently skipped rather than failing the scanMacroscope summarized 86bd726.
Summary by CodeRabbit
New Features
Documentation
Tests
Built with gpt-5.6-sol through the Codex harness.