cron: script payload kind + middleware hook payload visibility#17
Merged
cron: script payload kind + middleware hook payload visibility#17
Conversation
- Add CronScriptPayload type (command, args, env, cwd, timeoutSeconds, deliver) - New exec-script.ts: execCronScript() via child_process.execFile (no shell injection) - Relative paths resolved against OC home (basePath), not process.cwd() - Timeout/abort via AbortSignal; stdout as summary, stderr as error on non-zero exit - Add script branch in executeJobCore (before session routing — no session needed) - Gateway protocol schema: CronScriptPayloadSchema in CronPayloadSchema union - CLI cron add: --script, --script-arg (repeatable), --script-env KEY=VALUE, --script-cwd - normalize.ts: coerce kind='script' passthrough - service/jobs.ts: mergeCronPayload + buildPayloadFromPatch handle script kind - service/normalize.ts: normalizePayloadToSystemText handles script kind - Unit tests: 9 cases covering execution, args, env, cwd, relative paths, timeout, abort Closes #11
beforeRun/afterComplete/onFailure/afterRun hooks now receive ctx.payload so they can inspect the job's payload kind, command, message, etc. and make decisions (abort, audit, conditional logic) without guessing from the job name. - Add payload: CronPayload to CronHookContext type (hooks.ts) - Pass payload: job.payload in both makeHookCtx closures in timer.ts - Update hooks.test.ts: makeCtx accepts optional payload arg; add 3 tests for payload visibility (agentTurn kind check, script command check, allowed command does not abort)
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
CLI src/cli/cron-cli/register.cron-add.ts |
Added --script, repeatable --script-arg, repeatable --script-env (KEY=VALUE), and --script-cwd; payload selection and validation updated to accept script and build its fields (command, args, env, cwd, timeoutSeconds, deliver). |
Script executor & tests src/cron/exec-script.ts, src/cron/exec-script.test.ts |
New execCronScript that resolves script paths (expand ~, resolve relative to basePath), validates existence, merges env, runs via execFile (no shell), supports AbortSignal/timeout and returns stdout as summary or stderr/error on non-zero exit; comprehensive tests for args, env, cwd, abort, symlink handling, and missing script. |
Types & Gateway Schemas src/cron/types.ts, src/gateway/protocol/schema/cron.ts |
Introduced CronScriptPayload and patch variant; extended CronPayload/CronPayloadPatch unions and gateway schema unions to include strict script shape and patch semantics. |
Service: jobs, merge, normalize src/cron/service/jobs.ts, src/cron/service/normalize.ts, src/cron/normalize.ts |
Allow script in isolated-like session targets; add build/merge/validation logic for script payloads and merging semantics; normalize now preserves script kind and maps command for system text. |
Timer / Execution & Ops src/cron/service/timer.ts, src/cron/service/ops.ts |
Execute payload.kind === "script" via execCronScript (bypassing sessions/LLM) with computed basePath; ensure job.payload is cloned into CronHookContext for beforeRun/afterRun/onFailure/afterComplete in runtime, startup catch-up, and manual-run finish flows. |
Hooks & Tests src/cron/hooks.ts, src/cron/hooks.test.ts |
Added payload: CronPayload to CronHookContext; updated hook module path resolution to expand ~ and resolve relative to basePath/process.cwd(); tests assert hooks receive payload for agentTurn and script runs. |
Hook/Timer Contexts (propagation) src/cron/hooks.ts, src/cron/service/timer.ts, src/cron/service/ops.ts |
Hook contexts now include cloned job.payload across beforeRun/afterRun/onFailure/afterComplete hooks in main timer, startup catch-up, and manual-run flows. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
- [Improvement]: cron: beforeRun middleware — payload transform hook #14 — Implements propagation of payload into CronHookContext and adds script-payload handling/hooks/timer integration that this PR updates and exercises.
Possibly related PRs
- cron: add lifecycle hooks for job execution #3 — Adds/changes cron hook subsystem pieces (hooks, timer, ops); this PR extends that plumbing by adding payload propagation and native script execution.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately and specifically describes the main changes: adding script payload kind and exposing payload visibility in middleware hooks. |
| Description check | ✅ Passed | The description provides a comprehensive summary with clear problem statement, changes across multiple modules, architecture explanation, and test plan including both automated tests and manual verification steps. |
| Linked Issues check | ✅ Passed | The PR successfully implements all acceptance criteria from #11: new script payload kind, execFile execution, path resolution, stdout/stderr capture, timeout via AbortSignal, file validation, deliver flag support, CLI flags, schema updates, test coverage, and preserves existing job types. |
| Out of Scope Changes check | ✅ Passed | All changes directly support script payload implementation and middleware hook payload visibility per #11; no unrelated modifications detected in file summaries. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feature/11-cron-script-payload
📝 Coding Plan
- Generate coding plan for human review comments
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cron/service/timer.ts (1)
1025-1036:⚠️ Potential issue | 🟠 MajorMissing
payloadin startup catch-up hook context.The
makeHookCtxfunction inrunStartupCatchupCandidatedoes not includepayload: job.payload, unlike the equivalent inrunDueJob(line 649). This means hooks running during startup catch-up won't have access to the payload, creating inconsistent behavior.🐛 Proposed fix to add payload to catch-up hook context
const makeHookCtx = ( hookPoint: "beforeRun" | "afterComplete" | "onFailure" | "afterRun", extra?: Partial<CronHookContext>, ): CronHookContext => ({ hookPoint, workflow: "cron", job: { id: job.id, name: job.name, agentId: job.agentId, schedule: job.schedule }, + payload: job.payload, meta: hookMeta, log: state.deps.log, basePath: hookBasePath, ...extra, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/service/timer.ts` around lines 1025 - 1036, The makeHookCtx factory used in runStartupCatchupCandidate is missing the payload field, causing startup catch-up hooks to lack job.payload; update makeHookCtx to include payload: job.payload in the returned CronHookContext object (same as runDueJob's context) so hooks receive a consistent context that contains job.payload.
🧹 Nitpick comments (2)
src/cron/normalize.ts (1)
94-96: Consider trimming script payload fields for consistency.Other payload kinds trim their string fields (e.g.,
message,text,model). Script payloads pass through unchanged, which means leading/trailing whitespace incommandorcwdcould cause issues.💡 Optional: Trim script-specific fields
} else if (kindRaw === "script") { - // script payloads pass through normalization unchanged next.kind = "script"; + if (typeof next.command === "string") { + next.command = next.command.trim(); + } + if (typeof next.cwd === "string") { + const trimmed = next.cwd.trim(); + next.cwd = trimmed || undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/normalize.ts` around lines 94 - 96, In the branch handling kindRaw === "script" (where next.kind is set to "script"), trim any string fields in the script payload—specifically command and cwd—before assigning them to next.script to match the normalization applied to other kinds; ensure you only call .trim() when the value is a string (preserve null/undefined) and leave non-string fields untouched so whitespace is removed consistently without changing types.src/cron/exec-script.test.ts (1)
98-111: Potential flakiness in mid-run abort test.The 50ms delay before aborting assumes the script process starts within that window. On heavily loaded CI runners, this timing could be unreliable. Consider increasing the delay or using a synchronization mechanism.
💡 Increase abort delay for CI reliability
// Abort after a short delay to let the process start. - setTimeout(() => controller.abort(), 50); + setTimeout(() => controller.abort(), 200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/exec-script.test.ts` around lines 98 - 111, The test "kills script and returns error when aborted mid-run" is flaky because the setTimeout(() => controller.abort(), 50) assumes the child process started within 50ms; increase the wait to a more CI-safe value (e.g., 200ms) or replace the ad-hoc timeout with a synchronization approach: modify the test script created by writeScript("slow.sh", ...) to signal readiness (e.g., touch a ready file or print a marker) and have the test wait/poll for that marker before calling controller.abort() and awaiting execCronScript; update references to writeScript, execCronScript, AbortController/controller.abort(), and the setTimeout call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/cron-cli/register.cron-add.ts`:
- Around line 213-221: The script branch builds a payload with deliver but never
adds the delivery object into params, so delivery flags (--channel, --to,
--account, --best-effort-deliver) are ignored for kind: "script"; update the
code that constructs the payload for the script branch (in register.cron-add.ts
where the object with kind: "script", command: scriptCommand, args: ...,
deliver: hasAnnounce ? true : hasNoDeliver ? false : undefined is created) to
also construct and attach the same params.delivery object used for agentTurn
jobs (populate channel, to, account, bestEffort) and pass that params into
cron.add (or into payload.params) so delivery options are honored for scripts as
well.
In `@src/cron/exec-script.ts`:
- Around line 64-79: The timeout handler (onAbort) currently calls child.kill
and immediately settles the promise, which can mark the script finished before
the process actually exits; update onAbort to send SIGTERM then wait for the
child to emit "close" before calling settle, and if the child does not close
within a short grace period send a follow-up SIGKILL and then settle.
Specifically modify the abortSignal/onAbort flow around child.kill("SIGTERM") to
attach a one-time child.once("close", ...) that settles with the timeout error,
and add a timeout (e.g., setTimeout) that will call child.kill("SIGKILL") if the
"close" event hasn't occurred within the grace period to ensure the process is
actually terminated before settle is invoked.
In `@src/cron/service/jobs.ts`:
- Around line 672-674: The current guard returns buildPayloadFromPatch(patch)
whenever kinds differ OR when either kind isn't "agentTurn", which causes
same-kind "script" patches to be treated as full replacements and drop fields
like args/env/cwd/deliver; change the conditional logic in the function handling
patches so that if existing.kind === "script" && patch.kind === "script" you
perform a merge of script fields (preserve existing.command, args, env, cwd,
deliver, timeoutSeconds unless explicitly overwritten by patch) instead of
calling buildPayloadFromPatch; specifically update the branch that references
existing.kind, patch.kind and buildPayloadFromPatch to special-case "script"
(and apply the same fix to the analogous block later around the other mentioned
range).
- Around line 759-775: The new branch constructs a patch with patch.kind ===
"script" but service-side validation in assertSupportedJobSpec (used by
createJob and applyJobPatch) doesn't permit "script", so script jobs are
rejected; update assertSupportedJobSpec to treat "script" like the allowed job
kinds (add "script" to the permitted kinds check where agentTurn/systemEvent are
validated) so that script payloads pass validation for the intended targets
(e.g., the same isolated/current/session or main targets you allow
agentTurn/systemEvent for), and ensure any target-specific constraints in
assertSupportedJobSpec are applied to "script" as needed.
In `@src/cron/types.ts`:
- Around line 102-105: CronPayloadPatch is defined to allow partial script
payloads via Partial<CronScriptPayload>, but CronPayloadPatchSchema still uses
the full CronScriptPayloadSchema (which requires command); update the schema to
match the type by making the script payload schema partial/optional in
CronPayloadPatchSchema (e.g., use a partialized version of
CronScriptPayloadSchema or mark its fields optional) so validation aligns with
the TypeScript type CronPayloadPatch, and ensure the change is applied where
CronPayloadPatchSchema is defined (referencing CronPayloadPatchSchema and
CronScriptPayloadSchema).
---
Outside diff comments:
In `@src/cron/service/timer.ts`:
- Around line 1025-1036: The makeHookCtx factory used in
runStartupCatchupCandidate is missing the payload field, causing startup
catch-up hooks to lack job.payload; update makeHookCtx to include payload:
job.payload in the returned CronHookContext object (same as runDueJob's context)
so hooks receive a consistent context that contains job.payload.
---
Nitpick comments:
In `@src/cron/exec-script.test.ts`:
- Around line 98-111: The test "kills script and returns error when aborted
mid-run" is flaky because the setTimeout(() => controller.abort(), 50) assumes
the child process started within 50ms; increase the wait to a more CI-safe value
(e.g., 200ms) or replace the ad-hoc timeout with a synchronization approach:
modify the test script created by writeScript("slow.sh", ...) to signal
readiness (e.g., touch a ready file or print a marker) and have the test
wait/poll for that marker before calling controller.abort() and awaiting
execCronScript; update references to writeScript, execCronScript,
AbortController/controller.abort(), and the setTimeout call accordingly.
In `@src/cron/normalize.ts`:
- Around line 94-96: In the branch handling kindRaw === "script" (where
next.kind is set to "script"), trim any string fields in the script
payload—specifically command and cwd—before assigning them to next.script to
match the normalization applied to other kinds; ensure you only call .trim()
when the value is a string (preserve null/undefined) and leave non-string fields
untouched so whitespace is removed consistently without changing types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deecb8d8-a4b1-4611-b64f-d5fa6d745e13
📒 Files selected for processing (11)
src/cli/cron-cli/register.cron-add.tssrc/cron/exec-script.test.tssrc/cron/exec-script.tssrc/cron/hooks.test.tssrc/cron/hooks.tssrc/cron/normalize.tssrc/cron/service/jobs.tssrc/cron/service/normalize.tssrc/cron/service/timer.tssrc/cron/types.tssrc/gateway/protocol/schema/cron.ts
- assertSupportedJobSpec: allow script kind on isolated/current/session targets - mergeCronPayload: add script merge branch to preserve existing fields on partial patch - CronPayloadPatchSchema: use partial script schema so command is not required on update - register.cron-add: build delivery object for script jobs when --announce/--no-deliver passed - exec-script: wait for process close before settling; add SIGKILL grace period after SIGTERM - timer.ts catch-up makeHookCtx: add missing payload field
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli/cron-cli/register.cron-add.ts (2)
266-271: Minor: Error message could be clearer.Line 267 says "Main jobs require --system-event" but the condition at line 266 checks
payload.kind === "agentTurn". While technically correct (agentTurn implies --message was used), the message could explicitly mention that --message and --script aren't allowed for main sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/cron-cli/register.cron-add.ts` around lines 266 - 271, Update the thrown error messages to be explicit: in the branch checking sessionTarget === "main" && payload.kind === "agentTurn" (symbols: sessionTarget, payload.kind, agentTurn) change the error to say that main sessions do not allow --message or --script and require --system-event instead; in the branch checking isIsolatedLikeSessionTarget && payload.kind === "systemEvent" (symbols: isIsolatedLikeSessionTarget, payload.kind, systemEvent) change the error to say that isolated/child sessions do not allow --system-event and must use --message or --script instead so the user knows which flags are invalid for each session target.
199-201: Consider warning on malformed--script-enventries.Malformed entries (missing
=or key-only likeKEY=) are silently skipped. Users might not realize their environment variable wasn't set.💡 Optional: Add a warning for skipped entries
scriptEnvEntries .map((e) => { const idx = e.indexOf("="); - return idx > 0 ? [e.slice(0, idx), e.slice(idx + 1)] : null; + if (idx > 0) { + return [e.slice(0, idx), e.slice(idx + 1)]; + } + console.warn(`Skipping malformed --script-env entry: ${e}`); + return null; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/cron-cli/register.cron-add.ts` around lines 199 - 201, The --script-env parsing currently maps entries with .map((e) => { const idx = e.indexOf("="); return idx > 0 ? [e.slice(0, idx), e.slice(idx + 1)] : null; } ) and silently skips malformed inputs; update this logic to emit a warning for any skipped entry: if idx <= 0 warn that the entry is missing a key or '=' and if idx > 0 but the value slice(idx+1) is empty warn that the key-only entry (e.g., "KEY=") will be ignored; use the existing logger (processLogger.warn or equivalent) or console.warn if no logger is available and keep the returned map behavior (null or filtered) intact so other code using the parsed env continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/cron-cli/register.cron-add.ts`:
- Around line 266-271: Update the thrown error messages to be explicit: in the
branch checking sessionTarget === "main" && payload.kind === "agentTurn"
(symbols: sessionTarget, payload.kind, agentTurn) change the error to say that
main sessions do not allow --message or --script and require --system-event
instead; in the branch checking isIsolatedLikeSessionTarget && payload.kind ===
"systemEvent" (symbols: isIsolatedLikeSessionTarget, payload.kind, systemEvent)
change the error to say that isolated/child sessions do not allow --system-event
and must use --message or --script instead so the user knows which flags are
invalid for each session target.
- Around line 199-201: The --script-env parsing currently maps entries with
.map((e) => { const idx = e.indexOf("="); return idx > 0 ? [e.slice(0, idx),
e.slice(idx + 1)] : null; } ) and silently skips malformed inputs; update this
logic to emit a warning for any skipped entry: if idx <= 0 warn that the entry
is missing a key or '=' and if idx > 0 but the value slice(idx+1) is empty warn
that the key-only entry (e.g., "KEY=") will be ignored; use the existing logger
(processLogger.warn or equivalent) or console.warn if no logger is available and
keep the returned map behavior (null or filtered) intact so other code using the
parsed env continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b15e8bf-c326-4057-84e7-1d0e219e9481
⛔ Files ignored due to path filters (1)
docs/repo-map.jsonis excluded by!docs/repo-map.json
📒 Files selected for processing (5)
src/cli/cron-cli/register.cron-add.tssrc/cron/exec-script.tssrc/cron/service/jobs.tssrc/cron/service/timer.tssrc/gateway/protocol/schema/cron.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cron/exec-script.ts
- src/cron/service/timer.ts
…exts - register.cron-add: clarify session target error messages to name invalid flags - register.cron-add: extend main-session check to cover script kind (not just agentTurn) - hooks.ts: fix resolveUserPath called with 4 args; resolve relative paths against basePath separately - ops.ts makeHookCtx: add missing payload field (same gap as timer.ts catch-up path)
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cron/service/ops.ts (1)
469-485:⚠️ Potential issue | 🟠 MajorPass a snapshot of
payloadinto hooks.Line 481 hands hooks the live
executionJob.payloadobject, so abeforeRunhook can mutatectx.payloadand change the command/message thatexecuteJobCoreWithTimeout()actually runs.metais already the mutable channel here;payloadshould be a cloned or frozen view.Proposed fix
): CronHookContext => ({ hookPoint, workflow: "cron", job: { id: executionJob.id, name: executionJob.name, agentId: executionJob.agentId, schedule: executionJob.schedule, }, - payload: executionJob.payload, + payload: structuredClone(executionJob.payload), meta: hookMeta, log: state.deps.log, basePath: hookBasePath, ...extra, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/service/ops.ts` around lines 469 - 485, makeHookCtx currently injects the live executionJob.payload into the hook context so hooks can mutate ctx.payload and affect the actual job run; change makeHookCtx to pass an immutable snapshot (e.g., a shallow/deep clone or Object.freeze result) of executionJob.payload instead of the live object. Update the return value in makeHookCtx (used by hooks like beforeRun/afterRun/onFailure/afterComplete) to use the cloned/frozen payload so mutations in hooks cannot change what executeJobCoreWithTimeout or other execution code sees.src/cli/cron-cli/register.cron-add.ts (1)
283-346:⚠️ Potential issue | 🟠 MajorReject delivery-only flags when effective delivery is off.
For
scriptjobs,deliveryModestays unset unless--announceor--no-deliveris present. That means--to,--account, and--best-effort-deliverare currently accepted but dropped becauseparams.deliveryis never built. Please validate explicit delivery config against the effective mode before constructingparams.Proposed fix
const deliveryMode = payload.kind === "agentTurn" && isIsolatedLikeSessionTarget ? hasAnnounce ? "announce" : hasNoDeliver ? "none" : "announce" : payload.kind === "script" && isIsolatedLikeSessionTarget ? hasAnnounce ? "announce" : hasNoDeliver ? "none" : undefined : undefined; + + const hasExplicitDeliveryOptions = + optionSource("channel") === "cli" || + optionSource("to") === "cli" || + optionSource("account") === "cli" || + optionSource("bestEffortDeliver") === "cli"; + if (!deliveryMode && hasExplicitDeliveryOptions) { + throw new Error( + "Delivery options require an effective delivery mode; pass --announce for script jobs.", + ); + } + if (deliveryMode === "none" && hasExplicitDeliveryOptions) { + throw new Error("Delivery options cannot be combined with --no-deliver."); + } const nameRaw = typeof opts.name === "string" ? opts.name : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/cron-cli/register.cron-add.ts` around lines 283 - 346, The code currently computes an effective deliveryMode but still allows delivery-only flags to be passed and silently dropped; add a validation after computing deliveryMode that, when deliveryMode is undefined or equals "none" (for payload.kind === "script" or other relevant kinds), rejects explicit delivery flags by throwing an error if opts.to, opts.account, opts.bestEffortDeliver, or opts.channel are present; use the existing local symbols (deliveryMode, payload.kind, opts.to, opts.account, opts.bestEffortDeliver, opts.channel, hasAnnounce, hasNoDeliver) and perform this check before building params.delivery so callers get a clear error instead of having their delivery flags ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/cron-cli/register.cron-add.ts`:
- Around line 191-205: The current parsing of opts.scriptEnv in register
cron-add silently drops malformed entries (e.g., "FOO" without "=") via
scriptEnvEntries -> map/filter, which should instead fail fast; update the logic
around scriptEnvEntries and scriptEnv to validate each entry (ensure it contains
'=' with a non-empty key) and if any entry is malformed throw or return a CLI
error (with a clear message listing the bad entries) before proceeding to job
creation so the user is informed rather than silently losing env vars.
---
Outside diff comments:
In `@src/cli/cron-cli/register.cron-add.ts`:
- Around line 283-346: The code currently computes an effective deliveryMode but
still allows delivery-only flags to be passed and silently dropped; add a
validation after computing deliveryMode that, when deliveryMode is undefined or
equals "none" (for payload.kind === "script" or other relevant kinds), rejects
explicit delivery flags by throwing an error if opts.to, opts.account,
opts.bestEffortDeliver, or opts.channel are present; use the existing local
symbols (deliveryMode, payload.kind, opts.to, opts.account,
opts.bestEffortDeliver, opts.channel, hasAnnounce, hasNoDeliver) and perform
this check before building params.delivery so callers get a clear error instead
of having their delivery flags ignored.
In `@src/cron/service/ops.ts`:
- Around line 469-485: makeHookCtx currently injects the live
executionJob.payload into the hook context so hooks can mutate ctx.payload and
affect the actual job run; change makeHookCtx to pass an immutable snapshot
(e.g., a shallow/deep clone or Object.freeze result) of executionJob.payload
instead of the live object. Update the return value in makeHookCtx (used by
hooks like beforeRun/afterRun/onFailure/afterComplete) to use the cloned/frozen
payload so mutations in hooks cannot change what executeJobCoreWithTimeout or
other execution code sees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f87832a8-8702-4375-8658-d65bf1429cb0
⛔ Files ignored due to path filters (1)
docs/repo-map.jsonis excluded by!docs/repo-map.json
📒 Files selected for processing (3)
src/cli/cron-cli/register.cron-add.tssrc/cron/hooks.tssrc/cron/service/ops.ts
- register.cron-add: throw on malformed --script-env entries instead of silently skipping - register.cron-add: throw when delivery flags (--to/--channel/--account/--best-effort-deliver) are passed without --announce - ops.ts + timer.ts: pass structuredClone(payload) to makeHookCtx so hooks cannot mutate the live payload and affect execution
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cron/service/timer.ts (1)
1025-1037:⚠️ Potential issue | 🟡 MinorMissing
structuredClonefor payload in startup catch-up hook context.Line 1032 passes
job.payloaddirectly, but line 649 (main timer path) usesstructuredClone(job.payload). This inconsistency allows hooks in the startup catch-up path to mutate the live payload, violating the PR objective for hook payload immutability.🐛 Proposed fix
const makeHookCtx = ( hookPoint: "beforeRun" | "afterComplete" | "onFailure" | "afterRun", extra?: Partial<CronHookContext>, ): CronHookContext => ({ hookPoint, workflow: "cron", job: { id: job.id, name: job.name, agentId: job.agentId, schedule: job.schedule }, - payload: job.payload, + payload: structuredClone(job.payload), meta: hookMeta, log: state.deps.log, basePath: hookBasePath, ...extra, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/service/timer.ts` around lines 1025 - 1037, The makeHookCtx factory is passing job.payload by reference which allows startup catch-up hooks to mutate the live payload; update makeHookCtx (used to build CronHookContext) to assign payload: structuredClone(job.payload) instead of payload: job.payload so the hook context always receives an immutable copy (consistent with the main timer path that uses structuredClone(job.payload)); keep the rest of the returned object and preserve optional extra merging.src/cli/cron-cli/register.cron-add.ts (2)
313-365:⚠️ Potential issue | 🟠 MajorReject delivery targeting flags unless mode is actually
announce.Current validation only blocks targeting flags when
deliveryModeisundefined. With--no-deliver,deliveryModebecomes"none", so--to/--channel/--account/--best-effort-deliverare accepted even though announce is disabled.Proposed fix
- // When no delivery mode is active, delivery-targeting flags have nowhere to go. - if (!deliveryMode) { + // Delivery-targeting flags only make sense in announce mode. + if (deliveryMode !== "announce") { const hasExplicitDeliveryFlags = (typeof opts.to === "string" && opts.to.trim().length > 0) || Boolean(accountId) || opts.bestEffortDeliver === true || optionSource("channel") === "cli"; if (hasExplicitDeliveryFlags) { throw new Error( "Delivery flags (--to, --channel, --account, --best-effort-deliver) require --announce. Add --announce to enable delivery.", ); } } @@ delivery: deliveryMode ? { mode: deliveryMode, channel: - typeof opts.channel === "string" && opts.channel.trim() + deliveryMode === "announce" && + typeof opts.channel === "string" && + opts.channel.trim() ? opts.channel.trim() : undefined, - to: typeof opts.to === "string" && opts.to.trim() ? opts.to.trim() : undefined, - accountId, - bestEffort: opts.bestEffortDeliver ? true : undefined, + to: + deliveryMode === "announce" && + typeof opts.to === "string" && + opts.to.trim() + ? opts.to.trim() + : undefined, + accountId: deliveryMode === "announce" ? accountId : undefined, + bestEffort: + deliveryMode === "announce" && opts.bestEffortDeliver ? true : undefined, } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/cron-cli/register.cron-add.ts` around lines 313 - 365, The validation incorrectly only rejects delivery-targeting flags when deliveryMode is undefined; update the check around deliveryMode/hasExplicitDeliveryFlags so it rejects those flags unless deliveryMode is explicitly "announce" (i.e., treat both undefined and "none"/other non-"announce" values as invalid for targeting flags). Locate the logic that computes deliveryMode and the hasExplicitDeliveryFlags/optionSource checks and ensure the error condition uses something like deliveryMode !== "announce" instead of !deliveryMode so params.delivery remains set only when mode === "announce".
103-103:⚠️ Potential issue | 🟡 MinorUpdate
--timeout-secondshelp text to match behavior.Line 103 says timeout is “for agent jobs”, but script payloads also consume
timeoutSeconds(Lines 215–223). This is now misleading CLI UX.Also applies to: 215-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/cron-cli/register.cron-add.ts` at line 103, Update the .option("--timeout-seconds <n>", ...) help text in register.cron-add.ts to reflect that the timeout applies to both agent jobs and script payloads (e.g., "Timeout seconds for agent jobs and script payloads") and make the same wording consistent with the payload field timeoutSeconds used in the code paths around the script payload construction (lines referencing timeoutSeconds) so the CLI description accurately matches runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/cli/cron-cli/register.cron-add.ts`:
- Around line 313-365: The validation incorrectly only rejects
delivery-targeting flags when deliveryMode is undefined; update the check around
deliveryMode/hasExplicitDeliveryFlags so it rejects those flags unless
deliveryMode is explicitly "announce" (i.e., treat both undefined and
"none"/other non-"announce" values as invalid for targeting flags). Locate the
logic that computes deliveryMode and the hasExplicitDeliveryFlags/optionSource
checks and ensure the error condition uses something like deliveryMode !==
"announce" instead of !deliveryMode so params.delivery remains set only when
mode === "announce".
- Line 103: Update the .option("--timeout-seconds <n>", ...) help text in
register.cron-add.ts to reflect that the timeout applies to both agent jobs and
script payloads (e.g., "Timeout seconds for agent jobs and script payloads") and
make the same wording consistent with the payload field timeoutSeconds used in
the code paths around the script payload construction (lines referencing
timeoutSeconds) so the CLI description accurately matches runtime behavior.
In `@src/cron/service/timer.ts`:
- Around line 1025-1037: The makeHookCtx factory is passing job.payload by
reference which allows startup catch-up hooks to mutate the live payload; update
makeHookCtx (used to build CronHookContext) to assign payload:
structuredClone(job.payload) instead of payload: job.payload so the hook context
always receives an immutable copy (consistent with the main timer path that uses
structuredClone(job.payload)); keep the rest of the returned object and preserve
optional extra merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94dd4193-d501-4818-918f-473f067bb94a
📒 Files selected for processing (3)
src/cli/cron-cli/register.cron-add.tssrc/cron/service/ops.tssrc/cron/service/timer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cron/service/ops.ts
- timer.ts catch-up makeHookCtx: apply structuredClone(job.payload) (indentation mismatch caused replace_all to miss it) - register.cron-add: reject delivery-targeting flags when deliveryMode !== "announce" (was !deliveryMode, missing the --no-deliver/"none" case)
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cron/service/timer.ts`:
- Around line 1194-1198: The script branch currently returns execCronScript(...)
raw result which only contains status/summary/error and therefore never runs the
announce/delivery pipeline, leaving delivered/deliveryAttempted unset; after
calling execCronScript({ payload: job.payload, basePath, abortSignal }) invoke
the same delivery flow used for isolated jobs (the announce delivery path) to
attempt announcement delivery, capture its metadata (at minimum delivered and
deliveryAttempted) and merge those fields into the returned result before
returning; update the return value from execCronScript to include the delivery
metadata so applyJobResult() can correctly mark delivery state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a69f223f-61b6-4b26-9541-e8aabc60548d
📒 Files selected for processing (2)
src/cli/cron-cli/register.cron-add.tssrc/cron/service/timer.ts
Owner
Author
|
Deferring script delivery to #21 — there is no existing |
3 tasks
This was referenced Mar 15, 2026
14 tasks
Arry8
added a commit
that referenced
this pull request
Mar 16, 2026
* cron: add script payload kind for deterministic job execution - Add CronScriptPayload type (command, args, env, cwd, timeoutSeconds, deliver) - New exec-script.ts: execCronScript() via child_process.execFile (no shell injection) - Relative paths resolved against OC home (basePath), not process.cwd() - Timeout/abort via AbortSignal; stdout as summary, stderr as error on non-zero exit - Add script branch in executeJobCore (before session routing — no session needed) - Gateway protocol schema: CronScriptPayloadSchema in CronPayloadSchema union - CLI cron add: --script, --script-arg (repeatable), --script-env KEY=VALUE, --script-cwd - normalize.ts: coerce kind='script' passthrough - service/jobs.ts: mergeCronPayload + buildPayloadFromPatch handle script kind - service/normalize.ts: normalizePayloadToSystemText handles script kind - Unit tests: 9 cases covering execution, args, env, cwd, relative paths, timeout, abort Closes #11 * cron: expose payload on CronHookContext for middleware hooks beforeRun/afterComplete/onFailure/afterRun hooks now receive ctx.payload so they can inspect the job's payload kind, command, message, etc. and make decisions (abort, audit, conditional logic) without guessing from the job name. - Add payload: CronPayload to CronHookContext type (hooks.ts) - Pass payload: job.payload in both makeHookCtx closures in timer.ts - Update hooks.test.ts: makeCtx accepts optional payload arg; add 3 tests for payload visibility (agentTurn kind check, script command check, allowed command does not abort) * cron: fix script payload validation, delivery, and hook context gaps - assertSupportedJobSpec: allow script kind on isolated/current/session targets - mergeCronPayload: add script merge branch to preserve existing fields on partial patch - CronPayloadPatchSchema: use partial script schema so command is not required on update - register.cron-add: build delivery object for script jobs when --announce/--no-deliver passed - exec-script: wait for process close before settling; add SIGKILL grace period after SIGTERM - timer.ts catch-up makeHookCtx: add missing payload field * cron: improve CLI error messages and fix missing payload in hook contexts - register.cron-add: clarify session target error messages to name invalid flags - register.cron-add: extend main-session check to cover script kind (not just agentTurn) - hooks.ts: fix resolveUserPath called with 4 args; resolve relative paths against basePath separately - ops.ts makeHookCtx: add missing payload field (same gap as timer.ts catch-up path) * cron: harden script job CLI validation and freeze hook payload snapshot - register.cron-add: throw on malformed --script-env entries instead of silently skipping - register.cron-add: throw when delivery flags (--to/--channel/--account/--best-effort-deliver) are passed without --announce - ops.ts + timer.ts: pass structuredClone(payload) to makeHookCtx so hooks cannot mutate the live payload and affect execution * cron: fix remaining structuredClone gap and tighten delivery flag guard - timer.ts catch-up makeHookCtx: apply structuredClone(job.payload) (indentation mismatch caused replace_all to miss it) - register.cron-add: reject delivery-targeting flags when deliveryMode !== "announce" (was !deliveryMode, missing the --no-deliver/"none" case) * cron: document script delivery deferral with TODO comment
Arry8
added a commit
that referenced
this pull request
Mar 16, 2026
Squash of feature/11-cron-script-payload improvements on top of PR #17. Changes: - Normalize: trim command/cwd in coercePayload, delete whitespace-only command - Delivery pipeline: wire announceScriptOutput dep so --announce works for script jobs - Dashboard UI: add script payload kind to cron form (command, args, env, cwd fields) - CronPayloadPatch: require kind discriminant for script variant - Interpreter detection: auto-detect sh/bash/zsh/node/bun/python3/ruby from extension — no chmod +x needed - cwd/command validation: clearer errors for missing cwd and empty command - Delivery error handling: log failures instead of silently swallowing them - CLI: simplify sessionTarget ternary; better error messages for session/payload conflicts - Tests: abort timeout 50ms→500ms for CI reliability; add no-executable-bit test Closes #21, #23, #24 Related #26 (Windows script formats — tracked separately) AI-assisted (Claude Code)
Arry8
added a commit
that referenced
this pull request
Mar 19, 2026
* cron: add script payload kind for deterministic job execution - Add CronScriptPayload type (command, args, env, cwd, timeoutSeconds, deliver) - New exec-script.ts: execCronScript() via child_process.execFile (no shell injection) - Relative paths resolved against OC home (basePath), not process.cwd() - Timeout/abort via AbortSignal; stdout as summary, stderr as error on non-zero exit - Add script branch in executeJobCore (before session routing — no session needed) - Gateway protocol schema: CronScriptPayloadSchema in CronPayloadSchema union - CLI cron add: --script, --script-arg (repeatable), --script-env KEY=VALUE, --script-cwd - normalize.ts: coerce kind='script' passthrough - service/jobs.ts: mergeCronPayload + buildPayloadFromPatch handle script kind - service/normalize.ts: normalizePayloadToSystemText handles script kind - Unit tests: 9 cases covering execution, args, env, cwd, relative paths, timeout, abort Closes #11 * cron: expose payload on CronHookContext for middleware hooks beforeRun/afterComplete/onFailure/afterRun hooks now receive ctx.payload so they can inspect the job's payload kind, command, message, etc. and make decisions (abort, audit, conditional logic) without guessing from the job name. - Add payload: CronPayload to CronHookContext type (hooks.ts) - Pass payload: job.payload in both makeHookCtx closures in timer.ts - Update hooks.test.ts: makeCtx accepts optional payload arg; add 3 tests for payload visibility (agentTurn kind check, script command check, allowed command does not abort) * cron: fix script payload validation, delivery, and hook context gaps - assertSupportedJobSpec: allow script kind on isolated/current/session targets - mergeCronPayload: add script merge branch to preserve existing fields on partial patch - CronPayloadPatchSchema: use partial script schema so command is not required on update - register.cron-add: build delivery object for script jobs when --announce/--no-deliver passed - exec-script: wait for process close before settling; add SIGKILL grace period after SIGTERM - timer.ts catch-up makeHookCtx: add missing payload field * cron: improve CLI error messages and fix missing payload in hook contexts - register.cron-add: clarify session target error messages to name invalid flags - register.cron-add: extend main-session check to cover script kind (not just agentTurn) - hooks.ts: fix resolveUserPath called with 4 args; resolve relative paths against basePath separately - ops.ts makeHookCtx: add missing payload field (same gap as timer.ts catch-up path) * cron: harden script job CLI validation and freeze hook payload snapshot - register.cron-add: throw on malformed --script-env entries instead of silently skipping - register.cron-add: throw when delivery flags (--to/--channel/--account/--best-effort-deliver) are passed without --announce - ops.ts + timer.ts: pass structuredClone(payload) to makeHookCtx so hooks cannot mutate the live payload and affect execution * cron: fix remaining structuredClone gap and tighten delivery flag guard - timer.ts catch-up makeHookCtx: apply structuredClone(job.payload) (indentation mismatch caused replace_all to miss it) - register.cron-add: reject delivery-targeting flags when deliveryMode !== "announce" (was !deliveryMode, missing the --no-deliver/"none" case) * cron: document script delivery deferral with TODO comment
Arry8
added a commit
that referenced
this pull request
Mar 19, 2026
Squash of feature/11-cron-script-payload improvements on top of PR #17. Changes: - Normalize: trim command/cwd in coercePayload, delete whitespace-only command - Delivery pipeline: wire announceScriptOutput dep so --announce works for script jobs - Dashboard UI: add script payload kind to cron form (command, args, env, cwd fields) - CronPayloadPatch: require kind discriminant for script variant - Interpreter detection: auto-detect sh/bash/zsh/node/bun/python3/ruby from extension — no chmod +x needed - cwd/command validation: clearer errors for missing cwd and empty command - Delivery error handling: log failures instead of silently swallowing them - CLI: simplify sessionTarget ternary; better error messages for session/payload conflicts - Tests: abort timeout 50ms→500ms for CI reliability; add no-executable-bit test Closes #21, #23, #24 Related #26 (Windows script formats — tracked separately) AI-assisted (Claude Code)
14 tasks
Arry8
added a commit
that referenced
this pull request
Mar 23, 2026
* cron: add script payload kind for deterministic job execution - Add CronScriptPayload type (command, args, env, cwd, timeoutSeconds, deliver) - New exec-script.ts: execCronScript() via child_process.execFile (no shell injection) - Relative paths resolved against OC home (basePath), not process.cwd() - Timeout/abort via AbortSignal; stdout as summary, stderr as error on non-zero exit - Add script branch in executeJobCore (before session routing — no session needed) - Gateway protocol schema: CronScriptPayloadSchema in CronPayloadSchema union - CLI cron add: --script, --script-arg (repeatable), --script-env KEY=VALUE, --script-cwd - normalize.ts: coerce kind='script' passthrough - service/jobs.ts: mergeCronPayload + buildPayloadFromPatch handle script kind - service/normalize.ts: normalizePayloadToSystemText handles script kind - Unit tests: 9 cases covering execution, args, env, cwd, relative paths, timeout, abort Closes #11 * cron: expose payload on CronHookContext for middleware hooks beforeRun/afterComplete/onFailure/afterRun hooks now receive ctx.payload so they can inspect the job's payload kind, command, message, etc. and make decisions (abort, audit, conditional logic) without guessing from the job name. - Add payload: CronPayload to CronHookContext type (hooks.ts) - Pass payload: job.payload in both makeHookCtx closures in timer.ts - Update hooks.test.ts: makeCtx accepts optional payload arg; add 3 tests for payload visibility (agentTurn kind check, script command check, allowed command does not abort) * cron: fix script payload validation, delivery, and hook context gaps - assertSupportedJobSpec: allow script kind on isolated/current/session targets - mergeCronPayload: add script merge branch to preserve existing fields on partial patch - CronPayloadPatchSchema: use partial script schema so command is not required on update - register.cron-add: build delivery object for script jobs when --announce/--no-deliver passed - exec-script: wait for process close before settling; add SIGKILL grace period after SIGTERM - timer.ts catch-up makeHookCtx: add missing payload field * cron: improve CLI error messages and fix missing payload in hook contexts - register.cron-add: clarify session target error messages to name invalid flags - register.cron-add: extend main-session check to cover script kind (not just agentTurn) - hooks.ts: fix resolveUserPath called with 4 args; resolve relative paths against basePath separately - ops.ts makeHookCtx: add missing payload field (same gap as timer.ts catch-up path) * cron: harden script job CLI validation and freeze hook payload snapshot - register.cron-add: throw on malformed --script-env entries instead of silently skipping - register.cron-add: throw when delivery flags (--to/--channel/--account/--best-effort-deliver) are passed without --announce - ops.ts + timer.ts: pass structuredClone(payload) to makeHookCtx so hooks cannot mutate the live payload and affect execution * cron: fix remaining structuredClone gap and tighten delivery flag guard - timer.ts catch-up makeHookCtx: apply structuredClone(job.payload) (indentation mismatch caused replace_all to miss it) - register.cron-add: reject delivery-targeting flags when deliveryMode !== "announce" (was !deliveryMode, missing the --no-deliver/"none" case) * cron: document script delivery deferral with TODO comment
Arry8
added a commit
that referenced
this pull request
Mar 23, 2026
Squash of feature/11-cron-script-payload improvements on top of PR #17. Changes: - Normalize: trim command/cwd in coercePayload, delete whitespace-only command - Delivery pipeline: wire announceScriptOutput dep so --announce works for script jobs - Dashboard UI: add script payload kind to cron form (command, args, env, cwd fields) - CronPayloadPatch: require kind discriminant for script variant - Interpreter detection: auto-detect sh/bash/zsh/node/bun/python3/ruby from extension — no chmod +x needed - cwd/command validation: clearer errors for missing cwd and empty command - Delivery error handling: log failures instead of silently swallowing them - CLI: simplify sessionTarget ternary; better error messages for session/payload conflicts - Tests: abort timeout 50ms→500ms for CI reliability; add no-executable-bit test Closes #21, #23, #24 Related #26 (Windows script formats — tracked separately) AI-assisted (Claude Code)
Arry8
pushed a commit
that referenced
this pull request
Apr 5, 2026
* feat: add QQ Bot channel extension * fix(qqbot): add setupWizard to runtime plugin for onboard re-entry * fix: fix review * fix: fix review * chore: sync lockfile and config-docs baseline for qqbot extension * refactor: 移除图床服务器相关代码 * fix * docs: 新增 QQ Bot 插件文档并修正链接路径 * refactor: remove credential backup functionality and update setup logic - Deleted the credential backup module to streamline the codebase. - Updated the setup surface to handle client secrets more robustly, allowing for configured secret inputs. - Simplified slash commands by removing unused hot upgrade compatibility checks and related functions. - Adjusted types to use SecretInput for client secrets in QQBot configuration. - Modified bundled plugin metadata to allow additional properties in the config schema. * feat: 添加本地媒体路径解析功能，修正 QQBot 媒体路径处理 * feat: 添加本地媒体路径解析功能，修正 QQBot 媒体路径处理 * feat: remove qqbot-media and qqbot-remind skills, add tests for config and setup - Deleted the qqbot-media and qqbot-remind skills documentation files. - Added unit tests for qqbot configuration and setup processes, ensuring proper handling of SecretRef-backed credentials and account configurations. - Implemented tests for local media path remapping, verifying correct resolution of media file paths. - Removed obsolete channel and remind tools, streamlining the codebase. * feat: 更新 QQBot 配置模式，添加音频格式和账户定义 * feat: 添加 QQBot 频道管理和定时提醒技能，更新媒体路径解析功能 * fix * feat: 添加 /bot-upgrade 指令以查看 QQBot 插件升级指引 * feat: update reminder and qq channel skills * feat: 更新remind工具投递目标地址格式 * feat: Refactor QQBot payload handling and improve code documentation - Simplified and clarified the structure of payload interfaces for Cron reminders and media messages. - Enhanced the parsing function to provide clearer error messages and improved validation. - Updated platform utility functions for better cross-platform compatibility and clearer documentation. - Improved text parsing utilities for better readability and consistency in emoji representation. - Optimized upload cache management with clearer comments and reduced redundancy. - Integrated QQBot plugin into the bundled channel plugins and updated metadata for installation. * OK apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift > openclaw@2026.3.26 check:bundled-channel-config-metadata /Users/yuehuali/code/PR/openclaw > node --import tsx scripts/generate-bundled-channel-config-metadata.ts --check [bundled-channel-config-metadata] stale generated output at src/config/bundled-channel-config-metadata.generated.ts ELIFECYCLE Command failed with exit code 1. ELIFECYCLE Command failed with exit code 1. * feat: 添加 QQBot 渠道配置及相关账户设置 * fix(qqbot): resolve 14 high-priority bugs from PR openclaw#52986 review DM routing (7 fixes): - #1: DM slash-command replies use sendDmMessage(guildId) instead of sendC2CMessage(senderId) - #2: DM qualifiedTarget uses qqbot:dm:${guildId} instead of qqbot:c2c:${senderId} - #3: sendTextChunks adds DM branch - #4: sendMarkdownReply adds DM branch for text and Base64 images - #5: parseAndSendMediaTags maps DM to targetType:dm + guildId - #6: sendTextToTarget DM branch uses sendDmMessage; MessageTarget adds guildId field - #7: handleImage/Audio/Video/FilePayload add DM branches Other high-priority fixes: - #8: Fix sendC2CVoiceMessage/sendGroupVoiceMessage parameter misalignment - #9: broadcastMessage uses groupOpenid instead of member_openid for group users - #10: Unify KnownUser storage - proactive.ts delegates to known-users.ts - #11: Remove invalid recordKnownUser calls for guild/DM users - #12: sendGroupMessage uses sendAndNotify to trigger onMessageSent hook - #13: sendPhoto channel unsupported returns error field - #14: sendTextAfterMedia adds channel and dm branches Type fixes: - DeliverEventContext adds guildId field - MediaTargetContext.targetType adds dm variant - sendPlainTextReply imgMediaTarget adds DM branch * fix(qqbot): resolve 2 blockers + 7 medium-priority bugs from PR openclaw#52986 review Blocker-1: Remove unused dmPolicy config knob - dmPolicy was declared in schema/types/plugin.json but never consumed at runtime - Removed from config-schema.ts, types.ts, and openclaw.plugin.json - allowFrom remains active (already wired into framework command-auth) Blocker-2: Gate sensitive slash commands with allowFrom authorization - SlashCommand interface adds requireAuth?: boolean - SlashCommandContext adds commandAuthorized: boolean - /bot-logs set to requireAuth: true (reads local log files) - matchSlashCommand rejects unauthorized senders for requireAuth commands - trySlashCommandOrEnqueue computes commandAuthorized from allowFrom config Medium-priority fixes: - #15: Strip non-HTTP/non-local markdown image tags to prevent path leakage - #16: applyQQBotAccountConfig clears clientSecret when setting clientSecretFile and vice versa - #17: getAdminMarkerFile sanitizes accountId to prevent path traversal - #18: URGENT_COMMANDS uses exact match instead of startsWith prefix match - #19: isCronExpression validates each token starts with a cron-valid character - #20: --token format validation rejects malformed input without colon separator - #21: resolveDefaultQQBotAccountId checks QQBOT_APP_ID environment variable * test(qqbot): add focused tests for slash command authorization path - Unauthorized sender rejected for /bot-logs (requireAuth: true) - Authorized sender allowed for /bot-logs - Non-requireAuth commands (/bot-ping, /bot-help, /bot-version) work for all senders - Unknown slash commands return null (passthrough) - Non-slash messages return null - Usage query (/bot-logs ?) also gated by auth check * fix(qqbot): align global TTS fallback with framework config resolution - Extract isGlobalTTSAvailable to utils/audio-convert.ts, mirroring core resolveTtsConfig logic: check auto !== 'off', fall back to legacy enabled boolean, default to off when neither is set. - Add pre-check in reply-dispatcher before calling globalTextToSpeech to avoid unnecessary TTS calls and noisy error logs when TTS is not configured. - Remove inline as any casts; use OpenClawConfig type throughout. - Refactor handleAudioPayload into flat early-return structure with unified send path (plugin TTS → global fallback → send). * fix(qqbot): break ESM circular dependency causing multi-account startup crash The bundled gateway chunk had a circular static import on the channel chunk (gateway -> outbound-deliver -> channel, while channel dynamically imports gateway). When two accounts start concurrently via Promise.all, the first dynamic import triggers module graph evaluation; the circular reference causes api exports (including runDiagnostics) to resolve as undefined before the module finishes evaluating. Fix: extract chunkText and TEXT_CHUNK_LIMIT from channel.ts into a new text-utils.ts leaf module. outbound-deliver.ts now imports from text-utils.ts, breaking the cycle. channel.ts re-exports for backward compatibility. * fix(qqbot): serialize gateway module import to prevent multi-account startup race When multiple accounts start concurrently via Promise.all, each calls await import('./gateway.js') independently. Due to ESM circular dependencies in the bundled output, the first import can resolve transitive exports as undefined before module evaluation completes. Fix: cache the dynamic import promise in a module-level variable so all concurrent startAccount calls share the same import, ensuring the gateway module is fully evaluated before any account uses it. * refactor(qqbot): remove startup greeting logic Remove getStartupGreetingPlan and related startup greeting delivery: - Delete startup-greeting.ts (greeting plan, marker persistence) - Delete admin-resolver.ts (admin resolution, greeting dispatch) - Remove startup greeting calls from gateway READY/RESUMED handlers - Remove isFirstReadyGlobal flag and adminCtx * fix(qqbot): skip octal escape decoding for Windows local paths Windows paths like C:\Users\1\file.txt contain backslash-digit sequences that were incorrectly matched as octal escape sequences and decoded, corrupting the file path. Detect Windows local paths (drive letter or UNC prefix) and skip the octal decoding step for them. * fix bot issue * feat: 支持 TTS 自动开关并清理配置中的 clientSecretFile * docs: 添加 QQBot 配置和消息处理的设计说明 * rebase * fix(qqbot): align slash-command auth with shared command-auth model Route requireAuth:true slash commands (e.g. /bot-logs) through the framework's api.registerCommand() so resolveCommandAuthorization() applies commands.allowFrom.qqbot precedence and qqbot: prefix normalization before any handler runs. - slash-commands.ts: registerCommand() now auto-routes by requireAuth into two maps (commands / frameworkCommands); getFrameworkCommands() exports the auth-required set for framework registration; bot-help lists both maps - index.ts: registerFull() iterates getFrameworkCommands() and calls api.registerCommand() for each; handler derives msgType from ctx.from, sends file attachments via sendDocument, supports multi-account via ctx.accountId - gateway.ts (inbound): replace raw allowFrom string comparison with qqbotPlugin.config.formatAllowFrom() to strip qqbot: prefix and uppercase before matching event.senderId - gateway.ts (pre-dispatch): remove stale auth computation; commandAuthorized is true (requireAuth:true commands never reach matchSlashCommand) - command-auth.test.ts: add regression tests for qqbot: prefix normalization in the inbound commandAuthorized computation - slash-commands.test.ts: update /bot-logs tests to expect null (command routed to framework, not in local registry) * rebase and solve conflict * fix(qqbot): preserve mixed env setup credentials --------- Co-authored-by: yuehuali <yuehuali@tencent.com> Co-authored-by: walli <walli@tencent.com> Co-authored-by: WideLee <limkuan24@gmail.com> Co-authored-by: Frank Yang <frank.ekn@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
payload.kind: "script"to cron jobs — executes scripts directly viachild_process.execFile, no LLM turn, no token costctx.payloadonCronHookContextso existing middleware hooks (beforeRun,afterComplete,onFailure,afterRun) can inspect and act on the payload kind/command/messageCloses #11
Changes
payload.kind: "script"(commit 869a40d)src/cron/types.ts—CronScriptPayloadtype (command,args,env,cwd,timeoutSeconds,deliver); updatedCronPayloadandCronPayloadPatchunionssrc/cron/exec-script.ts— newexecCronScript(): resolves paths viaresolveUserPath, validates file exists, spawns viaexecFile(no shell injection), captures stdout assummary, stderr aserroron non-zero exit, honoursAbortSignalfor timeoutsrc/cron/service/timer.ts— script branch inexecuteJobCorefires before session routing; scripts bypass LLM/session, return torunDueJobfor shared hooks/delivery/statesrc/gateway/protocol/schema/cron.ts—CronScriptPayloadSchemaadded toCronPayloadSchemaandCronPayloadPatchSchemaunionssrc/cli/cron-cli/register.cron-add.ts—--script,--script-arg(repeatable),--script-env KEY=VALUE(repeatable),--script-cwdflagssrc/cron/normalize.ts—coercePayloadpasses throughkind: "script"unchangedsrc/cron/service/jobs.ts—mergeCronPayloadandbuildPayloadFromPatchhandlescriptkindsrc/cron/service/normalize.ts—normalizePayloadToSystemTexthandlesscriptkindsrc/cron/exec-script.test.ts— 9 unit testsMiddleware hook payload visibility (commit 19499c4)
src/cron/hooks.ts—CronHookContextgainspayload: CronPayloadsrc/cron/service/timer.ts— bothmakeHookCtxclosures (scheduled + catch-up) passpayload: job.payloadsrc/cron/hooks.test.ts—makeCtxaccepts optional payload; 3 new tests forbeforeRunhooks inspectingpayload.kindandpayload.commandArchitecture
Script jobs go through the full
runDueJobpipeline:beforeRun/afterComplete/onFailure/afterRunhooks ✓resolveCronJobTimeoutMs) ✓lastRunStatus,lastDurationMs, failure alerts) ✓deliverflag) ✓They bypass only session routing and LLM context — by design.
Test plan
src/cron/exec-script.test.ts— 9 passing (stdout, stderr, args, env, relative paths, missing file, abort)src/cron/hooks.test.ts— 28 passing (all existing + 3 new payload visibility tests)openclaw cron add --name test --every 1m --script ~/scripts/foo.shAI-assisted
Generated with Claude Code (claude-sonnet-4-6).
Summary by CodeRabbit
New Features
Bug Fixes
Tests