From f9ce81bfb384e909353813fb6f6760cadd508ae7 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 7 Apr 2026 22:03:52 +0530 Subject: [PATCH] fix: handle missing skill parameter in SkillTool (#485) * fix: handle missing skill parameter in SkillTool * fix: preserve SkillTool schema contract * fix: align SkillTool schema error output --- src/services/tools/toolExecution.test.ts | 33 ++++++++++++++++++++++ src/services/tools/toolExecution.ts | 36 ++++++++++++++++++++++-- src/tools/SkillTool/SkillTool.test.ts | 31 ++++++++++++++++++++ src/tools/SkillTool/SkillTool.ts | 14 +++++++-- src/utils/api.test.ts | 14 +++++++++ 5 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 src/services/tools/toolExecution.test.ts create mode 100644 src/tools/SkillTool/SkillTool.test.ts diff --git a/src/services/tools/toolExecution.test.ts b/src/services/tools/toolExecution.test.ts new file mode 100644 index 00000000..60e1a8a1 --- /dev/null +++ b/src/services/tools/toolExecution.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, test } from 'bun:test' + +import { SkillTool } from '../../tools/SkillTool/SkillTool.js' +import { + getSchemaValidationErrorOverride, + getSchemaValidationToolUseResult, +} from './toolExecution.js' + +describe('getSchemaValidationErrorOverride', () => { + test('returns actionable missing-skill error for SkillTool', () => { + expect(getSchemaValidationErrorOverride(SkillTool, {})).toBe( + 'Missing skill name. Pass the slash command name as the skill parameter (e.g., skill: "commit" for /commit, skill: "review-pr" for /review-pr).', + ) + }) + + test('does not override unrelated tool schema failures', () => { + expect(getSchemaValidationErrorOverride({ name: 'Read' } as never, {})).toBe( + null, + ) + }) + + test('does not override SkillTool when skill is present', () => { + expect( + getSchemaValidationErrorOverride(SkillTool, { skill: 'commit' }), + ).toBe(null) + }) + + test('uses the actionable override for structured toolUseResult too', () => { + expect(getSchemaValidationToolUseResult(SkillTool, {} as never)).toBe( + 'InputValidationError: Missing skill name. Pass the slash command name as the skill parameter (e.g., skill: "commit" for /commit, skill: "review-pr" for /review-pr).', + ) + }) +}) diff --git a/src/services/tools/toolExecution.ts b/src/services/tools/toolExecution.ts index 40ef38ec..518f4623 100644 --- a/src/services/tools/toolExecution.ts +++ b/src/services/tools/toolExecution.ts @@ -43,6 +43,7 @@ import { FILE_READ_TOOL_NAME } from '../../tools/FileReadTool/prompt.js' import { FILE_WRITE_TOOL_NAME } from '../../tools/FileWriteTool/prompt.js' import { NOTEBOOK_EDIT_TOOL_NAME } from '../../tools/NotebookEditTool/constants.js' import { POWERSHELL_TOOL_NAME } from '../../tools/PowerShellTool/toolName.js' +import { SKILL_TOOL_NAME } from '../../tools/SkillTool/constants.js' import { parseGitCommitId } from '../../tools/shared/gitOperationTracking.js' import { isDeferredTool, @@ -596,6 +597,31 @@ export function buildSchemaNotSentHint( ) } +export function getSchemaValidationErrorOverride( + tool: Tool, + input: unknown, +): string | null { + if (tool.name !== SKILL_TOOL_NAME || !input || typeof input !== 'object') { + return null + } + + const skill = (input as { skill?: unknown }).skill + if (skill === undefined || skill === null) { + return 'Missing skill name. Pass the slash command name as the skill parameter (e.g., skill: "commit" for /commit, skill: "review-pr" for /review-pr).' + } + + return null +} + +export function getSchemaValidationToolUseResult( + tool: Tool, + input: unknown, + fallbackMessage?: string, +): string { + const override = getSchemaValidationErrorOverride(tool, input) + return `InputValidationError: ${override ?? fallbackMessage ?? ''}` +} + async function checkPermissionsAndCallTool( tool: Tool, toolUseID: string, @@ -614,7 +640,9 @@ async function checkPermissionsAndCallTool( // Validate input types with zod (surprisingly, the model is not great at generating valid input) const parsedInput = tool.inputSchema.safeParse(input) if (!parsedInput.success) { - let errorContent = formatZodValidationError(tool.name, parsedInput.error) + const fallbackErrorContent = formatZodValidationError(tool.name, parsedInput.error) + let errorContent = + getSchemaValidationErrorOverride(tool, input) ?? fallbackErrorContent const schemaHint = buildSchemaNotSentHint( tool, @@ -672,7 +700,11 @@ async function checkPermissionsAndCallTool( tool_use_id: toolUseID, }, ], - toolUseResult: `InputValidationError: ${parsedInput.error.message}`, + toolUseResult: getSchemaValidationToolUseResult( + tool, + input, + parsedInput.error.message, + ), sourceToolAssistantUUID: assistantMessage.uuid, }), }, diff --git a/src/tools/SkillTool/SkillTool.test.ts b/src/tools/SkillTool/SkillTool.test.ts new file mode 100644 index 00000000..dd09552a --- /dev/null +++ b/src/tools/SkillTool/SkillTool.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, test } from 'bun:test' + +import { SkillTool } from './SkillTool.js' + +describe('SkillTool missing parameter handling', () => { + test('missing skill stays required at the schema level', async () => { + const parsed = SkillTool.inputSchema.safeParse({}) + + expect(parsed.success).toBe(false) + }) + + test('validateInput still returns an actionable error when called with missing skill', async () => { + const result = await SkillTool.validateInput?.({} as never, { + options: { tools: [] }, + messages: [], + } as never) + + expect(result).toEqual({ + result: false, + message: + 'Missing skill name. Pass the slash command name as the skill parameter (e.g., skill: "commit" for /commit, skill: "review-pr" for /review-pr).', + errorCode: 1, + }) + }) + + test('valid skill input still parses and validates', async () => { + const parsed = SkillTool.inputSchema.safeParse({ skill: 'commit' }) + + expect(parsed.success).toBe(true) + }) +}) diff --git a/src/tools/SkillTool/SkillTool.ts b/src/tools/SkillTool/SkillTool.ts index befde568..519e957f 100644 --- a/src/tools/SkillTool/SkillTool.ts +++ b/src/tools/SkillTool/SkillTool.ts @@ -352,6 +352,16 @@ export const SkillTool: Tool = buildTool({ toAutoClassifierInput: ({ skill }) => skill ?? '', async validateInput({ skill }, context): Promise { + if (!skill || typeof skill !== 'string') { + return { + result: false, + message: + 'Missing skill name. Pass the slash command name as the skill parameter ' + + '(e.g., skill: "commit" for /commit, skill: "review-pr" for /review-pr).', + errorCode: 1, + } + } + // Skills are just skill names, no arguments const trimmed = skill.trim() if (!trimmed) { @@ -434,7 +444,7 @@ export const SkillTool: Tool = buildTool({ context, ): Promise { // Skills are just skill names, no arguments - const trimmed = skill.trim() + const trimmed = skill ?? '' // Remove leading slash if present (for compatibility) const commandName = trimmed.startsWith('/') ? trimmed.substring(1) : trimmed @@ -592,7 +602,7 @@ export const SkillTool: Tool = buildTool({ // - Skill is a prompt-based skill // Skills are just names, with optional arguments - const trimmed = skill.trim() + const trimmed = skill ?? '' // Remove leading slash if present (for compatibility) const commandName = trimmed.startsWith('/') ? trimmed.substring(1) : trimmed diff --git a/src/utils/api.test.ts b/src/utils/api.test.ts index 8c51142f..721746b0 100644 --- a/src/utils/api.test.ts +++ b/src/utils/api.test.ts @@ -1,6 +1,7 @@ import { expect, test } from 'bun:test' import { z } from 'zod/v4' import { getEmptyToolPermissionContext, type Tool, type Tools } from '../Tool.js' +import { SkillTool } from '../tools/SkillTool/SkillTool.js' import { toolToAPISchema } from './api.js' test('toolToAPISchema preserves provider-specific schema keywords in input_schema', async () => { @@ -64,3 +65,16 @@ test('toolToAPISchema preserves provider-specific schema keywords in input_schem }, }) }) + +test('toolToAPISchema keeps skill required for SkillTool', async () => { + const schema = await toolToAPISchema(SkillTool, { + getToolPermissionContext: async () => getEmptyToolPermissionContext(), + tools: [] as unknown as Tools, + agents: [], + }) + + expect((schema as { input_schema: unknown }).input_schema).toMatchObject({ + type: 'object', + required: ['skill'], + }) +})