Fix null params for parameterless tools#72673
Conversation
openclaw-barnacle
Bot
added
agents
Greptile SummaryThis PR fixes a bug where parameterless object-schema tools failed upstream validation when a model serialized an empty arguments object as Confidence Score: 4/5Safe to merge; the one edge case (required params inside allOf/oneOf) is unlikely in practice and is a P2 hardening concern. The fix is correct for the described bug and all standard object schemas. A single P2 concern exists around composite schemas (allOf/oneOf) where required params are nested rather than top-level — these would have their null args silently coerced to {} instead of failing validation. This is an uncommon schema pattern and does not affect the described use case. src/agents/pi-tools.schema.ts — specifically Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-tools.schema.ts
Line: 11-23
Comment:
**`required` inside `allOf`/`oneOf` subschemas is not detected**
`isObjectSchemaWithNoRequiredParams` only reads `record.required` at the top level of the schema object. A JSON-Schema like `{ type: "object", allOf: [{ required: ["q"] }] }` — which genuinely requires `q` — has no top-level `required` array, so the function returns `true` and a `prepareArguments` wrapper is installed. Any `null` call would be silently coerced to `{}`, bypassing the validation error that the caller would otherwise receive. The `normalizeToolParameterSchema` utility leaves top-level `allOf` schemas unchanged per the existing test suite, so the gap persists after normalization.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix tool null params for parameterless s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isObjectSchemaWithNoRequiredParams(schema: unknown): boolean { | ||
| if (!schema || typeof schema !== "object" || Array.isArray(schema)) { | ||
| return false; | ||
| } | ||
| const record = schema as Record<string, unknown>; | ||
| const type = record.type; | ||
| const hasObjectType = | ||
| type === "object" || (Array.isArray(type) && type.some((entry) => entry === "object")); | ||
| if (!hasObjectType) { | ||
| return false; | ||
| } | ||
| return !Array.isArray(record.required) || record.required.length === 0; | ||
| } |
There was a problem hiding this comment.
required inside allOf/oneOf subschemas is not detected
isObjectSchemaWithNoRequiredParams only reads record.required at the top level of the schema object. A JSON-Schema like { type: "object", allOf: [{ required: ["q"] }] } — which genuinely requires q — has no top-level required array, so the function returns true and a prepareArguments wrapper is installed. Any null call would be silently coerced to {}, bypassing the validation error that the caller would otherwise receive. The normalizeToolParameterSchema utility leaves top-level allOf schemas unchanged per the existing test suite, so the gap persists after normalization.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.schema.ts
Line: 11-23
Comment:
**`required` inside `allOf`/`oneOf` subschemas is not detected**
`isObjectSchemaWithNoRequiredParams` only reads `record.required` at the top level of the schema object. A JSON-Schema like `{ type: "object", allOf: [{ required: ["q"] }] }` — which genuinely requires `q` — has no top-level `required` array, so the function returns `true` and a `prepareArguments` wrapper is installed. Any `null` call would be silently coerced to `{}`, bypassing the validation error that the caller would otherwise receive. The `normalizeToolParameterSchema` utility leaves top-level `allOf` schemas unchanged per the existing test suite, so the gap persists after normalization.
How can I resolve this? If you propose a fix, please make it concise.
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Potential DoS via unbounded recursive traversal of untrusted JSON Schemas in schemaHasRequiredParams()
Description
In this codebase, tool
Vulnerable code: function schemaHasRequiredParams(schema: Record<string, unknown>): boolean {
if (Array.isArray(schema.required) && schema.required.length > 0) {
return true;
}
for (const key of ["allOf", "anyOf", "oneOf"]) {
const variants = schema[key];
if (!Array.isArray(variants)) {
continue;
}
if (
variants.some(
(variant) =>
variant !== null &&
typeof variant === "object" &&
!Array.isArray(variant) &&
schemaHasRequiredParams(variant as Record<string, unknown>),
)
) {
return true;
}
}
return false;
}This function is called during Because MCP tool schemas are not necessarily trusted, the recursion can be abused for denial-of-service. RecommendationAdd recursion guards when walking schemas:
Example (iterative + guards): function schemaHasRequiredParams(schema: Record<string, unknown>, opts?: { maxDepth?: number }) {
const maxDepth = opts?.maxDepth ?? 50;
const seen = new WeakSet<object>();
const stack: Array<{ node: Record<string, unknown>; depth: number }> = [{ node: schema, depth: 0 }];
while (stack.length) {
const { node, depth } = stack.pop()!;
if (depth > maxDepth) {
// Treat as "has required" (safe default) or bail out explicitly.
return true;
}
if (seen.has(node)) continue;
seen.add(node);
if (Array.isArray(node.required) && node.required.length > 0) return true;
for (const key of ["allOf", "anyOf", "oneOf"] as const) {
const variants = node[key];
if (!Array.isArray(variants)) continue;
for (const variant of variants) {
if (variant && typeof variant === "object" && !Array.isArray(variant)) {
stack.push({ node: variant as Record<string, unknown>, depth: depth + 1 });
}
}
}
}
return false;
}Also consider applying similar limits in Analyzed PR: #72673 at commit Last updated on: 2026-04-27T07:26:21Z |
amknight
changed the title
* fix tool null params for parameterless schemas * guard composite required tool schemas
* fix tool null params for parameterless schemas * guard composite required tool schemas
* fix tool null params for parameterless schemas * guard composite required tool schemas
* fix tool null params for parameterless schemas * guard composite required tool schemas
* fix tool null params for parameterless schemas * guard composite required tool schemas
* fix tool null params for parameterless schemas * guard composite required tool schemas
1 participant
Summary
nullor missing tool arguments to{}for root object schemas that have no required parameters.wiki_lint-style tool call whose arguments arenull.Root Cause
The upstream pi-agent loop validates tool arguments before invoking OpenClaw's wrapped
executehandler. When a model serialized an empty argument object asnull, parameterless object-schema tools failed validation before OpenClaw's loop/outcome tracking could observe execution. The existingprepareArgumentsseam runs before that validation step, so OpenClaw now uses it for schemas where{}is valid.Fixes #72587
Validation
pnpm test src/agents/pi-tools.schema.test.tspnpm exec oxfmt --check --threads=1 src/agents/pi-tools.schema.ts src/agents/pi-tools.schema.test.tspnpm check:changed