docs(adr): goal-driven cronjob (disable_on_success)#816
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening report## IntentPR #816 proposes an ADR for extending The operator-visible problem: recurring cron prompts keep firing even after the task’s goal is complete. This creates noise, duplicated work, and stale scheduled agent activity. The ADR aims to support “goal-driven” scheduled runs without introducing a full goal-runner system yet. FeatThis is a docs / architecture decision PR. Behavior described by the ADR:
Who It ServesPrimary beneficiaries:
Secondary beneficiary:
Rewritten PromptImplement the ADR for goal-driven usercron jobs by adding Before each scheduled run, if Do not introduce a separate goal-runner system or model-based judging in this phase. Add tests for success match, failed command, missing sentinel, notification behavior, and persisted disablement. Merge PitchThis is worth advancing because it documents a small, practical step toward goal-driven scheduled agents without committing OpenAB to a larger orchestration system too early. Risk profile: moderate for future implementation, low for this ADR. The main reviewer concern will be whether mutating Best-Practice ComparisonRelevant OpenClaw principles:
Relevant Hermes Agent principles:
Not relevant yet:
Those belong to the ADR’s stated Phase 2. Implementation OptionsOption 1: Conservative ADR-only merge Merge the ADR as design documentation, but require a follow-up implementation issue before code changes. Add reviewer notes about locking, atomic writes, exact sentinel parsing, and notification routing. Option 2: Balanced usercron implementation Implement Option 3: Ambitious goal-runner foundation Introduce a goal-runner abstraction behind usercron with structured state, run logs, disable conditions, retry metadata, and future hooks for stuck detection or LLM judging. Expose Comparison Table
RecommendationAdvance the ADR, but steer follow-up toward Option 2: balanced usercron implementation. It matches the PR’s stated philosophy: extend the existing scheduler, keep the success condition explicit, and avoid building a full goal-runner too early. For merge discussion, the important acceptance bar should be: file locking, atomic TOML writes, exact sentinel matching, stable delivery routing, and tests around non-match cases. Split Phase 2 explicitly into a later design item: run logs, stuck detection, escalation, state deltas, and LLM judging. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
I would hold this ADR until #818 is adjusted. Two contract details currently need alignment: the timeout mitigation depends on actually killing/reaping the command on timeout, and the ADR says missing id with disable_on_success is a startup error while the implementation currently warns and skips invalid usercron entries. Once those semantics are settled, the ADR should match the code exactly.
chaodu-agent
left a comment
There was a problem hiding this comment.
Group Review — 超渡法師 (Kiro)
✅ LGTM — Ready to merge
All three findings from 普渡法師's review have been addressed in commit 687bfec:
| Finding | Status | Verification |
|---|---|---|
| F1: Validation timing | ✅ Fixed | Step 1 now validates at load time (startup error); step 2 no longer duplicates validation |
| F2: Flow diagram orphan | ✅ Fixed | Diagram rewritten with clean dual-branch structure |
| F3: Match semantics | ✅ Fixed | Explicitly defined as case-sensitive substring match with unique marker guidance |
Design direction is sound — minimal extension of existing cron infra with clear Phase 1/Phase 2 boundary. ADR is ready to merge.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
LGTM ✅ — Well-structured ADR that documents the design decisions behind PR #818. Clear Phase 1 vs Phase 2 separation, correct constraints, ready to merge. 四問框架 Review1. What problem does it solve?Documents the architectural decision for extending usercron with goal-driven auto-disable. Provides rationale for why this approach was chosen over a new 2. How does it solve it?
3. What was considered?
4. Is this the best approach?Yes. The ADR is well-aligned with the implementation in PR #818. The explicit Traffic Light🟢 INFO — Good decision to require both exit 0 AND explicit marker match. This prevents false positives from commands that happen to succeed for unrelated reasons. 🟢 INFO — The "Smallest Useful Increment" principle is well-applied here. Phase 1 reuses 100% of existing scheduler infrastructure. 🟢 INFO — Security section correctly identifies that command injection is not a concern since config is static TOML, not runtime user input. 🟢 INFO — Open questions (multi-agent coordination, context overflow, observability) are honest about what Phase 1 doesn't solve — good for future contributors. |
|
Closing — superseded by #818 which includes the implementation and user-facing docs. The ADR content is captured in docs/cronjob.md. |
…818) * feat(cron): auto-disable usercron jobs on success * fix(cron): atomic write, kill child on timeout, add timeout test - update_usercron_job: write to .toml.tmp then rename (atomic on POSIX) - check_disable_on_success: use spawn() + wait_with_output() to retain child handle; explicitly kill on timeout to prevent orphan processes - Add disable_on_success_kills_child_on_timeout test (sleep 999 + 1s timeout) * fix(cron): remove unused timeout import * fix(cron): serialize usercron writebacks * docs: add re-enable instructions for goal-driven cronjobs --------- Co-authored-by: chaodufashi <[email protected]> Co-authored-by: 超渡法師 <[email protected]>
|
Closing — superseded by #818 (merged May 21) which includes both the implementation and user-facing docs. The ADR content is captured in docs/cronjob.md. |
Summary
ADR for extending usercron
[[jobs]]withdisable_on_success— enabling goal-driven "escape room" mode.Key Decisions
disable_on_success— command evaluates the goal before sending the regular promptdisable_on_success_matchbefore OpenAB treats the goal as achievedenabled = falsedirectly to$HOME/.openab/cronjob.toml, no separate state file✅ Goal achievedbefore disablingPhase 1 vs Phase 2
Context
Design discussion: https://discord.com/channels/1491295327620169908/1504239931940409587
cc @pahud