fix(canvas): validate CLI numeric options by rohitjavvadi · Pull Request #92490 · openclaw/openclaw

rohitjavvadi · 2026-06-12T15:57:32Z

Summary

This tightens Canvas CLI validation so bad numeric input fails before the command reaches node resolution or the gateway.

The two cases fixed here are:

--invoke-timeout 20ms, which could previously be parsed as invalid and then treated like no invoke timeout was supplied.
nodes canvas snapshot --quality 5, even though the CLI documents --quality <0-1> and the Canvas tool schema also bounds quality to 0..1.

The goal is just to keep the CLI boundary honest and aligned with the Canvas tool contract. This does not change the gateway node.invoke schema or add new Canvas behavior.

Review focus: the validation order in invokeCanvas, the snapshot quality bounds, and the regression tests proving invalid input does not reach node resolution or gateway invocation.

Linked context

Closes #92487

Related #92482

This was not specifically requested by a maintainer.

Real behavior proof (required for external PRs)

Behavior or issue addressed: Canvas CLI accepted invalid numeric options. In particular, --invoke-timeout 20ms could be treated like no invoke timeout, and snapshot --quality 5 could pass despite the documented/schema 0..1 range.
Real environment tested: Local OpenClaw source checkout on macOS after running pnpm build, using the built node openclaw.mjs launcher.

Exact steps or command run after this patch:

node openclaw.mjs nodes canvas snapshot --node ios-node --invoke-timeout 20ms
node openclaw.mjs nodes canvas snapshot --node ios-node --quality 5

Evidence after fix:

nodes canvas snapshot failed: --invoke-timeout must be a positive integer.
nodes canvas snapshot failed: --quality must be between 0 and 1.

Observed result after fix: Both invalid inputs fail at the CLI boundary before a successful node/gateway invocation path.
What was not tested: I did not run a live paired canvas node or capture a real canvas screenshot, because this change is limited to validation before node invocation.
Proof limitations or environment constraints: pnpm check:changed could not run in this local checkout because the configured Crabbox/Testbox binary failed its basic --version/--help sanity check before code checks started.
Before evidence: Source inspection showed parseTimeoutMs returned undefined for invalid values and snapshot --quality only checked for a finite number, not the documented/schema range.

Tests and validation

Commands run:

node scripts/run-vitest.mjs extensions/canvas/src/cli.test.ts
OPENCLAW_HEAVY_CHECK_LOCK_SCOPE=worktree pnpm test extensions/canvas/src/cli.test.ts extensions/canvas/src/tool.test.ts -- --reporter=verbose
./node_modules/.bin/oxfmt extensions/canvas/src/cli.ts extensions/canvas/src/cli.test.ts --check
node scripts/run-oxlint.mjs extensions/canvas/src/cli.ts extensions/canvas/src/cli.test.ts
pnpm check:test-types
git diff --check
pnpm build
pnpm check:changed

pnpm check:changed stopped before code checks because the local Crabbox/Testbox binary failed its basic sanity check.

Regression coverage added:

Invalid --invoke-timeout across snapshot, present, hide, navigate, eval, a2ui push, and a2ui reset.
Out-of-range snapshot --quality values.
Accepted --quality boundary values 0 and 1.

Risk checklist

User-visible behavior changes: yes. Invalid Canvas numeric values now fail immediately instead of being ignored or forwarded deeper into the node path.

Config, environment, or migration changes: no.

Security, auth, secrets, network, or tool execution changes: no.

The main risk is that someone relying on invalid Canvas numeric values will now get a CLI error. That seems acceptable because these values are outside the documented option contract and outside the Canvas tool schema, and the tests cover rejection before node resolution or gateway invocation.

Current review state

Ready for review. Nothing known is waiting on the author side, and there are no bot or reviewer comments to address yet.

clawsweeper · 2026-06-12T15:59:16Z

