fix(cron): accept opaque session target keys by ferminquant · Pull Request #86578 · openclaw/openclaw

ferminquant · 2026-05-25T17:02:40Z

Summary

Allow cron custom session:<id> targets to carry opaque channel-native session keys, including DingTalk-style keys with / or \.
Keep cron run-log/job-id path safety separate and strict; the relaxed validation only applies to sessionTarget routing keys.
Update cron and gateway tests that previously locked in path-separator rejection for custom session targets.

AI Assistance Disclosure

Codex assisted with issue research, implementation, tests, review, and proof collection. I reviewed and understand the affected cron session-target, Gateway cron, run-log, and session-key paths.

Root Cause

assertSafeCronSessionTargetId used filename-safety rules for session:<id> targets. That rejected / and \ even though these values are runtime routing/session keys, not filenames. Current cron job ids are UUID-backed, and run-log filename access validates job ids through a separate path-boundary guard.

Dependency Contract

No external dependency behavior is relied on here. Internal contract proof:

src/routing/session-key.ts and src/routing/resolve-route.ts keep channel peer ids opaque when building route/session keys.
src/cron/service/jobs.ts creates cron job ids with crypto.randomUUID(), so the raw session key is not the run-log filename.
src/cron/run-log.ts and src/gateway/protocol/schema/cron.ts separately reject slash-bearing job ids for run-log lookup.
src/config/sessions/types.ts and src/config/sessions/paths.ts use the store entry sessionId for transcript filenames rather than the store key/session key.

Real Behavior Proof

Behavior addressed: openclaw cron add --session session:<opaque channel key> accepts a DingTalk-style slash-bearing session key and preserves it through Gateway persistence/readback.

Real environment tested: Local OpenClaw CLI and foreground Gateway on a throwaway OPENCLAW_STATE_DIR/OPENCLAW_CONFIG_PATH, loopback port 41965, gateway.auth.mode=none, OPENCLAW_SKIP_CHANNELS=1, no real credentials.

Exact steps or command run after this patch:

OPENCLAW_STATE_DIR=$PROOF/state OPENCLAW_CONFIG_PATH=$PROOF/openclaw.json HOME=$PROOF/home OPENCLAW_SKIP_CHANNELS=1 OPENCLAW_GATEWAY_PORT=41965 corepack pnpm openclaw gateway run --port 41965 --bind loopback --auth none --allow-unconfigured --ws-log compact
OPENCLAW_STATE_DIR=$PROOF/state OPENCLAW_CONFIG_PATH=$PROOF/openclaw.json HOME=$PROOF/home OPENCLAW_SKIP_CHANNELS=1 OPENCLAW_GATEWAY_PORT=41965 corepack pnpm openclaw cron add --name proof-64030 --cron "0 0 * * *" --session "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==" --message "proof message for issue 64030" --no-deliver --disabled --json
corepack pnpm openclaw cron get f8960528-b58e-42ee-9d86-87e741bc9f23
corepack pnpm openclaw gateway call cron.runs --params '{"id":"bad/../id","limit":1}' --json

Evidence after fix:

cron.add id=f8960528-b58e-42ee-9d86-87e741bc9f23
cron.add sessionTarget=session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==
cron.get sessionTarget=session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==
persisted delivery={"mode":"none","channel":"last"}
run-log guard output:
  $ node scripts/run-node.mjs gateway call cron.runs --params '{"id":"bad/../id","limit":1}' --json
  Gateway call failed: GatewayClientRequestError: invalid cron.runs params: at /id: must match pattern "^[^/\\]+$"
  [ELIFECYCLE] Command failed with exit code 1.

Observed result after fix: The CLI/Gateway accepted and persisted the slash-bearing custom session target unchanged; the separate cron.runs job-id validator still rejected a slash-bearing job id.

What was not tested: Live DingTalk delivery and model execution were not run; the proof intentionally used no real channel credentials or model credentials.

Regression Test Plan

node scripts/run-vitest.mjs src/cron/session-target.test.ts src/cron/normalize.test.ts src/cron/service.jobs.test.ts src/cron/service/store.test.ts src/gateway/server-cron.test.ts src/gateway/server.cron.test.ts
node scripts/run-vitest.mjs src/cron/run-log.test.ts src/gateway/protocol/cron-validators.test.ts
PATH="/tmp/openclaw-pnpm-shim:$PATH" OPENCLAW_HEAVY_CHECK_LOCK_SCOPE=worktree corepack pnpm check:changed --base upstream/main --head HEAD
codex review --base upstream/main

Security Impact

