mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
Fix strict tool schemas for nullable objects
This commit is contained in:
parent
95a2390744
commit
dd5670df12
6 changed files with 301 additions and 47 deletions
|
|
@ -176,6 +176,25 @@ describe("BaseProvider", () => {
|
|||
expect(result.additionalProperties).toBe(false)
|
||||
expect(result.required).toEqual([])
|
||||
})
|
||||
|
||||
it("should inject required for nullable object schemas (type: ['object','null'])", () => {
|
||||
const schema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
indentation: {
|
||||
type: ["object", "null"],
|
||||
properties: {
|
||||
anchorLine: { type: ["integer", "null"] },
|
||||
maxLevels: { type: ["integer", "null"] },
|
||||
},
|
||||
additionalProperties: false,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
const result = provider.testConvertToolSchemaForOpenAI(schema)
|
||||
expect(result.properties.indentation.required).toEqual(["anchorLine", "maxLevels"])
|
||||
})
|
||||
})
|
||||
|
||||
describe("convertToolsForOpenAI", () => {
|
||||
|
|
|
|||
|
|
@ -98,4 +98,75 @@ describe("OpenAiCodexHandler native tool calls", () => {
|
|||
name: "attempt_completion",
|
||||
})
|
||||
})
|
||||
|
||||
it("normalizes nullable object schemas for strict tools (read_file indentation.anchorLine regression)", async () => {
|
||||
vi.spyOn(openAiCodexOAuthManager, "getAccessToken").mockResolvedValue("test-token")
|
||||
vi.spyOn(openAiCodexOAuthManager, "getAccountId").mockResolvedValue("acct_test")
|
||||
|
||||
let capturedBody: any
|
||||
;(handler as any).client = {
|
||||
responses: {
|
||||
create: vi.fn().mockImplementation(async (body: any) => {
|
||||
capturedBody = body
|
||||
return {
|
||||
async *[Symbol.asyncIterator]() {
|
||||
yield {
|
||||
type: "response.done",
|
||||
response: {
|
||||
output: [{ type: "message", content: [{ type: "output_text", text: "ok" }] }],
|
||||
usage: { input_tokens: 1, output_tokens: 1 },
|
||||
},
|
||||
}
|
||||
},
|
||||
}
|
||||
}),
|
||||
},
|
||||
}
|
||||
|
||||
const readFileLikeTool = {
|
||||
type: "function" as const,
|
||||
function: {
|
||||
name: "read_file",
|
||||
description: "Read files",
|
||||
parameters: {
|
||||
type: "object",
|
||||
properties: {
|
||||
files: {
|
||||
type: "array",
|
||||
items: {
|
||||
type: "object",
|
||||
properties: {
|
||||
path: { type: "string" },
|
||||
indentation: {
|
||||
type: ["object", "null"],
|
||||
properties: {
|
||||
anchorLine: { type: ["integer", "null"] },
|
||||
maxLevels: { type: ["integer", "null"] },
|
||||
},
|
||||
additionalProperties: false,
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
const stream = handler.createMessage("system", [{ role: "user", content: "hello" } as any], {
|
||||
taskId: "t",
|
||||
toolProtocol: "native",
|
||||
tools: [readFileLikeTool as any],
|
||||
})
|
||||
for await (const _ of stream) {
|
||||
// consume
|
||||
}
|
||||
|
||||
const tool = capturedBody.tools?.[0]
|
||||
expect(tool).toBeDefined()
|
||||
expect(tool.strict).toBe(true)
|
||||
const indentation = tool.parameters.properties.files.items.properties.indentation
|
||||
// Critical: nullable-object schemas must still have required containing every key in properties.
|
||||
expect(indentation.required).toEqual(["anchorLine", "maxLevels"])
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -218,6 +218,83 @@ describe("OpenAiNativeHandler MCP tool schema handling", () => {
|
|||
expect(tool.parameters.required).toEqual(["path", "encoding"]) // Should have all properties as required
|
||||
})
|
||||
|
||||
it("should inject required for nullable object schemas (read_file indentation.anchorLine regression)", async () => {
|
||||
let capturedRequestBody: any
|
||||
|
||||
const handler = new OpenAiNativeHandler({
|
||||
openAiNativeApiKey: "test-key",
|
||||
apiModelId: "gpt-4o",
|
||||
} as ApiHandlerOptions)
|
||||
|
||||
// Mock the responses API call
|
||||
const mockClient = {
|
||||
responses: {
|
||||
create: vi.fn().mockImplementation((body: any) => {
|
||||
capturedRequestBody = body
|
||||
return {
|
||||
[Symbol.asyncIterator]: async function* () {
|
||||
yield {
|
||||
type: "response.done",
|
||||
response: {
|
||||
output: [{ type: "message", content: [{ type: "output_text", text: "test" }] }],
|
||||
usage: { input_tokens: 10, output_tokens: 5 },
|
||||
},
|
||||
}
|
||||
},
|
||||
}
|
||||
}),
|
||||
},
|
||||
}
|
||||
;(handler as any).client = mockClient
|
||||
|
||||
const tools: OpenAI.Chat.ChatCompletionTool[] = [
|
||||
{
|
||||
type: "function",
|
||||
function: {
|
||||
name: "read_file",
|
||||
description: "Read files",
|
||||
parameters: {
|
||||
type: "object",
|
||||
properties: {
|
||||
files: {
|
||||
type: "array",
|
||||
items: {
|
||||
type: "object",
|
||||
properties: {
|
||||
path: { type: "string" },
|
||||
indentation: {
|
||||
type: ["object", "null"],
|
||||
properties: {
|
||||
anchorLine: { type: ["integer", "null"] },
|
||||
maxLevels: { type: ["integer", "null"] },
|
||||
},
|
||||
additionalProperties: false,
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
]
|
||||
|
||||
const stream = handler.createMessage("system prompt", [], {
|
||||
taskId: "test-task-id",
|
||||
tools,
|
||||
toolProtocol: "native" as const,
|
||||
})
|
||||
|
||||
for await (const _ of stream) {
|
||||
// consume
|
||||
}
|
||||
|
||||
const tool = capturedRequestBody.tools[0]
|
||||
expect(tool.strict).toBe(true)
|
||||
const indentation = tool.parameters.properties.files.items.properties.indentation
|
||||
expect(indentation.required).toEqual(["anchorLine", "maxLevels"])
|
||||
})
|
||||
|
||||
it("should recursively add additionalProperties: false to nested objects in MCP tools", async () => {
|
||||
let capturedRequestBody: any
|
||||
|
||||
|
|
|
|||
|
|
@ -61,7 +61,17 @@ export abstract class BaseProvider implements ApiHandler {
|
|||
* This matches the behavior of ensureAllRequired in openai-native.ts
|
||||
*/
|
||||
protected convertToolSchemaForOpenAI(schema: any): any {
|
||||
if (!schema || typeof schema !== "object" || schema.type !== "object") {
|
||||
const isObjectLikeSchema = (candidate: any): boolean => {
|
||||
if (!candidate || typeof candidate !== "object") return false
|
||||
const type = candidate.type
|
||||
return (
|
||||
type === "object" ||
|
||||
(Array.isArray(type) && type.includes("object")) ||
|
||||
candidate.properties !== undefined
|
||||
)
|
||||
}
|
||||
|
||||
if (!isObjectLikeSchema(schema)) {
|
||||
return schema
|
||||
}
|
||||
|
||||
|
|
@ -89,19 +99,39 @@ export abstract class BaseProvider implements ApiHandler {
|
|||
prop.type = nonNullTypes.length === 1 ? nonNullTypes[0] : nonNullTypes
|
||||
}
|
||||
|
||||
// Recursively process nested objects
|
||||
if (prop && prop.type === "object") {
|
||||
// Recursively process nested objects (including nullable objects)
|
||||
if (isObjectLikeSchema(prop)) {
|
||||
newProps[key] = this.convertToolSchemaForOpenAI(prop)
|
||||
} else if (prop && prop.type === "array" && prop.items?.type === "object") {
|
||||
} else if (prop && prop.type === "array" && prop.items && isObjectLikeSchema(prop.items)) {
|
||||
newProps[key] = {
|
||||
...prop,
|
||||
items: this.convertToolSchemaForOpenAI(prop.items),
|
||||
items: Array.isArray(prop.items)
|
||||
? prop.items.map((i: any) => this.convertToolSchemaForOpenAI(i))
|
||||
: this.convertToolSchemaForOpenAI(prop.items),
|
||||
}
|
||||
}
|
||||
}
|
||||
result.properties = newProps
|
||||
}
|
||||
|
||||
// Also recurse through unions if present
|
||||
if (Array.isArray((result as any).anyOf)) {
|
||||
;(result as any).anyOf = (result as any).anyOf.map((s: any) => this.convertToolSchemaForOpenAI(s))
|
||||
}
|
||||
if (Array.isArray((result as any).oneOf)) {
|
||||
;(result as any).oneOf = (result as any).oneOf.map((s: any) => this.convertToolSchemaForOpenAI(s))
|
||||
}
|
||||
if (Array.isArray((result as any).allOf)) {
|
||||
;(result as any).allOf = (result as any).allOf.map((s: any) => this.convertToolSchemaForOpenAI(s))
|
||||
}
|
||||
|
||||
if ((result as any).type === "array" && (result as any).items) {
|
||||
const items = (result as any).items
|
||||
;(result as any).items = Array.isArray(items)
|
||||
? items.map((i: any) => this.convertToolSchemaForOpenAI(i))
|
||||
: this.convertToolSchemaForOpenAI(items)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -207,8 +207,31 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion
|
|||
reasoningEffort: ReasoningEffortExtended | undefined,
|
||||
metadata?: ApiHandlerCreateMessageMetadata,
|
||||
): any {
|
||||
const isObjectLikeSchema = (schema: any): boolean => {
|
||||
if (!schema || typeof schema !== "object") return false
|
||||
const type = schema.type
|
||||
return (
|
||||
type === "object" || (Array.isArray(type) && type.includes("object")) || schema.properties !== undefined
|
||||
)
|
||||
}
|
||||
|
||||
const ensureAllRequired = (schema: any): any => {
|
||||
if (!schema || typeof schema !== "object" || schema.type !== "object") {
|
||||
// Only object-like schemas need "required" injection for OpenAI strict schemas.
|
||||
if (!isObjectLikeSchema(schema)) {
|
||||
// Still recurse into anyOf/oneOf/allOf arrays if present.
|
||||
if (schema && typeof schema === "object") {
|
||||
const out = { ...schema }
|
||||
if (Array.isArray(out.anyOf)) out.anyOf = out.anyOf.map(ensureAllRequired)
|
||||
if (Array.isArray(out.oneOf)) out.oneOf = out.oneOf.map(ensureAllRequired)
|
||||
if (Array.isArray(out.allOf)) out.allOf = out.allOf.map(ensureAllRequired)
|
||||
// Recurse into array item schemas too.
|
||||
if (out.type === "array" && out.items) {
|
||||
out.items = Array.isArray(out.items)
|
||||
? out.items.map(ensureAllRequired)
|
||||
: ensureAllRequired(out.items)
|
||||
}
|
||||
return out
|
||||
}
|
||||
return schema
|
||||
}
|
||||
|
||||
|
|
@ -224,28 +247,34 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion
|
|||
const newProps = { ...result.properties }
|
||||
for (const key of allKeys) {
|
||||
const prop = newProps[key]
|
||||
if (prop.type === "object") {
|
||||
if (prop && typeof prop === "object") {
|
||||
// Recurse for nested object schemas, nullable object schemas (type: ["object","null"]),
|
||||
// union schemas (anyOf), and arrays-of-objects.
|
||||
newProps[key] = ensureAllRequired(prop)
|
||||
} else if (prop.type === "array" && prop.items?.type === "object") {
|
||||
newProps[key] = {
|
||||
...prop,
|
||||
items: ensureAllRequired(prop.items),
|
||||
}
|
||||
}
|
||||
}
|
||||
result.properties = newProps
|
||||
}
|
||||
|
||||
if (Array.isArray(result.anyOf)) result.anyOf = result.anyOf.map(ensureAllRequired)
|
||||
if (Array.isArray(result.oneOf)) result.oneOf = result.oneOf.map(ensureAllRequired)
|
||||
if (Array.isArray(result.allOf)) result.allOf = result.allOf.map(ensureAllRequired)
|
||||
if (result.type === "array" && result.items) {
|
||||
result.items = Array.isArray(result.items)
|
||||
? result.items.map(ensureAllRequired)
|
||||
: ensureAllRequired(result.items)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
const ensureAdditionalPropertiesFalse = (schema: any): any => {
|
||||
if (!schema || typeof schema !== "object" || schema.type !== "object") {
|
||||
if (!schema || typeof schema !== "object") {
|
||||
return schema
|
||||
}
|
||||
|
||||
const result = { ...schema }
|
||||
if (result.additionalProperties !== false) {
|
||||
if (isObjectLikeSchema(result) && result.additionalProperties !== false) {
|
||||
result.additionalProperties = false
|
||||
}
|
||||
|
||||
|
|
@ -253,18 +282,23 @@ export class OpenAiCodexHandler extends BaseProvider implements SingleCompletion
|
|||
const newProps = { ...result.properties }
|
||||
for (const key of Object.keys(result.properties)) {
|
||||
const prop = newProps[key]
|
||||
if (prop && prop.type === "object") {
|
||||
if (prop && typeof prop === "object") {
|
||||
newProps[key] = ensureAdditionalPropertiesFalse(prop)
|
||||
} else if (prop && prop.type === "array" && prop.items?.type === "object") {
|
||||
newProps[key] = {
|
||||
...prop,
|
||||
items: ensureAdditionalPropertiesFalse(prop.items),
|
||||
}
|
||||
}
|
||||
}
|
||||
result.properties = newProps
|
||||
}
|
||||
|
||||
if (Array.isArray(result.anyOf)) result.anyOf = result.anyOf.map(ensureAdditionalPropertiesFalse)
|
||||
if (Array.isArray(result.oneOf)) result.oneOf = result.oneOf.map(ensureAdditionalPropertiesFalse)
|
||||
if (Array.isArray(result.allOf)) result.allOf = result.allOf.map(ensureAdditionalPropertiesFalse)
|
||||
|
||||
if (result.type === "array" && result.items) {
|
||||
result.items = Array.isArray(result.items)
|
||||
? result.items.map(ensureAdditionalPropertiesFalse)
|
||||
: ensureAdditionalPropertiesFalse(result.items)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -216,17 +216,37 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
|
|||
reasoningEffort: ReasoningEffortExtended | undefined,
|
||||
metadata?: ApiHandlerCreateMessageMetadata,
|
||||
): any {
|
||||
// Ensure all properties are in the required array for OpenAI's strict mode
|
||||
// This recursively processes nested objects and array items
|
||||
const isObjectLikeSchema = (schema: any): boolean => {
|
||||
if (!schema || typeof schema !== "object") return false
|
||||
const type = schema.type
|
||||
return (
|
||||
type === "object" || (Array.isArray(type) && type.includes("object")) || schema.properties !== undefined
|
||||
)
|
||||
}
|
||||
|
||||
// Ensure all properties are in the required array for OpenAI's strict mode.
|
||||
// This recursively processes nested objects and array items.
|
||||
// IMPORTANT: must handle nullable objects (type: ["object","null"]).
|
||||
const ensureAllRequired = (schema: any): any => {
|
||||
if (!schema || typeof schema !== "object" || schema.type !== "object") {
|
||||
if (!isObjectLikeSchema(schema)) {
|
||||
if (schema && typeof schema === "object") {
|
||||
const out = { ...schema }
|
||||
if (Array.isArray(out.anyOf)) out.anyOf = out.anyOf.map(ensureAllRequired)
|
||||
if (Array.isArray(out.oneOf)) out.oneOf = out.oneOf.map(ensureAllRequired)
|
||||
if (Array.isArray(out.allOf)) out.allOf = out.allOf.map(ensureAllRequired)
|
||||
if (out.type === "array" && out.items) {
|
||||
out.items = Array.isArray(out.items)
|
||||
? out.items.map(ensureAllRequired)
|
||||
: ensureAllRequired(out.items)
|
||||
}
|
||||
return out
|
||||
}
|
||||
return schema
|
||||
}
|
||||
|
||||
const result = { ...schema }
|
||||
|
||||
// OpenAI Responses API requires additionalProperties: false on all object schemas
|
||||
// Only add if not already set to false (to avoid unnecessary mutations)
|
||||
// OpenAI Responses API requires additionalProperties: false on all object schemas.
|
||||
if (result.additionalProperties !== false) {
|
||||
result.additionalProperties = false
|
||||
}
|
||||
|
|
@ -235,58 +255,61 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
|
|||
const allKeys = Object.keys(result.properties)
|
||||
result.required = allKeys
|
||||
|
||||
// Recursively process nested objects
|
||||
const newProps = { ...result.properties }
|
||||
for (const key of allKeys) {
|
||||
const prop = newProps[key]
|
||||
if (prop.type === "object") {
|
||||
if (prop && typeof prop === "object") {
|
||||
newProps[key] = ensureAllRequired(prop)
|
||||
} else if (prop.type === "array" && prop.items?.type === "object") {
|
||||
newProps[key] = {
|
||||
...prop,
|
||||
items: ensureAllRequired(prop.items),
|
||||
}
|
||||
}
|
||||
}
|
||||
result.properties = newProps
|
||||
}
|
||||
|
||||
if (Array.isArray(result.anyOf)) result.anyOf = result.anyOf.map(ensureAllRequired)
|
||||
if (Array.isArray(result.oneOf)) result.oneOf = result.oneOf.map(ensureAllRequired)
|
||||
if (Array.isArray(result.allOf)) result.allOf = result.allOf.map(ensureAllRequired)
|
||||
if (result.type === "array" && result.items) {
|
||||
result.items = Array.isArray(result.items)
|
||||
? result.items.map(ensureAllRequired)
|
||||
: ensureAllRequired(result.items)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
// Adds additionalProperties: false to all object schemas recursively
|
||||
// without modifying required array. Used for MCP tools with strict: false
|
||||
// to comply with OpenAI Responses API requirements.
|
||||
// Adds additionalProperties: false to all object-like schemas recursively without modifying required.
|
||||
// Used for MCP tools with strict: false to comply with OpenAI Responses API requirements.
|
||||
const ensureAdditionalPropertiesFalse = (schema: any): any => {
|
||||
if (!schema || typeof schema !== "object" || schema.type !== "object") {
|
||||
if (!schema || typeof schema !== "object") {
|
||||
return schema
|
||||
}
|
||||
|
||||
const result = { ...schema }
|
||||
|
||||
// OpenAI Responses API requires additionalProperties: false on all object schemas
|
||||
// Only add if not already set to false (to avoid unnecessary mutations)
|
||||
if (result.additionalProperties !== false) {
|
||||
if (isObjectLikeSchema(result) && result.additionalProperties !== false) {
|
||||
result.additionalProperties = false
|
||||
}
|
||||
|
||||
if (result.properties) {
|
||||
// Recursively process nested objects
|
||||
const newProps = { ...result.properties }
|
||||
for (const key of Object.keys(result.properties)) {
|
||||
const prop = newProps[key]
|
||||
if (prop && prop.type === "object") {
|
||||
if (prop && typeof prop === "object") {
|
||||
newProps[key] = ensureAdditionalPropertiesFalse(prop)
|
||||
} else if (prop && prop.type === "array" && prop.items?.type === "object") {
|
||||
newProps[key] = {
|
||||
...prop,
|
||||
items: ensureAdditionalPropertiesFalse(prop.items),
|
||||
}
|
||||
}
|
||||
}
|
||||
result.properties = newProps
|
||||
}
|
||||
|
||||
if (Array.isArray(result.anyOf)) result.anyOf = result.anyOf.map(ensureAdditionalPropertiesFalse)
|
||||
if (Array.isArray(result.oneOf)) result.oneOf = result.oneOf.map(ensureAdditionalPropertiesFalse)
|
||||
if (Array.isArray(result.allOf)) result.allOf = result.allOf.map(ensureAdditionalPropertiesFalse)
|
||||
|
||||
if (result.type === "array" && result.items) {
|
||||
result.items = Array.isArray(result.items)
|
||||
? result.items.map(ensureAdditionalPropertiesFalse)
|
||||
: ensureAdditionalPropertiesFalse(result.items)
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue