Skip to content

feat(cron): support custom job ids on add#2444

Open
BingqingLyu wants to merge 1 commit into
mainfrom
fork-pr-66023-feat-cron-custom-job-id
Open

feat(cron): support custom job ids on add#2444
BingqingLyu wants to merge 1 commit into
mainfrom
fork-pr-66023-feat-cron-custom-job-id

Conversation

@BingqingLyu

@BingqingLyu BingqingLyu commented Apr 28, 2026

Copy link
Copy Markdown
Owner

Summary

Describe the problem and fix in 2–5 bullets:

  • Problem: openclaw cron add always generated a UUID and had no supported way to create a stable human-readable job id.
  • Why it matters: operators then had to look up opaque ids before using cron edit, cron run, cron runs --id, or automation/scripts that refer back to an existing job.
  • What changed: added --id to cron add, validated safe custom ids at the protocol/service layer, preserved them on create, and reject collisions with an explicit error.
  • What did NOT change (scope boundary): default UUID behavior, cron scheduling semantics, and edit/run/remove behavior for existing jobs remain unchanged.
  • AI-assisted: Yes, built with Codex. Testing: lightly tested locally with focused CLI/normalize/service/protocol coverage.

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)

For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write N/A. If the cause is unclear, write Unknown.

  • Root cause: the create path omitted id end-to-end. CronJobCreate excluded it, the CLI exposed no flag, the add schema rejected it, and the service always called randomUUID().
  • Missing detection / guardrail: create-path tests covered names/schedules/delivery, but not user-supplied ids or duplicate-id rejection.
  • Contributing context (if known): store/load paths already tolerated string ids, so feature support drifted from what the underlying storage format already allowed.

Regression Test Plan (if applicable)

For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write N/A.

  • 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.test.ts
    • src/gateway/protocol/cron-validators.test.ts
    • src/cron/normalize.test.ts
    • src/cron/service.get-job.test.ts
  • Scenario the test should lock in: cron add --id daily-brief forwards the id, safe ids validate, whitespace-only ids are dropped during normalization, created jobs keep the requested id, and duplicate ids fail clearly.
  • Why this is the smallest reliable guardrail: these tests cover the exact add path layers touched by the feature without pulling in the full cron scheduler matrix.
  • Existing test that already covers this (if any): N/A
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

  • openclaw cron add now accepts --id <id>.
  • Safe custom ids are preserved on create instead of always being replaced with a UUID.
  • Reusing an existing job id now fails with cron job id already exists: <id>.

Diagram (if applicable)

Before:
user -> cron add -> UUID assigned -> later commands require lookup

After:
user -> cron add --id daily-brief -> stable id stored -> later commands can reuse daily-brief directly

Security Impact (required)

  • New permissions/capabilities? (Yes/No) Yes
  • 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: this adds a new user-controlled identifier surface for cron jobs, so the implementation restricts ids to a file-safe slug-like character set and rejects duplicates before persistence.

Repro + Verification

Environment

  • OS: macOS arm64
  • Runtime/container: Node v25.7.0, pnpm 10.32.1
  • Model/provider: N/A
  • Integration/channel (if any): Cron CLI / gateway / service
  • Relevant config (redacted): N/A

Steps

  1. Run the focused cron test suite.
  2. Create a cron job with a custom id through the add path.
  3. Attempt to create a second job with the same id.

Expected

  • Safe ids validate and flow through the add path.
  • Created jobs preserve the requested id.
  • Duplicate ids fail explicitly.

Actual

  • Verified by the focused suite listed below.

Evidence

Attach at least one:

  • 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: reviewed the end-to-end add path diff; ran npx --yes pnpm exec vitest run src/cli/cron-cli.test.ts src/gateway/protocol/cron-validators.test.ts src/cron/normalize.test.ts src/cron/service.get-job.test.ts and confirmed all targeted tests pass.
  • Edge cases checked: whitespace-only ids are dropped, ids with spaces/path traversal fail schema validation, and duplicate ids are rejected before persistence.
  • What you did not verify: full pnpm build && pnpm check && pnpm test across the entire monorepo.

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.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

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

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk: overly permissive ids could leak into filenames or make run-log paths unsafe.
    • Mitigation: custom ids are constrained to a conservative slug-like pattern and collisions are rejected before save.
  • Risk: callers relying on silent UUID generation for every create could be surprised if they now pass an id accidentally.
    • Mitigation: --id is explicit/optional, and omitting it keeps the old UUID behavior unchanged.

@BingqingLyu BingqingLyu added conflicting-group-1 Conflicting PR group 1 — review as a batch conflicting-pr Shares at least one cross-PR dependency with other PRs labels May 26, 2026
@BingqingLyu

Copy link
Copy Markdown
Owner Author

⚠️ Cross-PR Conflict Detected

This PR shares modified code with #2076, #2127, #2245, #2419, #2502, #2525.

Shared functions:

Function File Also modified by
createJob src/cron/service/jobs.ts #2076
cronIdOrJobIdParams src/gateway/protocol/schema/cron.ts #2076, #2127, #2419, #2525
normalizeCronJobInput src/cron/normalize.ts #2076, #2127
normalizeMainSystemEventCreateJob src/cron/normalize.test.ts #2127, #2245, #2502

Recommendation: Coordinate with #2076, #2127, #2245, #2419, #2502, #2525 before merging.


Auto-detected by codegraph — a code graph analysis tool built on neug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conflicting-group-1 Conflicting PR group 1 — review as a batch conflicting-pr Shares at least one cross-PR dependency with other PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: cron add: support custom job IDs via --id

2 participants