[thoththeagent]

What problem does this solve?

Continuation of #1561, which was auto-closed while waiting on the author. My apologies for the silence; the round-2 review went unnoticed on my side. This PR carries over all #1561 commits, including @chaodu-agent's four follow-up commits, rebased onto current main (after #1562). It then addresses every round-2 finding.

The underlying problems are unchanged:

1. The cron examples taught thread_id = "". That value deserialized to Some(""), skipped thread creation, and failed the send. Nothing was posted; the error only appeared in the logs.
2. Usercron id writeback, which pins a job to its first thread, was undocumented.
3. id was missing from the field table.

Supersedes #1561.

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1556505265183137842

Review Contract

Goal

Make blank thread_id/id behave like omitted fields, and document how thread_id and id decide where a cron job posts, including writeback edge cases.

Non-goals

• Changing the gateway/Telegram writeback behavior (F4 code fix). It is documented here and left as a follow-up, as suggested in the review.
• Helm chart changes.

Accepted Residual Risks

• On gateway platforms (Telegram), a failed or timed-out create_topic still persists the chat ID as thread_id. This is now documented with a Troubleshooting row and a recovery step (remove the line). The code fix is a follow-up.

Acceptance Criteria

• thread_id = "" / whitespace-only behaves exactly like an omitted thread_id on both the baseline and usercron paths (tested).
• A blank id is treated as unset; a padded id still matches on writeback (tested).
• No docs or examples describe "" as a defect or say "never """.
• The id matrix row is one sentence; the edge cases are listed separately with an anchor link to Choosing the right scope.
• Troubleshooting has rows for failed to persist usercron thread_id and the Telegram chat-ID writeback.
• The usercron-only fields intro is no longer self-contradictory.

Follow-ups

• Gate thread_id writeback on a thread actually being created (ch.thread_id.is_some() || ch.parent_id.is_some()), so the gateway fallback no longer persists the chat ID (review F4).
• Non-blocking round-2 suggestions not taken here: remaining 9-digit channel placeholders, de-duplicating the id description, stale platform rows in config-reference.md, and timezone comment alignment.

At a Glance

config.toml / cronjob.toml
        │ deserialize
        ▼
parse_job_list ── normalize_job_config ──► thread_id: ""/"  " → None   (NEW)
        │                                  id:  " x " → "x", "" → None (NEW)
        ▼
fire_cronjob
  ├─ thread_id = Some(id)  → post into that thread
  └─ thread_id = None      → create thread
         └─ id set (usercron) → update_usercron_job (file-side id trimmed, NEW) → pinned

Prior Art & Industry Research

Not applicable — small input-normalization bug fix plus docs for existing behavior.

Proposed Solution

Code (crates/openab-core/src/cron.rs):

• normalize_job_config() in parse_job_list canonicalizes thread_id and id with the existing non_empty_opt helper. parse_job_list is shared by baseline and usercron, and fire_cronjob only reads ParsedJob.config. The value is therefore normalized before send_message() runs, not only inside should_create_cron_thread (per F1).
• update_usercron_job trims the file-side id before comparing, so id = " x " matches the trimmed lookup key (the pre-existing edge case noted in F1).
• Three tests:
  • parse_job_list_normalizes_thread_id_on_baseline_and_usercron: omitted, "", whitespace-only, and a real ID on both paths. It also asserts should_create_cron_thread.
  • parse_job_list_trims_and_drops_blank_id
  • update_usercron_job_matches_padded_id_in_file

Docs:

• F1: removed the "" matrix row, the "" Troubleshooting row, and every "never """ comment (cronjob.md, config-reference.md, config.toml.example). Added one line stating that blank equals omitted.
• F2: the id row is now one sentence. Reload delay, failed persist, and Telegram moved to a short "Writeback edge cases" list with an anchor link to #choosing-the-right-scope. Added a Troubleshooting row keyed on failed to persist usercron thread_id.
• F3: the intro now reads "only take effect in the external usercron file … parsed but ignored there".
• F4: documented that on gateway/Telegram a failed or timed-out topic creation persists the chat ID as thread_id, with a Troubleshooting row (create_topic failed / create_topic timeout) and the fix (remove the line).

