fix(cron)!: treat blank thread_id/id as unset; document thread_id/id behavior

config.toml.example

@@ -334,12 +334,12 @@ error_hold_ms = 2500
 # [[cron.jobs]]
 # enabled = true                               # optional, default: true
 # schedule = "0 9 * * 1-5"                    # weekdays at 9:00 AM
-# channel = "123456789"                        # target channel/thread ID
+# channel = "123456789012345678"               # target channel/thread ID
 # message = "summarize yesterday's merged PRs" # prompt for the agent
 # platform = "discord"                         # "discord" or "slack"
 # sender_name = "DailyOps"                     # attribution (default: "openab-cron")
 # timezone = "America/New_York"                # IANA timezone (default: "UTC")
-# thread_id = ""                               # optional: post to existing thread
+# thread_id = "234567890123456789"             # optional: post to existing thread (omit for a new thread)
 
 # [[cron.jobs]]
 # schedule = "0 0 * * 0"

crates/openab-core/src/cron.rs

@@ -327,8 +327,23 @@ pub fn load_usercron_file(path: &Path, configured_platforms: &[&str]) -> Vec<Cro
             return vec![];
         }
     };
-    // Validate each entry individually — keep valid ones, skip bad ones
-    parsed.jobs.into_iter().enumerate().filter(|(i, job)| {
+    // Validate each entry individually — keep valid ones, skip bad ones.
+    // Writeback (`update_usercron_job`) targets the first raw TOML entry whose
+    // trimmed `id` matches, so the invariant the loader must enforce is: the
+    // first entry in file order with a given trimmed id owns that id, even if it
+    // is invalid or disabled. Reserve the id BEFORE any other validation so a
+    // cron-invalid/bad-timezone/disabled first entry still claims it; otherwise
+    // a later same-id entry would load, run, and write back onto the first raw
+    // entry — the cross-job write this guard exists to prevent. Do not move this
+    // check below the validations.
+    let mut seen_ids: HashSet<String> = HashSet::new();
+    parsed.jobs.into_iter().enumerate().filter(move |(i, job)| {
+        if let Some(id) = non_empty_opt(job.id.as_deref()) {
+            if !seen_ids.insert(id.to_owned()) {
+                warn!(index = i, id, "usercron: duplicate id (compared after trimming), skipping");
+                return false;
+            }
+        }
         if let Err(e) = parse_cron_expr(&job.schedule) {
             warn!(index = i, schedule = %job.schedule, error = %e, "usercron: invalid cron expression, skipping");
             return false;
@@ -411,12 +426,26 @@ fn parse_job_list(
         Some(ParsedJob {
             schedule,
             tz,
-            config: job.clone(),
+            config: normalize_job_config(job),
             usercron_path: usercron_path.map(Path::to_path_buf),
         })
     }).collect()
 }
 
+/// Canonicalize optional identifiers so blank values behave like omitted ones.
+///
+/// `thread_id = ""` would otherwise deserialize to `Some("")`, skip thread
+/// creation, and fail the send. Both fields also have surrounding whitespace
+/// trimmed: a padded `thread_id` never matches a real platform thread ID, and
+/// trimming `id` keeps writeback lookups consistent with `update_usercron_job`,
+/// which also compares trimmed values.
+fn normalize_job_config(job: &CronJobConfig) -> CronJobConfig {
+    let mut config = job.clone();
+    config.thread_id = non_empty_opt(job.thread_id.as_deref()).map(str::to_owned);
+    config.id = non_empty_opt(job.id.as_deref()).map(str::to_owned);
+    config
+}
+
 /// Run the internal cron scheduler. Evaluates cron expressions once per minute.
 /// `usercron_path` enables hot-reload of an external cronjob.toml file.
 pub async fn run_scheduler(
@@ -898,7 +927,14 @@ fn update_usercron_job(
 
     let mut found = false;
     for table in jobs.iter_mut() {
-        if table.get("id").and_then(|item| item.as_str()) != Some(id) {
+        // Trim the file side too: callers pass a trimmed id, so a padded
+        // `id = " x "` would otherwise never match and writeback would fail.
+        if table
+            .get("id")
+            .and_then(|item| item.as_str())
+            .map(str::trim)
+            != Some(id.trim())
+        {
             continue;
         }
         if let Some(enabled) = enabled {
@@ -1511,6 +1547,218 @@ message = "a"
         assert!(err.to_string().contains("not found"));
     }
 
+    /// Parse one job through `parse_job_list` and return the stored config.
+    fn parsed_config(job: CronJobConfig, usercron_path: Option<&Path>) -> CronJobConfig {
+        let parsed = parse_job_list(&[job], "test", usercron_path);
+        assert_eq!(parsed.len(), 1);
+        parsed[0].config.clone()
+    }
+
+    #[test]
+    fn parse_job_list_normalizes_thread_id_on_baseline_and_usercron() {
+        let usercron = Path::new("/tmp/cronjob.toml");
+        let cases: [(Option<&str>, Option<&str>); 5] = [
+            (None, None),
+            (Some(""), None),
+            (Some("   "), None),
+            (Some(" 1556488188002959532 "), Some("1556488188002959532")),
+            (Some("1556488188002959532"), Some("1556488188002959532")),
+        ];
+        for path in [None, Some(usercron)] {
+            for (input, expected) in cases {
+                let mut job = test_cron_job();
+                job.thread_id = input.map(str::to_owned);
+                let config = parsed_config(job, path);
+                assert_eq!(
+                    config.thread_id.as_deref(),
+                    expected,
+                    "thread_id {input:?} (usercron_path {path:?})"
+                );
+                // Blank thread_id must take the create-thread path, not send to "".
+                assert_eq!(should_create_cron_thread(&config), expected.is_none());
+            }
+        }
+    }
+
+    #[test]
+    fn parse_job_list_trims_and_drops_blank_id() {
+        let cases: [(Option<&str>, Option<&str>); 4] = [
+            (None, None),
+            (Some(""), None),
+            (Some("  "), None),
+            (Some(" daily-news "), Some("daily-news")),
+        ];
+        for (input, expected) in cases {
+            let mut job = test_cron_job();
+            job.id = input.map(str::to_owned);
+            assert_eq!(
+                parsed_config(job, None).id.as_deref(),
+                expected,
+                "id {input:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn update_usercron_job_matches_padded_id_in_file() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("cronjob.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = " padded "
+schedule = "* * * * *"
+channel = "123"
+message = "a"
+"#,
+        )
+        .unwrap();
+        update_usercron_job(&path, "padded", None, Some("thread-789")).unwrap();
+        let doc = std::fs::read_to_string(&path)
+            .unwrap()
+            .parse::<DocumentMut>()
+            .unwrap();
+        let job = doc["jobs"].as_array_of_tables().unwrap().get(0).unwrap();
+        assert_eq!(job["thread_id"].as_str(), Some("thread-789"));
+    }
+
+    #[test]
+    fn load_usercron_file_skips_ids_that_collide_after_trimming() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("cronjob.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = "x"
+schedule = "* * * * *"
+channel = "123"
+message = "first"
+
+[[jobs]]
+id = " x "
+schedule = "* * * * *"
+channel = "123"
+message = "second"
+"#,
+        )
+        .unwrap();
+        // Both ids trim to "x" and would share one writeback target, so only the
+        // first survives; the padded duplicate is skipped.
+        let jobs = load_usercron_file(&path, &["discord"]);
+        assert_eq!(jobs.len(), 1);
+        assert_eq!(jobs[0].message, "first");
+    }
+
+    #[test]
+    fn load_usercron_file_skips_exact_duplicate_ids() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("cronjob.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = "x"
+schedule = "* * * * *"
+channel = "123"
+message = "first"
+
+[[jobs]]
+id = "x"
+schedule = "* * * * *"
+channel = "123"
+message = "second"
+"#,
+        )
+        .unwrap();
+        // Exact duplicates also share one writeback target (behavior change from
+        // main, where both used to load and run): only the first survives.
+        let jobs = load_usercron_file(&path, &["discord"]);
+        assert_eq!(jobs.len(), 1);
+        assert_eq!(jobs[0].message, "first");
+    }
+
+    #[test]
+    fn load_usercron_file_first_entry_owns_id_even_when_invalid() {
+        // The first file-order entry with a given trimmed id owns that id, even
+        // if it is itself skipped. A later valid same-id entry must not load,
+        // because its writeback would land on the first raw entry.
+        let dir = tempfile::tempdir().unwrap();
+        let configured = &["discord"];
+
+        // (a) first entry has an invalid cron expression
+        let path = dir.path().join("invalid_cron.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = "x"
+schedule = "not a cron"
+channel = "123"
+message = "first"
+
+[[jobs]]
+id = " x "
+schedule = "* * * * *"
+channel = "123"
+message = "second"
+"#,
+        )
+        .unwrap();
+        assert!(load_usercron_file(&path, configured).is_empty());
+
+        // (b) first entry has an invalid timezone
+        let path = dir.path().join("invalid_tz.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = "x"
+schedule = "* * * * *"
+timezone = "Not/AZone"
+channel = "123"
+message = "first"
+
+[[jobs]]
+id = " x "
+schedule = "* * * * *"
+channel = "123"
+message = "second"
+"#,
+        )
+        .unwrap();
+        assert!(load_usercron_file(&path, configured).is_empty());
+
+        // (c) first entry is disabled
+        let path = dir.path().join("disabled.toml");
+        std::fs::write(
+            &path,
+            r#"
+[[jobs]]
+id = "x"
+schedule = "* * * * *"
+enabled = false
+channel = "123"
+message = "first"
+
+[[jobs]]
+id = " x "
+schedule = "* * * * *"
+channel = "123"
+message = "second"
+"#,
+        )
+        .unwrap();
+        // `enabled = false` entries are kept by the loader (parse_job_list drops
+        // them later), but they still claim the id, so the later entry is
+        // skipped and only the disabled first entry remains.
+        let jobs = load_usercron_file(&path, configured);
+        assert_eq!(jobs.len(), 1);
+        assert_eq!(jobs[0].message, "first");
+        assert!(!jobs[0].enabled);
+    }
+
     #[tokio::test]
     async fn disable_on_success_requires_exit_zero_and_marker() {
         let mut job = test_cron_job();

docs/config-reference.md

@@ -785,16 +785,16 @@ usercron_path = "cronjob.toml"               # relative to $HOME/.openab/, or ab
 [[cron.jobs]]
 enabled = true                               # optional, default: true
 schedule = "0 9 * * 1-5"                    # cron expression (5-field POSIX)
-channel = "123456789"                        # target channel/thread ID
+channel = "123456789012345678"               # target channel/thread ID
 message = "summarize yesterday's merged PRs" # message sent to agent
 platform = "discord"                         # optional, default: "discord"
 sender_name = "DailyOps"                     # optional, default: "openab-cron"
 timezone = "America/New_York"                # optional, default: "UTC"
-thread_id = ""                               # optional, post to existing thread
+# thread_id = "234567890123456789"           # optional, post to existing thread (omit for a new thread per run)
 
 [[cron.jobs]]
 schedule = "0 0 * * 0"
-channel = "123456789"
+channel = "123456789012345678"
 message = "generate weekly status report"
 platform = "discord"
 timezone = "UTC"
@@ -818,17 +818,17 @@ timezone = "UTC"
 | `platform` | string | `"discord"` | Target platform (`"discord"` or `"slack"`). |
 | `sender_name` | string | `"openab-cron"` | Sender attribution shown in the prompt context. |
 | `timezone` | string | `"UTC"` | IANA timezone for schedule evaluation (e.g. `"America/New_York"`, `"Europe/Berlin"`). |
-| `thread_id` | string | `""` | Optional thread ID to post into an existing thread. |
+| `thread_id` | string | — (unset) | Optional thread ID to post into an existing thread. Omit the field for a new thread per run, unless a usercron `id` pins the job to its first thread. See [Thread Behavior](cronjob.md#thread-behavior). |
 
 The external `cronjob.toml` uses `[[jobs]]` (same fields). See [Usercron docs](cronjob.md#usercron--hot-reload-with-cronjobtoml) for details.
 
 ### Usercron-only `[[jobs]]` fields
 
-These fields are valid only in the external usercron file, for example `$HOME/.openab/cronjob.toml`. They are rejected in baseline `[[cron.jobs]]` because OpenAB only writes state back to the user-managed cron file.
+These fields only take effect in the external usercron file, for example `$HOME/.openab/cronjob.toml`. `disable_on_success` is rejected in baseline `[[cron.jobs]]`; the others, including `id`, are parsed but ignored there, because OpenAB only writes state back to the user-managed cron file.
 
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
-| `id` | string | *required with `disable_on_success`* | Stable job ID used when the scheduler writes `enabled = false` or `thread_id` back to `cronjob.toml`. |
+| `id` | string | *required with `disable_on_success`* | Unique job ID that enables scheduler writeback of `enabled`/`thread_id` to `cronjob.toml`. See [Thread Behavior](cronjob.md#thread-behavior). |
 | `disable_on_success` | string | — | Command to run before sending the scheduled prompt. |
 | `disable_on_success_match` | string | *required with `disable_on_success`* | Marker that must appear in stdout or stderr, in addition to exit code `0`, before the job is considered complete. |
 | `disable_on_success_timeout_secs` | integer | `60` | Timeout for the completion check command. |