Merged
Conversation
Add a hook system that runs user-defined scripts at cron job lifecycle points (beforeRun, afterComplete, onFailure, afterRun). Hooks are configured via openclaw.json cron.hooks (global) and per-job hooks in the job store. Supports priority ordering, job/agent filtering, skipGlobal overrides, and abort capability on beforeRun hooks.
- Enforce workflow filter in matchesFilter() instead of leaving it as a no-op. The workflow string is now threaded from loadHookEntries. - Treat undefined job.agentId as non-matching when filter.agentId is set, preventing hooks from firing on unintended agent-less jobs. - Clear setTimeout handle after hook completion via createTimeout() pattern, preventing stale timer accumulation. - Add tests for all three fixes (14 total).
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughAdds a cron lifecycle hook system: new hook types and schema, hook loader/runner with filtering, priorities and timeouts, path validation, and integration into scheduled/manual cron flows where beforeRun can abort and afterComplete/onFailure/afterRun are invoked; includes comprehensive tests. Changes
sequenceDiagram
participant Timer as Timer Service
participant HookMgr as Hook Manager
participant HookMod as Hook Module
participant Job as Job Execution
rect rgba(100, 150, 200, 0.5)
Note over Timer,HookMgr: beforeRun Phase
Timer->>HookMgr: loadHookEntries(beforeRun, cronConfig, job, workflow?)
HookMgr-->>Timer: filtered & sorted entries
Timer->>HookMod: runCronHooks(beforeRun, ctx, entries)
HookMod->>HookMod: execute hook scripts sequentially
alt Hook returns abort
HookMod-->>Timer: { abort: true, reason }
Timer->>HookMgr: loadHookEntries(afterRun, ...)
Timer->>HookMod: runCronHooks(afterRun, ctx(skipped))
HookMod-->>Timer: afterRun result
Timer-->>Timer: return skipped result
else No abort
HookMod-->>Timer: continue
end
end
rect rgba(150, 200, 100, 0.5)
Note over Timer,Job: Job Execution
Timer->>Job: execute cron job
Job-->>Timer: result (success/error)
end
rect rgba(200, 150, 100, 0.5)
Note over Timer,HookMgr: afterComplete / onFailure Phase
alt Job succeeded
Timer->>HookMgr: loadHookEntries(afterComplete, ...)
Timer->>HookMod: runCronHooks(afterComplete, ctx)
else Job failed
Timer->>HookMgr: loadHookEntries(onFailure, ...)
Timer->>HookMod: runCronHooks(onFailure, ctx)
end
HookMod-->>Timer: result (logged, non-fatal)
end
rect rgba(200, 100, 150, 0.5)
Note over Timer,HookMod: afterRun Phase (Always)
Timer->>HookMgr: loadHookEntries(afterRun, ...)
Timer->>HookMod: runCronHooks(afterRun, ctx)
HookMod-->>Timer: result (logged, non-fatal)
Timer-->>Timer: return final job result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cron/service/timer.ts (1)
700-733: Clarify "fire-and-forget" comment.The comment on line 700 says "fire-and-forget, errors logged" but the hooks are awaited. While this is technically correct since
runCronHookshandles errors internally, the comment could be misleading to future maintainers.📝 Suggested comment clarification
- // afterComplete / onFailure hooks (fire-and-forget, errors logged). + // afterComplete / onFailure hooks (errors caught internally, do not affect job outcome).🤖 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 700 - 733, The "fire-and-forget, errors logged" comment is misleading because the hook invocations (loadHookEntries, runCronHooks with makeHookCtx for afterComplete/onFailure/afterRun) are awaited; update the comment to state that hooks are awaited but runCronHooks handles errors internally and logs them (i.e., "awaited hooks; errors are caught and logged by runCronHooks"), so future readers understand the hooks run to completion and errors are not propagated.
🤖 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/hooks.test.ts`:
- Around line 25-30: The module-scoped noopLog (debug/info/warn/error) uses
vi.fn() mocks that can leak call state across tests; update the test setup to
either call vi.clearAllMocks() (or vi.resetAllMocks()) in a beforeEach hook or
recreate noopLog inside a beforeEach so each test gets fresh vi.fn() instances,
and ensure tests referencing noopLog (e.g., assertions on noopLog.warn) rely on
those cleared/reset mocks.
---
Nitpick comments:
In `@src/cron/service/timer.ts`:
- Around line 700-733: The "fire-and-forget, errors logged" comment is
misleading because the hook invocations (loadHookEntries, runCronHooks with
makeHookCtx for afterComplete/onFailure/afterRun) are awaited; update the
comment to state that hooks are awaited but runCronHooks handles errors
internally and logs them (i.e., "awaited hooks; errors are caught and logged by
runCronHooks"), so future readers understand the hooks run to completion and
errors are not propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72d114c8-c0a5-4c52-80aa-c7745a595c75
📒 Files selected for processing (6)
src/config/types.cron.tssrc/config/zod-schema.tssrc/cron/hooks.test.tssrc/cron/hooks.tssrc/cron/service/timer.tssrc/cron/types.ts
- runStartupCatchupCandidate now runs the full beforeRun/afterComplete/ onFailure/afterRun hook lifecycle, matching runDueJob behavior. Previously, jobs replayed after a gateway restart silently bypassed all hooks, creating inconsistent auditing behavior. - loadHookModule checks path.isAbsolute() before the URL-scheme regex so Windows drive-letter paths (e.g. C:\hooks\audit.cjs) are treated as file paths rather than URL specifiers.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hooks.ts`:
- Around line 85-102: Move the timeout creation to before loading and executing
the hook so slow imports/top-level awaits are bounded: create an
AbortController/timeout via createTimeout(HOOK_TIMEOUT_MS) (or extend
createTimeout to return an AbortSignal) before calling
loadHookModule(entry.script), pass the resulting AbortSignal on the hook
invocation/context (ctx) so the hook function (hookFn) can detect cancellation,
and ensure timeout.clear() is still called in a finally block; also update any
loadHookModule/hookFn invocation to respect/forward the signal so a timed-out
hook can stop performing side effects.
In `@src/cron/service/timer.ts`:
- Around line 637-736: Extract the duplicated hook orchestration and core
execution logic into a single helper (e.g., runCronWithHooks) that accepts
(state, job, executeCoreFn) and performs: making hook context (reuse
makeHookCtx), running beforeRun (and abort handling + afterRun-on-abort),
invoking executeJobCoreWithTimeout (or provided executeCoreFn) with consistent
timeout normalization/logging, firing afterComplete/onFailure based on result,
then always firing afterRun; replace the duplicated blocks in this file and in
runStartupCatchupCandidate() and ensure executeJob() calls this new helper so
all entrypoints run identical hooks and logging behavior (use loadHookEntries,
runCronHooks, and preserve coreResult shape).
- Around line 659-675: The afterRun hook context for the aborted path doesn't
include the abort reason, so update the call that builds the afterRun context
(the makeHookCtx invocation used with runCronHooks and afterRunEntries) to
include the abort error detail (e.g., add an error or reason property populated
from beforeResult.reason or the catch-up equivalent) alongside status and
durationMs; do the same change in the catch-up branch so the always-run audit
hook can see why the job was skipped (refer to loadHookEntries, afterRunEntries,
runCronHooks, makeHookCtx and beforeResult).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f246f073-3ffd-4c8e-ac98-263c081fdf98
📒 Files selected for processing (2)
src/cron/hooks.tssrc/cron/service/timer.ts
Comment on lines
+85
to
+102
| for (const entry of entries) { | ||
| try { | ||
| const hookFn = await loadHookModule(entry.script); | ||
| if (typeof hookFn !== "function") { | ||
| ctx.log.warn( | ||
| { hookPoint, script: entry.script }, | ||
| "cron hook: module does not export a function, skipping", | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| const timeout = createTimeout(HOOK_TIMEOUT_MS); | ||
| let result: unknown; | ||
| try { | ||
| result = await Promise.race([hookFn(ctx), timeout.promise]); | ||
| } finally { | ||
| timeout.clear(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does JavaScript Promise.race()cancel the losing promise, and does Node.jsimport()wait for an imported module's top-levelawait to finish before the returned promise resolves?
💡 Result:
-
Does
Promise.race()cancel the “losing” promise(s)?
No.Promise.race()just returns a new promise that settles when the first input promise settles; it does not cancel or stop the other async operations. JavaScript Promises don’t have built-in cancellation—cancellation has to be implemented at the operation level (e.g., viaAbortController, library-specific cancel tokens, etc.). [1][2] -
Does Node.js
import()wait for an imported module’s top-levelawaitbefore resolving?
Yes. With top-levelawait, a module’s evaluation becomes asynchronous, and importers wait for that module to finish evaluating (i.e., for its top-levelawaitto settle) before continuing / being considered finished. This is the intended ECMAScript behavior for modules with top-levelawait, and Node’s ESM docs reflect that module loading can be async (e.g., CommonJSrequire()only supports synchronous ES modules—i.e., ones without top-levelawait). [3][4][5]
Sources
[1] MDN – Promise.race() https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/race
[2] MDN – Using promises (cancellation note) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises
[3] TC39 – proposal-top-level-await (importers wait for evaluation) https://github.com/tc39/proposal-top-level-await
[4] MDN – await (Top level await behavior) https://developer.mozilla.org/docs/Web/JavaScript/Reference/Operators/await
[5] Node.js docs – ECMAScript modules (sync vs async ESM; top-level await implications) https://nodejs.org/docs/latest/api/esm.html
The timeout doesn't actually bound hook lifetime.
loadHookModule(entry.script) is awaited before createTimeout(), so a slow import or top-level await can block this loop indefinitely. After Promise.race() rejects due to timeout, the hook continues running because Promise.race() doesn't cancel the losing promise—only the caller stops waiting. A timed-out beforeRun hook can still mutate shared state or perform side effects after control has moved on.
Start the timeout before module loading and pass an AbortSignal into the hook context so the hook can detect and respect the timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/hooks.ts` around lines 85 - 102, Move the timeout creation to before
loading and executing the hook so slow imports/top-level awaits are bounded:
create an AbortController/timeout via createTimeout(HOOK_TIMEOUT_MS) (or extend
createTimeout to return an AbortSignal) before calling
loadHookModule(entry.script), pass the resulting AbortSignal on the hook
invocation/context (ctx) so the hook function (hookFn) can detect cancellation,
and ensure timeout.clear() is still called in a finally block; also update any
loadHookModule/hookFn invocation to respect/forward the signal so a timed-out
hook can stop performing side effects.
Comment on lines
+637
to
+736
| const hookMeta: Record<string, unknown> = {}; | ||
| 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 }, | ||
| meta: hookMeta, | ||
| log: state.deps.log, | ||
| ...extra, | ||
| }); | ||
|
|
||
| // beforeRun hooks — may abort the job. | ||
| const beforeEntries = loadHookEntries("beforeRun", state.deps.cronConfig, job); | ||
| if (beforeEntries.length > 0) { | ||
| const beforeResult = await runCronHooks( | ||
| "beforeRun", | ||
| makeHookCtx("beforeRun"), | ||
| beforeEntries, | ||
| ); | ||
| if (beforeResult.aborted) { | ||
| const abortEndedAt = state.deps.nowMs(); | ||
| const abortDurationMs = abortEndedAt - startedAt; | ||
| // afterRun hooks still fire on abort — they must always run. | ||
| const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); | ||
| if (afterRunEntries.length > 0) { | ||
| await runCronHooks( | ||
| "afterRun", | ||
| makeHookCtx("afterRun", { status: "skipped", durationMs: abortDurationMs }), | ||
| afterRunEntries, | ||
| ); | ||
| } | ||
| return { | ||
| jobId: id, | ||
| status: "skipped", | ||
| error: `hook aborted: ${beforeResult.reason ?? "unknown"}`, | ||
| startedAt, | ||
| endedAt: abortEndedAt, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| let coreResult: TimedCronRunOutcome; | ||
| try { | ||
| const result = await executeJobCoreWithTimeout(state, job); | ||
| return { jobId: id, ...result, startedAt, endedAt: state.deps.nowMs() }; | ||
| coreResult = { jobId: id, ...result, startedAt, endedAt: state.deps.nowMs() }; | ||
| } catch (err) { | ||
| const errorText = isAbortError(err) ? timeoutErrorMessage() : String(err); | ||
| state.deps.log.warn( | ||
| { jobId: id, jobName: job.name, timeoutMs: jobTimeoutMs ?? null }, | ||
| `cron: job failed: ${errorText}`, | ||
| ); | ||
| return { | ||
| coreResult = { | ||
| jobId: id, | ||
| status: "error", | ||
| error: errorText, | ||
| startedAt, | ||
| endedAt: state.deps.nowMs(), | ||
| }; | ||
| } | ||
|
|
||
| const durationMs = coreResult.endedAt - coreResult.startedAt; | ||
|
|
||
| // afterComplete / onFailure hooks (fire-and-forget, errors logged). | ||
| if (coreResult.status === "ok") { | ||
| const entries = loadHookEntries("afterComplete", state.deps.cronConfig, job); | ||
| if (entries.length > 0) { | ||
| await runCronHooks( | ||
| "afterComplete", | ||
| makeHookCtx("afterComplete", { status: "ok", durationMs }), | ||
| entries, | ||
| ); | ||
| } | ||
| } else if (coreResult.status === "error") { | ||
| const entries = loadHookEntries("onFailure", state.deps.cronConfig, job); | ||
| if (entries.length > 0) { | ||
| await runCronHooks( | ||
| "onFailure", | ||
| makeHookCtx("onFailure", { error: coreResult.error, status: "error", durationMs }), | ||
| entries, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // afterRun hooks — always fire regardless of outcome. | ||
| const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); | ||
| if (afterRunEntries.length > 0) { | ||
| await runCronHooks( | ||
| "afterRun", | ||
| makeHookCtx("afterRun", { | ||
| status: coreResult.status, | ||
| error: coreResult.error, | ||
| durationMs, | ||
| }), | ||
| afterRunEntries, | ||
| ); | ||
| } | ||
|
|
||
| return coreResult; |
There was a problem hiding this comment.
Move the lifecycle into one shared execution helper.
This block is duplicated here and in runStartupCatchupCandidate(), and the two copies already drifted on timeout normalization/logging. executeJob() later in the file also still skips hooks even though its own docstring says the run command and other callers use it, so the same job behaves differently depending on which entrypoint executed it. Pull the hook orchestration around core execution into one helper and reuse it everywhere.
🤖 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 637 - 736, Extract the duplicated
hook orchestration and core execution logic into a single helper (e.g.,
runCronWithHooks) that accepts (state, job, executeCoreFn) and performs: making
hook context (reuse makeHookCtx), running beforeRun (and abort handling +
afterRun-on-abort), invoking executeJobCoreWithTimeout (or provided
executeCoreFn) with consistent timeout normalization/logging, firing
afterComplete/onFailure based on result, then always firing afterRun; replace
the duplicated blocks in this file and in runStartupCatchupCandidate() and
ensure executeJob() calls this new helper so all entrypoints run identical hooks
and logging behavior (use loadHookEntries, runCronHooks, and preserve coreResult
shape).
Comment on lines
+659
to
+675
| const abortEndedAt = state.deps.nowMs(); | ||
| const abortDurationMs = abortEndedAt - startedAt; | ||
| // afterRun hooks still fire on abort — they must always run. | ||
| const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); | ||
| if (afterRunEntries.length > 0) { | ||
| await runCronHooks( | ||
| "afterRun", | ||
| makeHookCtx("afterRun", { status: "skipped", durationMs: abortDurationMs }), | ||
| afterRunEntries, | ||
| ); | ||
| } | ||
| return { | ||
| jobId: id, | ||
| status: "skipped", | ||
| error: `hook aborted: ${beforeResult.reason ?? "unknown"}`, | ||
| startedAt, | ||
| endedAt: abortEndedAt, |
There was a problem hiding this comment.
Include the abort reason in afterRun.
The returned outcome includes error: hook aborted: ..., but the afterRun context on this path only gets status: "skipped" and durationMs. That leaves the always-run audit hook blind to why the run was skipped. The same omission exists in the catch-up copy below.
💡 Proposed fix
if (beforeResult.aborted) {
const abortEndedAt = state.deps.nowMs();
const abortDurationMs = abortEndedAt - startedAt;
+ const abortError = `hook aborted: ${beforeResult.reason ?? "unknown"}`;
// afterRun hooks still fire on abort — they must always run.
const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job);
if (afterRunEntries.length > 0) {
await runCronHooks(
"afterRun",
- makeHookCtx("afterRun", { status: "skipped", durationMs: abortDurationMs }),
+ makeHookCtx("afterRun", {
+ status: "skipped",
+ error: abortError,
+ durationMs: abortDurationMs,
+ }),
afterRunEntries,
);
}
return {
jobId: id,
status: "skipped",
- error: `hook aborted: ${beforeResult.reason ?? "unknown"}`,
+ error: abortError,
startedAt,
endedAt: abortEndedAt,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const abortEndedAt = state.deps.nowMs(); | |
| const abortDurationMs = abortEndedAt - startedAt; | |
| // afterRun hooks still fire on abort — they must always run. | |
| const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); | |
| if (afterRunEntries.length > 0) { | |
| await runCronHooks( | |
| "afterRun", | |
| makeHookCtx("afterRun", { status: "skipped", durationMs: abortDurationMs }), | |
| afterRunEntries, | |
| ); | |
| } | |
| return { | |
| jobId: id, | |
| status: "skipped", | |
| error: `hook aborted: ${beforeResult.reason ?? "unknown"}`, | |
| startedAt, | |
| endedAt: abortEndedAt, | |
| const abortEndedAt = state.deps.nowMs(); | |
| const abortDurationMs = abortEndedAt - startedAt; | |
| const abortError = `hook aborted: ${beforeResult.reason ?? "unknown"}`; | |
| // afterRun hooks still fire on abort — they must always run. | |
| const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); | |
| if (afterRunEntries.length > 0) { | |
| await runCronHooks( | |
| "afterRun", | |
| makeHookCtx("afterRun", { | |
| status: "skipped", | |
| error: abortError, | |
| durationMs: abortDurationMs, | |
| }), | |
| afterRunEntries, | |
| ); | |
| } | |
| return { | |
| jobId: id, | |
| status: "skipped", | |
| error: abortError, | |
| startedAt, | |
| endedAt: abortEndedAt, |
🤖 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 659 - 675, The afterRun hook context
for the aborted path doesn't include the abort reason, so update the call that
builds the afterRun context (the makeHookCtx invocation used with runCronHooks
and afterRunEntries) to include the abort error detail (e.g., add an error or
reason property populated from beforeResult.reason or the catch-up equivalent)
alongside status and durationMs; do the same change in the catch-up branch so
the always-run audit hook can see why the job was skipped (refer to
loadHookEntries, afterRunEntries, runCronHooks, makeHookCtx and beforeResult).
- Security: validate per-job hook paths (reject absolute + traversal) - Add configurable timeoutMs per hook entry (default 10s) - Distinguish error vs warn log level (runtime throw vs module-not-found) - Resolve hook scripts from OC home (storePath parent) not process.cwd() - Wire lifecycle hooks into finishPreparedManualRun (ops.ts) - Add hooks field to TypeBox CronJobSchema (fixes gateway rejection) - Add jobName filter for case-insensitive substring matching - Document lifecycle ordering on CronLifecycleHookPoint type - Add Node-only comment on inlineHook test helper - 7 new tests (path traversal, jobName filter, isValidJobHookPath)
Add optional `base` parameter to resolveUserPath so config-derived paths can resolve against a stable root (e.g. OC home) instead of process.cwd(). Hook module loading now uses this instead of manual path.resolve.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/cron/service/timer.ts (1)
662-679:⚠️ Potential issue | 🟡 MinorPass the abort reason into
afterRunon skipped runs.Both abort branches return
error: hook aborted: ..., but theafterRuncontext only getsstatus: "skipped"anddurationMs. That leaves always-run audit/cleanup hooks blind to why the run was skipped.💡 Minimal shape of the change
+ const abortError = `hook aborted: ${beforeResult.reason ?? "unknown"}`; const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job); if (afterRunEntries.length > 0) { await runCronHooks( "afterRun", - makeHookCtx("afterRun", { status: "skipped", durationMs: abortDurationMs }), + makeHookCtx("afterRun", { + status: "skipped", + error: abortError, + durationMs: abortDurationMs, + }), afterRunEntries, ); } return { jobId: id, status: "skipped", - error: `hook aborted: ${beforeResult.reason ?? "unknown"}`, + error: abortError,Mirror the same change in the catch-up block below.
Also applies to: 1040-1056
🤖 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 662 - 679, When a job is aborted you currently call makeHookCtx("afterRun", { status: "skipped", durationMs }) so afterRun hooks don't get the abort reason; update the afterRun context to include the abort reason (e.g. { status: "skipped", durationMs: abortDurationMs, reason: beforeResult.reason ?? "unknown" }) when you call runCronHooks after a before-run abort (the block using beforeResult, loadHookEntries("afterRun"), runCronHooks and makeHookCtx), and apply the identical change to the catch-up abort branch that similarly returns status "skipped" and error.src/cron/hooks.ts (1)
95-113:⚠️ Potential issue | 🔴 CriticalStart the timeout before import and make timed-out hooks cooperatively cancelable.
loadHookModule()is awaited beforecreateTimeout(), so a slow import or top-levelawaitcan block this loop indefinitely. AfterPromise.race()rejects, the losing hook keeps running, so a timed-out hook can still mutate shared state after the caller moved on.Does Promise.race cancel losing promises, and does Node.js import() wait for a module's top-level await to finish before resolving?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/hooks.ts` around lines 95 - 113, Move the timeout creation to before importing the hook and make hook execution cooperatively cancelable: create the timeout (via createTimeout(timeoutMs) or an AbortController) before calling loadHookModule(entry.script, ctx.basePath), pass an AbortSignal (or the timeout) into loadHookModule and into the hook function invocation (hookFn(ctx, { signal }) or similar) so top-level-await/imports and the hook body can observe cancellation, use Promise.race([hookPromise, timeout.promise]) to await the result, call timeout.clear() in finally, and after a timeout ensure you treat the hook as aborted (log/continue) so a losing promise cannot silently mutate shared state; update loadHookModule and hookFn call sites to accept and propagate the signal.
🤖 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/hooks.ts`:
- Around line 144-150: The runtime type guard isAbortResult currently treats any
truthy abort value as an abort; change the check to require abort === true so
only a boolean true short-circuits. Update isAbortResult to assert "abort" in
value and (value as { abort: unknown }).abort === true (or typeof (value as
any).abort === "boolean" && (value as any).abort === true) so malformed values
like "{ abort: 'false' }" won't be treated as aborts while preserving the type
guard behavior.
- Around line 198-207: isValidJobHookPath currently only blocks absolute paths
and leading "..", allowing empty strings and scheme-like specifiers (e.g.,
data:, file:, http:) which loadHookModule treats as import specifiers; update
isValidJobHookPath to also return false for empty/whitespace strings and for any
value that looks like a URI/scheme (e.g., matches a scheme pattern like
/^[a-zA-Z][a-zA-Z0-9+.-]*:/ or contains "://") or starts with "//" so only
relative filesystem paths without schemes are accepted; keep existing
path.isAbsolute and path.normalize checks and perform the extra validations
after normalization to ensure loadHookModule cannot be fed a scheme-like
specifier.
In `@src/cron/service/ops.ts`:
- Around line 463-479: The abort branch inside the beforeRun hooks currently
returns early and skips the finalization that clears runningAtMs and emits
finished events; replace the bare return with the same finalization flow used
for normal completion: compute abortEndedAt/durationMs, run afterRun hooks (as
is done), then call the routine that performs the final locked update for manual
runs (the code path invoked after hooks/when not aborted — e.g., the same
function that clears runningAtMs, sets status/duration and emits finished
events), or inline those update steps here so prepareManualRun's persisted
runningAtMs is cleared and next-run/one-shot recomputation occurs. Ensure you
reference the existing helpers (prepareManualRun, loadHookEntries, runCronHooks,
makeHookCtx, state.deps.nowMs, executionJob, startedAt) and do not leave an
early return that bypasses the final locked update.
---
Duplicate comments:
In `@src/cron/hooks.ts`:
- Around line 95-113: Move the timeout creation to before importing the hook and
make hook execution cooperatively cancelable: create the timeout (via
createTimeout(timeoutMs) or an AbortController) before calling
loadHookModule(entry.script, ctx.basePath), pass an AbortSignal (or the timeout)
into loadHookModule and into the hook function invocation (hookFn(ctx, { signal
}) or similar) so top-level-await/imports and the hook body can observe
cancellation, use Promise.race([hookPromise, timeout.promise]) to await the
result, call timeout.clear() in finally, and after a timeout ensure you treat
the hook as aborted (log/continue) so a losing promise cannot silently mutate
shared state; update loadHookModule and hookFn call sites to accept and
propagate the signal.
In `@src/cron/service/timer.ts`:
- Around line 662-679: When a job is aborted you currently call
makeHookCtx("afterRun", { status: "skipped", durationMs }) so afterRun hooks
don't get the abort reason; update the afterRun context to include the abort
reason (e.g. { status: "skipped", durationMs: abortDurationMs, reason:
beforeResult.reason ?? "unknown" }) when you call runCronHooks after a
before-run abort (the block using beforeResult, loadHookEntries("afterRun"),
runCronHooks and makeHookCtx), and apply the identical change to the catch-up
abort branch that similarly returns status "skipped" and error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8c6a295-2ec3-44d0-baed-6ed1472baf51
📒 Files selected for processing (7)
src/config/types.cron.tssrc/config/zod-schema.tssrc/cron/hooks.test.tssrc/cron/hooks.tssrc/cron/service/ops.tssrc/cron/service/timer.tssrc/gateway/protocol/schema/cron.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/zod-schema.ts
Comment on lines
+144
to
+150
| function isAbortResult(value: unknown): value is { abort: boolean; reason?: string } { | ||
| return ( | ||
| value != null && | ||
| typeof value === "object" && | ||
| "abort" in value && | ||
| Boolean((value as { abort: unknown }).abort) | ||
| ); |
There was a problem hiding this comment.
Require abort === true before skipping a job.
Boolean(result.abort) treats any truthy value as an abort, so malformed results like { abort: "false" } or { abort: 1 } will still short-circuit execution.
🛠️ Narrow the runtime check
function isAbortResult(value: unknown): value is { abort: boolean; reason?: string } {
return (
value != null &&
typeof value === "object" &&
"abort" in value &&
- Boolean((value as { abort: unknown }).abort)
+ (value as { abort: unknown }).abort === true
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isAbortResult(value: unknown): value is { abort: boolean; reason?: string } { | |
| return ( | |
| value != null && | |
| typeof value === "object" && | |
| "abort" in value && | |
| Boolean((value as { abort: unknown }).abort) | |
| ); | |
| function isAbortResult(value: unknown): value is { abort: boolean; reason?: string } { | |
| return ( | |
| value != null && | |
| typeof value === "object" && | |
| "abort" in value && | |
| (value as { abort: unknown }).abort === true | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/hooks.ts` around lines 144 - 150, The runtime type guard
isAbortResult currently treats any truthy abort value as an abort; change the
check to require abort === true so only a boolean true short-circuits. Update
isAbortResult to assert "abort" in value and (value as { abort: unknown }).abort
=== true (or typeof (value as any).abort === "boolean" && (value as any).abort
=== true) so malformed values like "{ abort: 'false' }" won't be treated as
aborts while preserving the type guard behavior.
Comment on lines
+198
to
+207
| export function isValidJobHookPath(scriptPath: string): boolean { | ||
| // Reject absolute paths and traversal segments in per-job entries. | ||
| if (path.isAbsolute(scriptPath)) { | ||
| return false; | ||
| } | ||
| const normalized = path.normalize(scriptPath); | ||
| if (normalized.startsWith("..")) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Reject scheme-like and blank per-job hook paths.
isValidJobHookPath() only rejects absolute paths and leading .., so values like data:text/javascript,..., file:///tmp/hook.mjs, or "" still pass. loadHookModule() treats scheme-like strings as import specifiers, which lets a jobs.json hook bypass the intended relative-path restriction entirely.
🔒 Tighten the validator
export function isValidJobHookPath(scriptPath: string): boolean {
- // Reject absolute paths and traversal segments in per-job entries.
- if (path.isAbsolute(scriptPath)) {
+ // Reject absolute paths, URL-like specifiers, blank values, and traversal
+ // segments in per-job entries.
+ if (
+ scriptPath.trim() === "" ||
+ path.isAbsolute(scriptPath) ||
+ /^[a-z][a-z0-9+.-]*:/i.test(scriptPath)
+ ) {
return false;
}
const normalized = path.normalize(scriptPath);
- if (normalized.startsWith("..")) {
+ if (normalized === "." || normalized === ".." || normalized.startsWith(`..${path.sep}`)) {
return false;
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function isValidJobHookPath(scriptPath: string): boolean { | |
| // Reject absolute paths and traversal segments in per-job entries. | |
| if (path.isAbsolute(scriptPath)) { | |
| return false; | |
| } | |
| const normalized = path.normalize(scriptPath); | |
| if (normalized.startsWith("..")) { | |
| return false; | |
| } | |
| return true; | |
| export function isValidJobHookPath(scriptPath: string): boolean { | |
| // Reject absolute paths, URL-like specifiers, blank values, and traversal | |
| // segments in per-job entries. | |
| if ( | |
| scriptPath.trim() === "" || | |
| path.isAbsolute(scriptPath) || | |
| /^[a-z][a-z0-9+.-]*:/i.test(scriptPath) | |
| ) { | |
| return false; | |
| } | |
| const normalized = path.normalize(scriptPath); | |
| if (normalized === "." || normalized === ".." || normalized.startsWith(`..${path.sep}`)) { | |
| return false; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/hooks.ts` around lines 198 - 207, isValidJobHookPath currently only
blocks absolute paths and leading "..", allowing empty strings and scheme-like
specifiers (e.g., data:, file:, http:) which loadHookModule treats as import
specifiers; update isValidJobHookPath to also return false for empty/whitespace
strings and for any value that looks like a URI/scheme (e.g., matches a scheme
pattern like /^[a-zA-Z][a-zA-Z0-9+.-]*:/ or contains "://") or starts with "//"
so only relative filesystem paths without schemes are accepted; keep existing
path.isAbsolute and path.normalize checks and perform the extra validations
after normalization to ensure loadHookModule cannot be fed a scheme-like
specifier.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cron/hooks.ts (2)
68-74: Consider logging rejected per-job hook paths.Invalid hook paths are silently skipped, which may make debugging difficult if a user misconfigures a job's hook path. A debug-level log would help diagnose why a hook isn't running.
💡 Optional: Add debug logging for rejected paths
+import { logVerbose } from "../globals.js"; + for (const script of jobScripts) { if (!isValidJobHookPath(script)) { + logVerbose(`Rejected invalid per-job hook path: ${script}`); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/hooks.ts` around lines 68 - 74, Invalid per-job hook paths are currently skipped silently in the loop over jobScripts; update the loop that uses isValidJobHookPath to emit a debug-level log when a script is rejected (include the script path and a short reason/context), and only push to jobEntries with { script, priority: DEFAULT_PRIORITY } when valid. Add the debug logging call next to the isValidJobHookPath check (e.g., logger.debug or processLogger.debug) so rejected paths are visible during troubleshooting.
166-171: Add defensive check forjob.namebefore callingtoLowerCase().If
job.nameisundefined, line 167 will throw. WhileCronJoblikely always has a name, a defensive fallback would prevent runtime errors from malformed job data.🛡️ Optional defensive fix
if (f.jobName && f.jobName.length > 0) { - const nameLower = job.name.toLowerCase(); + const nameLower = (job.name ?? "").toLowerCase(); if (!f.jobName.some((pattern) => nameLower.includes(pattern.toLowerCase()))) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cron/hooks.ts` around lines 166 - 171, The code calls job.name.toLowerCase() without ensuring job.name exists, which can throw if job data is malformed; update the block that defines nameLower (used in the f.jobName check) to defensively handle missing names — e.g., check if job.name is truthy before calling toLowerCase or use a safe fallback like an empty string — so the f.jobName.some(...) check uses nameLower safely; refer to the symbols f.jobName, job.name, and nameLower (and the surrounding filter logic) when making the change.
🤖 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/cron/hooks.ts`:
- Around line 68-74: Invalid per-job hook paths are currently skipped silently
in the loop over jobScripts; update the loop that uses isValidJobHookPath to
emit a debug-level log when a script is rejected (include the script path and a
short reason/context), and only push to jobEntries with { script, priority:
DEFAULT_PRIORITY } when valid. Add the debug logging call next to the
isValidJobHookPath check (e.g., logger.debug or processLogger.debug) so rejected
paths are visible during troubleshooting.
- Around line 166-171: The code calls job.name.toLowerCase() without ensuring
job.name exists, which can throw if job data is malformed; update the block that
defines nameLower (used in the f.jobName check) to defensively handle missing
names — e.g., check if job.name is truthy before calling toLowerCase or use a
safe fallback like an empty string — so the f.jobName.some(...) check uses
nameLower safely; refer to the symbols f.jobName, job.name, and nameLower (and
the surrounding filter logic) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bebab137-14e6-4cb3-921f-ddf99bcd4cc1
📒 Files selected for processing (2)
src/cron/hooks.tssrc/utils.ts
When a beforeRun hook aborted a manual run via finishPreparedManualRun, the early return skipped the locked cleanup that clears runningAtMs. This left the job stuck as "running" until STUCK_RUN_MS timed out. Now the abort path applies a "skipped" result and clears state under lock, matching the timer-path behavior.
This was referenced Mar 15, 2026
14 tasks
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
cron.hooks+ per-job overrides in jobs.jsonInternal review PR
This PR is for CodeRabbit review before submitting upstream.
Summary by CodeRabbit
New Features
Tests