src/cli/cron-cli/register.cron-add.ts (1)

249-257: Simplify ternary for session target inference.

The nested ternary can be simplified since both agentTurn and script map to "isolated".

♻️ Suggested simplification

-          const inferredSessionTarget =
-            payload.kind === "agentTurn"
-              ? "isolated"
-              : payload.kind === "script"
-                ? "isolated"
-                : "main";
+          const inferredSessionTarget =
+            payload.kind === "systemEvent" ? "main" : "isolated";

🤖 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 249 - 257, The nested
ternary creating inferredSessionTarget is verbose because both payload.kind
values "agentTurn" and "script" map to "isolated"; replace the nested ternary in
inferredSessionTarget with a single condition that checks if payload.kind ===
"agentTurn" || payload.kind === "script" and returns "isolated", otherwise
"main", and keep subsequent sessionTarget assignment (using sessionSource,
sessionTargetRaw) unchanged; update the symbol inferredSessionTarget and ensure
logic around sessionTarget and sessionSource remains identical.

src/cron/exec-script.ts (1)

38-39: Consider validating cwd directory existence.

If payload.cwd resolves to a non-existent directory, execFile will fail with a less clear error (e.g., ENOENT). Validating upfront would provide a clearer message.

🛡️ Optional validation for cwd

   const resolvedCwd = payload.cwd ? resolveScriptPath(payload.cwd, basePath) : basePath;
+  if (payload.cwd && !fs.existsSync(resolvedCwd)) {
+    return {
+      status: "error",
+      error: `script cwd not found: ${resolvedCwd} (cwd: ${payload.cwd})`,
+    };
+  }
   const childEnv = payload.env ? { ...process.env, ...payload.env } : process.env;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cron/exec-script.ts` around lines 38 - 39, resolvedCwd may point to a
non-existent path causing execFile to fail with unclear ENOENT; after computing
resolvedCwd (using resolveScriptPath(payload.cwd, basePath)), validate that it
exists and is a directory (e.g., fs.stat or fs.existsSync + isDirectory) before
calling execFile, and if the check fails, throw or return a descriptive error
indicating the invalid cwd (include payload.cwd and resolvedCwd) so callers of
exec-script (and logs) get a clear message; keep childEnv logic unchanged.

src/cron/exec-script.test.ts (1)

98-111: Potential test flakiness with fixed timeout.

The 50ms delay before aborting may be too short on slow CI runners, potentially causing the test to abort before the process fully starts, which could lead to intermittent failures.

♻️ Consider a more robust approach

   it("kills script and returns error when aborted mid-run", async () => {
-    const script = writeScript("slow.sh", "#!/bin/sh\nsleep 30");
+    // Write a script that signals readiness before sleeping
+    const script = writeScript("slow.sh", "#!/bin/sh\necho started\nsleep 30");
     const controller = new AbortController();
     const resultPromise = execCronScript({
       payload: { kind: "script", command: script },
       basePath: tmpDir,
       abortSignal: controller.signal,
     });
-    // Abort after a short delay to let the process start.
-    setTimeout(() => controller.abort(), 50);
+    // Give the process time to start; 100ms is more reliable across CI environments
+    setTimeout(() => controller.abort(), 100);
     const result = await resultPromise;
     expect(result.status).toBe("error");
     expect(result.error).toContain("aborted");
   });

Alternatively, you could poll for process existence or use a more sophisticated synchronization mechanism, but increasing the timeout is the simplest fix.

🤖 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" uses a fixed 50ms setTimeout to abort
which can be flaky on slow CI; update the test that calls execCronScript (and
uses writeScript, AbortController, tmpDir) to wait more robustly before aborting
— either poll/wait until the child process is confirmed started (e.g., detect
the process or a readiness marker) or simply increase the abort delay to a safer
value (e.g., 500ms) to avoid aborting before the script actually begins.

src/cron/service/timer.ts (1)

1193-1218: Script execution path looks well-structured; consider logging delivery failures.

The script payload execution logic correctly bypasses session/LLM flows and integrates with the existing delivery pipeline. However, the empty catch block on line 1212 silently swallows delivery errors, which could make troubleshooting difficult.

🔧 Proposed fix to log delivery errors

       try {
         const deliveryResult = await state.deps.announceScriptOutput({
           job,
           text: scriptResult.summary,
         });
         return { ...scriptResult, ...deliveryResult };
-      } catch {
+      } catch (err) {
+        state.deps.log.warn(
+          { jobId: job.id, err: String(err) },
+          "cron: script output delivery failed",
+        );
         return { ...scriptResult, delivered: false, deliveryAttempted: true };
       }

🤖 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 1193 - 1218, The empty catch in the
script payload delivery path swallows errors from announceScriptOutput; update
the catch to capture the thrown error and log it via the existing logging
mechanism (e.g., use state.deps.logger or an available logger) and still return
{ ...scriptResult, delivered: false, deliveryAttempted: true }; locate the block
handling script payloads around execCronScript and the announceScriptOutput call
(refer to job.payload, scriptResult, and state.deps.announceScriptOutput) and
add error capture and a descriptive process/log call including the job id and
error details.

src/cron/exec-script.ts (2)

6-7: Use import type for type-only imports.

These imports are only used as types, not values. Per coding guidelines, use import type { X } for type-only imports.

Suggested fix

-import type { CronRunOutcome } from "./types.js";
-import type { CronScriptPayload } from "./types.js";
+import type { CronRunOutcome, CronScriptPayload } from "./types.js";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cron/exec-script.ts` around lines 6 - 7, The two imports for
CronRunOutcome and CronScriptPayload are type-only and should use TypeScript's
type-only import syntax; update the import statements that bring in
CronRunOutcome and CronScriptPayload so they read "import type { CronRunOutcome
} from './types.js';" and "import type { CronScriptPayload } from './types.js';"
(ensure there are no value imports of those symbols elsewhere in
exec-script.ts).

