Greptile Summary

This PR introduces a lifecycle hook system for cron jobs, allowing user-defined scripts to run at beforeRun, afterComplete, onFailure, and afterRun points. The implementation is largely solid — filter logic, priority sorting, timeout cleanup, and error isolation are well-designed and match the previous review's fixes. However, there is one logic inconsistency and a few smaller concerns worth addressing before merge.

• afterRun hooks do not fire when beforeRun aborts — the early return in timer.ts exits before reaching the afterRun block, contradicting the stated guarantee that afterRun always fires. Hook authors relying on afterRun for cleanup or audit logging will silently miss it when a beforeRun hook aborts.
• CronHookContext.result field is defined but never populated — hook scripts that access ctx.result will always receive undefined; the field should be removed or actually set in the context builder.
• runCronHooks abort path has no unit test — the core beforeRun abort behaviour (returning { abort: true }) is not directly exercised in the test suite despite being the most security-sensitive path in the feature.

Confidence Score: 3/5

• Mostly safe to merge but the afterRun gap creates a correctness risk for hook authors relying on its "always fires" guarantee.
• The core implementation is solid and addresses all three issues from the prior review. The afterRun issue is a real logic bug that could silently break hook-based cleanup or audit workflows, not just a theoretical concern. The dead result field and missing abort test reduce confidence in the completeness of the change.
• src/cron/service/timer.ts — the beforeRun abort early-return path needs to be restructured so afterRun hooks still fire.

This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 657-664

Comment:
**`afterRun` hooks silently skip on `beforeRun` abort**

The early `return` on a `beforeRun` abort (lines 658–664) exits the function before `afterRun` hooks are reached (line 710). The comment at line 710 states `afterRun` hooks "always fire regardless of outcome", but a `beforeRun` abort is itself an outcome — and `afterRun` never fires in this path.

If an `afterRun` hook is used for cleanup, audit logging, or metrics recording, it will be silently missed whenever a `beforeRun` hook aborts the job. This could lead to incomplete audit trails or unreleased resources that the hook was expected to clean up.

To fix this, extract the abort-result build into a variable and fall through to the shared `afterRun` block rather than returning early:

```typescript
let abortResult: TimedCronRunOutcome | undefined;
if (beforeEntries.length > 0) {
  const beforeResult = await runCronHooks("beforeRun", makeHookCtx("beforeRun"), beforeEntries);
  if (beforeResult.aborted) {
    abortResult = {
      jobId: id,
      status: "skipped",
      error: `hook aborted: ${beforeResult.reason ?? "unknown"}`,
      startedAt,
      endedAt: state.deps.nowMs(),
    };
  }
}
if (abortResult) {
  // still run afterRun hooks before returning
  const afterRunEntries = loadHookEntries("afterRun", state.deps.cronConfig, job);
  if (afterRunEntries.length > 0) {
    await runCronHooks("afterRun", makeHookCtx("afterRun", { status: "skipped", durationMs: abortResult.endedAt - abortResult.startedAt }), afterRunEntries);
  }
  return abortResult;
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/cron/hooks.ts
Line: 13

Comment:
**`result` field is defined but never populated**

`CronHookContext` declares a `result?: unknown` field, but it is never set when `makeHookCtx` builds the context object in `timer.ts`. Hook script authors who reference `ctx.result` expecting the job's core result will always receive `undefined`.

Either remove the field to avoid confusion, or populate it in `timer.ts` with `coreResult` (or the raw `executeJobCoreWithTimeout` return value) so hook scripts can actually use it. The PR description's hook contract comment (`ctx.hookPoint, ctx.workflow, ctx.job, ctx.status …`) doesn't mention `ctx.result` either, which suggests it was left in the type by accident.

```suggestion
  job: Pick<CronJob, "id" | "name" | "agentId" | "schedule">;
  error?: string;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/cron/hooks.test.ts
Line: 166-206

Comment:
**`runCronHooks` abort path is untested**

The three `runCronHooks` tests cover empty entries and a missing module, but the core `beforeRun` abort contract — a hook returning `{ abort: true, reason: '...' }` — has no test. Given that abort is the most security-sensitive behaviour (it gates whether a job executes at all), this path should be explicitly covered:

```typescript
it("aborts execution when a beforeRun hook returns { abort: true }", async () => {
  const abortHook = vi.fn().mockResolvedValue({ abort: true, reason: "test abort" });
  vi.doMock("./abort-hook.cjs", () => ({ default: abortHook }));
  // or use a loadHookModule mock strategy consistent with the test setup
});

it("returns aborted:false when abort is false", async () => {
  // hook returns { abort: false } — should not abort
});
```

