Skip to content

fix(cron): coerce pad value so cron list survives non-string job id#70183

Closed
truffle-dev wants to merge 2 commits into
openclaw:mainfrom
truffle-dev:fix/cron-list-pad-coerce-to-string
Closed

fix(cron): coerce pad value so cron list survives non-string job id#70183
truffle-dev wants to merge 2 commits into
openclaw:mainfrom
truffle-dev:fix/cron-list-pad-coerce-to-string

Conversation

@truffle-dev
Copy link
Copy Markdown
Contributor

Summary

  • Problem: openclaw cron list crashes with TypeError: value.padEnd is not a function whenever a cron entry surfaces job.id as a non-string (numeric or undefined, from older-schema or corrupted store data). --json output is unaffected, which is how the reporter routed around it.
  • Why it matters: the default interactive list command is unusable for affected operators. [Bug]: openclaw cron list crashes with TypeError: value.padEnd is not a function #70128 confirmed on 2026.4.15 and 2026.4.21 (same unchanged pad implementation in both shipped dists).
  • What changed: pad() in src/cli/cron-cli/shared.ts now widens its parameter type to string | number | null | undefined and coerces non-string primitives via a typeof-guarded String() so padEnd always runs on a string.
  • What did NOT change (scope boundary): no changes to truncate(), table headers, --json output, or the store/migration path. CronJob.id schema type stays string; only the render-side helper is defensive.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: pad() was typed (value: string, width: number) and call site pad(job.id, CRON_ID_PAD) at src/cli/cron-cli/shared.ts:278 passes job.id unguarded. CronJob.id is typed string at compile time, but older-schema or corrupted entries can surface it as numeric/undefined at runtime. TypeScript does not enforce this for external data, so value.padEnd(width) throws on the first non-string row.
  • Missing detection / guardrail: no regression test covered cron entries with non-string job.id shapes.
  • Contributing context (if known): [Bug]: openclaw cron list crashes with TypeError: value.padEnd is not a function #70128 triage by @rafiki270 root-caused to this helper and confirmed the minimal fix; call-site audit showed all other pad() calls already go through string literals, truncate(), or ?? "-" fallbacks, so job.id at :278 is the only unguarded external field.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: src/cli/cron-cli/shared.test.ts
  • Scenario the test should lock in: printCronList must not throw when job.id is numeric or undefined, and must still emit the table row.
  • Why this is the smallest reliable guardrail: the crash is in the shared render helper, so a focused printCronList test catches it at the exact seam without needing full gateway/store wiring.
  • Existing test that already covers this (if any): none.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

  • openclaw cron list no longer crashes when a cron entry has a non-string job.id. Numeric ids render stringified (e.g. 42 shows as 42); undefined/null ids render as blank padding so the other columns still line up.

Diagram (if applicable)

N/A

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

  • OS: Linux x64
  • Runtime/container: Node 22.22.2, pnpm 10.33.0, local repo checkout
  • Model/provider: N/A
  • Integration/channel (if any): N/A
  • Relevant config (redacted): N/A

Steps

  1. Stash the pad() change and keep the two new tests in place.
  2. Run pnpm test src/cli/cron-cli/shared.test.ts.
  3. Observe both new cases fail with the exact reported TypeError.
  4. Pop the stash, rerun.
  5. Observe all 22 tests green.

Expected

  • Pre-fix: 2 new tests fail with TypeError: value.padEnd is not a function (numeric id) and TypeError: Cannot read properties of undefined (reading 'padEnd') (undefined id).
  • Post-fix: all 22 tests pass.

Actual

Stash-bisect output:

Pre-fix:

Tests  2 failed | 20 passed (22)
FAIL  src/cli/cron-cli/shared.test.ts > printCronList > handles job with non-string id (#70128)
AssertionError: expected [Function] to not throw an error but 'TypeError: value.padEnd is not a func…' was thrown
FAIL  src/cli/cron-cli/shared.test.ts > printCronList > handles job with undefined id (#70128)
AssertionError: expected [Function] to not throw an error but 'TypeError: Cannot read properties of …' was thrown

Post-fix:

Test Files  1 passed (1)
Tests  22 passed (22)

Evidence

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios:
    • Reproduced the .padEnd crash locally via stash-bisect against the new regression tests.
    • Confirmed all 22 src/cli/cron-cli/shared.test.ts cases pass post-fix.
    • Ran pnpm check:changed (conflict markers, typecheck core, typecheck core tests, lint core, runtime import cycles, webhook body guard, pairing store/account guards, tests changed) — all green.
  • Edge cases checked:
    • Numeric job.id renders as stringified digits (42 in the output).
    • Undefined/null job.id renders as blank padding, row still emits.
    • String job.id keeps existing .padEnd behavior (20 original tests still green).
  • What you did not verify:
    • Live openclaw cron list against a real gateway with an actually-corrupted store entry; the crash path is a pure render helper so the unit test is the correct seam.
    • Full pnpm check / pnpm test sweep — pre-commit oxfmt hit EAGAIN on this container (process spawn limit across tinypool workers); check:changed covered the touched surface and format diff was visually verified against existing file conventions.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: widening pad()'s parameter type could mask a genuine non-string leak upstream (e.g. a broken store migration) that would be better fixed at the schema layer.
    • Mitigation: the helper is the last-line render defense; the store and gateway contracts still type CronJob.id as string, so this change only affects what the operator sees when corruption already exists. The typeof guard preserves strict-string behavior for well-formed entries, and the fix is isolated enough to revert cleanly if a better upstream guard lands.

AI-assisted

  • Mark as AI-assisted: yes, authored by truffle (github.com/truffle-dev).
  • Degree of testing: fully tested — stash-bisect repro + check:changed green + 22/22 targeted tests.
  • Prompts / session logs: available on request.
  • Confirm you understand what the code does: yes.

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: XS labels Apr 22, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

Defensive coercion in the pad() render helper so openclaw cron list no longer crashes with TypeError: value.padEnd is not a function when job.id arrives as a number or undefined from older-schema or corrupted store entries. The fix is scoped to the last-line render path; the CronJob.id schema type and all upstream contracts remain string. Two targeted regression tests covering the numeric and undefined id shapes are included and the rest of the suite is unaffected.

Confidence Score: 5/5

Safe to merge — minimal, well-scoped render-side defensive fix with regression test coverage.

The change is a single-function type widening with a correct ternary coercion (string → passthrough, null/undefined"", other → String(value)). All other pad() call sites were verified to already pass string values. Two new tests directly reproduce the crash and confirm the fix. No logic, schema, or API surface changes.

No files require special attention.

Reviews (1): Last reviewed commit: "fix(cron): coerce pad value so cron list..." | Re-trigger Greptile

@steipete
Copy link
Copy Markdown
Contributor

Thanks for the careful version of this. Closing as superseded by the maintainer patch: malformed persisted cron job IDs are repaired through doctor, with regression coverage, instead of making cron list absorb corrupt store rows.

@steipete steipete closed this Apr 22, 2026
@truffle-dev
Copy link
Copy Markdown
Contributor Author

Thanks for the note. Store-layer repair via doctor is the more principled shape; it removes the malformed row instead of pushing defensive coercion into every downstream renderer.

@truffle-dev truffle-dev deleted the fix/cron-list-pad-coerce-to-string branch April 30, 2026 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli CLI command changes size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: openclaw cron list crashes with TypeError: value.padEnd is not a function

2 participants