Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
|
|
||
| // Remove properties/required from non-object types (Gemini rejects these) | ||
| if (result.type && result.type !== "object") { | ||
| if (result.type && result.type !== "object" && !hasCombiner(result)) { |
There was a problem hiding this comment.
🟠 Native deferral misses properties/required stripping.
--- a/packages/cli/src/provider/transform.ts
+++ b/packages/cli/src/provider/transform.ts
@@ -1027 +1027 @@
- if (result.type && result.type !== "object" && !hasCombiner(result)) {
+ if (result.type && !Array.isArray(result.type) && result.type !== "object" && !hasCombiner(result)) {🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1027-1030):
Problem: Native deferral misses properties/required stripping
Detail: The new `native` flag only gates the type-array→anyOf split (line 998), but the "Remove properties/required from non-object types" guard at line 1027 still fires for native adapters because `result.type` remains an array and `["object","null"] !== "object"`. This deletes `properties`/`required` from nullable-object nodes that the deferral was supposed to leave untouched for @ai-sdk/google and @ai-sdk/google-vertex, contradicting the comment at lines 941-942 and silently stripping the tool parameter's shape. Repro: schema `{ type: ["object","null"], properties: { q: { type: "string" } }, required: ["q"] }` on a native Google/Vertex model — the split is skipped (native), but `result.type` (array) !== "object" evaluates truthy, so properties/required are deleted and the adapter receives an object schema with no properties. The new deferral tests only cover `{ type: ["string","null"] }`, which has no properties to lose.
Suggested fix: Skip the deletion when the type is still an array (only possible for native/combiner nodes after the split): `if (result.type && !Array.isArray(result.type) && result.type !== "object" && !hasCombiner(result))`. Add a test with `{ type: ["object","null"], properties: {...}, required: [...] }` on a native model asserting the schema is returned unchanged.
Suggested patch:
--- a/packages/cli/src/provider/transform.ts
+++ b/packages/cli/src/provider/transform.ts
@@ -1027 +1027 @@
- if (result.type && result.type !== "object" && !hasCombiner(result)) {
+ if (result.type && !Array.isArray(result.type) && result.type !== "object" && !hasCombiner(result)) {
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new native flag only gates the type-array→anyOf split (line 998), but the "Remove properties/required from non-object types" guard at line 1027 still fires for native adapters because result.type remains an array and ["object","null"] !== "object". This deletes properties/required from nullable-object nodes that the deferral was supposed to leave untouched for @ai-sdk/google and @ai-sdk/google-vertex, contradicting the comment at lines 941-942 and silently stripping the tool parameter's shape. Repro: schema { type: ["object","null"], properties: { q: { type: "string" } }, required: ["q"] } on a native Google/Vertex model — the split is skipped (native), but result.type (array) !== "object" evaluates truthy, so properties/required are deleted and the adapter receives an object schema with no properties. The new deferral tests only cover { type: ["string","null"] }, which has no properties to lose.
}
// Remove properties/required from non-object types (Gemini rejects these)
if (result.type && result.type !== "object" && !hasCombiner(result)) {
delete result.properties
delete result.required
}| if (types.length === 0) { | ||
| result.type = "null" | ||
| } else { | ||
| delete result.type |
There was a problem hiding this comment.
🟡 Split nodes escape properties/required stripping.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1004-1008):
Problem: Split nodes escape properties/required stripping
Detail: The type-array split deletes `result.type` (line 1004) before the hygiene steps run, so a node like `{ type: ["string","null"], properties: {...}, required: [...] }` ends with `type` deleted and `anyOf` set; the truthy-`result.type` guard at 1027 and the `result.type === "object"` guard at 1011 then never fire. Post-PR, `properties`/`required` survive on a non-object node, which the code's own comment at line 1026 says Gemini rejects — pre-PR they were stripped. Same exemption applies to single-entry arrays like `["object"]`/`["array"]` non-null-only, which now get wrapped in anyOf instead of collapsing to a scalar type, skipping the required-filter and items-defaulting steps. Repro: non-native Gemini transport (e.g. providerID "github-copilot", api.id containing "gemini") + property `{ type: ["object","null"], properties: { q: {} }, required: ["q"] }` → emitted node is `{ anyOf: [{type:"object"}], properties: {...}, required: [...], nullable: true }`; the properties/required sit beside anyOf instead of inside the object branch, so the param shape can be dropped or rejected by the Gemini-side schema translation.
Suggested fix: When `types.length === 1 && !nullable`, collapse to `result.type = types[0]` instead of building anyOf (preserves the object/array hygiene steps). For genuinely mixed nodes, move `properties`/`required` into the `{ type: "object" }` anyOf branch (or strip them when no branch is object-typed) so the emitted schema keeps a valid shape.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The type-array split deletes result.type (line 1004) before the hygiene steps run, so a node like { type: ["string","null"], properties: {...}, required: [...] } ends with type deleted and anyOf set; the truthy-result.type guard at 1027 and the result.type === "object" guard at 1011 then never fire. Post-PR, properties/required survive on a non-object node, which the code's own comment at line 1026 says Gemini rejects — pre-PR they were stripped. Same exemption applies to single-entry arrays like ["object"]/["array"] non-null-only, which now get wrapped in anyOf instead of collapsing to a scalar type, skipping the required-filter and items-defaulting steps. Repro: non-native Gemini transport (e.g. providerID "github-copilot", api.id containing "gemini") + property { type: ["object","null"], properties: { q: {} }, required: ["q"] } → emitted node is { anyOf: [{type:"object"}], properties: {...}, required: [...], nullable: true }; the properties/required sit beside anyOf instead of inside the object branch, so the param shape can be dropped or rejected by the Gemini-side schema translation.
const nullable = result.type.includes("null")
const types = result.type.filter((entry: unknown) => entry !== "null")
if (types.length === 0) {
result.type = "null"
} else {
delete result.type
result.anyOf = types.map((entry: unknown) => ({ type: entry }))
if (nullable) result.nullable = true
}| // Ensure items has at least a type if it has no schema keywords | ||
| // This handles nested arrays like { type: "array", items: { type: "array", items: {} } } | ||
| if (typeof result.items === "object" && !Array.isArray(result.items) && !result.items.type) { | ||
| if (isPlainObject(result.items) && !hasSchemaIntent(result.items)) { |
There was a problem hiding this comment.
🟡 Enum-only items no longer get a type.
Suggested change
| if (isPlainObject(result.items) && !hasSchemaIntent(result.items)) { | |
| Treat `enum`/`const` without `type` as still needing a type default: either drop `enum`/`const` from the hasSchemaIntent key list, or infer `items.type` from the enum values (all-string → "string", all-number → "number"). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1021-1022):
Problem: Enum-only items no longer get a type
Detail: The old check (`!result.items.type`) forced `type: "string"` on enum-only `items` nodes like `{ enum: ["a","b"] }`; the new `hasSchemaIntent` list includes `enum`, so such items now keep no `type` at all. Gemini function-declaration schemas expect enum fields to carry a STRING type, so transports that forward this schema to Gemini (the PR's exact target: github-copilot OpenAI-compatible) may drop the constraint or reject the declaration — a behavior regression from the previous forced type. Repro: non-native Gemini transport + `{ type: "array", items: { enum: ["red","green"] } }` → hasSchemaIntent sees `enum` and skips the default, emitting items with no `type`, where pre-PR the items became `{ enum: [...], type: "string" }`.
Suggested fix: Treat `enum`/`const` without `type` as still needing a type default: either drop `enum`/`const` from the hasSchemaIntent key list, or infer `items.type` from the enum values (all-string → "string", all-number → "number").
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The old check (!result.items.type) forced type: "string" on enum-only items nodes like { enum: ["a","b"] }; the new hasSchemaIntent list includes enum, so such items now keep no type at all. Gemini function-declaration schemas expect enum fields to carry a STRING type, so transports that forward this schema to Gemini (the PR's exact target: github-copilot OpenAI-compatible) may drop the constraint or reject the declaration — a behavior regression from the previous forced type. Repro: non-native Gemini transport + { type: "array", items: { enum: ["red","green"] } } → hasSchemaIntent sees enum and skips the default, emitting items with no type, where pre-PR the items became { enum: [...], type: "string" }.
if (result.type === "array" && !hasCombiner(result)) {
if (result.items == null) {
result.items = {}
}
// Ensure items has at least a type if it has no schema keywords
// This handles nested arrays like { type: "array", items: { type: "array", items: {} } }
if (isPlainObject(result.items) && !hasSchemaIntent(result.items)) {
result.items.type = "string"
}
}| typeof node === "object" && node !== null && !Array.isArray(node) | ||
| const hasCombiner = (node: unknown) => | ||
| isPlainObject(node) && (Array.isArray(node.anyOf) || Array.isArray(node.oneOf) || Array.isArray(node.allOf)) | ||
| const hasSchemaIntent = (node: unknown) => { |
There was a problem hiding this comment.
🟡 hasSchemaIntent omits constraint keywords.
Suggested change
| const hasSchemaIntent = (node: unknown) => { | |
| Either extend the key list with the constraint/format keywords (minimum, maximum, pattern, format, minLength, maxLength, minItems, maxItems, ...), or narrow the fallback to only add `type: "string"` when items has no keys at all, leaving constrained items untouched. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:948-967):
Problem: hasSchemaIntent omits constraint keywords
Detail: The comment at line 1019 says items gets a type only "if it has no schema keywords", but hasSchemaIntent's key list omits common constraint/format keywords (minimum, maximum, exclusiveMinimum/Maximum, pattern, format, minLength/MaxLength, minItems/maxItems, multipleOf). An items node like `{ minimum: 0 }` is still forced to `type: "string"`, producing an invalid mix of a numeric constraint with a string type — inconsistent with the helper's stated intent (though it matches the old `!result.items.type` behavior, so not a regression).
Suggested fix: Either extend the key list with the constraint/format keywords (minimum, maximum, pattern, format, minLength, maxLength, minItems, maxItems, ...), or narrow the fallback to only add `type: "string"` when items has no keys at all, leaving constrained items untouched.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The comment at line 1019 says items gets a type only "if it has no schema keywords", but hasSchemaIntent's key list omits common constraint/format keywords (minimum, maximum, exclusiveMinimum/Maximum, pattern, format, minLength/MaxLength, minItems/maxItems, multipleOf). An items node like { minimum: 0 } is still forced to type: "string", producing an invalid mix of a numeric constraint with a string type — inconsistent with the helper's stated intent (though it matches the old !result.items.type behavior, so not a regression).
const hasSchemaIntent = (node: unknown) => {
if (!isPlainObject(node)) return false
if (hasCombiner(node)) return true
return [
"type",
"properties",
"items",
"prefixItems",
"enum",
"const",
"$ref",
"additionalProperties",
"patternProperties",
"required",
"not",
"if",
"then",
"else",
].some((key) => key in node)
}| if (Array.isArray(result.type) && !hasCombiner(result) && !native) { | ||
| const nullable = result.type.includes("null") | ||
| const types = result.type.filter((entry: unknown) => entry !== "null") | ||
| if (types.length === 0) { |
There was a problem hiding this comment.
🟡 Empty type array collapses to type:"null".
Suggested change
| if (types.length === 0) { | |
| Only collapse to `result.type = "null"` when `nullable` is true (the array actually contained "null"); for an empty type array `delete result.type` so the node stays unconstrained. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1001-1002):
Problem: Empty type array collapses to type:"null"
Detail: When `type: []` (an empty array, no "null" entry) reaches the split, `types.length === 0` and `nullable` is false, yet the code sets `result.type = "null"` — silently converting an unconstrained (or invalid-but-tolerated) schema into "must be null". Downstream consumers will then validate every value as null instead of accepting anything. Only collapse to `"null"` when the array actually contained "null".
Suggested fix: Only collapse to `result.type = "null"` when `nullable` is true (the array actually contained "null"); for an empty type array `delete result.type` so the node stays unconstrained.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
When type: [] (an empty array, no "null" entry) reaches the split, types.length === 0 and nullable is false, yet the code sets result.type = "null" — silently converting an unconstrained (or invalid-but-tolerated) schema into "must be null". Downstream consumers will then validate every value as null instead of accepting anything. Only collapse to "null" when the array actually contained "null".
const nullable = result.type.includes("null")
const types = result.type.filter((entry: unknown) => entry !== "null")
if (types.length === 0) {
result.type = "null"
} else {
delete result.type
result.anyOf = types.map((entry: unknown) => ({ type: entry }))
if (nullable) result.nullable = true
}
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 4 · ⚪ 0 · 0/5 resolved
🤖 Fix all 5 open findings with your agent📋 Out-of-diff findings (5)
Reviewed 3 files · 0 inline · view all 5 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
Review response — PR #119Verified all five schema-transform findings, reproduced 15 failing regression cases before the fix, and addressed them on the existing branch. Issues addressed (pushed to this PR)
Validation: 155 focused transform/session tests passed, including an actual pinned Google adapter request capture. CLI typecheck, Prettier, and diff checks passed. Pushed normally without verification bypass flags. Fixtures validate request construction; no live-model acceptance claim is made. Review claims verified false (no change needed)None. Not addressed hereNone of the five findings are deferred. Data-layer verdict persistence is unavailable in this session: matched 0, written 0, verified 0, failed 0, unrecorded 5. |
| const nullable = types.includes("null") | ||
| const nonNull = types.filter((entry: unknown) => entry !== "null") | ||
| if (nonNull.length === 0) { | ||
| result.type = "null" |
There was a problem hiding this comment.
🟡 Null-only type array emits bare type:"null".
Suggested change
| result.type = "null" | |
| Emit `result.nullable = true` and delete `result.type` (leaving an unconstrained nullable schema) instead of `result.type = "null"`; update the `["null"]` test expectation accordingly. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1030):
Problem: Null-only type array emits bare type:"null"
Detail: When a type array contains only "null" (e.g. `type: ["null"]`), the non-native split sets `result.type = "null"`. Gemini's Schema.type enum has no NULL value — nullability is expressed via `nullable`, which is exactly what every other branch in this same block does (anyOf + `nullable: true`). On the non-native transports this PR targets precisely because their translation layer does not normalize, a bare `{type:"null"}` node is invalid on the wire (or coerced to TYPE_UNSPECIFIED). Not a regression (pre-PR sent `["null"]` raw, equally invalid), but the rewrite should match the convention it establishes everywhere else. The "collapses a null-only type array" test pins the current shape.
Suggested fix: Emit `result.nullable = true` and delete `result.type` (leaving an unconstrained nullable schema) instead of `result.type = "null"`; update the `["null"]` test expectation accordingly.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
When a type array contains only "null" (e.g. type: ["null"]), the non-native split sets result.type = "null". Gemini's Schema.type enum has no NULL value — nullability is expressed via nullable, which is exactly what every other branch in this same block does (anyOf + nullable: true). On the non-native transports this PR targets precisely because their translation layer does not normalize, a bare {type:"null"} node is invalid on the wire (or coerced to TYPE_UNSPECIFIED). Not a regression (pre-PR sent ["null"] raw, equally invalid), but the rewrite should match the convention it establishes everywhere else. The "collapses a null-only type array" test pins the current shape.
if (Array.isArray(result.type) && !composed && !native) {
const nullable = types.includes("null")
const nonNull = types.filter((entry: unknown) => entry !== "null")
if (nonNull.length === 0) {
result.type = "null"
} else if (nonNull.length === 1 && !nullable) {
result.type = nonNull[0]
} else {| // Apply type-specific cleanup before splitting; afterwards type no longer | ||
| // identifies whether the union contains object or array members. | ||
| // Native adapters own this conversion to preserve their nullability rules. | ||
| if (Array.isArray(result.type) && !composed && !native) { |
There was a problem hiding this comment.
🟡 Combiner nodes keep raw type arrays on non-native.
Suggested change
| if (Array.isArray(result.type) && !composed && !native) { | |
| When `!native && composed && Array.isArray(result.type)`, consider dropping the redundant type array (expressing nullability via `nullable: true`) instead of passing it through, since the existing combiner already carries the union. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1026):
Problem: Combiner nodes keep raw type arrays on non-native
Detail: Nodes carrying both a type array and a pre-existing anyOf/oneOf/allOf skip the split (`!composed` at line 1026) and all hygiene steps (`!composed` at 997/1001/1018) on non-native transports, so e.g. `{type:["object","null"], anyOf:[...]}` still reaches the Gemini-side translation with a raw type array — the exact shape this PR exists to eliminate for non-native transports. The "preserves a pre-existing %s on a type-array node" test pins the passthrough for github-copilot. Skipping is a deliberate tradeoff (splitting would create competing nested combiners), but it leaves a known-bad wire shape unhandled on the non-native path.
Suggested fix: When `!native && composed && Array.isArray(result.type)`, consider dropping the redundant type array (expressing nullability via `nullable: true`) instead of passing it through, since the existing combiner already carries the union.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Nodes carrying both a type array and a pre-existing anyOf/oneOf/allOf skip the split (!composed at line 1026) and all hygiene steps (!composed at 997/1001/1018) on non-native transports, so e.g. {type:["object","null"], anyOf:[...]} still reaches the Gemini-side translation with a raw type array — the exact shape this PR exists to eliminate for non-native transports. The "preserves a pre-existing %s on a type-array node" test pins the passthrough for github-copilot. Skipping is a deliberate tradeoff (splitting would create competing nested combiners), but it leaves a known-bad wire shape unhandled on the non-native path.
// Apply type-specific cleanup before splitting; afterwards type no longer
// identifies whether the union contains object or array members.
// Native adapters own this conversion to preserve their nullability rules.
if (Array.isArray(result.type) && !composed && !native) {
const nullable = types.includes("null")
const nonNull = types.filter((entry: unknown) => entry !== "null")| } | ||
| } | ||
|
|
||
| if (Array.isArray(result.type) && result.type.length === 0) { |
There was a problem hiding this comment.
🟡 Empty type array throws even for native adapters.
Suggested change
| if (Array.isArray(result.type) && result.type.length === 0) { | |
| Either skip the throw when `native` (let the pinned adapter decide), or coerce `type: []` to an unconstrained node (`delete result.type`) instead of throwing; keep the loud failure only for non-native transports if desired. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:990-992):
Problem: Empty type array throws even for native adapters
Detail: The hard throw on `type: []` fires for every Gemini path, including native @ai-sdk/google / google-vertex models that this same PR otherwise defers to — the `!native` guard protects only the split (1026), not the throw (990). One malformed nested property anywhere in any tool schema now makes ProviderTransform.schema throw and kills the whole request for native google/vertex users, where pre-PR the schema passed through to the adapter. The test ("rejects an empty type array before %s converts it") shows this fail-fast is deliberate, but it converts a local, single-tool schema defect into a total request failure even on paths the PR explicitly leaves to the native adapter.
Suggested fix: Either skip the throw when `native` (let the pinned adapter decide), or coerce `type: []` to an unconstrained node (`delete result.type`) instead of throwing; keep the loud failure only for non-native transports if desired.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The hard throw on type: [] fires for every Gemini path, including native @ai-sdk/google / google-vertex models that this same PR otherwise defers to — the !native guard protects only the split (1026), not the throw (990). One malformed nested property anywhere in any tool schema now makes ProviderTransform.schema throw and kills the whole request for native google/vertex users, where pre-PR the schema passed through to the adapter. The test ("rejects an empty type array before %s converts it") shows this fail-fast is deliberate, but it converts a local, single-tool schema defect into a total request failure even on paths the PR explicitly leaves to the native adapter.
if (Array.isArray(result.type) && result.type.length === 0) {
throw new Error("Gemini tool schema contains an empty type array")
}
const types = Array.isArray(result.type) ? result.type : result.type ? [result.type] : []
const composed = hasCombiner(result)| ...(entry === "array" && result.items !== undefined ? { items: result.items } : {}), | ||
| })) | ||
| // Member schemas belong to their typed branch, not beside anyOf. | ||
| delete result.properties |
There was a problem hiding this comment.
🟡 Split mutates caller's schema object in place.
Suggested change
| delete result.properties | |
| Deep-clone at sanitizeGemini entry (structuredClone or rebuild objects during recursion) so ProviderTransform.schema is pure; at minimum clone nodes before entering the split branch. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1042-1044):
Problem: Split mutates caller's schema object in place
Detail: sanitizeGemini rewrites nodes in place (pre-existing pattern: `result.items.type = "string"`, `delete result.properties`), but the new split is far more destructive: it deletes properties/required/items and installs anyOf/nullable on the caller's schema object. If a tool's inputSchema object is registered once and reused across requests or models (the new tests build fresh literals, so they won't catch it), the first Gemini request corrupts the shared schema and a subsequent non-Gemini model receives the Gemini-mangled anyOf/nullable shape. No clone is visible in the diff; if the function's opening only shallow-copies, nested nodes (items, properties) are still shared.
Suggested fix: Deep-clone at sanitizeGemini entry (structuredClone or rebuild objects during recursion) so ProviderTransform.schema is pure; at minimum clone nodes before entering the split branch.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
sanitizeGemini rewrites nodes in place (pre-existing pattern: result.items.type = "string", delete result.properties), but the new split is far more destructive: it deletes properties/required/items and installs anyOf/nullable on the caller's schema object. If a tool's inputSchema object is registered once and reused across requests or models (the new tests build fresh literals, so they won't catch it), the first Gemini request corrupts the shared schema and a subsequent non-Gemini model receives the Gemini-mangled anyOf/nullable shape. No clone is visible in the diff; if the function's opening only shallow-copies, nested nodes (items, properties) are still shared.
delete result.type
result.anyOf = nonNull.map((entry: unknown) => ({
type: entry,
...(entry === "object" && result.properties !== undefined ? { properties: result.properties } : {}),
...(entry === "object" && result.required !== undefined ? { required: result.required } : {}),
...(entry === "array" && result.items !== undefined ? { items: result.items } : {}),
}))
// Member schemas belong to their typed branch, not beside anyOf.
delete result.properties
delete result.required
delete result.items| result.type = nonNull[0] | ||
| } else { | ||
| delete result.type | ||
| result.anyOf = nonNull.map((entry: unknown) => ({ |
There was a problem hiding this comment.
🟡 Split strands non-copied constraints beside anyOf.
Suggested change
| result.anyOf = nonNull.map((entry: unknown) => ({ | |
| Extend the branch copy to carry the relevant constraint families (additionalProperties/minProperties/propertyNames → object branch; minItems/maxItems/uniqueItems → array branch), or document the deliberate drop next to the comment. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1035-1039):
Problem: Split strands non-copied constraints beside anyOf
Detail: The split copies only properties/required (object branch) and items (array branch) off the parent. Other constraints stay beside anyOf on the now-typeless parent — additionalProperties, patternProperties, propertyNames, minProperties, minItems/maxItems/uniqueItems, parent-side enum — contradicting the new comment "Member schemas belong to their typed branch, not beside anyOf". Gemini's Schema has no field for the object-side keywords (silently dropped, so the constraint is expressed nowhere), while array-side ones (minItems etc.) are representable on the typed branch the split just created. E.g. `{type:["object","null"], additionalProperties:false, minProperties:1}` ends as `{anyOf:[{type:"object",...}], nullable:true, additionalProperties:false, minProperties:1}` with the constraints stranded.
Suggested fix: Extend the branch copy to carry the relevant constraint families (additionalProperties/minProperties/propertyNames → object branch; minItems/maxItems/uniqueItems → array branch), or document the deliberate drop next to the comment.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The split copies only properties/required (object branch) and items (array branch) off the parent. Other constraints stay beside anyOf on the now-typeless parent — additionalProperties, patternProperties, propertyNames, minProperties, minItems/maxItems/uniqueItems, parent-side enum — contradicting the new comment "Member schemas belong to their typed branch, not beside anyOf". Gemini's Schema has no field for the object-side keywords (silently dropped, so the constraint is expressed nowhere), while array-side ones (minItems etc.) are representable on the typed branch the split just created. E.g. {type:["object","null"], additionalProperties:false, minProperties:1} ends as {anyOf:[{type:"object",...}], nullable:true, additionalProperties:false, minProperties:1} with the constraints stranded.
delete result.type
result.anyOf = nonNull.map((entry: unknown) => ({
type: entry,
...(entry === "object" && result.properties !== undefined ? { properties: result.properties } : {}),
...(entry === "object" && result.required !== undefined ? { required: result.required } : {}),
...(entry === "array" && result.items !== undefined ? { items: result.items } : {}),
}))
// Member schemas belong to their typed branch, not beside anyOf.| isPlainObject(node) && (Array.isArray(node.anyOf) || Array.isArray(node.oneOf) || Array.isArray(node.allOf)) | ||
| // Default only unconstrained/annotation-only items. An allowlist of | ||
| // constraint keywords would miss extensions and silently change intent. | ||
| const annotations = new Set([ |
There was a problem hiding this comment.
⚪ annotations Set rebuilt on every schema() call.
Suggested change
| const annotations = new Set([ | |
| Move `const annotations = new Set([...])` (and optionally isPlainObject/hasCombiner) out of the `if` block to module scope near the top of the namespace; keep only `native` and `sanitizeGemini` inside the guard. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:950-963):
Problem: annotations Set rebuilt on every schema() call
Detail: The `annotations` Set (and the `isPlainObject`/`hasCombiner` helpers) are pure constants but are declared inside the `if (model.providerID === "google" || ...)` guard, so they are re-allocated on every ProviderTransform.schema call for Gemini models. Hoisting them to module/namespace scope avoids the repeated allocation on the hot path.
Suggested fix: Move `const annotations = new Set([...])` (and optionally isPlainObject/hasCombiner) out of the `if` block to module scope near the top of the namespace; keep only `native` and `sanitizeGemini` inside the guard.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The annotations Set (and the isPlainObject/hasCombiner helpers) are pure constants but are declared inside the if (model.providerID === "google" || ...) guard, so they are re-allocated on every ProviderTransform.schema call for Gemini models. Hoisting them to module/namespace scope avoids the repeated allocation on the hot path.
// Default only unconstrained/annotation-only items. An allowlist of
// constraint keywords would miss extensions and silently change intent.
const annotations = new Set([
"$schema",
"$id",| if ( | ||
| isPlainObject(result.items) && | ||
| Object.keys(result.items).every( | ||
| (key) => annotations.has(key) || (key === "enum" && Array.isArray(result.items.enum)), |
There was a problem hiding this comment.
⚪ Non-array enum leaves items without any type.
Suggested change
| (key) => annotations.has(key) || (key === "enum" && Array.isArray(result.items.enum)), | |
| Handle the non-array `enum` case explicitly (drop it, or treat the node as unconstrained and apply the string default) rather than letting it silently fall through the predicate. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/src/provider/transform.ts:1008-1013):
Problem: Non-array enum leaves items without any type
Detail: The items-default predicate treats `enum` as annotation-like only when it is array-valued; a non-array `enum` (invalid JSON Schema, but reachable from hand-written jsonSchema() tool input) makes every() false and leaves the items node with no type at all, where pre-PR it received type:"string". Constraint-only items staying type-less is deliberate and test-pinned, but this particular edge silently produces a type-less leaf from near-invalid input instead of normalizing or rejecting it.
Suggested fix: Handle the non-array `enum` case explicitly (drop it, or treat the node as unconstrained and apply the string default) rather than letting it silently fall through the predicate.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The items-default predicate treats enum as annotation-like only when it is array-valued; a non-array enum (invalid JSON Schema, but reachable from hand-written jsonSchema() tool input) makes every() false and leaves the items node with no type at all, where pre-PR it received type:"string". Constraint-only items staying type-less is deliberate and test-pinned, but this particular edge silently produces a type-less leaf from near-invalid input instead of normalizing or rejecting it.
if (
isPlainObject(result.items) &&
Object.keys(result.items).every(
(key) => annotations.has(key) || (key === "enum" && Array.isArray(result.items.enum)),
)
) {|
|
||
| test("strips object-only fields before splitting primitive nullable types", () => { | ||
| const schema = { type: ["string", "null"], properties: { unused: { type: "string" } }, required: ["unused"] } as any | ||
| expect(ProviderTransform.schema(geminiModel, schema) as unknown).toEqual({ |
There was a problem hiding this comment.
⚪ Mixed as unknown/as any casts in new tests.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #119, packages/cli/test/provider/transform.test.ts:586):
Problem: Mixed as unknown/as any casts in new tests
Detail: Four assertions in the new describe block cast the result with `as unknown` before toEqual (lines 586, 598, 612, 635), while sibling tests in the same block and the rest of the file use `as any`. The `as unknown` cast is a no-op for toEqual and breaks the file's prevailing convention.
Suggested fix: Drop the cast entirely or use `as any` to match the surrounding tests.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Four assertions in the new describe block cast the result with as unknown before toEqual (lines 586, 598, 612, 635), while sibling tests in the same block and the rest of the file use as any. The as unknown cast is a no-op for toEqual and breaks the file's prevailing convention.
test("strips object-only fields before splitting primitive nullable types", () => {
const schema = { type: ["string", "null"], properties: { unused: { type: "string" } }, required: ["unused"] } as any
expect(ProviderTransform.schema(geminiModel, schema) as unknown).toEqual({
anyOf: [{ type: "string" }],
nullable: true,
})
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 5 · ⚪ 3 · 0/8 resolved
🤖 Fix all 8 open findings with your agent📋 Out-of-diff findings (8)
Reviewed 3 files · 0 inline · view all 8 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
Review response — PR #119Verified the eight findings from the review of Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
Validation: 161 tests passed, 0 failed, 303 assertions across provider transform and session LLM tests; CLI typecheck, Prettier, and MCP verdict persistence is unavailable: matched 0, written 0, verified 0, failed 0, unrecorded 8. The GitHub sidecar records all eight reviewed findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #111
Intent
Some non-native Gemini transports reject JSON Schema type arrays even though native Google adapters handle nullable unions correctly. The transform needs to normalize only where the transport requires it.
Expected Impact on Users
Gemini requests through OpenAI-compatible or other non-native transports are less likely to be rejected for nullable tool schemas, while native Google and Vertex requests retain their adapter-owned semantics.
Expected Outcomes
anyOf/nullableschemas.anyOf,oneOf, andallOfcombiners are preserved.Implementation
Scope Caveat
This is a malformed-tool-call prevention measure, not evidence of recovery effectiveness and not an automatic retry policy.
Test Plan
Verification
Risks and Rollout
The change is limited to schema transformation. Native adapter behavior remains the source of truth for Google and Vertex nullable handling.