refactor(server): share the addJob/updateJob schedule guards (#tech-debt)

[selfcontained]

What

JobService.addJob and JobService.updateJob each inlined the same two schedule guards:

1. if (schedule && !validateCronExpression(schedule)) throw \Job "${label}" has an invalid cron expression: "${schedule}"``
2. if (enabled && !schedule && !continuationEnabled) throw \Job "${label}" needs a schedule or continuation enabled before it can be enabled.``

Both message templates were byte-identical once the label and schedule were substituted. Extracted a module-private assertScheduleValid({ label, schedule, enabled, continuationEnabled }).

Why this is tech debt

Two copies of the same user-facing error text with no shared source. It has already drifted once in spirit — a third caller (enableJob) carries a variant — and the whole point of the entry is preventing the two remaining copies from drifting apart.

What the consolidated sites have in common — precisely

Both call sites pass already-resolved values, so each keeps its own resolution policy inline and unchanged:

	addJob	updateJob
label	input.displayName?.trim() || input.name	input.displayName ?? existing.name
schedule	own resolved schedule	nextSchedule (falls back to existing.schedule)
continuation	input.continuationEnabled	nextContinuationEnabled (?? existing.continuationEnabled)
enabled	input.enabled	input.enabled

What is shared is exactly the two guard conditions and the two throw messages — nothing else moved. This is the same near-miss triage shape #1026 and #1032 used.

One statement reordering in updateJob: nextContinuationEnabled is now computed before the cron check rather than between the two guards. It is a pure ?? expression with no side effects and nothing between the old and new positions mutates its inputs, so the observable throw order is unchanged.

Deliberately excluded

enableJob (service.ts:813-827) — the third validateCronExpression call, with the identical invalid-cron message template. Left alone because it is a genuine near-miss, not a copy:

• It precedes the cron check with a different guard (!schedule && !job.continuationEnabled → "has no schedule configured; enable continuation first."), a different message from the shared one. Routing it through the helper would silently replace that curated message.
• It follows the cron check with validateCronInterval, which neither of the other two sites runs.

Folding it in would be a behavior change, not a sweep.

Proof the sweep is identity-preserving

Reverting is degenerate (the helper would cease to exist), so eight defects were probed into the new code, each reverted after, against apps/server/test/jobs/service.test.ts (38 tests):

probe	result
cron check disabled	2 failed ✓
enabled-guard disabled	2 failed ✓
updateJob schedule → null	1 failed ✓
addJob continuation → true	1 failed ✓
updateJob continuation → true	1 failed ✓
addJob label → "PROBE"	stayed green ✗

The label probe passing was the one real gap: both cron-rejection tests asserted only on the substring "invalid cron expression", never on the job label — and the label is exactly the parameter that differs between the two call sites. Tightened both assertions to the full message ('Job "bad-cron" has an invalid cron expression: "invalid cron"' and the upd-bad-cron equivalent). After that, both label probes (addJob and updateJob) go red. No new test files; two assertions tightened.

git diff --stat confirmed clean restore after every probe.

Checks

• pnpm run check ✓
• pnpm run test — server 187 files passed / 1 skipped, web 1924 tests, browser-extension 60 ✓
• pnpm run test:e2e — 198 passed / 12 skipped, green first try (the agent-surfaces.spec.ts:680 flake the last run saw did not recur)
• pnpm run finalize:web not run — no apps/web/ source files changed

Question for the reviewer

The helper takes a 4-field options object rather than positional params. Labelled alternatives:

• A (shipped) — options object. Two of the four params are adjacent boolean | undefined, which are trivially transposable positionally and would transpose silently since both are optional booleans.
• B — positional assertScheduleValid(label, schedule, enabled, continuationEnabled), matching the three-positional-arg shape the backlog entry originally sketched. Shorter call sites, no object literal.

Ruling 1308 ("keep explicit short prop lists") is about hook APIs at 5-7 fields and does not obviously govern a 4-param private function, so this is genuinely open.

Queued for the next run

resolveRepoRoot x3 in apps/server/src — shared/git/worktree.ts:347 and shared/github/pr.ts:167 are near-identical 9-line file-private wrappers differing only in the thrown error class.

🤖 Generated with Claude Code