This does not relax filesystem path validation for cron run logs. It only stops treating runtime session keys as filename components. The proof includes a negative cron.runs request showing slash-bearing job ids still fail Gateway protocol validation.

Human Verification

I checked the reported source path before changing code, checked issue-linked/broad PR context, and verified there was no exact open PR for #64030 before opening this draft.

CODEOWNERS / Owner Review

No exact CODEOWNERS pattern matched the touched files. Adjacent cron service production code has /src/cron/service/jobs.ts @openclaw/openclaw-secops, so cron/session-state owner review would still be useful.

Changelog

No CHANGELOG.md edit in this contributor PR.

Review Conversations

Exact live issue query found no closedByPullRequestsReferences and no exact open PR for 64030.
Broad open PRs reviewed did not already address this issue. fix(cron): validate custom session ids #59835 was an opposite separator-rejection shape, and fix(cron): keep current/session-bound jobs out of direct channel sessions #57831 covers direct-message session isolation rather than this cron session-target bug.
codex review --base upstream/main found no concrete regression in the touched runtime or gateway paths.

Risks

The main risk is that accepting separators in sessionTarget could be mistaken for a path traversal relaxation. The actual filename boundary remains job.id, with separate run-log and Gateway schema validation.

Behavior Tradeoffs

Cron now accepts any non-empty, non-NUL custom session key for session:<id>, matching the routing contract for channel-native session keys.

Scope Boundaries

This PR does not migrate existing jobs, encode/decode stored session targets, change cron job ids, relax run-log job-id validation, change live channel delivery, or alter model execution behavior.

Validation

node scripts/run-vitest.mjs src/cron/session-target.test.ts src/cron/normalize.test.ts src/cron/service.jobs.test.ts src/cron/service/store.test.ts src/gateway/server-cron.test.ts src/gateway/server.cron.test.ts
Test Files 6 passed (6)
Tests 172 passed (172)

node scripts/run-vitest.mjs src/cron/run-log.test.ts src/gateway/protocol/cron-validators.test.ts
Test Files 2 passed (2)
Tests 41 passed (41)

pnpm exec oxfmt --check --threads=1 src/cron/session-target.ts src/cron/session-target.test.ts src/cron/normalize.test.ts src/cron/service.jobs.test.ts src/cron/service/store.test.ts src/gateway/server-cron.test.ts src/gateway/server.cron.test.ts
passed

git diff --check upstream/main
passed

PATH="/tmp/openclaw-pnpm-shim:$PATH" OPENCLAW_HEAVY_CHECK_LOCK_SCOPE=worktree corepack pnpm check:changed --base upstream/main --head HEAD
passed

codex review --base upstream/main
No concrete regression found in the touched runtime or gateway paths.

clawsweeper · 2026-05-25T17:05:35Z

Codex review: needs maintainer review before merge. Reviewed May 25, 2026, 7:46 PM ET / 23:46 UTC.

Summary
The PR relaxes cron custom session:<id> validation to allow slash/backslash-bearing opaque session keys while keeping run-log job-id validation strict and updating cron/gateway regression tests.

PR surface: Source 0, Tests +67. Total +67 across 7 files.

Reproducibility: yes. from source: current main rejects / and \ in assertSafeCronSessionTargetId, so the DingTalk-style session key from the linked issue cannot be used. I did not execute a live failing current-main command in this read-only review, but the PR body supplies after-fix CLI/Gateway proof.

Review metrics: 1 noteworthy metric.

Cron session target validation: 1 rule relaxed. The patch allows slash and backslash in custom session targets, so maintainers should consciously accept the routing-boundary change before merge.

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

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

Rank-up moves:

Have a cron/session owner confirm the validation boundary before merge.
Confirm the current head's required checks are green before merge.

Risk before merge

This changes an existing fail-closed validation boundary: separator-bearing session:<id> targets that were previously rejected or skipped can now attach cron runs to those opaque session keys.
The source review supports the path-safety separation today, but maintainers should explicitly accept that sessionTarget is a runtime routing key and not a filesystem identifier before merge.

Maintainer options:

Accept Opaque Session Keys (recommended)
Confirm that cron sessionTarget is a routing/session key rather than filesystem input, then merge once required checks are green.
Tighten The Boundary First
If arbitrary separator-bearing custom keys are too broad, require a narrower channel-native key contract and matching upgrade tests before merge.
Pause For Session Policy
If related direct-session safety work should define the reusable-session policy first, pause this PR until that cron/session-state decision is settled.

Next step before merge
No automated repair is needed; the remaining action is maintainer review of the intentional cron/session-state validation boundary and final merge readiness.

