fix(cron): reject unsupported generic timeout edits (#119899)

Co-authored-by: zyw02 <zyw02@users.noreply.github.com>

Punchcard-Session: cobalt-willow-summit-z1

Co-authored-by: Vincent Koc <vincentkoc@ieee.org>

scripts/e2e/cron-cli-docker.sh

@@ -63,6 +63,16 @@ dump_logs_on_error() {
       /tmp/cron-cli-edit-exact.json \
       /tmp/cron-cli-edit-timeout.json \
       /tmp/cron-cli-get-after-edit.json \
+      /tmp/cron-cli-script-add.json \
+      /tmp/cron-cli-script-before.json \
+      /tmp/cron-cli-script-generic-timeout.log \
+      /tmp/cron-cli-script-after-generic.json \
+      /tmp/cron-cli-script-edit-timeout.json \
+      /tmp/cron-cli-script-after-specific.json \
+      /tmp/cron-cli-event-add.json \
+      /tmp/cron-cli-event-before.json \
+      /tmp/cron-cli-event-generic-timeout.log \
+      /tmp/cron-cli-event-after.json \
       /tmp/cron-cli-list.json \
       /tmp/cron-cli-show.json \
       /tmp/cron-cli-disable.json \
@@ -389,6 +399,80 @@ node --input-type=module -e '
   }
 '
 
+cat > /tmp/cron-cli-script.js <<'SCRIPT'
+return { notify: "cron script timeout proof" };
+SCRIPT
+cron_cli add \
+  "script timeout smoke" \
+  --every 1h \
+  --session isolated \
+  --script /tmp/cron-cli-script.js \
+  --script-timeout-seconds 15 \
+  --no-deliver \
+  --json > /tmp/cron-cli-script-add.json
+script_job_id="$(read_json_field /tmp/cron-cli-script-add.json id)"
+cron_cli get "$script_job_id" > /tmp/cron-cli-script-before.json
+if cron_cli edit "$script_job_id" --timeout-seconds 30 > /tmp/cron-cli-script-generic-timeout.log 2>&1; then
+  echo "generic timeout unexpectedly succeeded for script job" >&2
+  exit 1
+fi
+grep -q -- "Use --script-timeout-seconds for script jobs" /tmp/cron-cli-script-generic-timeout.log
+cron_cli get "$script_job_id" > /tmp/cron-cli-script-after-generic.json
+cron_cli edit "$script_job_id" --script-timeout-seconds 30 > /tmp/cron-cli-script-edit-timeout.json
+cron_cli get "$script_job_id" > /tmp/cron-cli-script-after-specific.json
+node --input-type=module -e '
+  const fs = await import("node:fs/promises");
+  const before = JSON.parse(await fs.readFile("/tmp/cron-cli-script-before.json", "utf8"));
+  const afterGeneric = JSON.parse(
+    await fs.readFile("/tmp/cron-cli-script-after-generic.json", "utf8"),
+  );
+  const afterSpecific = JSON.parse(
+    await fs.readFile("/tmp/cron-cli-script-after-specific.json", "utf8"),
+  );
+  if (before.payload?.kind !== "script" || before.payload.timeoutSeconds !== 15) {
+    throw new Error(`script setup mismatch: ${JSON.stringify(before.payload)}`);
+  }
+  if (JSON.stringify(afterGeneric.payload) !== JSON.stringify(before.payload)) {
+    throw new Error(
+      `rejected generic timeout changed script payload: ${JSON.stringify(afterGeneric.payload)}`,
+    );
+  }
+  if (afterSpecific.payload?.kind !== "script" || afterSpecific.payload.timeoutSeconds !== 30) {
+    throw new Error(
+      `script-specific timeout did not persist: ${JSON.stringify(afterSpecific.payload)}`,
+    );
+  }
+'
+cron_cli rm "$script_job_id" --json >/dev/null
+
+cron_cli add \
+  "event timeout smoke" \
+  --every 1h \
+  --session main \
+  --system-event "cron event timeout proof" \
+  --json > /tmp/cron-cli-event-add.json
+event_job_id="$(read_json_field /tmp/cron-cli-event-add.json id)"
+cron_cli get "$event_job_id" > /tmp/cron-cli-event-before.json
+if cron_cli edit "$event_job_id" --timeout-seconds 30 > /tmp/cron-cli-event-generic-timeout.log 2>&1; then
+  echo "generic timeout unexpectedly succeeded for systemEvent job" >&2
+  exit 1
+fi
+grep -q -- "--timeout-seconds is not supported for systemEvent jobs" \
+  /tmp/cron-cli-event-generic-timeout.log
+cron_cli get "$event_job_id" > /tmp/cron-cli-event-after.json
+node --input-type=module -e '
+  const fs = await import("node:fs/promises");
+  const before = JSON.parse(await fs.readFile("/tmp/cron-cli-event-before.json", "utf8"));
+  const after = JSON.parse(await fs.readFile("/tmp/cron-cli-event-after.json", "utf8"));
+  if (before.payload?.kind !== "systemEvent") {
+    throw new Error(`event setup mismatch: ${JSON.stringify(before.payload)}`);
+  }
+  if (JSON.stringify(after.payload) !== JSON.stringify(before.payload)) {
+    throw new Error(`rejected generic timeout changed event payload: ${JSON.stringify(after.payload)}`);
+  }
+'
+cron_cli rm "$event_job_id" --json >/dev/null
+
 cron_cli list --all --json > /tmp/cron-cli-list.json
 node --input-type=module -e '
   const fs = await import("node:fs/promises");

src/cli/cron-cli/register.cron-edit-options.ts

@@ -137,10 +137,22 @@ export async function resolveCronEditPayloadDeliveryPatch(
     typeof opts.lightContext === "boolean";
   const hasScriptSpecificPayloadField =
     Boolean(scriptPath) || scriptTimeoutSeconds !== undefined || scriptToolBudget !== undefined;
+  if (hasTimeoutSeconds && hasScriptSpecificPayloadField) {
+    throw new Error("Use --script-timeout-seconds for script jobs, not --timeout-seconds.");
+  }
+  if (hasTimeoutSeconds && hasSystemEventPatch) {
+    throw new Error("--timeout-seconds is not supported for systemEvent jobs.");
+  }
   let timeoutOnlyPayloadKind: "agentTurn" | "command" | undefined;
   if (hasTimeoutSeconds && !hasCommandSpecificPayloadField && !hasAgentTurnSpecificPayloadField) {
-    const existing = await loadExistingJob();
-    timeoutOnlyPayloadKind = existing.payload.kind === "command" ? "command" : "agentTurn";
+    const existingKind = (await loadExistingJob()).payload.kind;
+    if (existingKind === "script") {
+      throw new Error("Use --script-timeout-seconds for script jobs, not --timeout-seconds.");
+    }
+    if (existingKind === "systemEvent" || existingKind === "heartbeat") {
+      throw new Error(`--timeout-seconds is not supported for ${existingKind} jobs.`);
+    }
+    timeoutOnlyPayloadKind = existingKind;
   }
   let toolsOnlyPayloadKind: CronJob["payload"]["kind"] | undefined;
   if (
@@ -157,14 +169,11 @@ export async function resolveCronEditPayloadDeliveryPatch(
   }
   const hasAgentTurnPayloadField =
     hasAgentTurnSpecificPayloadField ||
-    (hasTimeoutSeconds &&
-      !hasCommandSpecificPayloadField &&
-      timeoutOnlyPayloadKind !== "command") ||
+    timeoutOnlyPayloadKind === "agentTurn" ||
     (hasToolsAllowPatch && toolsOnlyPayloadKind === "agentTurn");
   const hasCommandPayloadField =
     hasCommandSpecificPayloadField ||
-    (hasTimeoutSeconds &&
-      (hasCommandSpecificPayloadField || timeoutOnlyPayloadKind === "command")) ||
+    timeoutOnlyPayloadKind === "command" ||
     toolsOnlyPayloadKind === "command";
   const hasAgentTurnPatch = hasAgentTurnPayloadField;
   const hasCommandPatch = hasCommandPayloadField;

src/cli/cron-cli/register.cron-edit.test.ts

@@ -260,13 +260,19 @@ describe("cron edit command", () => {
     });
   });
 
-  it("preserves command payload kind for timeout-only edits", async () => {
+  it.each([
+    {
+      kind: "agentTurn",
+      payload: { kind: "agentTurn", message: "hello" },
+    },
+    {
+      kind: "command",
+      payload: { kind: "command", argv: ["sh", "-lc", "echo ok"] },
+    },
+  ])("preserves $kind payload kind for timeout-only edits", async ({ kind, payload }) => {
     callGatewayFromCli.mockImplementation(async (method: string) => {
       if (method === "cron.get") {
-        return {
-          id: "job-1",
-          payload: { kind: "command", argv: ["sh", "-lc", "echo ok"] },
-        };
+        return { id: "job-1", payload };
       }
       return { ok: true };
     });
@@ -285,7 +291,7 @@ describe("cron edit command", () => {
         id: "job-1",
         patch: {
           payload: {
-            kind: "command",
+            kind,
             timeoutSeconds: 12,
           },
         },
@@ -293,6 +299,128 @@ describe("cron edit command", () => {
     );
   });
 
+  it.each([
+    {
+      kind: "script",
+      payload: { kind: "script", script: "return { notify: 'hello' }", timeoutSeconds: 5 },
+      error: "Use --script-timeout-seconds for script jobs",
+    },
+    {
+      kind: "systemEvent",
+      payload: { kind: "systemEvent", text: "hello" },
+      error: "--timeout-seconds is not supported for systemEvent jobs",
+    },
+    {
+      kind: "heartbeat",
+      payload: { kind: "heartbeat" },
+      error: "--timeout-seconds is not supported for heartbeat jobs",
+    },
+  ])(
+    "rejects timeout-only edits for stored $kind payloads before cron.update",
+    async ({ payload, error }) => {
+      callGatewayFromCli.mockImplementation(async (method: string) => {
+        if (method === "cron.get") {
+          return { id: "job-1", payload };
+        }
+        return { ok: true };
+      });
+      const errorSpy = vi.spyOn(defaultRuntime, "error").mockImplementation(() => {});
+      const exitSpy = vi
+        .spyOn(defaultRuntime, "exit")
+        .mockImplementation((() => undefined) as never);
+
+      try {
+        await createCronProgram().parseAsync(["edit", "job-1", "--timeout-seconds", "12"], {
+          from: "user",
+        });
+
+        expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining(error));
+        expect(callGatewayFromCli).toHaveBeenCalledWith("cron.get", expect.anything(), {
+          id: "job-1",
+        });
+        expect(callGatewayFromCli.mock.calls.some(([method]) => method === "cron.update")).toBe(
+          false,
+        );
+      } finally {
+        errorSpy.mockRestore();
+        exitSpy.mockRestore();
+      }
+    },
+  );
+
+  it("rejects generic timeout combined with an explicit systemEvent before cron.update", async () => {
+    const errorSpy = vi.spyOn(defaultRuntime, "error").mockImplementation(() => {});
+    const exitSpy = vi.spyOn(defaultRuntime, "exit").mockImplementation((() => undefined) as never);
+
+    try {
+      await createCronProgram().parseAsync(
+        ["edit", "job-1", "--system-event", "hello", "--timeout-seconds", "12"],
+        { from: "user" },
+      );
+
+      expect(errorSpy).toHaveBeenCalledWith(
+        expect.stringContaining("--timeout-seconds is not supported for systemEvent jobs"),
+      );
+      expect(callGatewayFromCli.mock.calls.some(([method]) => method === "cron.update")).toBe(
+        false,
+      );
+    } finally {
+      errorSpy.mockRestore();
+      exitSpy.mockRestore();
+    }
+  });
+
+  it.each([
+    ["--script", "missing-script.js"],
+    ["--script-tool-budget", "3"],
+    ["--script-timeout-seconds", "20"],
+  ])(
+    "rejects generic timeout combined with script option %s before cron.update",
+    async (flag, value) => {
+      const errorSpy = vi.spyOn(defaultRuntime, "error").mockImplementation(() => {});
+      const exitSpy = vi
+        .spyOn(defaultRuntime, "exit")
+        .mockImplementation((() => undefined) as never);
+
+      try {
+        await createCronProgram().parseAsync(
+          ["edit", "job-1", "--timeout-seconds", "12", flag, value],
+          { from: "user" },
+        );
+
+        expect(errorSpy).toHaveBeenCalledWith(
+          expect.stringContaining("Use --script-timeout-seconds for script jobs"),
+        );
+        expect(callGatewayFromCli.mock.calls.some(([method]) => method === "cron.update")).toBe(
+          false,
+        );
+      } finally {
+        errorSpy.mockRestore();
+        exitSpy.mockRestore();
+      }
+    },
+  );
+
+  it("updates script timeout with the script-specific option", async () => {
+    await createCronProgram().parseAsync(["edit", "job-1", "--script-timeout-seconds", "20"], {
+      from: "user",
+    });
+
+    expect(callGatewayFromCli).toHaveBeenCalledWith(
+      "cron.update",
+      expect.objectContaining({ scriptTimeoutSeconds: "20" }),
+      {
+        id: "job-1",
+        patch: {
+          payload: {
+            kind: "script",
+            timeoutSeconds: 20,
+          },
+        },
+      },
+    );
+  });
+
   it("falls back to cron.list when an older Gateway does not support cron.get", async () => {
     const unknownMethodError = Object.assign(new Error("unknown method: cron.get"), {
       name: "GatewayClientRequestError",