Codex review: needs maintainer review before merge. Reviewed June 13, 2026, 7:38 PM ET / 23:38 UTC.

Summary
The PR tightens Canvas CLI numeric validation for --invoke-timeout and snapshot --quality, and adds regression tests for invalid values and quality boundaries.

PR surface: Source +12, Tests +82. Total +94 across 2 files.

Reproducibility: yes. Source inspection shows current main accepts --quality 5 through the finite-number parser and parses invalid invoke timeouts only after node resolution, matching the linked issue's reproduction path.

Review metrics: 1 noteworthy metric.

Stricter CLI validation: 2 invalid-input paths tightened. Maintainers should notice that the PR intentionally changes behavior for malformed invoke timeouts and out-of-range snapshot quality.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

[P2] Existing scripts that pass invalid Canvas numeric values such as --quality 5 or --invoke-timeout 20ms will now fail at the CLI boundary; that is an intentional compatibility tightening because those values are outside the CLI help and Canvas tool schema.

Maintainer options:

Land the validation tightening (recommended)
Accept the invalid-input compatibility change because the rejected values are outside the documented CLI range and Canvas tool schema.

Next step before merge

No automated repair is needed; the PR should proceed through normal maintainer landing review once required checks settle.

Security
Cleared: The diff only changes local CLI numeric validation and tests, with no dependency, workflow, permission, secret, install, publishing, or external code-execution changes.

Review details

Best possible solution:

Land the Canvas-local CLI validation with the added regression coverage, accepting the invalid-input compatibility tightening because it aligns the CLI with the existing tool contract.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main accepts --quality 5 through the finite-number parser and parses invalid invoke timeouts only after node resolution, matching the linked issue's reproduction path.

Is this the best way to solve the issue?

Yes. Validating in the Canvas CLI owner boundary is the narrowest maintainable fix; changing the gateway or Canvas tool schema would be broader and unnecessary because those contracts already express the desired bounds.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 47759c35063c.

Label changes

Label changes:

add merge-risk: 🚨 compatibility: Merging will make previously tolerated invalid Canvas CLI numeric values fail fast, which can affect scripts relying on the old permissive behavior.
add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix built-CLI commands and terminal output showing both invalid inputs now fail with the new validation errors before successful node or gateway invocation.

Label justifications:

P2: This fixes a bounded Canvas CLI validation bug that can confuse users but does not affect broader runtime availability or data safety.
merge-risk: 🚨 compatibility: Merging will make previously tolerated invalid Canvas CLI numeric values fail fast, which can affect scripts relying on the old permissive behavior.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix built-CLI commands and terminal output showing both invalid inputs now fail with the new validation errors before successful node or gateway invocation.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix built-CLI commands and terminal output showing both invalid inputs now fail with the new validation errors before successful node or gateway invocation.

Evidence reviewed

PR surface:

Source +12, Tests +82. Total +94 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	15	3	+12
Tests	1	83	1	+82
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	98	4	+94

What I checked:

Current-main timeout gap: Current main returns undefined for an invalid Canvas invoke timeout and only parses it after node resolution, so malformed values can be treated like no timeout after the command has already entered the node lookup path. (extensions/canvas/src/cli.ts:96, 47759c35063c)
Current-main quality gap: Current main advertises --quality <0-1> but parses snapshot quality with the generic finite-number parser, which accepts out-of-range finite values before building the Canvas request. (extensions/canvas/src/cli.ts:275, 47759c35063c)
Canvas tool contract: The model-facing Canvas tool schema already bounds quality to minimum 0 and maximum 1, so the PR aligns the CLI boundary with the existing Canvas tool contract rather than adding a new behavior surface. (extensions/canvas/src/tool-schema.ts:42, 47759c35063c)
PR implementation: The branch throws for invalid --invoke-timeout values before node resolution and adds a Canvas snapshot quality parser that rejects values outside the inclusive 0-1 range. (extensions/canvas/src/cli.ts:100, a970ce594ea6)
PR regression coverage: The branch tests out-of-range snapshot quality, accepts boundary values 0 and 1, and verifies invalid invoke timeouts across Canvas subcommands do not call node resolution or the gateway. (extensions/canvas/src/cli.test.ts:159, a970ce594ea6)
Latest release comparison: Release v2026.6.6 contains the same permissive Canvas timeout and quality parsing behavior as current main, so the PR is not redundant with shipped behavior. (extensions/canvas/src/cli.ts:96, 8c802aa68351)

