From 8dad1cdff5120ff03188814f94b9df93d3cef9b8 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Mon, 5 Oct 2026 03:12:36 +0000 Subject: [PATCH 01/14] docs(cron): document thread_id/id interaction and fix empty thread_id examples --- docs/config-reference.md | 2 +- docs/cronjob.md | 20 +++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/docs/config-reference.md b/docs/config-reference.md index 838ceacb6..d4715854f 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -790,7 +790,7 @@ 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 = "123456789" # optional, post to existing thread (omit for a new thread per run; never "") [[cron.jobs]] schedule = "0 0 * * 0" diff --git a/docs/cronjob.md b/docs/cronjob.md index 653d52e76..a27bdfbaa 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -37,7 +37,7 @@ message = "summarize yesterday's merged PRs" # required: prompt for the 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 = "123456789012345678" # optional: post to existing thread (omit to create a new thread per run) ``` | Field | Required | Default | Description | @@ -49,7 +49,21 @@ thread_id = "" # optional: post to existing thread | `platform` | | `"discord"` | `"discord"`, `"slack"`, `"telegram"`, `"googlechat"`, or `"lineworks"` (non-default platforms require their feature) | | `sender_name` | | `"openab-cron"` | Attribution shown in prompt context | | `timezone` | | `"UTC"` | IANA timezone (e.g. `"America/New_York"`, `"Europe/Berlin"`) | -| `thread_id` | | — | Post into an existing thread instead of the channel | +| `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field entirely to create a new thread per run — do not set it to `""` (see [Thread Behavior](#thread-behavior)) | +| `id` | | — | Usercron only. Stable job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)) | + +### Thread Behavior + +Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This applies to platforms with thread support; Google Chat and LINE WORKS differ (see [Platform Prerequisites](#platform-prerequisites)). + +| `thread_id` | `id` | Behavior | +|---|---|---| +| set | any | Every run posts into that thread. No new thread, no writeback. | +| omitted | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | +| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. All later runs post into that same thread. | +| `""` | any | Invalid. An empty string is treated as a thread ID, not as unset. On Discord the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted. | + +`id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. To get a new thread on every run, omit both fields. ## Cron Expression Format @@ -375,7 +389,7 @@ For an audit trail, record changes somewhere the agent cannot write: push commit - **Minute-aligned**: The scheduler aligns to minute boundaries (`:00`), so `0 9 * * *` fires at exactly 09:00:00, not at whatever second the process started. - **Overlap protection**: If a previous execution of the same job is still running, the next tick is skipped. - **Isolation**: Cron failures are logged but never block interactive chat traffic. -- **Usercron persistence**: For usercron jobs, the scheduler may write `thread_id` and `enabled = false` back to `cronjob.toml`. +- **Usercron persistence**: For usercron jobs with an `id`, the scheduler may write `thread_id` and `enabled = false` back to `cronjob.toml` (see [Thread Behavior](#thread-behavior)). - **Graceful shutdown**: In-flight cron tasks are waited on (up to 30 seconds) during shutdown. ## Sender Identity From 3a18463f558671ecbd690de438a80d6c8275c8e4 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 5 Oct 2026 20:56:30 +0000 Subject: [PATCH 02/14] docs(cron): address #1561 review findings - Thread Behavior: 'no thread_id writeback' (enabled=false can still be written back), qualify the pinned-thread row on successful writeback and link the protected-file caveat, mark '""' invalid on every platform - drop the sentence duplicating the 'omitted/omitted' row - config-reference: thread_id default is unset, not ""; baseline rejects only disable_on_success, other usercron-only fields are ignored - note id is ignored in baseline [[cron.jobs]] - use 18-digit example IDs consistently --- docs/config-reference.md | 10 +++++----- docs/cronjob.md | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/config-reference.md b/docs/config-reference.md index d4715854f..8fa4d912c 100644 --- a/docs/config-reference.md +++ b/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 = "123456789" # optional, post to existing thread (omit for a new thread per run; never "") +# thread_id = "123456789012345678" # optional, post to existing thread (omit for a new thread per run; never "") [[cron.jobs]] schedule = "0 0 * * 0" -channel = "123456789" +channel = "123456789012345678" message = "generate weekly status report" platform = "discord" timezone = "UTC" @@ -818,13 +818,13 @@ 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; never set it to `""` (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 are valid only in the external usercron file, for example `$HOME/.openab/cronjob.toml`. `disable_on_success` is rejected in baseline `[[cron.jobs]]`; the other fields, including `id`, are accepted there but ignored, because OpenAB only writes state back to the user-managed cron file. | Key | Type | Default | Description | |-----|------|---------|-------------| diff --git a/docs/cronjob.md b/docs/cronjob.md index a27bdfbaa..442cc7773 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -50,7 +50,7 @@ timezone = "America/New_York" # optional, default: "UTC" | `sender_name` | | `"openab-cron"` | Attribution shown in prompt context | | `timezone` | | `"UTC"` | IANA timezone (e.g. `"America/New_York"`, `"Europe/Berlin"`) | | `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field entirely to create a new thread per run — do not set it to `""` (see [Thread Behavior](#thread-behavior)) | -| `id` | | — | Usercron only. Stable job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)) | +| `id` | | — | Usercron only; ignored in baseline `[[cron.jobs]]`. Stable job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)) | ### Thread Behavior @@ -58,12 +58,12 @@ Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This appl | `thread_id` | `id` | Behavior | |---|---|---| -| set | any | Every run posts into that thread. No new thread, no writeback. | +| set | any (ignored) | Every run posts into that thread. No new thread and no `thread_id` writeback. | | omitted | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | -| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. All later runs post into that same thread. | -| `""` | any | Invalid. An empty string is treated as a thread ID, not as unset. On Discord the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted. | +| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds, all later runs post into that same thread. If OpenAB cannot write the file, a new thread is created on every run (see [Choosing the right scope](#choosing-the-right-scope)). | +| `""` | any | Invalid on every platform. An empty string is treated as a thread ID, not as unset. On Discord, for example, the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted. | -`id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. To get a new thread on every run, omit both fields. +`id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. ## Cron Expression Format From 9b76a4b5c13a3f4deda41ceb6b17409359b44aa7 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 5 Oct 2026 21:00:43 +0000 Subject: [PATCH 03/14] docs(cron): address remaining #1561 review findings - field table: omitting thread_id with a usercron id pins the job to its first thread; id must be non-empty and unique (writeback updates the first match), empty id counts as unset - replace the commented thread_id = "" in config.toml.example - add a Troubleshooting row for the empty thread_id symptom - use a thread_id example distinct from the channel example --- config.toml.example | 2 +- docs/config-reference.md | 2 +- docs/cronjob.md | 7 ++++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/config.toml.example b/config.toml.example index e5cf7df3d..fe3d7ac6d 100644 --- a/config.toml.example +++ b/config.toml.example @@ -339,7 +339,7 @@ error_hold_ms = 2500 # 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; never "") # [[cron.jobs]] # schedule = "0 0 * * 0" diff --git a/docs/config-reference.md b/docs/config-reference.md index 8fa4d912c..37a1bc36c 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -790,7 +790,7 @@ 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 = "123456789012345678" # optional, post to existing thread (omit for a new thread per run; never "") +# thread_id = "234567890123456789" # optional, post to existing thread (omit for a new thread per run; never "") [[cron.jobs]] schedule = "0 0 * * 0" diff --git a/docs/cronjob.md b/docs/cronjob.md index 442cc7773..19550cad1 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -37,7 +37,7 @@ message = "summarize yesterday's merged PRs" # required: prompt for the agent platform = "discord" # optional, default: "discord" sender_name = "DailyOps" # optional, default: "openab-cron" timezone = "America/New_York" # optional, default: "UTC" -# thread_id = "123456789012345678" # optional: post to existing thread (omit to create a new thread per run) +# thread_id = "234567890123456789" # optional: post to existing thread (omit to create a new thread; never "") ``` | Field | Required | Default | Description | @@ -49,8 +49,8 @@ timezone = "America/New_York" # optional, default: "UTC" | `platform` | | `"discord"` | `"discord"`, `"slack"`, `"telegram"`, `"googlechat"`, or `"lineworks"` (non-default platforms require their feature) | | `sender_name` | | `"openab-cron"` | Attribution shown in prompt context | | `timezone` | | `"UTC"` | IANA timezone (e.g. `"America/New_York"`, `"Europe/Berlin"`) | -| `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field entirely to create a new thread per run — do not set it to `""` (see [Thread Behavior](#thread-behavior)) | -| `id` | | — | Usercron only; ignored in baseline `[[cron.jobs]]`. Stable job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)) | +| `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field to create a new thread (a new one per run, unless a usercron `id` pins the job to its first thread); do not set it to `""` (see [Thread Behavior](#thread-behavior)) | +| `id` | | — | Usercron only; ignored in baseline `[[cron.jobs]]`. Non-empty, unique job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)). An empty `id` counts as unset; with duplicate IDs, writeback updates only the first matching entry | ### Thread Behavior @@ -453,3 +453,4 @@ See [Kubernetes CronJob Reference Architecture](cronjob_k8s_refarch.md) for the | Usercron not reloading | File not saved / wrong path | Check logs for `usercron file changed, reloading` | | Usercron parse error | Invalid TOML syntax | Check logs for `failed to parse usercron file` | | Goal job does not auto-disable | Command did not exit `0` or output did not include `disable_on_success_match` | Run the command manually and confirm both conditions | +| Nothing posted; logs show `failed to send cron message ... cannot parse integer from empty string` | `thread_id = ""` | Remove the `thread_id` line or set a real thread ID (see [Thread Behavior](#thread-behavior)) | From 48710feb7f88f2e7542c849425081bf3e1424155 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 5 Oct 2026 21:02:09 +0000 Subject: [PATCH 04/14] docs(cron): frame empty thread_id as a known defect, note reload delay Describe thread_id = "" as a known defect rather than a rule (only the Discord failure is verified; a future release may treat it as unset), and note that a written-back thread_id takes effect after the next usercron reload (about a minute). --- docs/cronjob.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cronjob.md b/docs/cronjob.md index 19550cad1..96f2a7476 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -60,8 +60,8 @@ Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This appl |---|---|---| | set | any (ignored) | Every run posts into that thread. No new thread and no `thread_id` writeback. | | omitted | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | -| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds, all later runs post into that same thread. If OpenAB cannot write the file, a new thread is created on every run (see [Choosing the right scope](#choosing-the-right-scope)). | -| `""` | any | Invalid on every platform. An empty string is treated as a thread ID, not as unset. On Discord, for example, the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted. | +| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds, later runs post into that same thread once the scheduler reloads the file (about a minute). If OpenAB cannot write the file, a new thread is created on every run (see [Choosing the right scope](#choosing-the-right-scope)). | +| `""` | any | Known defect; do not use. An empty string is not treated as unset: it is passed through as a thread ID. On Discord the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted; other platforms may behave differently. A future release may treat `""` as unset. | `id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. From 0ad5898c666956498a37ec7ccba21a9246064f5e Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 5 Oct 2026 21:05:20 +0000 Subject: [PATCH 05/14] docs(cron): tighten thread/id wording per re-review - matrix: id column 'any' (id still drives enabled=false writeback); qualify pinning on a successful thread creation that records a real thread ID; replace the anchor that does not exist on this base - config-reference: omitting thread_id pins with a usercron id; id must be non-empty and unique - config.toml.example: 18-digit channel example --- config.toml.example | 2 +- docs/config-reference.md | 4 ++-- docs/cronjob.md | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/config.toml.example b/config.toml.example index fe3d7ac6d..003545fb6 100644 --- a/config.toml.example +++ b/config.toml.example @@ -334,7 +334,7 @@ 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") diff --git a/docs/config-reference.md b/docs/config-reference.md index 37a1bc36c..bb1476091 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -818,7 +818,7 @@ 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 | — (unset) | Optional thread ID to post into an existing thread. Omit the field for a new thread per run; never set it to `""` (see [Thread Behavior](cronjob.md#thread-behavior)). | +| `thread_id` | string | — (unset) | Optional thread ID to post into an existing thread. Omit the field for a new thread (one per run, unless a usercron `id` pins the job to its first thread); never set it to `""` (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. @@ -828,7 +828,7 @@ These fields are valid only in the external usercron file, for example `$HOME/.o | 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`* | Non-empty, unique job ID used when the scheduler writes `enabled = false` or `thread_id` back to `cronjob.toml`. An empty `id` counts as unset; with duplicate IDs, writeback updates only the first matching entry. | | `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. | diff --git a/docs/cronjob.md b/docs/cronjob.md index 96f2a7476..400ee6486 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -58,9 +58,9 @@ Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This appl | `thread_id` | `id` | Behavior | |---|---|---| -| set | any (ignored) | Every run posts into that thread. No new thread and no `thread_id` writeback. | +| set | any | Every run posts into that thread. No new thread and no `thread_id` writeback. | | omitted | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | -| omitted | set (usercron) | The first run creates a thread and the scheduler writes its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds, later runs post into that same thread once the scheduler reloads the file (about a minute). If OpenAB cannot write the file, a new thread is created on every run (see [Choosing the right scope](#choosing-the-right-scope)). | +| omitted | set (usercron) | The first run that successfully creates a thread has the scheduler write its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds and records a real thread ID, later runs post into that same thread once the scheduler reloads the file (about a minute). If OpenAB cannot write the file, a new thread is created on every run (see "Choosing the right scope" under Autonomy and Permission Scope below). | | `""` | any | Known defect; do not use. An empty string is not treated as unset: it is passed through as a thread ID. On Discord the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted; other platforms may behave differently. A future release may treat `""` as unset. | `id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. From 5ec86b78770a7a81ba3c6784931dc6163722268f Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:36:35 +0000 Subject: [PATCH 06/14] fix(cron): treat blank thread_id/id as unset and trim id on writeback thread_id = "" deserialized to Some(""), skipped thread creation and failed the send. Normalize thread_id and id with non_empty_opt in parse_job_list (shared by baseline and usercron), and trim the file-side id in update_usercron_job so a padded id still matches on writeback. --- crates/openab-core/src/cron.rs | 98 +++++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 2 deletions(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index 40e9a67ca..5bb41d888 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -411,12 +411,24 @@ 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. 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 +910,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 +1530,81 @@ 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>); 4] = [ + (None, None), + (Some(""), None), + (Some(" "), None), + (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::() + .unwrap(); + let job = doc["jobs"].as_array_of_tables().unwrap().get(0).unwrap(); + assert_eq!(job["thread_id"].as_str(), Some("thread-789")); + } + #[tokio::test] async fn disable_on_success_requires_exit_zero_and_marker() { let mut job = test_cron_job(); From 0445799d4014eb31427b1c0f6e62468c2dcffd35 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:36:36 +0000 Subject: [PATCH 07/14] docs(cron): drop empty thread_id defect notes, document writeback edge cases Blank thread_id/id now behave like omitted fields, so remove the "" matrix row, Troubleshooting row and "never \"\"" comments. Shorten the id row, move writeback edge cases (reload delay, failed persist, Telegram chat-ID writeback) below the matrix, add matching Troubleshooting rows, and fix the contradictory usercron-only fields intro. --- config.toml.example | 2 +- docs/config-reference.md | 6 +++--- docs/cronjob.md | 20 ++++++++++++++------ 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/config.toml.example b/config.toml.example index 003545fb6..c3d1df7b2 100644 --- a/config.toml.example +++ b/config.toml.example @@ -339,7 +339,7 @@ error_hold_ms = 2500 # platform = "discord" # "discord" or "slack" # sender_name = "DailyOps" # attribution (default: "openab-cron") # timezone = "America/New_York" # IANA timezone (default: "UTC") -# thread_id = "234567890123456789" # optional: post to existing thread (omit for a new thread; never "") +# thread_id = "234567890123456789" # optional: post to existing thread (omit for a new thread) # [[cron.jobs]] # schedule = "0 0 * * 0" diff --git a/docs/config-reference.md b/docs/config-reference.md index bb1476091..ad450174d 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -790,7 +790,7 @@ 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 = "234567890123456789" # optional, post to existing thread (omit for a new thread per run; never "") +# thread_id = "234567890123456789" # optional, post to existing thread (omit for a new thread per run) [[cron.jobs]] schedule = "0 0 * * 0" @@ -818,13 +818,13 @@ 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 | — (unset) | Optional thread ID to post into an existing thread. Omit the field for a new thread (one per run, unless a usercron `id` pins the job to its first thread); never set it to `""` (see [Thread Behavior](cronjob.md#thread-behavior)). | +| `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`. `disable_on_success` is rejected in baseline `[[cron.jobs]]`; the other fields, including `id`, are accepted there but ignored, 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 | |-----|------|---------|-------------| diff --git a/docs/cronjob.md b/docs/cronjob.md index 400ee6486..30a69dda9 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -37,7 +37,7 @@ message = "summarize yesterday's merged PRs" # required: prompt for the agent platform = "discord" # optional, default: "discord" sender_name = "DailyOps" # optional, default: "openab-cron" timezone = "America/New_York" # optional, default: "UTC" -# thread_id = "234567890123456789" # optional: post to existing thread (omit to create a new thread; never "") +# thread_id = "234567890123456789" # optional: post to existing thread (omit to create a new thread) ``` | Field | Required | Default | Description | @@ -49,7 +49,7 @@ timezone = "America/New_York" # optional, default: "UTC" | `platform` | | `"discord"` | `"discord"`, `"slack"`, `"telegram"`, `"googlechat"`, or `"lineworks"` (non-default platforms require their feature) | | `sender_name` | | `"openab-cron"` | Attribution shown in prompt context | | `timezone` | | `"UTC"` | IANA timezone (e.g. `"America/New_York"`, `"Europe/Berlin"`) | -| `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field to create a new thread (a new one per run, unless a usercron `id` pins the job to its first thread); do not set it to `""` (see [Thread Behavior](#thread-behavior)) | +| `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field to create a new thread per run, unless a usercron `id` pins the job to its first thread. See [Thread Behavior](#thread-behavior) | | `id` | | — | Usercron only; ignored in baseline `[[cron.jobs]]`. Non-empty, unique job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)). An empty `id` counts as unset; with duplicate IDs, writeback updates only the first matching entry | ### Thread Behavior @@ -59,12 +59,19 @@ Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This appl | `thread_id` | `id` | Behavior | |---|---|---| | set | any | Every run posts into that thread. No new thread and no `thread_id` writeback. | -| omitted | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | -| omitted | set (usercron) | The first run that successfully creates a thread has the scheduler write its ID back to `cronjob.toml` as `thread_id`. If that writeback succeeds and records a real thread ID, later runs post into that same thread once the scheduler reloads the file (about a minute). If OpenAB cannot write the file, a new thread is created on every run (see "Choosing the right scope" under Autonomy and Permission Scope below). | -| `""` | any | Known defect; do not use. An empty string is not treated as unset: it is passed through as a thread ID. On Discord the send fails (`failed to send cron message ... cannot parse integer from empty string` in the logs) and nothing is posted; other platforms may behave differently. A future release may treat `""` as unset. | +| omitted or blank | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | +| omitted or blank | set (usercron) | The first run creates a thread and writes its ID back as `thread_id`; later runs post into that thread. | + +A blank value (`""` or whitespace) is treated the same as omitting the field, for both `thread_id` and `id`. `id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. +Writeback edge cases for the `id` row: + +- The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). +- If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. +- On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. + ## Cron Expression Format Standard 5-field POSIX cron, same as Linux crontab, K8s CronJob, and GitHub Actions: @@ -453,4 +460,5 @@ See [Kubernetes CronJob Reference Architecture](cronjob_k8s_refarch.md) for the | Usercron not reloading | File not saved / wrong path | Check logs for `usercron file changed, reloading` | | Usercron parse error | Invalid TOML syntax | Check logs for `failed to parse usercron file` | | Goal job does not auto-disable | Command did not exit `0` or output did not include `disable_on_success_match` | Run the command manually and confirm both conditions | -| Nothing posted; logs show `failed to send cron message ... cannot parse integer from empty string` | `thread_id = ""` | Remove the `thread_id` line or set a real thread ID (see [Thread Behavior](#thread-behavior)) | +| Usercron job with `id` creates a new thread on every run | `thread_id` writeback failed (e.g. agent or OpenAB cannot write the file) | Check logs for `failed to persist usercron thread_id`; see [Thread Behavior](#thread-behavior) | +| Telegram job with `id`: written-back `thread_id` equals the chat ID | Topic creation failed or timed out on the first run, so the chat ID was persisted | Check logs for `create_topic failed` / `create_topic timeout`; remove the `thread_id` line | From 1ad66c60e8a8044806879634c60eb208c1bb1862 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Sat, 10 Oct 2026 13:03:21 +0000 Subject: [PATCH 08/14] =?UTF-8?q?docs(cron):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20document=20thread=5Fid=20trim,=20dedup=20colliding?= =?UTF-8?q?=20ids,=20tighten=20id=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: document and test non-blank thread_id trimming (normalize_job_config doc comment, Thread Behavior note, padded case in the thread_id test table). F2: reduce both id field rows to one sentence linking to Thread Behavior, and keep Thread Behavior as the single canonical copy of the writeback edge cases (collapse the duplicated 'writeback fails' prose). F3: skip usercron entries whose ids collide after trimming (warn + skip at load, consistent with other load_usercron_file validation), so a padded duplicate id can no longer overwrite another job's writeback entry. Adds load_usercron_file_skips_ids_that_collide_after_trimming. --- crates/openab-core/src/cron.rs | 53 ++++++++++++++++++++++++++++++---- docs/config-reference.md | 2 +- docs/cronjob.md | 9 +++--- 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index 5bb41d888..ccf38576a 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -327,12 +327,24 @@ pub fn load_usercron_file(path: &Path, configured_platforms: &[&str]) -> Vec = HashSet::new(); + parsed.jobs.into_iter().enumerate().filter(move |(i, job)| { if let Err(e) = parse_cron_expr(&job.schedule) { warn!(index = i, schedule = %job.schedule, error = %e, "usercron: invalid cron expression, skipping"); return false; } + 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 after trimming, skipping"); + return false; + } + } if job.timezone.parse::().is_err() { warn!(index = i, timezone = %job.timezone, "usercron: invalid timezone, skipping"); return false; @@ -420,8 +432,10 @@ fn parse_job_list( /// Canonicalize optional identifiers so blank values behave like omitted ones. /// /// `thread_id = ""` would otherwise deserialize to `Some("")`, skip thread -/// creation, and fail the send. Trimming `id` keeps writeback lookups -/// consistent with `update_usercron_job`, which also compares trimmed values. +/// 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); @@ -1540,10 +1554,11 @@ message = "a" #[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>); 4] = [ + 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)] { @@ -1605,6 +1620,34 @@ message = "a" 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"); + } + #[tokio::test] async fn disable_on_success_requires_exit_zero_and_marker() { let mut job = test_cron_job(); diff --git a/docs/config-reference.md b/docs/config-reference.md index ad450174d..0d3362076 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -828,7 +828,7 @@ These fields only take effect in the external usercron file, for example `$HOME/ | Key | Type | Default | Description | |-----|------|---------|-------------| -| `id` | string | *required with `disable_on_success`* | Non-empty, unique job ID used when the scheduler writes `enabled = false` or `thread_id` back to `cronjob.toml`. An empty `id` counts as unset; with duplicate IDs, writeback updates only the first matching entry. | +| `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. | diff --git a/docs/cronjob.md b/docs/cronjob.md index 30a69dda9..04d711557 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -50,7 +50,7 @@ timezone = "America/New_York" # optional, default: "UTC" | `sender_name` | | `"openab-cron"` | Attribution shown in prompt context | | `timezone` | | `"UTC"` | IANA timezone (e.g. `"America/New_York"`, `"Europe/Berlin"`) | | `thread_id` | | — | Post into an existing thread instead of creating a new one. Omit the field to create a new thread per run, unless a usercron `id` pins the job to its first thread. See [Thread Behavior](#thread-behavior) | -| `id` | | — | Usercron only; ignored in baseline `[[cron.jobs]]`. Non-empty, unique job identifier, required for `disable_on_success`. Enables scheduler writeback, including persisting an auto-created `thread_id` (see [Thread Behavior](#thread-behavior)). An empty `id` counts as unset; with duplicate IDs, writeback updates only the first matching entry | +| `id` | | — | Usercron only (ignored in baseline `[[cron.jobs]]`); a unique job identifier that enables scheduler writeback and is required for `disable_on_success`. See [Thread Behavior](#thread-behavior) | ### Thread Behavior @@ -62,14 +62,15 @@ Where a job posts depends on `thread_id` and, for usercron jobs, `id`. This appl | omitted or blank | omitted | Every run creates a new thread. Since sessions are keyed by thread, each run also starts a fresh agent session. | | omitted or blank | set (usercron) | The first run creates a thread and writes its ID back as `thread_id`; later runs post into that thread. | -A blank value (`""` or whitespace) is treated the same as omitting the field, for both `thread_id` and `id`. +A blank value (`""` or whitespace) is treated the same as omitting the field, for both `thread_id` and `id`. Surrounding whitespace is trimmed from both fields, so `thread_id = " 123 "` is stored and matched as `"123"`. `id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. Writeback edge cases for the `id` row: +- A blank `id` counts as unset. Ids are compared after trimming, so entries whose ids differ only by surrounding whitespace collide; the scheduler keeps the first and skips the rest at load (`usercron: duplicate id after trimming, skipping`). - The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). -- If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. +- If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. This also means `disable_on_success` cannot write `enabled = false` back, so the goal command keeps running on every match — avoid both patterns when the file is unwritable. - On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. ## Cron Expression Format @@ -387,7 +388,7 @@ Prompts and `disable_on_success` live in the same file, so write access cannot b `usercron_path` may be absolute, so protect the file and directory it actually points to. Agent CLI path rules alone are not enough, because an agent with a shell tool can bypass them. - OpenAB rewrites the file through a temporary file in the same directory, so scheduler writeback fails whenever OpenAB itself cannot write the file's directory. In that case, do not use `disable_on_success`: an achieved goal cannot be written back as `enabled = false`, so the command keeps running and `✅ Goal achieved` is posted on every schedule match. Also avoid jobs that set `id` but leave `thread_id` unset: the new thread's ID cannot be saved, so a new thread is created on every run. Instead, set a real `thread_id`, or omit `id` if you want a new thread per run. + OpenAB rewrites the file through a temporary file in the same directory, so scheduler writeback fails whenever OpenAB itself cannot write the file's directory. In that case, do not use `disable_on_success`, and avoid jobs that set `id` but leave `thread_id` unset; see the [Thread Behavior](#thread-behavior) writeback edge cases for the resulting symptoms. For an audit trail, record changes somewhere the agent cannot write: push commits to a protected remote, or persist file-monitor events (e.g. from `inotifywait`) to an external or append-only store. A local git repository is not enough, because an agent with a shell tool can rewrite its history. This is detective, not preventive: changes are hot-reloaded and take effect within about a minute. From b84a7f3e7747f446a792cf243ed29a3c940de215 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Sun, 11 Oct 2026 01:05:53 +0000 Subject: [PATCH 09/14] fix(cron): reserve usercron id before validation to protect writeback target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 F1: move the duplicate-id reservation to the top of the load_usercron_file filter, before parse_cron_expr/timezone/platform checks, so the first file-order entry owns its trimmed id even when it is invalid or disabled. Previously a cron-invalid first entry failed to claim its id, letting a later same-id entry load and run — its writeback would then land on the first raw entry (the cross-job write the guard was meant to prevent). Add a comment that the early reservation is intentional. Round-2 F2: document the full duplicate-id policy — all duplicates (compared after trimming, including exact matches) keep only the first entry, and an invalid/disabled first entry still blocks later same-id entries. Reword the warn to 'duplicate id (compared after trimming)'. Add load_usercron_file_skips_exact_duplicate_ids and load_usercron_file_first_entry_owns_id_even_when_invalid (invalid cron, invalid timezone, disabled first entry). --- crates/openab-core/src/cron.rs | 131 ++++++++++++++++++++++++++++++--- 1 file changed, 121 insertions(+), 10 deletions(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index ccf38576a..8eca6eb97 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -328,23 +328,26 @@ pub fn load_usercron_file(path: &Path, configured_platforms: &[&str]) -> Vec = HashSet::new(); parsed.jobs.into_iter().enumerate().filter(move |(i, job)| { - if let Err(e) = parse_cron_expr(&job.schedule) { - warn!(index = i, schedule = %job.schedule, error = %e, "usercron: invalid cron expression, skipping"); - return false; - } 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 after trimming, skipping"); + 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; + } if job.timezone.parse::().is_err() { warn!(index = i, timezone = %job.timezone, "usercron: invalid timezone, skipping"); return false; @@ -1648,6 +1651,114 @@ message = "second" 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_eq!(jobs[0].enabled, false); + } + #[tokio::test] async fn disable_on_success_requires_exit_zero_and_marker() { let mut job = test_cron_job(); From a28ca499c93eda962224c1d6b43eff0cb2f16afa Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Sun, 11 Oct 2026 01:06:12 +0000 Subject: [PATCH 10/14] docs(cron): state full duplicate-id policy and align warn wording Round-2 F2: the writeback edge-case bullet now covers all duplicate ids (exact matches and whitespace-only differences) sharing one writeback target, keeping only the first file-order entry, and that an invalid or disabled first entry still blocks later same-id entries. Match the warn text updated in code (duplicate id (compared after trimming)). --- docs/cronjob.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cronjob.md b/docs/cronjob.md index 04d711557..3ecc6fa76 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -68,7 +68,7 @@ A blank value (`""` or whitespace) is treated the same as omitting the field, fo Writeback edge cases for the `id` row: -- A blank `id` counts as unset. Ids are compared after trimming, so entries whose ids differ only by surrounding whitespace collide; the scheduler keeps the first and skips the rest at load (`usercron: duplicate id after trimming, skipping`). +- A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run. - The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). - If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. This also means `disable_on_success` cannot write `enabled = false` back, so the goal command keeps running on every match — avoid both patterns when the file is unwritable. - On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. From 17e8a2b485a6162be62a63e0a0ba5a590d5c3784 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Thu, 15 Oct 2026 06:21:03 +0800 Subject: [PATCH 11/14] test(cron): use assert! for bool in first-entry-owns-id test Fixes clippy::bool_assert_comparison introduced in b84a7f3e. --- crates/openab-core/src/cron.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/openab-core/src/cron.rs b/crates/openab-core/src/cron.rs index 8eca6eb97..ea6bec58a 100644 --- a/crates/openab-core/src/cron.rs +++ b/crates/openab-core/src/cron.rs @@ -1756,7 +1756,7 @@ message = "second" let jobs = load_usercron_file(&path, configured); assert_eq!(jobs.len(), 1); assert_eq!(jobs[0].message, "first"); - assert_eq!(jobs[0].enabled, false); + assert!(!jobs[0].enabled); } #[tokio::test] From 8bfa926ebccc40b490ee2a42d5fa3537bffe1f10 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Fri, 16 Oct 2026 01:02:17 +0000 Subject: [PATCH 12/14] docs(cron): flag duplicate-id skip as breaking, document writeback re-resolution and Telegram limitation Address round-4 review (chaodu-obk): docs/metadata only, no code change. - F1: Breaking Change callout + migration step + Troubleshooting row for the duplicate-id skip (duplicate ids that ran on main now stop after upgrade). - F2: writeback re-resolves first trimmed-id match at write time; external edits while a job is in flight can redirect thread_id/enabled to another entry. - F3: mark the Telegram chat-ID fallback as a known limitation; tracking issue pending. --- docs/cronjob.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/cronjob.md b/docs/cronjob.md index 3ecc6fa76..445188f69 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -69,9 +69,15 @@ A blank value (`""` or whitespace) is treated the same as omitting the field, fo Writeback edge cases for the `id` row: - A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run. + + > [!CAUTION] + > **Breaking Change** — Duplicate usercron ids now skip. On earlier versions, two `[[jobs]]` entries with the same `id` both loaded and ran; as of this version only the first (in file order) runs and the rest are skipped at load. A job with an accidental copy-paste duplicate id stops firing after upgrading. + > Before upgrading, check `cronjob.toml` for ids that are equal after trimming (including values that differ only by surrounding whitespace) and make them unique. + - The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). +- Writeback re-resolves the first entry by trimmed `id` at write time, not against the snapshot the job was loaded from. `usercron_write_lock` serializes OpenAB's own writes but not external edits. Editing `cronjob.toml` while a job is in flight — inserting a same-id entry above it, reordering entries, or reusing an id for another job — can redirect the writeback to a different entry: `thread_id` pins the wrong entry (the job that ran keeps creating a new thread each run), or `enabled = false` disables an unrelated job. Avoid reusing or moving ids while jobs are running. - If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. This also means `disable_on_success` cannot write `enabled = false` back, so the goal command keeps running on every match — avoid both patterns when the file is unwritable. -- On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. +- **Known limitation** — On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Because Telegram chat IDs and topic IDs are both numeric, a written-back value cannot be distinguished from a correct topic ID. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. This is a documented workaround, not the final behavior; the root-cause fix (gating the writeback on a thread actually being created) is a follow-up (tracking issue pending). ## Cron Expression Format @@ -460,6 +466,7 @@ See [Kubernetes CronJob Reference Architecture](cronjob_k8s_refarch.md) for the | Channel not found | Bot not in channel | Invite the bot to the target channel | | Usercron not reloading | File not saved / wrong path | Check logs for `usercron file changed, reloading` | | Usercron parse error | Invalid TOML syntax | Check logs for `failed to parse usercron file` | +| Usercron job stopped running (e.g. after upgrade) | Another entry earlier in the file has the same `id` (compared after trimming), so this one is skipped | Check logs for `usercron: duplicate id (compared after trimming), skipping`; make the ids unique | | Goal job does not auto-disable | Command did not exit `0` or output did not include `disable_on_success_match` | Run the command manually and confirm both conditions | | Usercron job with `id` creates a new thread on every run | `thread_id` writeback failed (e.g. agent or OpenAB cannot write the file) | Check logs for `failed to persist usercron thread_id`; see [Thread Behavior](#thread-behavior) | | Telegram job with `id`: written-back `thread_id` equals the chat ID | Topic creation failed or timed out on the first run, so the chat ID was persisted | Check logs for `create_topic failed` / `create_topic timeout`; remove the `thread_id` line | From 77c5feac5a672262a9861089ac41d15750023bee Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Fri, 16 Oct 2026 08:24:03 +0000 Subject: [PATCH 13/14] docs(cron): render breaking-change callout as an alert; drop nonexistent tracking issue ref - Move the [!CAUTION] Breaking Change callout out of the duplicate-id list item to a top-level blockquote under Thread Behavior, so GitHub renders it as an alert instead of a plain blockquote with a literal [!CAUTION] marker (verified via the GitHub GFM markdown API). Addresses round-5 F1. - Drop the "(tracking issue pending)" parenthetical that pointed to an issue that does not exist and cannot be created from this account; keep the known-limitation marker and recovery step. Addresses round-5 F2. --- docs/cronjob.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/cronjob.md b/docs/cronjob.md index 445188f69..6d6b1fdd7 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -66,18 +66,17 @@ A blank value (`""` or whitespace) is treated the same as omitting the field, fo `id` does not select a thread — it only tells the scheduler which entry to update. If you remove a written-back `thread_id` but keep `id`, the next run creates a new thread and pins the job to it again. -Writeback edge cases for the `id` row: - -- A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run. +> [!CAUTION] +> **Breaking Change** — Duplicate usercron ids now skip. On earlier versions, two `[[jobs]]` entries with the same `id` both loaded and ran; as of this version only the first (in file order) runs and the rest are skipped at load. A job with an accidental copy-paste duplicate id stops firing after upgrading. +> Before upgrading, check `cronjob.toml` for ids that are equal after trimming (including values that differ only by surrounding whitespace) and make them unique. - > [!CAUTION] - > **Breaking Change** — Duplicate usercron ids now skip. On earlier versions, two `[[jobs]]` entries with the same `id` both loaded and ran; as of this version only the first (in file order) runs and the rest are skipped at load. A job with an accidental copy-paste duplicate id stops firing after upgrading. - > Before upgrading, check `cronjob.toml` for ids that are equal after trimming (including values that differ only by surrounding whitespace) and make them unique. +Writeback edge cases for the `id` row: +- A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run (see the Breaking Change note below). - The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). - Writeback re-resolves the first entry by trimmed `id` at write time, not against the snapshot the job was loaded from. `usercron_write_lock` serializes OpenAB's own writes but not external edits. Editing `cronjob.toml` while a job is in flight — inserting a same-id entry above it, reordering entries, or reusing an id for another job — can redirect the writeback to a different entry: `thread_id` pins the wrong entry (the job that ran keeps creating a new thread each run), or `enabled = false` disables an unrelated job. Avoid reusing or moving ids while jobs are running. - If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. This also means `disable_on_success` cannot write `enabled = false` back, so the goal command keeps running on every match — avoid both patterns when the file is unwritable. -- **Known limitation** — On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Because Telegram chat IDs and topic IDs are both numeric, a written-back value cannot be distinguished from a correct topic ID. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. This is a documented workaround, not the final behavior; the root-cause fix (gating the writeback on a thread actually being created) is a follow-up (tracking issue pending). +- **Known limitation** — On gateway platforms (Telegram), if topic creation fails or times out (5s), the job posts to the chat itself and the **chat ID** is written back as `thread_id`. Because Telegram chat IDs and topic IDs are both numeric, a written-back value cannot be distinguished from a correct topic ID. Later runs then target that ID as a topic. Remove the written-back `thread_id` line to retry topic creation. This is a documented workaround, not the final behavior; the root-cause fix (gating the writeback on a thread actually being created) is a follow-up. ## Cron Expression Format From cc5303cf3974e2fa3a39759629c34f434e5e55a0 Mon Sep 17 00:00:00 2001 From: charlie0228 <24387287+charlie0228@users.noreply.github.com> Date: Sat, 17 Oct 2026 01:00:55 +0000 Subject: [PATCH 14/14] docs(cron): drop stale direction in Breaking Change cross-reference The callout now sits above the duplicate-id bullet, so 'see the Breaking Change note below' pointed the wrong way. Drop the direction word so the reference survives future moves (round-6 F1). --- docs/cronjob.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cronjob.md b/docs/cronjob.md index 6d6b1fdd7..0a44c10d7 100644 --- a/docs/cronjob.md +++ b/docs/cronjob.md @@ -72,7 +72,7 @@ A blank value (`""` or whitespace) is treated the same as omitting the field, fo Writeback edge cases for the `id` row: -- A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run (see the Breaking Change note below). +- A blank `id` counts as unset. Ids are compared after trimming, so any duplicates — including identical values and ids that differ only by surrounding whitespace — share one writeback target. The scheduler keeps the first entry in file order and skips the rest at load (`usercron: duplicate id (compared after trimming), skipping`). The first entry owns the id even when it is itself invalid or disabled, so a later entry with the same id still does not run (see the Breaking Change note). - The pinned thread takes effect once the scheduler reloads the file (next tick, normally within 60s). - Writeback re-resolves the first entry by trimmed `id` at write time, not against the snapshot the job was loaded from. `usercron_write_lock` serializes OpenAB's own writes but not external edits. Editing `cronjob.toml` while a job is in flight — inserting a same-id entry above it, reordering entries, or reusing an id for another job — can redirect the writeback to a different entry: `thread_id` pins the wrong entry (the job that ran keeps creating a new thread each run), or `enabled = false` disables an unrelated job. Avoid reusing or moving ids while jobs are running. - If OpenAB cannot write `cronjob.toml` (see [Choosing the right scope](#choosing-the-right-scope)), nothing is pinned and every run creates a new thread. The logs show `failed to persist usercron thread_id`. This also means `disable_on_success` cannot write `enabled = false` back, so the goal command keeps running on every match — avoid both patterns when the file is unwritable.