From 59ab01339d31e3975bdb312cd2d776a7042f1508 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 09:20:02 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8E=20fix:=20Report=20Measured=20Skill?= =?UTF-8?q?=20Catalog=20Truncation=20(#15590)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ”Ž fix: Report Measured Skill Catalog Truncation The catalog truncation warning assumed `SKILL_CATALOG_MAX_ENTRY_CHARS` was the delivered length. It is only the first rung of the SDK's ladder: `formatSkillCatalog` applies the per-entry cap, then truncates further against its own context budget, and finally falls back to names-only. Two consequences, both silent: - A description under the 250-char cap can still be cut โ€” no warning fired at all. Measured: 8 skills x 200 chars at a 20k context window reach the model at 85 chars each. - When the ladder does engage past the cap, the warning overstated what landed โ€” reporting 250 where the true figure was 60. Measure the emitted catalog instead of assuming the cap held, and warn separately when descriptions are dropped entirely. `formatSkillCatalog` is no longer stubbed in the spec. The passthrough mock never truncated, which is why no test could observe this class of defect. * ๐Ÿ” fix: Measure Duplicate-Named Catalog Entries Positionally Keying delivered lengths by skill name collapsed duplicates: the catalog keeps every invocable duplicate as its own entry, so a name-keyed map held only the last one's length and reported it for all of them. A 400-char and a 100-char description sharing a name warned "truncated to 100 of 400" when the first entry actually reached the model at 250. Match positionally instead. Entry order is the order passed to the formatter, and the names-only fallback drops from the tail, so a skill the catalog never reached keeps its 0 and reads as dropped. * ๐Ÿงต fix: Detect Catalog Truncation By Containment, Not Line Parsing Nothing strips newlines from a skill description โ€” validateSkillDescription checks type, length and trim-emptiness only โ€” so a catalog entry is not one physical line, and parsing it as one misreports three ways: - a multiline description the catalog kept intact was measured from its first line alone and flagged as truncated - a description starting with a newline measured 0 and read as dropped - a continuation line imitating the next entry's marker desynchronized every measurement after it (a 400-char entry measured 8) Entry boundaries are not recoverable from the emitted text, because a description may contain the exact byte sequence that starts the next entry. So stop parsing it. A description the catalog kept appears verbatim, and containment detects truncation from any rung of the ladder without needing boundaries at all โ€” including the duplicate-name case, which the positional pass already handled and which this preserves. Drops the delivered-length figure from the warning. That number is what kept being wrong, and the actionable signal for an author is that their description is being cut, not by precisely how much. * ๐ŸŽฏ fix: Scope Catalog Containment To The Matching Entry Matching the description alone missed a drop whenever that text occurred anywhere else in the catalog. The sharp case is the names-only fallback: a one-word description like "research" collides with a skill *named* research, so containment held and the warning was skipped for a description that had been dropped entirely. Match the whole `- name: description` rendering instead. That is still containment โ€” no entry parsing, so multiline descriptions, duplicate names and continuation lines imitating the next marker all stay correct โ€” but a collision now has to reproduce the entry's full rendering rather than just its text. Skips empty descriptions, which render without the `: ` separator and have nothing to truncate. * ๐Ÿ”ฌ fix: Measure Catalog Truncation On A Length-Equivalent Probe Matching the rendered entry still had a hole: when a multiline description is cut immediately before text like "\n- next-skill: next description" and the following entry is exactly that, the catalog holds the full rendering across two entries and the check passes for a description that was truncated. That is the fourth variant of one problem โ€” the real catalog cannot be parsed or matched, because descriptions are arbitrary text that can imitate any structure the check relies on. So measure somewhere unambiguous. Every decision in the truncation ladder reads description `.length` and never description content, so formatting a probe whose descriptions are same-length filler reproduces the real cuts exactly, and the probe is parseable by construction: filler carries no newline and no entry marker, and skill names are validated to `^[a-z0-9][a-z0-9-]*$`. This closes the class rather than the instance, and restores the delivered length to the warning โ€” verified against all six adversarial cases raised in review, correct on 6/6. --- .../api/src/agents/__tests__/skills.test.ts | 206 ++++++++++++++++-- packages/api/src/agents/skills.ts | 87 ++++++-- 2 files changed, 259 insertions(+), 34 deletions(-) diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 1e20d5f12e..ceda9e3e0f 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -1,7 +1,11 @@ /** * Mock the pieces of `@librechat/agents` the installed SDK version may not - * export yet. Includes both the `Constants.SKILL_TOOL` stub and the skill - * catalog/tool-definition helpers needed to exercise `injectSkillCatalog`. + * export yet โ€” the `Constants.SKILL_TOOL` stub and the tool definitions + * needed to exercise `injectSkillCatalog`. + * + * `formatSkillCatalog` is deliberately NOT stubbed. Its truncation ladder is + * the behaviour the catalog warnings report on, and a passthrough stub hides + * every truncation the model actually sees. */ jest.mock('@librechat/agents', () => ({ ...jest.requireActual('@librechat/agents'), @@ -10,8 +14,6 @@ jest.mock('@librechat/agents', () => ({ .Constants, SKILL_TOOL: 'skill', }, - formatSkillCatalog: (skills: Array<{ name: string; description: string }>) => - skills.map((s) => `- ${s.name}: ${s.description}`).join('\n'), SkillToolDefinition: { name: 'skill', description: 'skill tool', parameters: {} }, ReadFileToolDefinition: { name: 'read_file', @@ -943,38 +945,204 @@ describe('injectSkillCatalog', () => { expect(agent.additional_instructions).toContain('desc-my-skill'); }); + /** Truncated descriptions, as `[skillName, reachedChars, authoredChars]`. */ + function truncationWarnings(warnSpy: jest.SpyInstance): Array<[string, number, number]> { + return warnSpy.mock.calls + .map((call) => String(call[0])) + .map((msg) => + /skill "([^"]+)" description reached the model truncated to (\d+) of (\d+)/.exec(msg), + ) + .filter((m): m is RegExpExecArray => m !== null) + .map((m) => [m[1], Number(m[2]), Number(m[3])]); + } + + /** Skills whose description the catalog dropped entirely. */ + function droppedWarnings(warnSpy: jest.SpyInstance): string[] { + return warnSpy.mock.calls + .map((call) => String(call[0])) + .map((msg) => /skill "([^"]+)" description was dropped from the model catalog/.exec(msg)) + .filter((m): m is RegExpExecArray => m !== null) + .map((m) => m[1]); + } + it('warns when a skill description exceeds the catalog entry cap', async () => { const { logger } = await import('@librechat/data-schemas'); const warnSpy = jest.spyOn(logger, 'warn'); - const longDesc = 'x'.repeat(400); const longSkill: PageSkill = { ...makeSkill('long-skill', userObjectId), - description: longDesc, + description: 'x'.repeat(400), }; const shortSkill = makeSkill('short-skill', userObjectId); const listSkillsByAccess = buildPager([[longSkill, shortSkill]]); const agent = makeAgent(); await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); - const truncWarns = warnSpy.mock.calls - .map((call) => String(call[0])) - .filter((msg) => msg.includes('truncated to')); - expect(truncWarns).toHaveLength(1); - expect(truncWarns[0]).toContain('"long-skill"'); - expect(truncWarns[0]).toContain('was 400'); - /* Short description is not flagged. */ - expect( - warnSpy.mock.calls - .map((call) => String(call[0])) - .filter((msg) => msg.includes('"short-skill"') && msg.includes('truncated')), - ).toHaveLength(0); - + const warnings = truncationWarnings(warnSpy); + expect(warnings).toHaveLength(1); + const [name, reached, authored] = warnings[0]; + expect(name).toBe('long-skill'); + expect(authored).toBe(400); + expect(reached).toBeLessThan(authored); /* The catalog still reaches the model โ€” the warning is additive. */ expect(agent.additional_instructions).toContain('long-skill'); expect(agent.additional_instructions).toContain('short-skill'); warnSpy.mockRestore(); }); + it('warns for a sub-cap description the catalog budget still truncates', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* Every description sits under the per-entry cap, so a warning keyed to + that cap alone stays silent โ€” but a catalog this size overruns its + context budget and gets cut well below it anyway. */ + const skills = Array.from({ length: 8 }, (_, i) => ({ + ...makeSkill(`budget-skill-${i}`, userObjectId), + description: 'x'.repeat(200), + })); + const listSkillsByAccess = buildPager([skills]); + const agent = makeAgent(); + await injectSkillCatalog( + baseParams({ listSkillsByAccess, agent, contextWindowTokens: 20_000 }), + ); + + const warnings = truncationWarnings(warnSpy); + expect(warnings).toHaveLength(skills.length); + for (const [, reached, authored] of warnings) { + expect(authored).toBe(200); + expect(reached).toBeGreaterThan(0); + expect(reached).toBeLessThan(200); + } + warnSpy.mockRestore(); + }); + + it('reports descriptions as dropped when the catalog falls back to names-only', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + const skills = Array.from({ length: 10 }, (_, i) => ({ + ...makeSkill(`dropped-skill-${i}`, userObjectId), + description: 'x'.repeat(200), + })); + const listSkillsByAccess = buildPager([skills]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent, contextWindowTokens: 2_000 })); + + expect(droppedWarnings(warnSpy)).toHaveLength(skills.length); + /* Names still reach the model even when every description is dropped. */ + expect(agent.additional_instructions).toContain('dropped-skill-0'); + warnSpy.mockRestore(); + }); + + it('measures duplicate-named skills per entry rather than collapsing them', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* The catalog keeps both entries. Measuring by name alone would report the + last entry's length for both and understate the first. */ + const longDup: PageSkill = { + ...makeSkill('dup-skill', userObjectId), + description: 'x'.repeat(400), + }; + const shortDup: PageSkill = { + ...makeSkill('dup-skill', userObjectId), + description: 'y'.repeat(100), + }; + const listSkillsByAccess = buildPager([[longDup, shortDup]]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + + const warnings = truncationWarnings(warnSpy); + expect(warnings).toHaveLength(1); + const [name, reached, authored] = warnings[0]; + expect(name).toBe('dup-skill'); + expect(authored).toBe(400); + expect(reached).toBeGreaterThan(100); + warnSpy.mockRestore(); + }); + + it('does not flag multiline descriptions the catalog kept intact', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* Nothing strips newlines from a description, so a catalog entry is not + one physical line โ€” and a leading newline leaves the first one empty. */ + const multiline: PageSkill = { + ...makeSkill('multiline-skill', userObjectId), + description: 'First line of the description.\nSecond line with more triggers.', + }; + const leading: PageSkill = { + ...makeSkill('leading-newline-skill', userObjectId), + description: '\nAll the real trigger text lives on line two.', + }; + const listSkillsByAccess = buildPager([[multiline, leading]]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + + expect(truncationWarnings(warnSpy)).toEqual([]); + expect(droppedWarnings(warnSpy)).toEqual([]); + warnSpy.mockRestore(); + }); + + it('stays aligned when a description imitates the next entry', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* A continuation line can look exactly like the next entry's marker. */ + const imitator: PageSkill = { + ...makeSkill('imitator-skill', userObjectId), + description: 'start\n- victim-skill: hijacked', + }; + const victim: PageSkill = { + ...makeSkill('victim-skill', userObjectId), + description: 'y'.repeat(400), + }; + const listSkillsByAccess = buildPager([[imitator, victim]]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + + expect(truncationWarnings(warnSpy).map(([name]) => name)).toEqual(['victim-skill']); + warnSpy.mockRestore(); + }); + + it('still flags a dropped description that collides with another skill name', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* Names-only drops every description, but a one-word description can occur + verbatim in the catalog as another skill's name. */ + const named = makeSkill('research', userObjectId); + const collider: PageSkill = { + ...makeSkill('other-skill', userObjectId), + description: 'research', + }; + const filler = Array.from({ length: 20 }, (_, i) => ({ + ...makeSkill(`filler-skill-${i}`, userObjectId), + description: 'y'.repeat(200), + })); + const listSkillsByAccess = buildPager([[named, collider, ...filler]]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent, contextWindowTokens: 2_000 })); + + expect(droppedWarnings(warnSpy)).toContain('other-skill'); + warnSpy.mockRestore(); + }); + + it('flags truncation that splices an entry tail onto the next entry rendering', async () => { + const { logger } = await import('@librechat/data-schemas'); + const warnSpy = jest.spyOn(logger, 'warn'); + /* Cutting here leaves the catalog holding this description's full text + across two entries, so matching the rendering would suppress the warning. */ + const spliced: PageSkill = { + ...makeSkill('spliced-skill', userObjectId), + description: `${'z'.repeat(249)}\u2026\n- next-skill: next description`, + }; + const next: PageSkill = { + ...makeSkill('next-skill', userObjectId), + description: 'next description', + }; + const listSkillsByAccess = buildPager([[spliced, next]]); + const agent = makeAgent(); + await injectSkillCatalog(baseParams({ listSkillsByAccess, agent })); + + expect(truncationWarnings(warnSpy).map(([name]) => name)).toEqual(['spliced-skill']); + warnSpy.mockRestore(); + }); + it('honors a configured maxCatalogSkills below the default hard limit', async () => { const first = makeSkill('first-skill', userObjectId); const second = makeSkill('second-skill', userObjectId); diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index ae40ab12ba..338ff36829 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -95,10 +95,12 @@ const MAX_CATALOG_PAGES = 10; /** Page size used when paginating to fill the active-skill quota. */ const CATALOG_PAGE_SIZE = 100; /** - * Per-entry description cap applied by `formatSkillCatalog` before the - * catalog is injected into agent context. `@librechat/agents` truncates - * silently, so this mirrors the default so we can warn when authors' skill - * descriptions will not reach the model verbatim. + * Per-entry description cap requested of `formatSkillCatalog`, mirroring the + * SDK default. It is a ceiling, not a guarantee: `@librechat/agents` applies + * it first, then truncates further โ€” proportionally against its own context + * budget, and finally to names-only โ€” so a description well under this cap + * can still be cut. Delivered length is measured from the emitted catalog + * rather than assumed from this value. */ const SKILL_CATALOG_MAX_ENTRY_CHARS = 250; /** Hard ceiling on skill names a model spec can request by config. */ @@ -583,6 +585,54 @@ export async function resolveSkillCatalog( }; } +/** Filler used to build the measurement probe; never reaches the model. */ +const CATALOG_PROBE_CHAR = 'x'; + +/** + * How much of each skill's description reaches the model, aligned to `skills`. + * + * Measured on a probe rather than on the real catalog. Every decision in + * `formatSkillCatalog`'s truncation ladder reads description `.length` and + * never description content, so formatting same-length filler reproduces the + * real cuts exactly โ€” while guaranteeing the output can be parsed, since + * filler carries no newline and no entry marker and skill names are validated + * to `^[a-z0-9][a-z0-9-]*$`. + * + * The real catalog cannot be measured: a description may contain newlines, so + * an entry is not one line; duplicate names share a rendering; and truncation + * can splice one entry's tail onto the next, so even a whole-entry match can + * be satisfied by text the model never received as that entry. + */ +function measureCatalogDescriptions( + skills: Array<{ name: string; description: string }>, + options: Parameters[1], +): number[] { + const probe = formatSkillCatalog( + skills.map((s) => ({ + name: s.name, + description: CATALOG_PROBE_CHAR.repeat(s.description.length), + })), + options, + ); + const delivered = new Array(skills.length).fill(0); + let index = 0; + for (const line of probe.split('\n')) { + if (index >= skills.length) { + break; + } + const prefix = `- ${skills[index].name}`; + if (line === prefix) { + index++; + continue; + } + if (line.startsWith(`${prefix}: `)) { + delivered[index] = line.length - prefix.length - 2; + index++; + } + } + return delivered; +} + /** * Queries accessible skills, formats a budget-aware catalog, appends it to the * agent's additional_instructions, and registers the SkillTool definition. @@ -695,21 +745,28 @@ export async function injectSkillCatalog( * and those reads would otherwise be impossible. */ if (catalogVisibleSkills.length > 0) { - for (const s of catalogVisibleSkills) { - if (s.description.length > SKILL_CATALOG_MAX_ENTRY_CHARS) { - logger.warn( - `[injectSkillCatalog] skill "${s.name}" description truncated to ${SKILL_CATALOG_MAX_ENTRY_CHARS} chars for the model catalog (was ${s.description.length})`, - ); - } - } + const catalogOptions = { + contextWindowTokens: contextWindowTokens || 200_000, + maxEntryChars: SKILL_CATALOG_MAX_ENTRY_CHARS, + }; const catalog = formatSkillCatalog( catalogVisibleSkills.map((s) => ({ name: s.name, description: s.description })), - { - contextWindowTokens: contextWindowTokens || 200_000, - maxEntryChars: SKILL_CATALOG_MAX_ENTRY_CHARS, - }, + catalogOptions, ); if (catalog) { + const delivered = measureCatalogDescriptions(catalogVisibleSkills, catalogOptions); + for (let i = 0; i < catalogVisibleSkills.length; i++) { + const s = catalogVisibleSkills[i]; + const reached = delivered[i]; + if (reached >= s.description.length) { + continue; + } + logger.warn( + reached === 0 + ? `[injectSkillCatalog] skill "${s.name}" description was dropped from the model catalog (was ${s.description.length} chars) โ€” the catalog exceeded its context budget` + : `[injectSkillCatalog] skill "${s.name}" description reached the model truncated to ${reached} of ${s.description.length} chars`, + ); + } agent.additional_instructions = agent.additional_instructions ? `${agent.additional_instructions}\n\n${catalog}` : catalog;