From 5ceabad5f3e009cbe085c8e246eccdb41a6d97c2 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 13 Jun 2026 11:16:14 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A2=20fix:=20Prune=20Dangling=20Skill?= =?UTF-8?q?=20IDs=20from=20Agent=20Allowlists=20(#13702)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿงน fix: Prune Dangling Skill IDs from Agent Allowlists Deleted skills left their ids behind in every agent's `skills` allowlist: nothing removed them on skill deletion, the builder rendered no chip for unresolvable ids (so users could neither see nor remove them), and at runtime the non-empty allowlist intersected with accessible skills to an empty set โ€” silently disabling the entire skills catalog for the agent even though the panel looked like "no skills selected." - deleteSkill / deleteUserSkills now $pull deleted ids from all agent allowlists (no versioning, timestamps untouched) - createAgent / updateAgent prune allowlist ids whose skill doc no longer exists (existence-only check, never ACL), so poisoned agents self-heal on the next save โ€” including duplicates and sync paths - the builder renders unresolvable allowlist entries as removable "Unavailable skill" chips once the catalog query resolves * ๐Ÿชž fix: Keep Skill Queries and Authoring Labels Truthful After Chat Edits Skills authored mid-chat via create_file/edit_file never reached the Skills panel or builder without a manual refresh, and a create_file that overwrote an existing file still announced "Created" in the tool card. - invalidate all skill query caches (refetchType: 'all', since the skill hooks opt out of refetchOnMount) when a completed create_file/edit_file call targets a skills/ path - label create_file completions from the host-authored output summary: overwrites now read "Updated " with the edit icon * โ™ป๏ธ refactor: Inject Skill Authoring Callback Instead of Query Client useStepHandler took useQueryClient directly, forcing a QueryClientProvider wrapper onto all 54 renderHook calls in its spec. Its only consumer, useEventHandlers, already holds the query client and does this exact invalidation pattern for project/MCP keys โ€” so pass an optional onSkillAuthoringComplete callback instead. Detection stays in the completion handler; the side effect lives with the client. Spec diff collapses to pure additions. * ๐Ÿฉน fix: Resolve Codex Review Findings on Allowlist Pruning - normalize allowlist candidates to lowercase in filterExistingSkillIds: isValidObjectIdString accepts uppercase hex, but _id.toString() is lowercase, so a casing mismatch silently emptied a valid allowlist (widening scope to the full catalog) - prune agent allowlists immediately after the Skill row deletion in deleteSkill: a SkillFile cleanup failure previously skipped the prune forever, since retries exit early on deletedCount === 0 - filter version-snapshot skills through filterExistingSkillIds in revertAgentVersion so reverting to a pre-delete version cannot resurrect dangling ids - resolve allowlist ids missing from the builder's first catalog page individually via getSkill before labeling them unavailable โ€” a cache miss on a >100-skill catalog no longer invites removing a valid skill * ๐Ÿšช fix: Fail Closed When Pruning Empties a Skill Allowlist Codex round 2: an automated prune that empties an enabled allowlist would silently widen the agent to the full accessible catalog (empty + enabled = full per the #13526 semantics). Hygiene must only ever narrow. - deleteSkill/deleteUserSkills: agents whose entire allowlist is being deleted get skills disabled instead of an emptied-but-enabled list; ids are lowercased before the $pull so an uppercase-but-valid id cannot leave the dangling entry behind - createAgent/updateAgent/revertAgentVersion: pruning a non-empty allowlist to zero survivors disables skills; an explicit user-sent skills: [] keeps the full-catalog semantics - builder: a per-id skill lookup only renders the removable "Unavailable skill" chip on a confirmed 404/403 โ€” transient and server errors keep the chip hidden rather than inviting removal --- .../Content/Parts/FileAuthoringCall.tsx | 15 +- .../__tests__/FileAuthoringCall.test.tsx | 18 ++ .../SidePanel/Agents/AgentConfig.tsx | 72 +++++++- .../SSE/__tests__/useStepHandler.spec.ts | 82 +++++++++ client/src/hooks/SSE/useEventHandlers.ts | 19 +++ client/src/hooks/SSE/useStepHandler.ts | 40 +++++ client/src/locales/en/translation.json | 2 + .../data-schemas/src/methods/agent.spec.ts | 158 +++++++++++++++++- packages/data-schemas/src/methods/agent.ts | 46 +++++ .../data-schemas/src/methods/skill.spec.ts | 105 ++++++++++++ packages/data-schemas/src/methods/skill.ts | 77 +++++++++ 11 files changed, 615 insertions(+), 19 deletions(-) diff --git a/client/src/components/Chat/Messages/Content/Parts/FileAuthoringCall.tsx b/client/src/components/Chat/Messages/Content/Parts/FileAuthoringCall.tsx index be8cebf4e0..ae8b6fe993 100644 --- a/client/src/components/Chat/Messages/Content/Parts/FileAuthoringCall.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/FileAuthoringCall.tsx @@ -123,6 +123,10 @@ export default function FileAuthoringCall({ }) { const localize = useLocalize(); const isCreate = toolName === 'create_file'; + /** `create_file` can overwrite an existing file (sandbox `overwrite: true`, + * or skill SKILL.md updates). The host-authored summary always opens with + * `Created`/`Updated`, so key the finished label off it for truthfulness. */ + const overwrote = isCreate && output.startsWith('Updated '); const filePath = useMemo(() => parseJsonField(args, 'file_path'), [args]); const authoredContent = useMemo(() => parseJsonField(args, 'content'), [args]); const editArgsPreview = useMemo(() => buildEditArgsPreview(args), [args]); @@ -145,7 +149,12 @@ export default function FileAuthoringCall({ useToolCallState(initialProgress, isSubmitting, output, !!filePath || !!preview, onExpand); const highlighted = useLazyHighlight(preview || undefined, previewLang); - const Icon = isCreate ? FilePlus2 : FilePenLine; + const Icon = isCreate && !overwrote ? FilePlus2 : FilePenLine; + let finishedKey: 'com_ui_created_file' | 'com_ui_updated_file' | 'com_ui_edited_file' = + 'com_ui_edited_file'; + if (isCreate) { + finishedKey = overwrote ? 'com_ui_updated_file' : 'com_ui_created_file'; + } return ( <> @@ -157,9 +166,7 @@ export default function FileAuthoringCall({ 0: fileName, })} finishedText={ - cancelled - ? localize('com_ui_cancelled') - : localize(isCreate ? 'com_ui_created_file' : 'com_ui_edited_file', { 0: fileName }) + cancelled ? localize('com_ui_cancelled') : localize(finishedKey, { 0: fileName }) } errorSuffix={hasError && !cancelled ? localize('com_ui_tool_failed') : undefined} icon={ diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAuthoringCall.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAuthoringCall.test.tsx index 5565e94919..a57a00c954 100644 --- a/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAuthoringCall.test.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/FileAuthoringCall.test.tsx @@ -9,6 +9,7 @@ jest.mock('~/hooks', () => ({ const translations: Record = { com_ui_created_file: 'Created {{0}}', com_ui_creating_file: 'Creating {{0}}', + com_ui_updated_file: 'Updated {{0}}', com_ui_edited_file: 'Edited {{0}}', com_ui_editing_file: 'Editing {{0}}', com_ui_cancelled: 'Cancelled', @@ -104,6 +105,23 @@ describe('FileAuthoringCall', () => { expect(screen.getByText('Created skills/demo/SKILL.md (4096 chars).')).toBeInTheDocument(); }); + it('labels a create_file overwrite as Updated when the output summary says so', () => { + render( + , + ); + + expect(screen.getByTestId('progress-text')).toHaveTextContent('Updated SKILL.md'); + }); + it('prefers the output diff over the args preview after edit_file completes', () => { const output = [ 'Edited skills/demo/SKILL.md (exact match).', diff --git a/client/src/components/SidePanel/Agents/AgentConfig.tsx b/client/src/components/SidePanel/Agents/AgentConfig.tsx index 2ee88a59c8..bb8db1a770 100644 --- a/client/src/components/SidePanel/Agents/AgentConfig.tsx +++ b/client/src/components/SidePanel/Agents/AgentConfig.tsx @@ -1,11 +1,14 @@ import React, { useState, useMemo, useCallback } from 'react'; import { X } from 'lucide-react'; +import { useQueries } from '@tanstack/react-query'; import { Switch, useToastContext } from '@librechat/client'; import { Controller, useWatch, useFormContext } from 'react-hook-form'; import { EModelEndpoint, PermissionTypes, Permissions, + QueryKeys, + dataService, getEndpointField, } from 'librechat-data-provider'; import type { AgentForm, IconComponentTypes } from '~/common'; @@ -18,14 +21,14 @@ import { cn, } from '~/utils'; import { ToolSelectDialog, MCPToolSelectDialog } from '~/components/Tools'; -import { SkillSelectDialog } from '~/components/Skills/dialogs'; import useAgentCapabilities from '~/hooks/Agents/useAgentCapabilities'; +import { useListSkillsQuery, useGetAgentFiles } from '~/data-provider'; import { useFileMapContext, useAgentPanelContext } from '~/Providers'; +import { useLocalize, useVisibleTools, useHasAccess } from '~/hooks'; +import { SkillSelectDialog } from '~/components/Skills/dialogs'; import AgentCategorySelector from './AgentCategorySelector'; import Action from '~/components/SidePanel/Builder/Action'; -import { useLocalize, useVisibleTools, useHasAccess } from '~/hooks'; import { Panel, isEphemeralAgent } from '~/common'; -import { useListSkillsQuery, useGetAgentFiles } from '~/data-provider'; import { icons } from '~/hooks/Endpoint/Icons'; import Instructions from './Instructions'; import AgentAvatar from './AgentAvatar'; @@ -37,6 +40,14 @@ import AgentTool from './AgentTool'; import CodeForm from './Code/Form'; import MCPTools from './MCPTools'; +/** A skill lookup only counts as a confirmed miss on 404/403 โ€” deleted or no + * longer shared. Transient/network/server errors must not present a valid + * configured skill as removable. */ +const isConfirmedSkillMiss = (error: unknown): boolean => { + const status = (error as { response?: { status?: number } } | null)?.response?.status; + return status === 404 || status === 403; +}; + const labelClass = 'mb-2 text-token-text-primary block text-sm font-medium'; const inputClass = cn( defaultTextProps, @@ -111,6 +122,36 @@ export default function AgentConfig() { return map; }, [skillsData?.skills]); + /** Allowlist ids missing from the first catalog page (`limit: 100`) are + * resolved individually โ€” a cache miss alone must never present a valid + * configured skill as unavailable and invite its removal. */ + const unresolvedSkillIds = useMemo( + () => (skillsData === undefined ? [] : (skills ?? []).filter((id) => !skillsMap.has(id))), + [skills, skillsMap, skillsData], + ); + const unresolvedSkillQueries = useQueries({ + queries: unresolvedSkillIds.map((skillId) => ({ + queryKey: [QueryKeys.skill, skillId], + queryFn: () => dataService.getSkill(skillId), + retry: false, + refetchOnMount: false, + refetchOnReconnect: false, + refetchOnWindowFocus: false, + })), + }); + const unresolvedSkills = useMemo(() => { + const map = new Map(); + unresolvedSkillIds.forEach((skillId, index) => { + const query = unresolvedSkillQueries[index]; + if (query?.isError === true && isConfirmedSkillMiss(query.error)) { + map.set(skillId, { missing: true }); + } else if (query?.data?.name) { + map.set(skillId, { name: query.data.name, missing: false }); + } + }); + return map; + }, [unresolvedSkillIds, unresolvedSkillQueries]); + const { data: agentFiles = [] } = useGetAgentFiles(agent_id); const mergedFileMap = useMemo(() => { @@ -380,16 +421,31 @@ export default function AgentConfig() { >
{(skills ?? []).map((skillId) => { - const skillName = skillsMap.get(skillId); - if (!skillName) { + const skillName = skillsMap.get(skillId) ?? unresolvedSkills.get(skillId)?.name; + /** Hide chips while the catalog page or per-id lookup is in + * flight. Once the backend confirms a miss (deleted or no + * longer shared), the id must stay visible and removable โ€” + * otherwise the allowlist silently scopes the agent to + * zero skills with no way to fix it in the UI. */ + if (!skillName && unresolvedSkills.get(skillId)?.missing !== true) { return null; } + const isUnavailable = !skillName; return (
- {skillName} + + {skillName ?? localize('com_ui_skill_unavailable')} +