fix(cron): warn when --system-event on main session contains shell commands#63112
fix(cron): warn when --system-event on main session contains shell commands#63112liaoandi wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ad38b55aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // are never executed — the text is only dispatched as a context notification. | ||
| if ( | ||
| hasSystemEventPatch && | ||
| opts.session === "main" && |
There was a problem hiding this comment.
The new warning in cron edit only runs when opts.session === "main", so it is skipped for the common patch flow where users edit just --system-event and leave session unchanged. For an existing main-session job, cron edit <id> --system-event "python3 foo.py" still updates silently with no warning, which reintroduces the exact confusion this fix is meant to prevent. Determine the effective session target (including existing job state when --session is omitted) before applying this guard.
Useful? React with 👍 / 👎.
Greptile SummaryAdds a non-blocking Confidence Score: 5/5Safe to merge — all findings are P2 style/improvement suggestions that don't affect correctness of the warning or job creation. The feature itself is non-blocking by design and works correctly for its primary use case (explicit --session main + --system-event with common runtimes). The three findings are: a broken ./ regex alternative that only affects an edge case, a warning gap in cron edit when --session is omitted, and code duplication. None of these cause incorrect behavior or data loss. Both files share the ./ regex issue and the code duplication; register.cron-edit.ts additionally has the session-inference gap.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -18,6 +18,18 @@ import { | |||
| warnIfCronSchedulerDisabled, | |||
| } from "./shared.js"; | |||
|
|
|||
| const SHELL_COMMAND_PATTERN = | |||
| /(?:^|\s)(?:python3?|bash|sh|node|bun|deno|uv run|npx|tsx|ts-node|ruby|perl|php|make|cargo|go run|java|dotnet|\.\/)(?:\s|$)/m; | |||
There was a problem hiding this comment.
./ pattern won't match real path invocations
The trailing (?:\s|$) anchor means ./script.sh (no space after ./) never matches. The \.\/ alternative is only useful for the bare string ./ followed by a space — practically never written. Consider replacing it with a broader path token so invocations like ./run.sh and ./deploy.sh are actually caught.
| /(?:^|\s)(?:python3?|bash|sh|node|bun|deno|uv run|npx|tsx|ts-node|ruby|perl|php|make|cargo|go run|java|dotnet|\.\/)(?:\s|$)/m; | |
| const SHELL_COMMAND_PATTERN = | |
| /(?:^|\s)(?:python3?|bash|sh|node|bun|deno|uv run|npx|tsx|ts-node|ruby|perl|php|make|cargo|go run|java|dotnet|\.\/\S+)(?:\s|$)/m; |
Same fix applies to the identical pattern in register.cron-edit.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/cron-cli/register.cron-add.ts
Line: 22
Comment:
**`./` pattern won't match real path invocations**
The trailing `(?:\s|$)` anchor means `./script.sh` (no space after `./`) never matches. The `\.\/` alternative is only useful for the bare string `./` followed by a space — practically never written. Consider replacing it with a broader path token so invocations like `./run.sh` and `./deploy.sh` are actually caught.
```suggestion
const SHELL_COMMAND_PATTERN =
/(?:^|\s)(?:python3?|bash|sh|node|bun|deno|uv run|npx|tsx|ts-node|ruby|perl|php|make|cargo|go run|java|dotnet|\.\/\S+)(?:\s|$)/m;
```
Same fix applies to the identical pattern in `register.cron-edit.ts`.
How can I resolve this? If you propose a fix, please make it concise.| if ( | ||
| hasSystemEventPatch && | ||
| opts.session === "main" && | ||
| looksLikeShellCommand(String(opts.systemEvent)) |
There was a problem hiding this comment.
--session
The guard opts.session === "main" only fires when the user explicitly passes --session main. A user who edits only the payload of an already-main-session job — cron edit <id> --system-event "python3 foo.py" — gets no warning, even though the job will still dispatch to the main session and never execute the script.
The simplest improvement is to fetch the existing job (the schedule-patch path already does this) and fall back to existing.sessionTarget:
if (
hasSystemEventPatch &&
(opts.session === "main" || (!opts.session && existing?.sessionTarget === "main")) &&
looksLikeShellCommand(String(opts.systemEvent))
) {This is a best-effort heuristic so it's non-blocking, but the current gap defeats the purpose for incremental edits.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/cron-cli/register.cron-edit.ts
Line: 327-330
Comment:
**Warning silently skipped when editing existing main-session job without `--session`**
The guard `opts.session === "main"` only fires when the user explicitly passes `--session main`. A user who edits only the payload of an already-main-session job — `cron edit <id> --system-event "python3 foo.py"` — gets no warning, even though the job will still dispatch to the main session and never execute the script.
The simplest improvement is to fetch the existing job (the schedule-patch path already does this) and fall back to `existing.sessionTarget`:
```ts
if (
hasSystemEventPatch &&
(opts.session === "main" || (!opts.session && existing?.sessionTarget === "main")) &&
looksLikeShellCommand(String(opts.systemEvent))
) {
```
This is a best-effort heuristic so it's non-blocking, but the current gap defeats the purpose for incremental edits.
How can I resolve this? If you propose a fix, please make it concise.| const SHELL_COMMAND_PATTERN = | ||
| /(?:^|\s)(?:python3?|bash|sh|node|bun|deno|uv run|npx|tsx|ts-node|ruby|perl|php|make|cargo|go run|java|dotnet|\.\/)(?:\s|$)/m; | ||
|
|
||
| /** | ||
| * Returns true when the system-event text looks like it contains a shell | ||
| * command invocation. Used to warn users that systemEvent payloads on the | ||
| * main session do not execute shell commands. | ||
| */ | ||
| function looksLikeShellCommand(text: string): boolean { | ||
| return SHELL_COMMAND_PATTERN.test(text); | ||
| } |
There was a problem hiding this comment.
shared.ts
SHELL_COMMAND_PATTERN and looksLikeShellCommand are copy-pasted verbatim into both files. Extracting them to src/cli/cron-cli/shared.ts and importing them removes the synchronization risk (a future tweak to the pattern in one file won't silently diverge from the other).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/cron-cli/register.cron-edit.ts
Line: 17-27
Comment:
**Duplicated helper — move to `shared.ts`**
`SHELL_COMMAND_PATTERN` and `looksLikeShellCommand` are copy-pasted verbatim into both files. Extracting them to `src/cli/cron-cli/shared.ts` and importing them removes the synchronization risk (a future tweak to the pattern in one file won't silently diverge from the other).
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 3:50 PM ET / 19:50 UTC. Summary PR surface: Source +74, Tests +127, Docs +1. Total +202 across 6 files. Reproducibility: yes. Source inspection shows current main still queues main-session systemEvent text through enqueueSystemEvent while actual command execution is handled by payload.kind command; I did not run a live gateway in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Refresh the branch so both warnings and tests point at --command or --command-argv, resolve current-main conflicts, and add redacted terminal proof from the live PR head. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main still queues main-session systemEvent text through enqueueSystemEvent while actual command execution is handled by payload.kind command; I did not run a live gateway in this read-only review. Is this the best way to solve the issue? No, not as written. A CLI warning is the right layer, but the warning should point to command cron now that the supported command payload path exists. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a320f775f0f3. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +74, Tests +127, Docs +1. Total +202 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
5c8d4c6 to
0e49adc
Compare
|
Codex review: needs maintainer review before merge. What this changes: Adds a shared shell-command detector and non-blocking stderr warnings to cron add/edit for main-session systemEvent payloads that look like shell commands, plus CLI/shared tests. Maintainer follow-up before merge: This is an active contributor PR tied to open #63107, so the next action is normal maintainer PR review and validation rather than a separate automated repair branch. Review detailsBest possible solution: Land a narrow cron CLI fix that warns before the gateway RPC for both add and edit, including existing-main edits where --session is omitted, with tests for matching and non-matching payloads and no change to cron runtime execution semantics. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against acae48b790fa. |
openclaw-barnacle
Bot
added
size: M
channel: discord
5a53091 to
9a192dc
Compare
openclaw-barnacle
Bot
added
triage: needs-real-behavior-proof
9a192dc to
df5e878
Compare
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
added
channel: zalo
clawsweeper
Bot
added
rating: 🦐 gold shrimp
openclaw-barnacle
Bot
added
channel: matrix
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…mmand When a user creates a cron job with --system-event <text> on --session main, and the text looks like a shell command (python3, bash, node, uv run, etc.), print a warning to stderr explaining that systemEvent payloads are dispatched as notifications to the main agent session and do not execute shell commands. Add looksLikeShellCommand() helper with a SHELL_COMMAND_PATTERN regex and emit the warning in registerCronAddCommand before the gateway RPC. Fixes openclaw#63107
…h shell command Mirror the shell-command warning from registerCronAddCommand into registerCronEditCommand: when --system-event is being patched and --session main is explicitly passed and the new text matches SHELL_COMMAND_PATTERN, print a warning to stderr. Fixes openclaw#63107
…rget - Extract SHELL_COMMAND_PATTERN + looksLikeShellCommand to shared.ts (P2) - Fix `./script.sh` regex: `\.\/` → `\.\/\S` so paths match without trailing space (P2) - In cron edit: when --session is omitted, fetch existing job to check sessionTarget before suppressing warning (P2) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
8c260c0 to
57253e3
Compare
|
@liaoandi thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward. Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive. Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it. |
1 participant
Summary
cron add --system-event "./run.sh" --session mainand similar commands used to create or edit the job silently even though main-session system events do not execute shell commands.looksLikeShellCommand()helper for common runtimes and relative script paths such as./run.sh.cron.add/cron.updatewhen the effective session target ismain, the payload issystemEvent, and the text looks like a shell command.Fixes #63107
Root Cause
System events on the main session are dispatched as context notifications to the agent. They are not executed as shell commands, so shell-looking
--system-eventpayloads on--session mainwere silently non-functional.Changes
src/cli/cron-cli/shared.ts: Adds the shared shell-command heuristic.src/cli/cron-cli/register.cron-add.ts: Warns before thecron.addRPC for main-session shell-looking system events.src/cli/cron-cli/register.cron-edit.ts: Warns before thecron.updateRPC for explicit main-session edits and existing-main jobs edited without--session.src/cli/cron-cli.test.tsandsrc/cli/cron-cli/shared.test.ts: Cover add/edit warning paths, non-matches, existing-main edit fallback, paginated existing-job lookup, and relative script paths.Real behavior proof
Behavior or issue addressed: Main-session cron
--system-eventpayloads that look like shell commands now warn before the gateway RPC, so./run.shis no longer accepted silently as if it would execute.Real environment tested: Local checkout of current PR head
5715fb72a2a1f4bde8339a8cd2b805b407e69454on 2026-05-29, running the OpenClaw source CLI throughnode --import tsx src/entry.tswith isolatedOPENCLAW_HOMEandOPENCLAW_STATE_DIR, bundled plugins disabled, and a redacted gateway token.Exact steps or command run after this patch: Rebased onto current
upstream/main, ran focused cron CLI/shared regression tests, rangit diff --check upstream/main...HEAD, then ran real source CLI commands for bothcron addand explicit-maincron editagainst a deliberately closed local gateway endpoint to observe the pre-RPC warning.Evidence after fix: copied terminal output from the current-head real CLI path for both
cron addand explicit-maincron edit.Observed result after fix: Both real CLI commands print the shell-command warning before the gateway RPC fails, proving the changed user-visible behavior runs on the current source CLI path.
What was not tested: A successful
cron.addorcron.updateagainst a live authenticated gateway was not required because the changed behavior is the pre-RPC warning path. The focused regression suite covers the live RPC parameter paths around this warning.Test plan
node scripts/run-vitest.mjs src/cli/cron-cli.test.ts src/cli/cron-cli/shared.test.ts- 2 files passed, 110 tests passedgit diff --check upstream/main...HEADcron addand explicit-maincron editprint the warning before the gateway RPC.