Likely related people:

vincentkoc: Current main blame and local history attribute the Canvas CLI file and tests to commit eb1b640854bb6b7dc82db9a5bcb652836574d0b3, which carries the parser and command wiring relevant to this validation change; history appears compressed, so routing confidence is medium. (role: recent area contributor; confidence: medium; commits: eb1b640854bb; files: extensions/canvas/src/cli.ts, extensions/canvas/src/cli.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

vincentkoc

No blocking findings.

This is the right fix shape for the Canvas CLI: keep numeric validation inside the plugin CLI boundary, use the shared strict numeric parser, parse --invoke-timeout before node resolution in the shared invoke path, and align snapshot --quality with the existing 0..1 Canvas tool schema.

Verification:

Focused local: node scripts/run-vitest.mjs extensions/canvas/src/cli.test.ts --maxWorkers=1 — 20 passed.
Autoreview: .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main ... — clean, no accepted/actionable findings.
Remote changed gate: AWS Crabbox cbx_b7838c58daba, run run_bbb58ca536f4, check:changed — passed.
Exact PR head a970ce594ea6e7284ace519352fc258b1b81cb80 — GitHub checks green; only routine superseded auto-response cancellation ignored.

Code read: extensions/canvas/src/cli.ts, extensions/canvas/src/cli.test.ts, extensions/canvas/src/tool-schema.ts, extensions/canvas/src/tool.test.ts, src/plugin-sdk/number-runtime.ts, extensions/AGENTS.md.

Best-fix verdict: best. Alternatives rejected: per-command timeout parsing would duplicate the invariant; accepting wider quality values would conflict with the model-facing schema.

openclaw-barnacle Bot added size: S triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. labels Jun 12, 2026

openclaw-barnacle Bot removed the triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. label Jun 12, 2026

clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 12, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 12, 2026

This was referenced Jun 13, 2026

fix(canvas): validate --quality range and --invoke-timeout at the CLI boundary #92689

Closed

fix(canvas): validate snapshot quality range and reject invalid invoke-timeout #92693

Closed

fix(canvas): validate CLI numeric options

a970ce5

vincentkoc force-pushed the fix/canvas-cli-validation branch from 582c314 to a970ce5 Compare June 13, 2026 23:33

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 13, 2026

clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 13, 2026

vincentkoc approved these changes Jun 13, 2026

View reviewed changes

vincentkoc merged commit e55cebf into openclaw:main Jun 13, 2026
170 of 173 checks passed

Alix-007 mentioned this pull request Jun 14, 2026

fix(cli): reject present-but-invalid --timeout on status/health fast path #92996

Merged

rohitjavvadi mentioned this pull request Jun 15, 2026

fix(cli): allow zero Discord timeout duration #93329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(canvas): validate CLI numeric options#92490

fix(canvas): validate CLI numeric options#92490
vincentkoc merged 1 commit into
openclaw:mainfrom
rohitjavvadi:fix/canvas-cli-validation

rohitjavvadi commented Jun 12, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 12, 2026 •

edited

Loading

Uh oh!

vincentkoc left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

rohitjavvadi commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Linked context

Real behavior proof (required for external PRs)

Tests and validation

Risk checklist

Current review state

Uh oh!

clawsweeper Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentkoc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rohitjavvadi commented Jun 12, 2026 •

edited

Loading

clawsweeper Bot commented Jun 12, 2026 •

edited

Loading