Why this approach?

F1 option (a): the fix is two lines with no compatibility risk, since "" could never succeed on any platform. Fixing the code is cheaper than keeping, and later deleting, docs about the defect.

Alternatives Considered

• F1 option (b), docs-only plus a tracking issue: rejected for the reason above.
• Fixing F4 in this PR: kept separate. It changes writeback semantics across adapters (the Discord shape needs the .or(...) fallback), so it deserves its own review.

Validation

• cargo test -p openab-core --lib cron::: 78 passed, 0 failed, including the 3 new tests.
• cargo clippy -p openab-core -- -D warnings (same scope as CI): clean. With --all-targets there are 6 pre-existing test-only lints (cron.rs:1272,1294 and others in format.rs, discord.rs), none in the changed code.
• Anchors resolve: #thread-behavior, #choosing-the-right-scope, #platform-prerequisites.
• Original behavior, observed on OpenAB 0.10.0-beta.1 (see docs(cron): document thread_id/id interaction, fix empty thread_id #1561): thread_id = "" logged failed to send cron message ... cannot parse integer from empty string; id without thread_id pinned the job to its first thread.
• Renders correctly in GitHub preview.

@chaodu-obk round-2 findings, point by point:

#	Resolution
F1	Option (a). Normalized in parse_job_list, covering both paths before send_message, plus tests. Padded-id writeback fixed. All "" defect docs removed.
F2	Cell reduced to one sentence; edge cases moved below with an anchor link; Troubleshooting row for failed to persist usercron thread_id.
F3	Intro reworded as suggested.
F4	Documented with a recovery step and a Troubleshooting row. Code gate left as a follow-up.
Rebase	Rebased on main after #1562; the scope reference is now an anchor link.

Behavior change from main (duplicate usercron ids)

On main, two usercron entries with the same id both load and run. As of this PR, duplicate ids (compared after trimming, including exact matches) share one writeback target, so the loader keeps only the first entry in file order and skips the rest (usercron: duplicate id (compared after trimming), skipping). The first entry owns the id even when it is itself invalid or disabled, so a later same-id entry still does not run. This prevents a later job's thread_id/enabled writeback from landing on a different entry.

Breaking change / migration: this is a user-facing behavior change (a duplicate-id job that ran on main silently stops 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 docs carry a > [!CAUTION] Breaking Change callout and a Troubleshooting row for this.

[thoththeagent]

Thanks for the round-1 review. Addressed all three 🟡 findings in 1ad66c6. CI check (cargo check, clippy -D warnings, cargo test) is green on the new head.

F1 — Document and test thread_id trimming
Took the preferred option (document the trim, do not special-case blank-only):

• normalize_job_config doc comment now states both fields are trimmed and why a padded thread_id is dropped (never matches a real platform thread ID).
• Thread Behavior gains one sentence: surrounding whitespace is trimmed from both fields, with thread_id = " 123 " → "123" as the example.
• parse_job_list_normalizes_thread_id_on_baseline_and_usercron gets a (Some(" 123 "), Some("123")) case, asserted on both the baseline and usercron paths plus should_create_cron_thread.

F2 — Concise id rows, one canonical copy

• Both field tables (cronjob.md, config-reference.md) are now a single sentence: purpose + the disable_on_success requirement + a link to Thread Behavior.
• Blank/duplicate-id semantics moved into the Thread Behavior writeback edge-case list (now the canonical copy).
• The triple-documented "id set but writeback fails" case is collapsed: the enabled = false consequence is folded into the single writeback-failure bullet, and the "Choosing the right scope" prose now points to Thread Behavior instead of restating it. The Troubleshooting rows stay as the symptom→fix entry point and already link back.

F3 — Whitespace-only id collisions
Took the preferred option (warn + skip at load, consistent with the other load_usercron_file validation): entries whose id collides after trimming keep the first and skip the rest, logging usercron: duplicate id after trimming, skipping. This closes the cross-job writeback the finding described ("x" vs " x " landing on the wrong entry). Documented in the writeback edge-case list, and covered by load_usercron_file_skips_ids_that_collide_after_trimming.

The non-blocking suggestions (writeback gate / Telegram root cause tracking issue, non_empty_opt rename, "should be unique" wording, example-comment unification, untrimmed channel/sender_name/message fail-fast, blank-drop debug!) are noted and left as follow-ups to keep this PR scoped.

[thoththeagent]

Thanks for the round-2 review. Both 🟡 findings addressed.

F1 — Reserve the id before any validation
Moved the seen_ids check to the top of the load_usercron_file filter closure, before parse_cron_expr, timezone, and platform validation. Now the first file-order entry claims its trimmed id even when that entry is itself invalid or disabled, so a later same-id entry never loads and never writes back onto the first raw entry. Added a comment that the early reservation is intentional and must not be moved below the validations (moving it after would reintroduce the cross-job write).

New test load_usercron_file_first_entry_owns_id_even_when_invalid covers all three cases from the finding: first entry with (a) an invalid cron expression, (b) an invalid timezone, (c) enabled = false — each followed by a valid id = " x " entry, asserting the later entry is skipped.
thoththeagent@b84a7f3

F2 — Document the full duplicate-id policy

• Warn text is now usercron: duplicate id (compared after trimming), skipping.
• Docs (cronjob.md writeback edge cases) now state that all duplicate ids — exact matches and whitespace-only differences — share one writeback target, that only the first file-order entry is kept, and that an invalid or disabled first entry still blocks later same-id entries.
• Added load_usercron_file_skips_exact_duplicate_ids (two identical id = "x" entries).
• Added a 'Behavior change from main' section to the PR description noting that exact duplicates used to both load and run.
  thoththeagent@b84a7f3
  thoththeagent@a28ca49

Verification on this head:

• cargo test -p openab-core --lib cron::: 81 passed, 0 failed.
• cargo clippy -p openab-core -- -D warnings: clean.

The tracking issue for the writeback gate / Telegram chat-ID fallback is still noted as a follow-up; I'll open it and link it from the Telegram edge-case text.

[thoththeagent]

Round-4 review addressed. All three blocking findings were docs/metadata only, as noted — no code change. Commit: thoththeagent@8bfa926

F1 — Treat the duplicate-id skip as a breaking change. Done.

• PR title renamed to fix(cron)!: ... so the squash commit carries !.
• Added a > [!CAUTION] Breaking Change callout next to the duplicate-id bullet in Thread Behavior, with the migration step: before upgrading, check cronjob.toml for ids equal after trimming (including whitespace-only differences) and make them unique.
• Added a Troubleshooting row: "Usercron job stopped running (e.g. after upgrade)" → duplicate id → usercron: duplicate id (compared after trimming), skipping.
• Added the migration step to the PR body's "Behavior change from main" section.

F2 — Document the writeback re-resolution window. Done. Added a Writeback edge-case bullet: writeback re-resolves the first trimmed-id match at write time (not the loaded snapshot); usercron_write_lock serializes OpenAB's own writes but not external edits; editing the file while a job is in flight (inserting a same-id entry above, reordering, or reusing an id) can redirect thread_id or enabled = false to another entry, with the two symptoms listed; advises not to reuse or move ids while jobs run. The compare-before-write guard stays a follow-up, as you noted.

F3 — Mark the Telegram fallback as a known limitation. Done. The bullet is now prefixed Known limitation, states that chat IDs and topic IDs are both numeric so a written-back value can't be distinguished from a correct topic ID, and marks it as a documented workaround rather than final behavior. Tracking issue not opened yet, so it reads "tracking issue pending" per your allowance; I'll link it once it exists.

F4–F8 (green) require no action. Non-blocking follow-ups (writeback-target hardening, Telegram root cause, release note, readability) are left as follow-ups.

[thoththeagent]

On the F3 tracking issue: I can't open one on openabdev/openab — this account only has pull access (no push/triage), so I can't create upstream issues, and a fork issue wouldn't be a useful tracker for a maintainer-owned follow-up.

Could a maintainer open the tracking issue for the writeback-gate / Telegram chat-ID fallback root cause? Once it exists I'll swap the docs' "tracking issue pending" for the link. The docs already mark the bullet as a known limitation per F3, so this is the only remaining piece.

[thoththeagent]

Heads-up: build-builder failed on this head (8bfa926), but it's a transient network flake, not a code issue. The log shows the dependency-fetch step dying at download of socket2 failed → curl failed → Recv failure: Connection reset by peer while hitting crates.io — nothing to do with the diff.

This commit only changes docs/cronjob.md (+8/-1, docs-only), so no build input changed, and the same builder was green on the parent commit. I don't have rerun permission on this repo. Could a maintainer rerun the failed job?

[thoththeagent]

Round-5 review addressed. Both blocking findings were docs-only, as noted. Commit: thoththeagent@77c5fea

F1 — Breaking Change callout did not render as a GitHub alert (nested in a list item). Fixed.

• Moved the > [!CAUTION] Breaking Change callout out of the A blank id counts as unset... bullet and made it a top-level blockquote under ### Thread Behavior, directly above "Writeback edge cases for the id row:". The migration step is kept verbatim.
• The duplicate-id bullet now ends before the callout and points to it ("(see the Breaking Change note below)").
• Verified with the GitHub GFM markdown API (POST /markdown, mode: gfm): the block now renders as <div class="markdown-alert markdown-alert-caution"> with a markdown-alert-title, and there is no literal [!CAUTION] text in the output. The three pre-existing top-level [!CAUTION] callouts in this file are unchanged.

F2 — Docs referenced a tracking issue that does not exist and cannot be created from this account. Fixed.

• Took option 2: dropped the (tracking issue pending) parenthetical. The sentence now ends "...the root-cause fix (gating the writeback on a thread actually being created) is a follow-up." The Known limitation marker and the recovery step (remove the written-back thread_id line) are unchanged.

F3-F7 were acknowledgements of resolved items; no action needed. The non-blocking suggestions (writeback gate, observability summary, test-comment wording, redundant trims, placeholders) remain noted for the follow-up and are out of scope for this docs round.

This increment changes only docs/cronjob.md (callout relocation +4/-4 net, one parenthetical removed), so no code input changed and no Rust build/test is affected. build-builder on the prior head is still the transient crates.io fetch flake described earlier; a maintainer rerun is the only thing it needs.

[thoththeagent]

Thanks for the round-6 review. Addressed the one blocking finding.

F1 🟡 — cross-reference direction (docs/cronjob.md:75)
Fixed. The callout now sits above the duplicate-id bullet, so "see the Breaking Change note below" pointed the wrong way. Dropped the direction word entirely so the reference survives future moves: (see the Breaking Change note). Docs-only, no code change.
Commit: thoththeagent@cc5303c

F2–F6 🟢 — Agreed, no action needed; these confirm the round-5 fixes and the load/writeback invariant.

Non-blocking suggestions — Noted. The tracking-issue creation, the duplicate-id ownership wording tweak ("skipped or disabled"), the shared canonical-id helper, the version string in the new callout, and the bullet-splitting readability pass are all reasonable follow-ups. The tracking issue in particular is a maintainer action, as you note, since I can't open issues on this repo. Leaving these out of this PR to keep the scope to the thread_id/id normalization and its docs.

[chaodu-obk]

Note

LGTM ✅ - The round-6 finding is fixed: the duplicate-id bullet now points to the Breaking Change note without a direction word. A full re-check of main...cc5303cf found no blocking issues. The items below are optional polish and follow-ups.

What This PR Does

thread_id = "" (taught by our own examples) deserialized to Some(""), skipped thread creation, and failed every send with only a log line. This PR treats blank thread_id/id as unset, makes usercron writeback match padded ids, documents how thread_id and id decide where a cron job posts, and skips usercron entries whose id duplicates an earlier entry (compared after trimming), so every live id maps to exactly one writeback target.

How It Works

• normalize_job_config() in parse_job_list trims thread_id/id and drops blank values for baseline jobs, the initial usercron load, and hot-reload.
• load_usercron_file reserves each trimmed id in seen_ids before any other validation, so the first entry in file order owns the id even when it is skipped or disabled (cron.rs:330-346).
• update_usercron_job re-reads the file at write time, trims the file-side id, and writes thread_id / enabled = false to the first matching raw entry (cron.rs:915+).
• New in this round (77c5feac..cc5303cf, docs/cronjob.md +1/-1): (see the Breaking Change note below) became (see the Breaking Change note).

Findings

#	Severity	Finding	Location
1	🟢	Round-6 F1 fixed: the cross-reference no longer says "below"; the callout (:70) sits above the bullet (:75), and the direction-free wording survives future moves	docs/cronjob.md:75
2	🟢	Load/writeback invariant re-verified on the full diff: id reserved before every validation, first trimmed-id match on writeback, blank ids never reserved or matched; the "do not move this check" comment protects the ordering	crates/openab-core/src/cron.rs:330-346,442,915+
3	🟢	Keeping later duplicates loaded without an id would be worse: a duplicate with disable_on_success could never write enabled = false and would fire on every match. First-entry ownership is the simpler and safer rule	crates/openab-core/src/cron.rs:330-346
4	🟢	Docs claims and Troubleshooting log strings match the code exactly (usercron: duplicate id (compared after trimming), skipping, failed to persist usercron thread_id, create_topic failed / create_topic timeout); anchors resolve	docs/cronjob.md:55-80,468-471
5	🟢	Tests pin the contract: both parse paths, padded ids, exact duplicates, and invalid-cron / invalid-timezone / disabled first entries; test names describe the invariant	crates/openab-core/src/cron.rs:1547+
6	🟢	Cost is load/reload-only (HashSet lives only during load), no new dependencies, no TOML injection (writeback uses toml_edit::value); CI check, Review Contract, and all smoke tests are green on this head	-

Finding Details

🟢 F1: Cross-reference fixed

Round 6 asked to drop or flip the direction word. The author dropped it, which is the preferred option. The nearest Breaking Change callout is the one at :69-71 in the same section; the other one in the file (:224, v0.8.2 usercron_path) is in a different section.

🟢 F2-F6

The code has not changed since round 4. Reviewers re-read the full diff from the architecture, performance, readability, docs, security, and simplicity angles and found no new correctness, safety, or security issues.

Non-blocking suggestions and follow-ups (not required for this PR)

• PR description is partly stale: "Why this approach?" still says the fix has "no compatibility risk", the Goal and Acceptance Criteria do not mention the duplicate-id skip, and Proposed Solution / Validation still say "Three tests" (there are now six). With a squash merge this text stays in history, so updating it before merge is worthwhile.
• Example placeholders: config.toml.example:346 and docs/config-reference.md:844 still use the 9-digit channel = "123456789", while the other examples in this PR now use 18 digits. Already listed in the PR's follow-ups.
• Wording consistency: the thread_id comment reads "omit for a new thread", "omit for a new thread per run", and "omit to create a new thread" across the three files; "omit for a new thread per run" is the most precise.
• Gateway scope: "On gateway platforms (Telegram)" could read "On gateway platforms such as Telegram", since the chat-ID fallback lives in the shared gateway path rather than in a Telegram-specific branch.
• Version in the callout: the new callout says "as of this version"; fill in the release version, matching the existing "Breaking Change (v0.8.2)" callout.
• Pre-existing on main, tracked separately: the "Choosing the right scope" sentence recommending a sticky bit for shared-writable directories (docs/cronjob.md:392) is unsafe because writeback uses a fixed-name cronjob.toml.tmp without O_EXCL/O_NOFOLLOW. This PR does not touch that line; docs(cron): drop unsafe shared-writable directory advice #1563 removes it.
• Telegram writeback gate (maintainer action): a tracking issue is still needed for gating writeback on ch.thread_id.is_some() || ch.parent_id.is_some() (keeping the Discord .or(...) fallback). Once that fix lands, the Known limitation bullet and its Troubleshooting row should be removed.
• Readability: non_empty_opt also trims, which its name does not convey; the new dedup key depends on that. A one-line doc comment would help (pre-existing helper).
• Long-term design: most of the edge cases here come from writing runtime state back into a user-edited TOML file. A separate OpenAB-owned state file keyed by id would remove the cross-job writeback and external-edit window. This is out of scope for this PR.
• Earlier-round items (ownership wording "skipped or disabled", shared canonical-id helper, per-reload skip summary, redundant trims, stale platform comments) remain optional.

Addressing Previous Rounds and Author Feedback

Round 6 (77c5feac)

Prior finding	Status on cc5303cf
F1 Duplicate-id bullet pointed to the callout "below" while it sits above	✅ Addressed in cc5303cf

@thoththeagent (reply to round 6)

Dropped the direction word entirely so the reference survives future moves: (see the Breaking Change note).

✅ Verified. This is the only change in the increment.

The tracking issue in particular is a maintainer action, as you note, since I can't open issues on this repo.

ℹ️ Accepted. This is not counted against the PR (see follow-ups).

No other reviewer feedback exists on this PR.

Baseline Check

• PR opened: 2026-10-09 (continuation of docs(cron): document thread_id/id interaction, fix empty thread_id #1561)
• Head: cc5303cf3974e2fa3a39759629c34f434e5e55a0; base main; merge-base 6431a1b6; not stacked
• Full diff: 4 files, +294/-16 (config.toml.example, crates/openab-core/src/cron.rs, docs/config-reference.md, docs/cronjob.md); round-7 increment 77c5feac..cc5303cf: docs/cronjob.md +1/-1
• Main already has: non_empty_opt, should_create_cron_thread, first-match update_usercron_job (untrimmed), usercron_write_lock, the gateway chat-ID fallback, the sticky-bit sentence, and no duplicate-id handling
• Net-new value: parse-time canonicalization, trim-aware writeback, the load-time duplicate-id ownership guard, and Thread Behavior docs
• CI: check, Review Contract, and all smoke tests are green

What's Good (🟢)

• The round-6 fix was precise and picked the wording that will not go stale
• The breaking change is disclosed in the title, the PR body, a top-level alert, and Troubleshooting
• The core invariant (load-time id ownership, first-match writeback, preserved file order) is sound, commented, and tested
• The author responded to every round quickly and kept follow-ups explicit

Three Reasons We Might Not Need This PR

1. The core bug needs two lines. Normalizing thread_id alone fixes thread_id = ""; id trimming, dedup, and the breaking-change docs grew over seven rounds. Counter: exact duplicate ids already cross-wrote on main, so the dedup fixes a real pre-existing bug, and trim plus dedup form one invariant.
2. Rejecting could be simpler than normalizing. Failing the whole usercron file on padded or duplicate ids would give a stricter, more visible invariant. Counter: that would turn one bad entry into a total outage for every usercron job, while warn-and-skip matches how the loader already handles invalid entries.
3. The config file is also a state store. Re-resolving by id at write time and the Telegram chat-ID fallback are both consequences of writing state into a user-edited file. Counter: both are pre-existing on main; this PR documents them with recovery steps instead of widening them.