CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-27 04:07:05 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 13

🪢 fix: Prune Dangling Skill IDs from Agent Allowlists (#13702)

Browse source 
* 🧹 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 <file>" 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
This commit is contained in:
Danny Avila 2026-06-13 11:16:14 -04:00 • committed by GitHub
parent dea71c8396
commit 5ceabad5f3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 615 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

158
packages/data-schemas/src/methods/agent.spec.ts
View file

@ -558,6 +558,94 @@ describe('Agent Methods', () => {
expect(updatedAgent!.mcpServerNames).toEqual(['authorizedServer']);
});
test('should prune nonexistent skill ids from the allowlist on create', async () => {
const { agentId, authorId } = createTestIds();
const realSkill = await mongoose.models.Skill.create({
name: 'create-prune-skill',
description: 'Skill backing the create-time allowlist pruning test.',
author: authorId,
authorName: 'Test Author',
});
const danglingId = new mongoose.Types.ObjectId().toString();
const newAgent = await createAgent({
id: agentId,
name: 'Skill Prune Agent',
provider: 'test',
model: 'test-model',
author: authorId,
skills: [realSkill._id.toString(), danglingId],
skills_enabled: true,
});
expect(newAgent.skills).toEqual([realSkill._id.toString()]);
});
test('should prune nonexistent skill ids from the allowlist on update', async () => {
const { agentId, authorId } = createTestIds();
const realSkill = await mongoose.models.Skill.create({
name: 'update-prune-skill',
description: 'Skill backing the update-time allowlist pruning test.',
author: authorId,
authorName: 'Test Author',
});
const danglingId = new mongoose.Types.ObjectId().toString();
await createAgent({
id: agentId,
name: 'Skill Prune Agent',
provider: 'test',
model: 'test-model',
author: authorId,
});
const updatedAgent = await updateAgent(
{ id: agentId },
{ skills: [danglingId, realSkill._id.toString()], skills_enabled: true },
);
expect(updatedAgent!.skills).toEqual([realSkill._id.toString()]);
expect(updatedAgent!.skills_enabled).toBe(true);
});
test('should fail closed when pruning empties the allowlist on update', async () => {
const { agentId, authorId } = createTestIds();
const danglingId = new mongoose.Types.ObjectId().toString();
await createAgent({
id: agentId,
name: 'Skill Heal Agent',
provider: 'test',
model: 'test-model',
author: authorId,
});
const updatedAgent = await updateAgent(
{ id: agentId },
{ skills: [danglingId], skills_enabled: true },
);
expect(updatedAgent!.skills).toEqual([]);
expect(updatedAgent!.skills_enabled).toBe(false);
});
test('should keep full-catalog semantics for an explicit empty allowlist on update', async () => {
const { agentId, authorId } = createTestIds();
await createAgent({
id: agentId,
name: 'Explicit Empty Agent',
provider: 'test',
model: 'test-model',
author: authorId,
});
const updatedAgent = await updateAgent({ id: agentId }, { skills: [], skills_enabled: true });
expect(updatedAgent!.skills).toEqual([]);
expect(updatedAgent!.skills_enabled).toBe(true);
});
test('should delete an agent', async () => {
const agentId = `agent_${uuidv4()}`;
const authorId = new mongoose.Types.ObjectId();
@ -1741,6 +1829,38 @@ describe('Agent Methods', () => {
expect(revertedAgent.description).toBe('Original description');
});
test('should prune deleted skill ids when reverting to an older version', async () => {
const agentId = `agent_${uuidv4()}`;
const authorId = new mongoose.Types.ObjectId();
const Skill = mongoose.models.Skill;
const skill = await Skill.create({
name: 'revert-prune-skill',
description: 'Skill backing the revert pruning test.',
author: authorId,
authorName: 'Test Author',
});
const skillId = skill._id.toString();
await createAgent({
id: agentId,
name: 'Revert Skill Agent',
provider: 'test',
model: 'test-model',
author: authorId,
skills: [skillId],
skills_enabled: true,
});
await updateAgent({ id: agentId }, { skills: [], name: 'No Skills Anymore' });
await Skill.deleteOne({ _id: skill._id });
const revertedAgent = await revertAgentVersion({ id: agentId }, 0);
expect(revertedAgent.name).toBe('Revert Skill Agent');
expect(revertedAgent.skills).toEqual([]);
expect(revertedAgent.skills_enabled).toBe(false);
});
test('should detect action metadata changes and force version update', async () => {
const agentId = `agent_${uuidv4()}`;
const authorId = new mongoose.Types.ObjectId();
@ -3373,10 +3493,21 @@ describe('Support Contact Field', () => {
});
test('should omit skill configuration from the default list projection', async () => {
const targetSkillIds = [
new mongoose.Types.ObjectId().toString(),
new mongoose.Types.ObjectId().toString(),
];
const skillDocs = await mongoose.models.Skill.create([
{
name: 'projection-skill-a',
description: 'Skill backing projection test.',
author: userA,
authorName: 'Test Author',
},
{
name: 'projection-skill-b',
description: 'Skill backing projection test.',
author: userA,
authorName: 'Test Author',
},
]);
const targetSkillIds = skillDocs.map((doc) => doc._id.toString());
const scopedAgent = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Scoped Agent',
@ -3399,10 +3530,21 @@ describe('Support Contact Field', () => {
});
test('should include skill configuration only when explicitly requested', async () => {
const targetSkillIds = [
new mongoose.Types.ObjectId().toString(),
new mongoose.Types.ObjectId().toString(),
];
const skillDocs = await mongoose.models.Skill.create([
{
name: 'scoped-skill-a',
description: 'Skill backing inclusion test.',
author: userA,
authorName: 'Test Author',
},
{
name: 'scoped-skill-b',
description: 'Skill backing inclusion test.',
author: userA,
authorName: 'Test Author',
},
]);
const targetSkillIds = skillDocs.map((doc) => doc._id.toString());
const scopedAgent = await createAgent({
id: `agent_${uuidv4().slice(0, 12)}`,
name: 'Scoped Agent',

46
packages/data-schemas/src/methods/agent.ts
View file

@ -9,6 +9,7 @@ import {
import type { AgentToolResources } from 'librechat-data-provider';
import type { FilterQuery, Model, Types } from 'mongoose';
import type { IAgent, IAclEntry } from '~/types';
import { filterExistingSkillIds } from './skill';
import logger from '~/config/winston';
const { mcp_delimiter } = Constants;
@ -330,6 +331,15 @@ export function createAgentMethods(
*/
async function createAgent(agentData: Record<string, unknown>): Promise<IAgent> {
const Agent = mongoose.models.Agent as Model<IAgent>;
if (Array.isArray(agentData.skills) && agentData.skills.length > 0) {
const prunedSkills = await filterExistingSkillIds(mongoose, agentData.skills as string[]);
agentData.skills = prunedSkills;
/** Fail closed when pruning empties a non-empty allowlist — empty +
* enabled means the full catalog, and hygiene must never widen scope. */
if (prunedSkills.length === 0) {
agentData.skills_enabled = false;
}
}
const { author: _author, ...versionData } = agentData;
const timestamp = new Date();
const initialAgentData = {
@ -438,6 +448,27 @@ export function createAgentMethods(
} = currentAgent.toObject() as unknown as Record<string, unknown>;
const { $push, $pull, $addToSet, ...directUpdates } = updateData;
/** Self-heal: drop allowlist ids whose skill doc no longer exists.
* A dangling id keeps the allowlist non-empty while scoping the
* runtime catalog to an empty intersection — silently disabling
* skills for the agent. When pruning empties a non-empty allowlist,
* fail closed and disable skills: empty + enabled means the full
* catalog, and hygiene must never widen scope. (An explicit user
* `skills: []` submission skips this branch and keeps the
* full-catalog semantics.) */
if (Array.isArray(directUpdates.skills) && directUpdates.skills.length > 0) {
const prunedSkills = await filterExistingSkillIds(
mongoose,
directUpdates.skills as string[],
);
directUpdates.skills = prunedSkills;
updateData.skills = prunedSkills;
if (prunedSkills.length === 0) {
directUpdates.skills_enabled = false;
updateData.skills_enabled = false;
}
}
// Sync mcpServerNames when tools are updated
if ((directUpdates as Record<string, unknown>).tools !== undefined) {
const mcpServerNames = extractMCPServerNames(
@ -910,6 +941,21 @@ export function createAgentMethods(
delete revertToVersion.author;
delete revertToVersion.updatedBy;
/** Version snapshots can predate skill deletions; restoring one verbatim
* would resurrect dangling allowlist ids that scope the catalog to
* nothing. Same self-heal (and fail-closed-on-empty rule) as
* `createAgent`/`updateAgent`. */
if (Array.isArray(revertToVersion.skills) && revertToVersion.skills.length > 0) {
const prunedSkills = await filterExistingSkillIds(
mongoose,
revertToVersion.skills as string[],
);
revertToVersion.skills = prunedSkills;
if (prunedSkills.length === 0) {
revertToVersion.skills_enabled = false;
}
}
const revertedAgent = await Agent.findOneAndUpdate(searchParameter, revertToVersion, {
new: true,
}).lean<IAgent>();

105
packages/data-schemas/src/methods/skill.spec.ts
View file

@ -15,6 +15,7 @@ import {
validateAlwaysApply,
validateRelativePath,
inferSkillFileCategory,
filterExistingSkillIds,
deriveStructuredFrontmatterFields,
} from './skill';
import { createAclEntryMethods } from './aclEntry';
@ -107,8 +108,22 @@ afterEach(async () => {
await Skill.deleteMany({});
await SkillFile.deleteMany({});
await AclEntry.deleteMany({});
await mongoose.models.Agent.deleteMany({});
});
function makeAgentDoc(skillIds: string[], overrides: Record<string, unknown> = {}) {
return {
id: `agent_${new mongoose.Types.ObjectId().toString()}`,
name: 'Allowlist Agent',
provider: 'openai',
model: 'gpt-4',
author: owner._id,
skills: skillIds,
skills_enabled: true,
...overrides,
};
}
async function grantOwner(resourceId: mongoose.Types.ObjectId | string) {
const role = (await AccessRole.findOne({ accessRoleId: AccessRoleIds.SKILL_OWNER }).lean()) as {
_id: mongoose.Types.ObjectId;
@ -533,6 +548,79 @@ describe('Skill CRUD methods', () => {
expect(await SkillFile.countDocuments({ skillId: skill._id })).toBe(0);
});
it('filterExistingSkillIds matches uppercase-hex candidates and returns canonical ids', async () => {
const { skill } = await methods.createSkill(makeSkillInput({ name: 'case-skill' }));
const canonical = skill._id.toString();
const filtered = await filterExistingSkillIds(mongoose, [canonical.toUpperCase()]);
expect(filtered).toEqual([canonical]);
});
it('deleteSkill prunes agent allowlists even when skill file cleanup fails', async () => {
const { skill } = await methods.createSkill(makeSkillInput({ name: 'flaky-files' }));
const Agent = mongoose.models.Agent;
const agent = await Agent.create(makeAgentDoc([skill._id.toString()]));
const deleteManySpy = jest
.spyOn(SkillFile, 'deleteMany')
.mockRejectedValueOnce(new Error('transient storage failure'));
try {
await expect(methods.deleteSkill(skill._id.toString())).rejects.toThrow(
'transient storage failure',
);
} finally {
deleteManySpy.mockRestore();
}
const agentAfter = (await Agent.findById(agent._id).lean()) as {
skills?: string[];
skills_enabled?: boolean;
} | null;
expect(agentAfter?.skills).toEqual([]);
expect(agentAfter?.skills_enabled).toBe(false);
});
it('deleteSkill disables skills when the deleted id was the entire allowlist', async () => {
const { skill } = await methods.createSkill(makeSkillInput({ name: 'only-skill' }));
const Agent = mongoose.models.Agent;
const agent = await Agent.create(makeAgentDoc([skill._id.toString()]));
const res = await methods.deleteSkill(skill._id.toString().toUpperCase());
expect(res.deleted).toBe(true);
const agentAfter = (await Agent.findById(agent._id).lean()) as {
skills?: string[];
skills_enabled?: boolean;
} | null;
expect(agentAfter?.skills).toEqual([]);
expect(agentAfter?.skills_enabled).toBe(false);
});
it('deleteSkill prunes the deleted id from agent skill allowlists', async () => {
const { skill: doomed } = await methods.createSkill(makeSkillInput({ name: 'doomed-skill' }));
const { skill: kept } = await methods.createSkill(makeSkillInput({ name: 'kept-skill' }));
const Agent = mongoose.models.Agent;
const scoped = await Agent.create(makeAgentDoc([doomed._id.toString(), kept._id.toString()]));
const untouched = await Agent.create(
makeAgentDoc([kept._id.toString()], { name: 'Untouched Agent' }),
);
const res = await methods.deleteSkill(doomed._id.toString());
expect(res.deleted).toBe(true);
const scopedAfter = (await Agent.findById(scoped._id).lean()) as {
skills?: string[];
skills_enabled?: boolean;
} | null;
expect(scopedAfter?.skills).toEqual([kept._id.toString()]);
expect(scopedAfter?.skills_enabled).toBe(true);
const untouchedAfter = (await Agent.findById(untouched._id).lean()) as {
skills?: string[];
} | null;
expect(untouchedAfter?.skills).toEqual([kept._id.toString()]);
});
it('findSkillBySourceIdentity searches only the requested tenant bucket', async () => {
const upstreamId = 'librechat-skills:skills/research';
const sourceMetadata = {
@ -1792,4 +1880,21 @@ describe('deleteUserSkills', () => {
expect(await Skill.countDocuments()).toBe(1);
expect(await Skill.countDocuments({ _id: sharedId })).toBe(1);
});
it('prunes deleted sole-owned skill ids from agent allowlists', async () => {
const { skill: mine } = await methods.createSkill(makeSkillInput({ name: 'mine' }));
await grantOwner(mine._id);
const Agent = mongoose.models.Agent;
const agent = await Agent.create(makeAgentDoc([mine._id.toString()]));
const deleted = await methods.deleteUserSkills(owner._id as mongoose.Types.ObjectId);
expect(deleted).toBe(1);
const agentAfter = (await Agent.findById(agent._id).lean()) as {
skills?: string[];
skills_enabled?: boolean;
} | null;
expect(agentAfter?.skills).toEqual([]);
expect(agentAfter?.skills_enabled).toBe(false);
});
});

77
packages/data-schemas/src/methods/skill.ts
View file

@ -16,6 +16,7 @@ import type {
ISkillFileDocument,
ISkillSummary,
} from '~/types/skill';
import type { IAgent } from '~/types/agent';
import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite';
import { isValidObjectIdString } from '~/utils/objectId';
import { stripYamlTrailingComment } from '~/utils/yaml';
@ -834,6 +835,35 @@ function resolveAlwaysApplyFromInput(
return fallback;
}
/**
* Narrows candidate skill ids to those backed by an existing Skill doc.
* Existence-only check (no ACL) so pruning an agent allowlist never drops
* skills the saving user merely can't view. Preserves input order, dedupes,
* and drops malformed ids — they can't reference anything. Candidates are
* lowercased before comparison: `isValidObjectIdString` accepts uppercase
* hex, but `_id.toString()` is always lowercase, and a casing mismatch
* would silently drop a valid id (and an emptied allowlist means the full
* catalog — the opposite of the configured scope).
*/
export async function filterExistingSkillIds(
mongoose: typeof import('mongoose'),
skillIds: string[],
): Promise<string[]> {
const candidates = [
...new Set(skillIds.filter(isValidObjectIdString).map((id) => id.toLowerCase())),
];
if (candidates.length === 0) {
return [];
}
const Skill = mongoose.models.Skill as Model<ISkillDocument>;
const docs = await Skill.find(
{ _id: { $in: candidates.map((id) => new mongoose.Types.ObjectId(id)) } },
{ _id: 1 },
).lean<Array<{ _id: Types.ObjectId }>>();
const existing = new Set(docs.map((doc) => doc._id.toString()));
return candidates.filter((id) => existing.has(id));
}
/**
* Validate the `always-apply` value that would be derived from the
* SKILL.md body's inline frontmatter. Only reports an issue when the
@ -1469,6 +1499,48 @@ export function createSkillMethods(
};
}
/**
* Removes deleted skill ids from every agent's `skills` allowlist. A dangling
* id is invisible in the builder yet keeps the allowlist non-empty, so the
* runtime scopes the catalog to an empty intersection and the agent silently
* loses all skills. Direct `updateMany` on purpose: hygiene, not an authored
* edit — no version entry, timestamps untouched.
*
* Ids are lowercased first: allowlists store canonical `_id.toString()`
* values, and an uppercase-but-valid id would delete the Skill doc yet
* leave the dangling entry behind.
*
* Agents whose ENTIRE allowlist is being deleted fail closed instead:
* an emptied allowlist with `skills_enabled: true` means the full
* accessible catalog at runtime, so a plain `$pull` would silently widen
* a deliberately restricted agent. Disabling skills preserves the
* restriction until an author makes a new explicit choice.
*/
async function removeSkillsFromAgentAllowlists(skillIds: string[]): Promise<void> {
if (skillIds.length === 0) {
return;
}
const ids = skillIds.map((id) => id.toLowerCase());
const Agent = mongoose.models.Agent as Model<IAgent>;
try {
await Agent.updateMany(
{ skills: { $in: ids, $not: { $elemMatch: { $nin: ids } } } },
{ $set: { skills: [], skills_enabled: false } },
{ timestamps: false },
);
await Agent.updateMany(
{ skills: { $in: ids } },
{ $pull: { skills: { $in: ids } } },
{ timestamps: false },
);
} catch (error) {
logger.error(
'[removeSkillsFromAgentAllowlists] Error pruning agent skill allowlists:',
error,
);
}
}
async function deleteSkill(id: string): Promise<{ deleted: boolean }> {
if (!isValidObjectIdString(id)) {
return { deleted: false };
@ -1480,6 +1552,10 @@ export function createSkillMethods(
if (!res.deletedCount) {
return { deleted: false };
}
/** Prune allowlists immediately after the Skill row is gone: if the
* SkillFile cleanup below throws, a retry exits early on
* `deletedCount === 0` and would never reach a later prune. */
await removeSkillsFromAgentAllowlists([id]);
await SkillFile.deleteMany({ skillId: objectId });
try {
await deps.removeAllPermissions({ resourceType: ResourceType.SKILL, resourceId: id });
@ -1499,6 +1575,7 @@ export function createSkillMethods(
const SkillFile = mongoose.models.SkillFile as Model<ISkillFileDocument>;
await SkillFile.deleteMany({ skillId: { $in: soleOwned } });
const res = await Skill.deleteMany({ _id: { $in: soleOwned } });
await removeSkillsFromAgentAllowlists(soleOwned.map((rid) => rid.toString()));
await Promise.allSettled(
soleOwned.map((rid) =>
deps