Also consider a test where `reason` is omitted — `isAbortResult` still triggers but `reason` should default to `"aborted by hook"` per line 108 of `hooks.ts`.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: a8043de}

@coderabbitai review

@coderabbitai review

All review findings addressed

Greptile (3 findings — fixed in 1660fbe)

• afterRun silent skip on abort: afterRun hooks now fire before the early return
• Unused result field: removed from CronHookContext
• Abort path untested: 4 new tests using data: URI inline hooks

Codex Connector P1 (2 findings — fixed in cdfea18)

• Zod schema missing hooks: CronHookEntrySchema added; hooks wired into cron object schema
• Relative path resolution: loadHookModule uses path.resolve + importFileModule; URL schemes (data:, file:) pass through directly

Codex Connector P2 (3 findings — fixed in 1660fbe and faab903)

• afterRun on beforeRun abort: same fix as Greptile finding above
• Catch-up run hooks: runStartupCatchupCandidate now runs the full beforeRun/afterComplete/onFailure/afterRun lifecycle
• Windows absolute paths: path.isAbsolute() checked before URL-scheme regex

Local integration test (passed)

Hooks confirmed firing on a live OC instance. Hook log files created for beforeRun, afterComplete, and afterRun across multiple cron jobs.

Review findings addressed (41c7282)

xkonjin (human review) — all 6 items fixed

• Security (path traversal): per-job hook paths now validated — absolute paths and ../ traversal rejected via isValidJobHookPath()
• Configurable timeout: added timeoutMs field to CronHookEntry (default 10s); wired into Zod schema
• Error vs warn log level: runtime throws log at error level; module-not-found stays at warn
• Type docs for lifecycle ordering: CronLifecycleHookPoint now has JSDoc explaining afterRun fires like a finally block
• JSDoc for meta: expanded comment explaining hook-to-hook communication pattern
• inlineHook Node-only: added comment noting data: URI imports require Node.js

Latest Codex review (faab903) — both P1/P2 fixed

• P1: Stable base directory: hook scripts now resolved from OC home (derived from storePath) instead of process.cwd()
• P2: Manual run hooks: finishPreparedManualRun in ops.ts now runs full beforeRun/afterComplete/onFailure/afterRun lifecycle

Additional improvements

• Gateway schema: added hooks field to TypeBox CronJobSchema — fixes "not a recognized key" rejection
• jobName filter: hooks can now filter by job name (case-insensitive substring match) in addition to job ID
• Tests: 7 new tests (path traversal x2, absolute path rejection, isValidJobHookPath x3, jobName filter x2) — total 25

All checks pass: pnpm build, pnpm check, pnpm test src/cron/hooks.test.ts (25/25).

Rebased and squashed this branch onto the current upstream/main.

What changed:

• Branch was originally based on a commit 735 commits behind upstream/main, causing the PR diff to show ~1840 unrelated files
• Rebased all 9 fixup commits onto current upstream/main and squashed them into a single clean commit
• Resolved one conflict in src/utils.ts: merged upstream's resolveHomeRelativePath refactor with the base parameter added by this PR for relative hook script path resolution
• PR now shows only the 9 cron-related files it actually touches

No functional changes — same behavior, cleaner history.

Hey @vincentkoc — you recently merged some cron work (#45459). This PR adds lifecycle hooks for cron job execution and has been sitting open with all Codex findings addressed. Would you be able to take a look when you get a chance? Happy to address any feedback.

Hey @ImLukeF — saw you recently merged some cron work (#16511). This PR adds lifecycle hooks for cron job execution and has been sitting open with all Codex findings addressed. Would you be able to take a look when you get a chance? Happy to address any feedback.

Codex review: found issues before merge.

Summary
The PR adds cron lifecycle hook config/types, a hook runner, protocol schema, tests, and scheduled/startup/manual execution wiring for beforeRun, afterComplete, onFailure, and afterRun scripts.

Reproducibility: yes. Source inspection shows current main has no cron lifecycle hook config or dispatch, and the PR defects are reproducible by following persisted job hook values through loadHookEntries, isValidJobHookPath, resolveUserPath, and importFileModule.

Next step before merge
Needs maintainer/security approval for the new core cron code-execution surface before repair automation; the mechanical fixes are clear, but the product and trust-boundary decision is not.

Security
Needs attention: The diff introduces a new cron-driven local script execution surface and still has a concrete per-job path restriction bypass.

Review findings

• [P1] Trim paths before enforcing per-job hook restrictions — src/cron/hooks.ts:201-203
• [P2] Validate persisted job hooks before iterating — src/cron/hooks.ts:67-71
• [P2] Reuse stable hook import specifiers — src/cron/hooks.ts:192