fix(agents): guard gemini schema properties against null#32332
fix(agents): guard gemini schema properties against null#32332steipete merged 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a defensive guard in Key points:
Confidence Score: 5/5
Last reviewed commit: df95058 |
| it("coerces non-object properties to an empty object", () => { | ||
| const cleaned = cleanSchemaForGemini({ | ||
| type: "object", | ||
| properties: "invalid", | ||
| }) as { properties?: unknown }; | ||
|
|
||
| expect(cleaned.properties).toEqual({}); | ||
| }); |
There was a problem hiding this comment.
Missing test for array-valued properties
The new !Array.isArray(value) branch in cleanSchemaForGeminiWithDefs means properties: [] (or any array) is now coerced to {}. This is the correct behaviour — previously the old typeof value === "object" check would have let an array through and tried to enumerate it with Object.fromEntries(Object.entries([])) — but there's no test covering it. Consider adding a case alongside the existing non-object test:
| it("coerces non-object properties to an empty object", () => { | |
| const cleaned = cleanSchemaForGemini({ | |
| type: "object", | |
| properties: "invalid", | |
| }) as { properties?: unknown }; | |
| expect(cleaned.properties).toEqual({}); | |
| }); | |
| it("coerces non-object properties to an empty object", () => { | |
| const cleaned = cleanSchemaForGemini({ | |
| type: "object", | |
| properties: "invalid", | |
| }) as { properties?: unknown }; | |
| expect(cleaned.properties).toEqual({}); | |
| }); | |
| it("coerces array properties to an empty object", () => { | |
| const cleaned = cleanSchemaForGemini({ | |
| type: "object", | |
| properties: [], | |
| }) as { properties?: unknown }; | |
| expect(cleaned.properties).toEqual({}); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/schema/clean-for-gemini.test.ts
Line: 15-22
Comment:
**Missing test for array-valued `properties`**
The new `!Array.isArray(value)` branch in `cleanSchemaForGeminiWithDefs` means `properties: []` (or any array) is now coerced to `{}`. This is the correct behaviour — previously the old `typeof value === "object"` check would have let an array through and tried to enumerate it with `Object.fromEntries(Object.entries([]))` — but there's no test covering it. Consider adding a case alongside the existing non-object test:
```suggestion
it("coerces non-object properties to an empty object", () => {
const cleaned = cleanSchemaForGemini({
type: "object",
properties: "invalid",
}) as { properties?: unknown };
expect(cleaned.properties).toEqual({});
});
it("coerces array properties to an empty object", () => {
const cleaned = cleanSchemaForGemini({
type: "object",
properties: [],
}) as { properties?: unknown };
expect(cleaned.properties).toEqual({});
});
```
How can I resolve this? If you propose a fix, please make it concise.df95058 to
95da268
Compare
|
Landed via temp rebase onto main.
✓ src/agents/schema/clean-for-gemini.test.ts (4 tests) 2ms Test Files 1 passed (1) Thanks @webdevtodayjason! |
Summary
Describe the problem and fix in 2–5 bullets:
propertiesisnull/non-object, which can lead to downstreamObject.*conversion failures in strict validators.cleanSchemaForGemininow coerces invalidpropertiesvalues to{}and keeps recursive schema cleaning behavior.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
propertiesvalues are normalized to{}instead of passing through.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
pnpm vitest run src/agents/schema/clean-for-gemini.test.ts src/agents/schema/clean-for-xai.test.ts.npx -y node@25.5.0 ./node_modules/vitest/vitest.mjs run src/agents/schema/clean-for-gemini.test.ts.Expected
propertiesrobustly.Actual
properties: nulland non-objectpropertiesare coerced to{}.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
properties: null, and non-objectpropertiesvalues.Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
fix(agents): guard gemini tool schema properties against null.src/agents/schema/clean-for-gemini.ts.propertieswas previously passed through.Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.propertiescould hide upstream bad schema authorship.