feat(skills): added skills to agent block#3149
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile OverviewGreptile SummaryThis PR introduces Agent Skills as workspace-scoped, reusable instruction packages that can be attached to Agent blocks. It adds a new Main must-fix items are around runtime authorization/containment for Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Skills UI (Settings/Agent block)
participant API as /api/skills
participant DB as DB (skill table)
participant Exec as AgentBlockHandler
participant LLM as Provider LLM
participant Tools as executeTool(load_skill)
UI->>API: GET /api/skills?workspaceId
API->>DB: SELECT skills WHERE workspaceId
DB-->>API: rows
API-->>UI: skills list
UI->>API: POST /api/skills (create/update)
API->>DB: INSERT/UPDATE skill rows
DB-->>API: updated list
API-->>UI: success
UI->>Exec: Run workflow with agent.skills=[{skillId,...}]
Exec->>DB: resolveSkillMetadata(skillIds, workspaceId)
DB-->>Exec: [{name,description}]
Exec->>LLM: system prompt + <available_skills>
Exec->>LLM: tools include load_skill(enum=skillNames)
LLM->>Tools: tool_call load_skill(skill_name)
Tools->>DB: resolveSkillContent(skill_name, workspaceId)
DB-->>Tools: content
Tools-->>LLM: tool response {content}
LLM-->>Exec: final assistant output
|
|
@greptile |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| logger.info(`[${requestId}] Updated skill ${s.id}`) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Skill rename to conflicting name returns raw 500 error
Medium Severity
The update path in upsertSkills issues a continue after updating, which skips the duplicate name check that only runs for new skill inserts. When a user renames a skill to a name that already exists in the workspace, the DB unique constraint (skill_workspace_name_unique) throws a raw PostgreSQL error (code 23505). The API route's catch block checks for "already exists" in the error message, but the DB error message says "duplicate key value violates unique constraint" — so it falls through to a generic 500 response instead of a user-friendly 409.
Additional Locations (1)
There was a problem hiding this comment.
Fixed — added a duplicate name check on the update path (excluding the skill being updated) so renames to conflicting names now return a friendly 409 instead of a raw DB constraint error.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const providerId = getProviderFromModel(model) | ||
| const formattedTools = await this.formatTools(ctx, filteredInputs.tools || []) | ||
|
|
There was a problem hiding this comment.
Unhandled permission error path
validateSkillsAllowed(ctx.userId, ctx) can throw SkillsNotAllowedError, but there’s no local handling here. Unless executeProviderRequest / the generic block runner catches this specific error and converts it to a user-friendly block error, the workflow run will fail with an uncaught exception when a permission group has disableSkills enabled and an agent block has skills configured.
Consider catching SkillsNotAllowedError in the agent handler (or in a central executor layer) and returning a structured block failure (similar to other access-control validations) so runs don’t crash unexpectedly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/executor/handlers/agent/agent-handler.ts
Line: 63:66
Comment:
**Unhandled permission error path**
`validateSkillsAllowed(ctx.userId, ctx)` can throw `SkillsNotAllowedError`, but there’s no local handling here. Unless `executeProviderRequest` / the generic block runner catches this specific error and converts it to a user-friendly block error, the workflow run will fail with an uncaught exception when a permission group has `disableSkills` enabled and an agent block has skills configured.
Consider catching `SkillsNotAllowedError` in the agent handler (or in a central executor layer) and returning a structured block failure (similar to other access-control validations) so runs don’t crash unexpectedly.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
No change needed — this follows the same pattern as validateMcpToolsAllowed and validateCustomToolsAllowed (lines 169/173), which also throw and are caught by the executor's higher-level error handling.
| import type { SkillInput } from '@/executor/handlers/agent/types' | ||
|
|
||
| const logger = createLogger('SkillsResolver') | ||
|
|
||
| function escapeXml(str: string): string { | ||
| return str | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') |
There was a problem hiding this comment.
Invalid tool schema field
buildLoadSkillTool returns both params: {} and parameters: {...}. Elsewhere in the codebase provider tool configs generally use one schema field; if downstream tooling expects params (or expects parameters) this mixed shape will break tool registration for some providers.
Make the returned object match the project’s ProviderToolConfig shape consistently (remove the unused field and ensure the remaining field name is what the executor/provider adapter actually reads).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/executor/handlers/agent/skills-resolver.ts
Line: 5:12
Comment:
**Invalid tool schema field**
`buildLoadSkillTool` returns both `params: {}` and `parameters: {...}`. Elsewhere in the codebase provider tool configs generally use *one* schema field; if downstream tooling expects `params` (or expects `parameters`) this mixed shape will break tool registration for some providers.
Make the returned object match the project’s `ProviderToolConfig` shape consistently (remove the unused field and ensure the remaining field name is what the executor/provider adapter actually reads).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
No change needed — both params and parameters are standard in this codebase. params holds user-provided parameter values (empty for load_skill), parameters holds the JSON schema for the LLM. Same convention at agent-handler.ts:300-301 and providers/utils.ts:520-521.
|
|
||
| const providerId = getProviderFromModel(model) | ||
| const formattedTools = await this.formatTools(ctx, filteredInputs.tools || []) | ||
|
|
There was a problem hiding this comment.
Permission errors bubble up
validateSkillsAllowed(ctx.userId, ctx) throws SkillsNotAllowedError, and this handler doesn’t catch it (similar for validateMcpToolsAllowed / validateCustomToolsAllowed). If the executor doesn’t have a central error-mapper for these access-control errors, a user with disableSkills enabled will get a hard workflow failure rather than a structured “permission denied” block error.
If there isn’t already a global handler, add one (or wrap just the skills validation) so these errors are converted to a consistent block-level error response.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/executor/handlers/agent/agent-handler.ts
Line: 63:66
Comment:
**Permission errors bubble up**
`validateSkillsAllowed(ctx.userId, ctx)` throws `SkillsNotAllowedError`, and this handler doesn’t catch it (similar for `validateMcpToolsAllowed` / `validateCustomToolsAllowed`). If the executor doesn’t have a central error-mapper for these access-control errors, a user with `disableSkills` enabled will get a hard workflow failure rather than a structured “permission denied” block error.
If there isn’t already a global handler, add one (or wrap just the skills validation) so these errors are converted to a consistent block-level error response.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Same as above — identical to how MCP/custom tool permission errors are handled. The executor catches these at a higher level.
Additional Comments (1)
At minimum, gate Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/tools/index.ts
Line: 218:220
Comment:
**Missing authz for skill loads**
`executeTool` handles `load_skill` by trusting `params._context?.workspaceId`, but it never verifies that the running agent/workflow is actually allowed to read skills for that workspace (or that the requested skill is among the agent’s selected skills). Any prompt/tool-call that can set `_context.workspaceId` can fetch arbitrary skill content from that workspace, bypassing the permission-group `disableSkills` enforcement added in the agent handler.
At minimum, gate `load_skill` with the same permission check used for execution (e.g. `validateSkillsAllowed(ctx.userId, ctx)`), and ideally restrict loads to the skill IDs/names attached to the agent block for this run (so the model can’t enumerate/fetch other workspace skills).
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Summary
Adds Agent Skills as a first-class feature — reusable prompt/instruction packages that users can create, manage, and attach to Agent blocks. Follows the agentskills.io open specification.
Architecture
Skills use progressive disclosure to keep context lean:
<available_skills>XML so the LLM knows what's availableload_skilltool lets the LLM load full skill content on-demand when it decides a skill is relevantThis works across all providers (OpenAI, Anthropic, Gemini, etc.) using standard tool-calling — no provider-specific code needed.
What's included
skilltable with workspace-scoped unique name index/api/skillswith workspace permission checksskill-inputsubblock (Combobox dropdown for skill selection)skills-resolver.tsfor metadata/content resolution,load_skillhandler in tools/index.tsdisableSkillswired end-to-end (permission groups API, access control UI, settings modal, executor validation)Type of Change
Testing
Tested manually
Checklist