From 6fb57cd4dcab9e965260a0a7f1d74a985f38869a Mon Sep 17 00:00:00 2001
From: Brad Harris <bmharris@gmail.com>
Date: Sat, 12 Sep 2026 03:07:47 -0600
Subject: [PATCH] refactor(server): share the addJob/updateJob schedule guards
 (#tech-debt)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

addJob and updateJob each inlined the same two schedule guards — the
invalid-cron throw and the enabled-without-schedule-and-without-continuation
throw — with identical message templates and only the resolved label,
schedule and continuation flag differing. Extract a module-private
assertScheduleValid() taking those three already-resolved values plus
`enabled`, so each caller keeps its own resolution policy local and only the
two thrown messages are shared.

Also tighten the two existing cron-rejection assertions to match the full
message including the job label — a probe replacing `label` with a constant
previously left the suite green.

Behavior unchanged. enableJob's third validateCronExpression call is
deliberately excluded: it precedes the cron check with a different guard and
a different message, and follows it with validateCronInterval.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 apps/server/src/jobs/service.ts       | 56 +++++++++++++++++----------
 apps/server/test/jobs/service.test.ts |  8 +++-
 2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/apps/server/src/jobs/service.ts b/apps/server/src/jobs/service.ts
index 8a6a372f1..ee3f735b0 100644
--- a/apps/server/src/jobs/service.ts
+++ b/apps/server/src/jobs/service.ts
@@ -592,16 +592,12 @@ export class JobService {
   async addJob(input: AddJobInput): Promise<JobRecord> {
     const displayName = input.displayName?.trim() || input.name;
     const schedule = input.schedule === "" ? null : (input.schedule ?? null);
-    if (schedule && !validateCronExpression(schedule)) {
-      throw new Error(
-        `Job "${displayName}" has an invalid cron expression: "${schedule}"`
-      );
-    }
-    if (input.enabled && !schedule && !input.continuationEnabled) {
-      throw new Error(
-        `Job "${displayName}" needs a schedule or continuation enabled before it can be enabled.`
-      );
-    }
+    assertScheduleValid({
+      label: displayName,
+      schedule,
+      enabled: input.enabled,
+      continuationEnabled: input.continuationEnabled,
+    });
 
     const agentConfig = applyAgentConfigDefaults(input);
 
@@ -677,18 +673,14 @@ export class JobService {
     const schedule = input.schedule === "" ? null : input.schedule;
     const nextSchedule =
       input.schedule === undefined ? existing.schedule : schedule;
-    if (nextSchedule && !validateCronExpression(nextSchedule)) {
-      throw new Error(
-        `Job "${input.displayName ?? existing.name}" has an invalid cron expression: "${nextSchedule}"`
-      );
-    }
     const nextContinuationEnabled =
       input.continuationEnabled ?? existing.continuationEnabled;
-    if (input.enabled && !nextSchedule && !nextContinuationEnabled) {
-      throw new Error(
-        `Job "${input.displayName ?? existing.name}" needs a schedule or continuation enabled before it can be enabled.`
-      );
-    }
+    assertScheduleValid({
+      label: input.displayName ?? existing.name,
+      schedule: nextSchedule,
+      enabled: input.enabled,
+      continuationEnabled: nextContinuationEnabled,
+    });
 
     const config: Parameters<JobStore["updateJobConfig"]>[1] = {};
     const displayName = normalizeOptionalString(input.displayName);
@@ -1347,6 +1339,30 @@ function formatCompletionCriteria(
   return criteria.map((criterion) => `- ${criterion}`).join("\n");
 }
 
+/**
+ * Shared schedule guards for addJob and updateJob. Each caller resolves the
+ * label, schedule and continuation flag with its own policy and passes the
+ * already-resolved values, so only the two thrown messages are shared.
+ */
+function assertScheduleValid(params: {
+  label: string;
+  schedule: string | null | undefined;
+  enabled: boolean | undefined;
+  continuationEnabled: boolean | undefined;
+}): void {
+  const { label, schedule, enabled, continuationEnabled } = params;
+  if (schedule && !validateCronExpression(schedule)) {
+    throw new Error(
+      `Job "${label}" has an invalid cron expression: "${schedule}"`
+    );
+  }
+  if (enabled && !schedule && !continuationEnabled) {
+    throw new Error(
+      `Job "${label}" needs a schedule or continuation enabled before it can be enabled.`
+    );
+  }
+}
+
 function normalizeOptionalString(
   value: string | undefined
 ): string | undefined {
diff --git a/apps/server/test/jobs/service.test.ts b/apps/server/test/jobs/service.test.ts
index b3acdc280..a0867560e 100644
--- a/apps/server/test/jobs/service.test.ts
+++ b/apps/server/test/jobs/service.test.ts
@@ -635,7 +635,9 @@ describe("JobService", () => {
           prompt: "Test",
           schedule: "invalid cron",
         })
-      ).rejects.toThrow("invalid cron expression");
+      ).rejects.toThrow(
+        'Job "bad-cron" has an invalid cron expression: "invalid cron"'
+      );
 
       await service.shutdown();
     });
@@ -759,7 +761,9 @@ describe("JobService", () => {
           directory: "/tmp/test-upd-cron",
           schedule: "not a cron",
         })
-      ).rejects.toThrow("invalid cron expression");
+      ).rejects.toThrow(
+        'Job "upd-bad-cron" has an invalid cron expression: "not a cron"'
+      );
 
       await service.shutdown();
     });