Security
Cleared: No concrete security or supply-chain regression was found; the diff only changes cron validation/tests, and separate cron run-log job-id validation remains strict.

Review details

Best possible solution:

Merge after cron/session owners confirm the routing-vs-filesystem boundary, preserving strict run-log job-id validation and the new regression coverage.

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

Yes, from source: current main rejects / and \ in assertSafeCronSessionTargetId, so the DingTalk-style session key from the linked issue cannot be used. I did not execute a live failing current-main command in this read-only review, but the PR body supplies after-fix CLI/Gateway proof.

Is this the best way to solve the issue?

Yes, with maintainer sign-off: the patch keeps the change narrow by relaxing only the runtime session-target validator while leaving cron job IDs and run-log validation strict. The safer alternative would be a narrower channel-native allowlist if maintainers do not want arbitrary separator-bearing custom session keys.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3a4f2b17fcfc.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal proof showing the CLI/Gateway accept and persist a slash-bearing session target while cron.runs still rejects a slash-bearing job ID.

Label justifications:

P2: The PR fixes a concrete cron/session routing bug with limited blast radius but requires normal maintainer review.
merge-risk: 🚨 compatibility: Merging changes separator-bearing cron custom session targets from rejected/fail-closed to accepted, which can affect existing persisted jobs and operator expectations.
merge-risk: 🚨 session-state: The accepted value determines which session key a cron run attaches to, so the validation change is directly session-state sensitive.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal proof showing the CLI/Gateway accept and persist a slash-bearing session target while cron.runs still rejects a slash-bearing job ID.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal proof showing the CLI/Gateway accept and persist a slash-bearing session target while cron.runs still rejects a slash-bearing job ID.

Evidence reviewed

PR surface:

Source 0, Tests +67. Total +67 across 7 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	1	1	0
Tests	6	116	49	+67
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	7	117	50	+67

What I checked:

Repository policy read: Read the full root AGENTS.md and the scoped Gateway AGENTS.md; the review applied OpenClaw guidance that cron/session state, fallback behavior, and validation changes are compatibility-sensitive. (AGENTS.md:22, 3a4f2b17fcfc)
Current-main source reproduction: Current main rejects slash and backslash in cron custom session target IDs, which matches the linked DingTalk-style failure from cron: encode sessionKey when deriving jobId to support channel-native conversation IDs #64030. (src/cron/session-target.ts:12, 3a4f2b17fcfc)
PR implementation boundary: The PR changes assertSafeCronSessionTargetId to reject only NUL bytes, so separator-bearing session keys can pass as routing/session keys. (src/cron/session-target.ts:12, 97398e0f1b20)
Filesystem boundary remains separate: Cron jobs created through the service use crypto.randomUUID() for the job ID, and run-log path resolution still validates the job ID before building the .jsonl path. (src/cron/service/jobs.ts:669, 3a4f2b17fcfc)
Gateway run-log protocol guard: The Gateway cron schema still rejects slash/backslash-bearing cron.runs job IDs with ^[^/\\]+$, matching the PR proof's negative run-log check. (src/gateway/protocol/schema/cron.ts:138, 3a4f2b17fcfc)
Regression coverage in PR: The PR adds/updates Gateway tests for accepting opaque custom targets on add/update and for running a persisted slash-bearing custom session target through the isolated cron run path. (src/gateway/server.cron.test.ts:787, 97398e0f1b20)

Likely related people:

steipete: Current blame on the cron session-target helper and cron job creation path points to commit 5b6d03e, and recent history also shows cron/gateway test work in this area. (role: recent area contributor; confidence: high; commits: 5b6d03e3e2f1, 93ab2ac69d25; files: src/cron/session-target.ts, src/cron/service/jobs.ts, src/gateway/server.cron.test.ts)
kkhomej33-netizen: Git history shows this author introduced custom cron session IDs and current-session autobinding, which is the central feature being adjusted here. (role: feature introducer; confidence: medium; commits: e7d9648fba27; files: src/cron/service/jobs.ts, src/cron/normalize.ts)
Neerav Makwana: Recent history includes cron jobId load-path and isolated cron auth-profile fixes, both adjacent to the job-id/session-target boundary reviewed here. (role: adjacent cron job-id contributor; confidence: medium; commits: 242b2e66f29b, 12544e24d7ab; files: src/cron/service/store.ts, src/cron/service/jobs.ts)
openclaw/openclaw-secops: CODEOWNERS owns adjacent cron service production code at src/cron/service/jobs.ts, which is part of the validation and run-log safety boundary even though this PR does not edit that file. (role: adjacent CODEOWNERS reviewer; confidence: medium; files: .github/CODEOWNERS, src/cron/service/jobs.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.

clawsweeper · 2026-05-25T17:16:41Z

ClawSweeper PR egg

✨ Hatched: 🥚 common Neon Diff Drake

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

Merged PRs are hatchable.
Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: watches the merge queue.
Image traits: location green-check meadow; accessory little merge flag; palette charcoal, cyan, and signal green; mood sparkly; pose pointing at a small proof artifact; shell woven fiber shell; lighting moonlit rim light; background little resolved-comment flags.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Neon Diff Drake in ClawSweeper.

What is this egg doing here?

Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

steipete · 2026-05-26T00:38:45Z

Proof before merge:

Reviewed beyond the diff: cron session targets route as opaque session-store keys through session-target parsing, cron store/run-log, gateway cron schema, isolated-agent session use, session-store/transcript helpers, reply-run registry, and channel/browser session registries. The right contract is session:<non-empty, non-NUL key>; slash/backslash are valid inside the key. Cron job IDs remain separate UUID/non-path IDs.
Rebased PR head to 51949741a333363586ddfb4445b82116c3bcea43 on current main at the time; current GitHub mergeability is clean, with later main movement not chased after the green run.
Local proof: git diff --check origin/main...HEAD passed.
Local proof: OPENCLAW_VITEST_MAX_WORKERS=1 node scripts/run-vitest.mjs src/cron/session-target.test.ts src/cron/normalize.test.ts src/cron/service.jobs.test.ts src/cron/service/store.test.ts src/gateway/server-cron.test.ts src/gateway/server.cron.test.ts src/cron/run-log.test.ts src/gateway/protocol/cron-validators.test.ts src/agents/tools/message-tool.test.ts src/agents/tools/image-tool.custom-provider-auth.regression.test.ts --reporter dot passed: 13 files, 347 tests.
GitHub proof on 51949741a333363586ddfb4445b82116c3bcea43: previously failing checks-node-agentic-agents reran green; core cron/runtime shards, lint, type, build-artifacts, security, and critical-quality checks shown green. One changed-path scan runner was still reporting in progress during polling; no failing required check observed.

What was not tested: no live DingTalk/model delivery; this is cron target normalization and routing-contract coverage.

openclaw-barnacle Bot added gateway Gateway runtime size: S proof: supplied External PR includes structured after-fix real behavior proof. labels May 25, 2026

ferminquant marked this pull request as ready for review May 25, 2026 17:18

steipete force-pushed the codex/64030-cron-session-target branch from 9c2f51f to 875e7d8 Compare May 25, 2026 22:34

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

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

steipete self-assigned this May 25, 2026

steipete force-pushed the codex/64030-cron-session-target branch from 875e7d8 to 97398e0 Compare May 25, 2026 23:39

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

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

steipete requested a review from a team as a code owner May 26, 2026 00:19

openclaw-barnacle Bot added the agents Agent runtime and tooling label May 26, 2026

ferminquant and others added 2 commits May 26, 2026 01:21

fix(cron): accept opaque session target keys

10a3e28

test: stabilize agent tool CI shard

5194974

steipete force-pushed the codex/64030-cron-session-target branch from 58e61b2 to 5194974 Compare May 26, 2026 00:29

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

steipete merged commit c9364f0 into openclaw:main May 26, 2026
98 of 100 checks passed

ferminquant deleted the codex/64030-cron-session-target branch May 26, 2026 01:27

Haderach-Ram mentioned this pull request May 26, 2026

Ecosystem Digest — 2026-05-26 Haderach-Ram/openclaw-radar#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cron): accept opaque session target keys#86578

fix(cron): accept opaque session target keys#86578
steipete merged 2 commits into
openclaw:mainfrom
ferminquant:codex/64030-cron-session-target

ferminquant commented May 25, 2026

Uh oh!

clawsweeper Bot commented May 25, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 25, 2026

Uh oh!

steipete commented May 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ferminquant commented May 25, 2026

Summary

AI Assistance Disclosure

Root Cause

Dependency Contract

Real Behavior Proof

Regression Test Plan

Security Impact

Human Verification

CODEOWNERS / Owner Review

Changelog

Review Conversations

Risks

Behavior Tradeoffs

Scope Boundaries

Validation

Uh oh!

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

Uh oh!

Uh oh!

clawsweeper Bot commented May 25, 2026

Hatch command

Uh oh!

steipete commented May 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented May 25, 2026 •

edited

Loading