fix(cron): handle wrapped and unwrapped job objects in cron.add and cron tool#7230
fix(cron): handle wrapped and unwrapped job objects in cron.add and cron tool#7230hongxuWei wants to merge 6 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
gateway
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "add": { | ||
| if (!params.job || typeof params.job !== "object") { | ||
| throw new Error("job required"); | ||
| } | ||
| const job = normalizeCronJobCreate(params.job) ?? params.job; | ||
| const rawJob = (params.job ?? params.data ?? params) as Record<string, unknown>; | ||
| const job = normalizeCronJobCreate(rawJob) ?? rawJob; | ||
| if (job && typeof job === "object" && !("agentId" in job)) { |
There was a problem hiding this comment.
[P1] cron tool add can silently accept invalid/non-job inputs and forward them to the gateway.
rawJob falls back to params (which always includes tool-level keys like action, gatewayUrl, etc.), and it’s cast to Record<string, unknown> even when it’s a primitive. If the caller forgets to pass job/data or passes a non-object, this no longer errors early (it used to throw job required) and can produce confusing gateway validation errors or (worse) accidentally treat tool metadata as part of the job.
This shows up when args are like { action: "add" } or { action: "add", data: "oops" }. Consider restoring an explicit “job required and must be object” guard for job/data cases before normalizing/calling the gateway.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tools/cron-tool.ts
Line: 223:226
Comment:
[P1] `cron` tool `add` can silently accept invalid/non-job inputs and forward them to the gateway.
`rawJob` falls back to `params` (which always includes tool-level keys like `action`, `gatewayUrl`, etc.), and it’s cast to `Record<string, unknown>` even when it’s a primitive. If the caller forgets to pass `job`/`data` or passes a non-object, this no longer errors early (it used to throw `job required`) and can produce confusing gateway validation errors or (worse) accidentally treat tool metadata as part of the job.
This shows up when `args` are like `{ action: "add" }` or `{ action: "add", data: "oops" }`. Consider restoring an explicit “job required and must be object” guard for `job`/`data` cases before normalizing/calling the gateway.
How can I resolve this? If you propose a fix, please make it concise.
src/gateway/server-methods/cron.ts
Outdated
| "cron.add": async ({ params, respond, context }) => { | ||
| const normalized = normalizeCronJobCreate(params) ?? params; | ||
| const jobInput = | ||
| (params as { job?: unknown; data?: unknown } | null)?.job ?? | ||
| (params as { job?: unknown; data?: unknown } | null)?.data ?? | ||
| params; | ||
| const normalized = normalizeCronJobCreate(jobInput) ?? jobInput; |
There was a problem hiding this comment.
[P2] The cron.add unwrapping is duplicated and could be safer/clearer with a single typeof params === "object" && params !== null guard.
Right now the code casts params twice to { job?: unknown; data?: unknown } | null and uses optional chaining to avoid crashes. Functionally it works, but it’s a bit hard to read and easy to drift if more wrappers are added. A single object-null check plus a local const p = params as Record<string, unknown> would make the intent clearer and avoid repeated casts.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/cron.ts
Line: 72:77
Comment:
[P2] The `cron.add` unwrapping is duplicated and could be safer/clearer with a single `typeof params === "object" && params !== null` guard.
Right now the code casts `params` twice to `{ job?: unknown; data?: unknown } | null` and uses optional chaining to avoid crashes. Functionally it works, but it’s a bit hard to read and easy to drift if more wrappers are added. A single object-null check plus a local `const p = params as Record<string, unknown>` would make the intent clearer and avoid repeated casts.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
hongxuWei
force-pushed
the
fix-cron-add-validation
branch
4 times, most recently
from
118d0aa to
f8cccd0
Compare
…ron tool This PR fixes issue openclaw#7182 where the cron.add Gateway method would fail validation when the job object was wrapped in a 'job' or 'data' property, or when extra tool-level parameters (like action) were passed to the gateway. Changes: - src/gateway/server-methods/cron.ts: Explicitly unwrap 'job' or 'data' from the request params if present before validation. Added stripping of common tool-level metadata fields. - src/agents/tools/cron-tool.ts: Simplified the add action to handle flat arguments and ensured only the normalized job object is passed to the Gateway. - src/agents/tools/cron-tool.test.ts: Added test cases for flat arguments. - src/gateway/server.cron.e2e.test.ts: Added E2E test cases for wrapped job objects and leaked tool parameters.
hongxuWei
force-pushed
the
fix-cron-add-validation
branch
from
f8cccd0 to
66899dc
Compare
|
@steipete review this? |
|
Closing as duplicate of #9430. If this is incorrect, comment and we can reopen. |
3 participants
Summary
This PR fixes issue #7182 where the
cron.addGateway method would fail validation when the job object was wrapped in a 'job' or 'data' property, or when extra tool-level parameters (likeaction) were passed to the gateway.Changes
addaction to correctly handle the case where the job might be passed viajob,data, or at the root of the arguments.actionfrom causing validation errors due to the Gateway's strict schema (additionalProperties: false).Motivation
The
cron.addmethod was rejecting valid job creation attempts because the input structure was sometimes mismatched between what the Agent tool provided and what the Gateway expected. By adding explicit unwrapping and improving the tool's parameter handling, we ensure a more consistent experience across all interfaces.Testing
cron.addGateway method now correctly handles both flat and wrapped job objects.cronagent tool no longer sends redundant fields to the Gateway.Fixes #7182
Greptile Overview
Greptile Summary
This PR makes
cron.addmore tolerant of different client/tool calling conventions by unwrapping a job payload fromparams.joborparams.databefore applyingnormalizeCronJobCreateand AJV validation, and by updating thecronagent tool’saddaction to normalize inputs coming fromjob,data, or the top-level args.The gateway change aligns with existing normalization/validation patterns (normalize → validate → call
context.cron.*) and addresses schema strictness (additionalProperties: false) when callers accidentally wrap the job object.Main concern: the
crontool’s new fallback toparamscan mask missing/invalid job inputs and may forward tool-level keys (e.g.,action) as part of the “job” whenjob/dataaren’t present or aren’t objects, leading to confusing failures or unintended behavior. The gateway change itself is straightforward and covered by existing cron e2e coverage for wrapped inputs.Confidence Score: 4/5
cron.addand still runs the same normalization + strict AJV validation. Existing gateway cron e2e tests already cover wrappeddatainputs, which this PR aligns with. The main risk is the cron tool now accepting missing/invalid job payloads (falling back toparams), which changes error behavior and could make failures harder to diagnose.(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!