Conversation
Greptile SummaryThis PR introduces the full skills feature: DB schema, Rust service/route layer, and a rich React UI covering creation, GitHub/filesystem import, slash-command typeahead, and model invocation via a single
Confidence Score: 4/5Mostly safe to merge; one new P1 (name collision in skill cache) should be resolved before shipping to users with multi-owner skill catalogs. Many prior P1 concerns were fixed in this revision. The remaining new P1 (skillsByName collision) is a correctness defect that silently drops a skill from the executor in a multi-owner setup. Pre-existing open items (TOCTOU, authz context, SkillFormModal UTF-8) were carried over from earlier threads. ui/src/pages/chat/utils/skillCache.ts (name collision), src/routes/admin/skills.rs (TOCTOU + authz) Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant CI as ChatInput
participant UC as useChat
participant SE as skillExecutor
participant SC as skillCache
participant API as /api/v1/skills/:id
U->>CI: Types /skill-name (slash command)
CI->>CI: detectSlashQuery → matchSkills
CI-->>U: SlashCommandPopover (user_invocable skills)
U->>CI: Selects skill → inserts /skill-name into message
U->>CI: Sends message
UC->>UC: Build tools array (filter disable_model_invocation)
UC->>UC: buildSkillToolDescription(invocableSkills)
UC->>API: POST /api/chat with Skill tool in tools[]
Note over API: Model decides to call Skill({command})
API-->>UC: SSE: tool_call Skill({command:"skill-name"})
UC->>SE: skillExecutor({command:"skill-name"})
SE->>SC: getSkillByName("skill-name")
SC-->>SE: Skill summary (or undefined)
SE->>SE: Check disable_model_invocation
SE->>SC: getFullSkill(id) → cache hit?
alt Cache miss
SE->>API: GET /api/v1/skills/:id
API-->>SE: Full Skill with file contents
SE->>SC: setFullSkill(skill)
end
SE-->>UC: ToolExecutionResult (SKILL.md + manifest)
UC-->>U: Render artifact (SKILL.md content)
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/pages/chat/utils/skillCache.ts
Line: 31-48
Comment:
**Name collision silently shadows one skill**
`skillsByName` is keyed by `skill.name`, but the server only enforces name uniqueness per owner — a user skill and an org skill can share the same name with different IDs. When `setSkillCatalog` iterates the merged list (user skills first, then org skills from `useUserSkills`), the second skill with the same name overwrites the first. The `Skill` tool executor calls `getSkillByName(command)` and can only ever reach the latter entry; the earlier skill becomes permanently unreachable for that session, regardless of which one the model tries to invoke.
The tool description would also list the name twice (once per enabled skill), giving the model conflicting entries in the `command` enum.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/components/SkillImportModal/parseFrontmatter.ts
Line: 54-62
Comment:
**Inline list parser breaks on tool patterns with commas**
`parseInlineList` splits on every bare comma, so an `allowed_tools` value like `[Bash(git:*, gh:*), Read]` is split into `["Bash(git:*", " gh:*)", "Read"]` — the parenthesized entry is mangled into two broken fragments. Block-list syntax (`- Bash(git:*, gh:*)`) parses correctly; only inline `[…]` lists are affected.
Consider a parenthesis-aware split for this field, or document that inline lists must not contain tool patterns with embedded commas.
How can I resolve this? If you propose a fix, please make it concise.Reviews (7): Last reviewed commit: "Review fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| CREATE INDEX IF NOT EXISTS idx_skill_files_skill ON skill_files(skill_id); | ||
|
|
||
| DO $$ BEGIN | ||
| CREATE TRIGGER update_skill_files_updated_at BEFORE UPDATE ON skill_files FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); | ||
| EXCEPTION WHEN duplicate_object THEN null; | ||
| END $$; |
There was a problem hiding this comment.
The pagination queries in both the SQLite and PostgreSQL implementations sort and filter on (owner_type, owner_id, created_at, id), but the migration only creates idx_skills_owner ON skills(owner_type, owner_id). Without created_at and id in the index, every paginated request must sort the filtered result set in memory. For skill counts beyond a few hundred per owner, this will become a full-table sort per page request.
Consider adding a composite index:
CREATE INDEX IF NOT EXISTS idx_skills_owner_created ON skills(owner_type, owner_id, created_at DESC, id DESC) WHERE deleted_at IS NULL;The same gap exists in the SQLite migration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: migrations_sqlx/postgres/20250101000000_initial.sql
Line: 1268-1273
Comment:
**Missing compound index for cursor-based pagination**
The pagination queries in both the SQLite and PostgreSQL implementations sort and filter on `(owner_type, owner_id, created_at, id)`, but the migration only creates `idx_skills_owner ON skills(owner_type, owner_id)`. Without `created_at` and `id` in the index, every paginated request must sort the filtered result set in memory. For skill counts beyond a few hundred per owner, this will become a full-table sort per page request.
Consider adding a composite index:
```sql
CREATE INDEX IF NOT EXISTS idx_skills_owner_created ON skills(owner_type, owner_id, created_at DESC, id DESC) WHERE deleted_at IS NULL;
```
The same gap exists in the SQLite migration.
How can I resolve this? If you propose a fix, please make it concise.| let max = state.config.limits.resource_limits.max_skills_per_owner; | ||
| if max > 0 { | ||
| let count = services | ||
| .skills | ||
| .count_by_owner(input.owner.owner_type(), input.owner.owner_id(), false) | ||
| .await?; | ||
| if count >= max as i64 { | ||
| return Err(AdminError::Conflict(format!( | ||
| "Owner has reached the maximum number of skills ({max})" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The count check and the subsequent create are two separate DB operations with no transaction or lock between them. Concurrent requests for the same owner can both pass the count >= max check simultaneously and both proceed to create, exceeding the configured max_skills_per_owner limit by up to the number of concurrent requests. If this limit is strictly enforced (e.g., billing/quota), this is a real bypass under load.
Mitigations: enforce the constraint at the database layer (a partial unique index or a CHECK via trigger), or use a serialized lock per-owner.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/admin/skills.rs
Line: 71-82
Comment:
**TOCTOU race in per-owner skill count limit**
The count check and the subsequent `create` are two separate DB operations with no transaction or lock between them. Concurrent requests for the same owner can both pass the `count >= max` check simultaneously and both proceed to create, exceeding the configured `max_skills_per_owner` limit by up to the number of concurrent requests. If this limit is strictly enforced (e.g., billing/quota), this is a real bypass under load.
Mitigations: enforce the constraint at the database layer (a partial unique index or a `CHECK` via trigger), or use a serialized lock per-owner.
How can I resolve this? If you propose a fix, please make it concise.| #[tracing::instrument(name = "admin.skills.get", skip(state, authz), fields(%id))] | ||
| pub async fn get( | ||
| State(state): State<AppState>, | ||
| Extension(authz): Extension<AuthzContext>, | ||
| Path(id): Path<Uuid>, | ||
| ) -> Result<Json<Skill>, AdminError> { | ||
| let services = get_services(&state)?; | ||
|
|
||
| authz.require("skill", "read", None, None, None, None)?; | ||
|
|
||
| let skill = services | ||
| .skills | ||
| .get_by_id(id) | ||
| .await? | ||
| .ok_or_else(|| AdminError::NotFound("Skill not found".to_string()))?; | ||
|
|
||
| Ok(Json(skill)) | ||
| } |
There was a problem hiding this comment.
get, update, and delete don't pass resource context to authz
The list endpoints (list_by_org, list_by_team, list_by_project) each pass the org/team/project ID to authz.require(...) so the auth layer can enforce org-scoped access. The single-resource endpoints—get, update, and delete—all call authz.require("skill", ..., None, None, None, None), meaning the authz check carries no ownership context. If the authz system relies on those context params to scope admin access to a specific org, any admin with any skill permission can read or mutate skills owned by other orgs.
Is the intention for these admin endpoints to be globally scoped, or should they verify the skill's org ownership (similar to how get_by_id_and_org is available in the service layer)? Are the get, update, and delete skill admin endpoints intentionally globally-scoped (any admin can touch any skill), or should they be org-scoped like the list endpoints?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/admin/skills.rs
Line: 124-141
Comment:
**`get`, `update`, and `delete` don't pass resource context to authz**
The list endpoints (`list_by_org`, `list_by_team`, `list_by_project`) each pass the org/team/project ID to `authz.require(...)` so the auth layer can enforce org-scoped access. The single-resource endpoints—`get`, `update`, and `delete`—all call `authz.require("skill", ..., None, None, None, None)`, meaning the authz check carries no ownership context. If the authz system relies on those context params to scope admin access to a specific org, any admin with any skill permission can read or mutate skills owned by other orgs.
Is the intention for these admin endpoints to be globally scoped, or should they verify the skill's org ownership (similar to how `get_by_id_and_org` is available in the service layer)? Are the `get`, `update`, and `delete` skill admin endpoints intentionally globally-scoped (any admin can touch any skill), or should they be org-scoped like the list endpoints?
How can I resolve this? If you propose a fix, please make it concise.
| ...skillListByUserOptions({ | ||
| path: { user_id: user?.id ?? "" }, | ||
| query: { limit: 50 }, | ||
| }), | ||
| staleTime: 5 * 60 * 1000, | ||
| enabled: !!user?.id, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Each source (user skills, per-org skills) fetches at most 50 items. hasMore is computed and exported, but nothing ever calls a second page — so any user or org with more than 50 skills will silently see a truncated list in the Skills picker and the model's Skill tool description. The truncation is invisible to the end-user.
The simplest fix for v1 is to either raise the limit to a safer ceiling or display a notice in the SkillsButton when hasMore is true. A proper infinite-fetch approach would use useInfiniteQuery here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/hooks/useUserSkills.ts
Line: 43-50
Comment:
**50-skill hard cap silently truncates catalog**
Each source (user skills, per-org skills) fetches at most 50 items. `hasMore` is computed and exported, but nothing ever calls a second page — so any user or org with more than 50 skills will silently see a truncated list in the Skills picker and the model's `Skill` tool description. The truncation is invisible to the end-user.
The simplest fix for v1 is to either raise the limit to a safer ceiling or display a notice in the `SkillsButton` when `hasMore` is `true`. A proper infinite-fetch approach would use `useInfiniteQuery` here.
How can I resolve this? If you propose a fix, please make it concise.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
|
||
| const isLoading = createMutation.isPending || updateMutation.isPending || isLoadingFiles; | ||
| const error = createMutation.error || updateMutation.error; | ||
|
|
There was a problem hiding this comment.
bodySize and bundled file sizes use UTF-16 code units, not UTF-8 bytes
form.watch("body").length returns UTF-16 code units; formatBytes(bodySize) on line 376 then displays this as a byte count. The server measures the same content with Rust's String::len() (UTF-8 bytes). For any SKILL.md containing non-ASCII text (multi-byte characters, emoji, CJK), the displayed size will be smaller than what the server will actually count, and the user may submit a skill that the server silently rejects with a 400.
The same applies on line 308 (f.content.length used as byte_size for bundled file rows). Both filesystemImport.ts and githubImport.ts already use new TextEncoder().encode(content).length for this reason — SkillFormModal needs the same fix:
const utf8Encoder = new TextEncoder();
const bodySize = useMemo(
() => utf8Encoder.encode(form.watch("body")).length,
[form]
);And for bundled rows:
bundledFiles.map((f) => ({
path: f.path,
byte_size: utf8Encoder.encode(f.content).length,
}))Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/Admin/SkillFormModal/SkillFormModal.tsx
Line: 248
Comment:
**`bodySize` and bundled file sizes use UTF-16 code units, not UTF-8 bytes**
`form.watch("body").length` returns UTF-16 code units; `formatBytes(bodySize)` on line 376 then displays this as a byte count. The server measures the same content with Rust's `String::len()` (UTF-8 bytes). For any SKILL.md containing non-ASCII text (multi-byte characters, emoji, CJK), the displayed size will be smaller than what the server will actually count, and the user may submit a skill that the server silently rejects with a 400.
The same applies on line 308 (`f.content.length` used as `byte_size` for bundled file rows). Both `filesystemImport.ts` and `githubImport.ts` already use `new TextEncoder().encode(content).length` for this reason — `SkillFormModal` needs the same fix:
```typescript
const utf8Encoder = new TextEncoder();
const bodySize = useMemo(
() => utf8Encoder.encode(form.watch("body")).length,
[form]
);
```
And for bundled rows:
```typescript
bundledFiles.map((f) => ({
path: f.path,
byte_size: utf8Encoder.encode(f.content).length,
}))
```
How can I resolve this? If you propose a fix, please make it concise.
| export function setSkillCatalog(skills: Skill[]): void { | ||
| const seen = new Set<string>(); | ||
| skillsByName.clear(); | ||
| for (const s of skills) { | ||
| skillsByName.set(s.name, s); | ||
| seen.add(s.id); | ||
|
|
||
| const cached = fullSkillsById.get(s.id); | ||
| if (cached && cached.updated_at !== s.updated_at) { | ||
| fullSkillsById.delete(s.id); | ||
| } | ||
| } | ||
|
|
||
| // Drop any cached full skills that have disappeared from the catalog. | ||
| for (const id of fullSkillsById.keys()) { | ||
| if (!seen.has(id)) fullSkillsById.delete(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
skillsByName is keyed by skill.name, but the server only enforces name uniqueness per owner — a user skill and an org skill can share the same name with different IDs. When setSkillCatalog iterates the merged list (user skills first, then org skills from useUserSkills), the second skill with the same name overwrites the first. The Skill tool executor calls getSkillByName(command) and can only ever reach the latter entry; the earlier skill becomes permanently unreachable for that session, regardless of which one the model tries to invoke.
The tool description would also list the name twice (once per enabled skill), giving the model conflicting entries in the command enum.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/pages/chat/utils/skillCache.ts
Line: 31-48
Comment:
**Name collision silently shadows one skill**
`skillsByName` is keyed by `skill.name`, but the server only enforces name uniqueness per owner — a user skill and an org skill can share the same name with different IDs. When `setSkillCatalog` iterates the merged list (user skills first, then org skills from `useUserSkills`), the second skill with the same name overwrites the first. The `Skill` tool executor calls `getSkillByName(command)` and can only ever reach the latter entry; the earlier skill becomes permanently unreachable for that session, regardless of which one the model tries to invoke.
The tool description would also list the name twice (once per enabled skill), giving the model conflicting entries in the `command` enum.
How can I resolve this? If you propose a fix, please make it concise.1 participant
No description provided.