---

136-140: Clarify fallback behavior for TypeScript files.

The comment states "falls back to node+tsx if bun is unavailable," but no fallback logic is implemented—the spawn will fail with a "command not found" error if bun isn't installed. Consider either:

1. Updating the comment to reflect actual behavior (spawn error surfaced to user)
2. Adding runtime probing to check for bun availability

The current behavior is acceptable (clear spawn error), but the comment is misleading.

Suggested comment update

     case ".ts":
-      // Prefer bun for TypeScript; falls back to node+tsx if bun is unavailable,
-      // but we let the caller surface that as a spawn error rather than probing.
+      // Uses bun for TypeScript. If bun is not installed, the spawn will fail
+      // with an error message. Users can ensure bun is available or use a shebang.
       return "bun";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cron/exec-script.ts` around lines 136 - 140, The comment in the switch
branch for TypeScript (case ".ts") claims it "falls back to node+tsx if bun is
unavailable" but no such fallback exists; update the comment in exec-script.ts
at the case ".ts" branch (the logic that returns "bun") to accurately state that
we prefer bun and that if bun is not installed the spawn will fail and the
caller will surface the error, or alternatively implement a runtime probe for
bun availability and return "node+tsx" when bun is absent—pick one approach and
make the comment and/or code consistent with that choice.

src/cron/exec-script.ts (1)

27-35: Consider validating non-empty command.

If payload.command is empty or whitespace-only, resolveScriptPath will return basePath itself. Since basePath is a directory, fs.existsSync would return true, and execFile would then fail with a confusing error (e.g., EACCES or EISDIR).

🛡️ Proposed fix to add early validation

   const resolvedCommand = resolveScriptPath(payload.command, basePath);
+
+  if (!payload.command.trim()) {
+    return { status: "error", error: "script command is empty" };
+  }

   // Validate file exists before spawning to give a clear error message.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cron/exec-script.ts` around lines 27 - 35, The code fails to validate
that payload.command is non-empty/whitespace and may treat basePath (a
directory) as a valid script because resolveScriptPath returns basePath; update
the pre-check before calling resolveScriptPath/execFile to reject empty or
whitespace-only payload.command and/or ensure the resolved path is a regular
file (not a directory) using fs.stat or similar; specifically add validation
around payload.command and after resolveScriptPath confirm the path is a file
(e.g., using fs.statSync(resolvedCommand).isFile()) and return a clear error
object if the command is empty or the resolved path is not a file so execFile
won’t produce confusing EISDIR/EACCES errors.