fix: preserve SkillTool schema contract
This commit is contained in:
24
src/services/tools/toolExecution.test.ts
Normal file
24
src/services/tools/toolExecution.test.ts
Normal file
@@ -0,0 +1,24 @@
|
|||||||
|
import { describe, expect, test } from 'bun:test'
|
||||||
|
|
||||||
|
import { SkillTool } from '../../tools/SkillTool/SkillTool.js'
|
||||||
|
import { getSchemaValidationErrorOverride } 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)
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -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 { FILE_WRITE_TOOL_NAME } from '../../tools/FileWriteTool/prompt.js'
|
||||||
import { NOTEBOOK_EDIT_TOOL_NAME } from '../../tools/NotebookEditTool/constants.js'
|
import { NOTEBOOK_EDIT_TOOL_NAME } from '../../tools/NotebookEditTool/constants.js'
|
||||||
import { POWERSHELL_TOOL_NAME } from '../../tools/PowerShellTool/toolName.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 { parseGitCommitId } from '../../tools/shared/gitOperationTracking.js'
|
||||||
import {
|
import {
|
||||||
isDeferredTool,
|
isDeferredTool,
|
||||||
@@ -596,6 +597,22 @@ 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
|
||||||
|
}
|
||||||
|
|
||||||
async function checkPermissionsAndCallTool(
|
async function checkPermissionsAndCallTool(
|
||||||
tool: Tool,
|
tool: Tool,
|
||||||
toolUseID: string,
|
toolUseID: string,
|
||||||
@@ -614,7 +631,9 @@ async function checkPermissionsAndCallTool(
|
|||||||
// Validate input types with zod (surprisingly, the model is not great at generating valid input)
|
// Validate input types with zod (surprisingly, the model is not great at generating valid input)
|
||||||
const parsedInput = tool.inputSchema.safeParse(input)
|
const parsedInput = tool.inputSchema.safeParse(input)
|
||||||
if (!parsedInput.success) {
|
if (!parsedInput.success) {
|
||||||
let errorContent = formatZodValidationError(tool.name, parsedInput.error)
|
let errorContent =
|
||||||
|
getSchemaValidationErrorOverride(tool, input) ??
|
||||||
|
formatZodValidationError(tool.name, parsedInput.error)
|
||||||
|
|
||||||
const schemaHint = buildSchemaNotSentHint(
|
const schemaHint = buildSchemaNotSentHint(
|
||||||
tool,
|
tool,
|
||||||
|
|||||||
@@ -3,15 +3,14 @@ import { describe, expect, test } from 'bun:test'
|
|||||||
import { SkillTool } from './SkillTool.js'
|
import { SkillTool } from './SkillTool.js'
|
||||||
|
|
||||||
describe('SkillTool missing parameter handling', () => {
|
describe('SkillTool missing parameter handling', () => {
|
||||||
test('missing skill reaches validateInput with an actionable error', async () => {
|
test('missing skill stays required at the schema level', async () => {
|
||||||
const parsed = SkillTool.inputSchema.safeParse({})
|
const parsed = SkillTool.inputSchema.safeParse({})
|
||||||
|
|
||||||
expect(parsed.success).toBe(true)
|
expect(parsed.success).toBe(false)
|
||||||
if (!parsed.success) {
|
})
|
||||||
throw new Error('expected SkillTool schema to allow missing skill for custom validation')
|
|
||||||
}
|
|
||||||
|
|
||||||
const result = await SkillTool.validateInput?.(parsed.data as never, {
|
test('validateInput still returns an actionable error when called with missing skill', async () => {
|
||||||
|
const result = await SkillTool.validateInput?.({} as never, {
|
||||||
options: { tools: [] },
|
options: { tools: [] },
|
||||||
messages: [],
|
messages: [],
|
||||||
} as never)
|
} as never)
|
||||||
|
|||||||
@@ -292,7 +292,6 @@ export const inputSchema = lazySchema(() =>
|
|||||||
z.object({
|
z.object({
|
||||||
skill: z
|
skill: z
|
||||||
.string()
|
.string()
|
||||||
.optional()
|
|
||||||
.describe('The skill name. E.g., "commit", "review-pr", or "pdf"'),
|
.describe('The skill name. E.g., "commit", "review-pr", or "pdf"'),
|
||||||
args: z.string().optional().describe('Optional arguments for the skill'),
|
args: z.string().optional().describe('Optional arguments for the skill'),
|
||||||
}),
|
}),
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import { expect, test } from 'bun:test'
|
import { expect, test } from 'bun:test'
|
||||||
import { z } from 'zod/v4'
|
import { z } from 'zod/v4'
|
||||||
import { getEmptyToolPermissionContext, type Tool, type Tools } from '../Tool.js'
|
import { getEmptyToolPermissionContext, type Tool, type Tools } from '../Tool.js'
|
||||||
|
import { SkillTool } from '../tools/SkillTool/SkillTool.js'
|
||||||
import { toolToAPISchema } from './api.js'
|
import { toolToAPISchema } from './api.js'
|
||||||
|
|
||||||
test('toolToAPISchema preserves provider-specific schema keywords in input_schema', async () => {
|
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'],
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user