feat(cron): per-job max_turns override + record budget stops as PARTIAL - #45322
feat(cron): per-job max_turns override + record budget stops as PARTIAL#45322colingreig wants to merge 1 commit into
Conversation
run_job now resolves the agent iteration budget in priority order: job.max_turns (a positive int on the job record) -> agent.max_turns config -> max_turns config -> 90. This lets long-running worker jobs carry a larger turn budget without inflating the global default for every cron job. The field is settable through the normal API: create_job() gains a max_turns parameter and the cronjob tool exposes it on create/update (bool is excluded explicitly since it is an int subclass, so `max_turns: true` falls back to the default rather than silently capping at one turn). Previously, any result with completed is False was wrapped in RuntimeError, so a job that hit its turn budget but produced a multi-KB handoff report was stored as last_status=failed with the whole report buried in last_error -- indistinguishable from a turn-1 crash. This is the inverse of the over-reporting fixed in NousResearch#17855. Now, when turn_exit_reason starts with max_iterations_reached or budget_exhausted, failed is not True, and the final response is >= 300 chars (to reject the short boilerplate finalize_turn injects when the post-budget summary call itself fails), the result is recorded as partial success: a "PARTIAL -- iteration budget hit" banner is prepended and it flows through the normal success path. Genuine failures and empty/short responses still raise as before. Adds tests/cron/test_run_job_partial_budget.py (partial path, the >=300-char floor, genuine-failure passthrough, max_turns override incl. bool/zero/negative/string guards) plus create_job and cronjob tool coverage in tests/cron/test_jobs.py and tests/tools/test_cronjob_tools.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean, well-scoped fix/feature with comprehensive tests. No issues found.
- Logic is correct and focused
- Tests cover the new behavior
- No security concerns
- Good error handling
Reviewed by Hermes Agent
|
Verified the per-job
Clean implementation. |
|
Great fix for the inverse of #17855. We hit this exact mis-reporting in production and went one step further than this PR — sharing in case it informs a follow-up. In our deployment, a 23:30 daily-summary LLM cron ( That's why we'd argue for the "reasonable follow-up" the PR notes as out of scope: a distinct
Our local implementation (4 files, applied + verified live):
This keeps your Happy to open a follow-up PR or extract our |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused per-job budget work. The per-job max_turns capability is still absent on current main (cron/scheduler.py:2899), so that portion remains useful.
Problems
- The new PARTIAL branch changes only response text.
run_one_job()will still callmark_job_run(..., success=True)(cron/scheduler.py:3452,:3531-3533), andmark_job_run()persists every success aslast_status="ok"(cron/jobs.py:1485). The immediate-run tool also treats only"ok"as successful (tools/cronjob_tools.py:643). A partial run would therefore remain falsely healthy to status consumers, as the production report in this discussion describes. - Current main already delivers non-empty
max_iterations_reached(...)fallback reports (cron/scheduler.py:3226-3246; regression testtests/cron/test_scheduler.py:1489-1534, added byae7e85742). Please reconcile with that implementation rather than replacing the older branch wholesale.
Suggested changes
- Preserve the per-job override, but thread an explicit partial outcome through
run_job→run_one_job→mark_job_run, then update status consumers and add a real-path temporary-HERMES_HOMEregression test.
Automated hermes-sweeper review.
| job_name, _exit_reason, | ||
| ) | ||
| result["final_response"] = ( | ||
| f"⚠️ PARTIAL — iteration budget hit ({_exit_reason}). " |
There was a problem hiding this comment.
This only annotates response text. run_job() still returns success=True, so mark_job_run() persists last_status="ok" and the immediate-run tool reports the run healthy. Please thread an explicit partial outcome to storage and consumers instead of using the banner as the sole signal.
What does this PR do?
Two related changes to
run_jobincron/scheduler.py, plus the plumbing to make the new field settable through the normal API.1. Per-job
max_turnsoverride (feature)max_iterationsnow resolves the turn budget in priority order:job.max_turns(a positive int field on the job record) →agent.max_turnsconfig →max_turnsconfig → 90. Long-running worker jobs need a larger turn budget than the default without raising the global limit for every other cron job. The field is settable through the normal API —create_job()gains amax_turnsparameter and thecronjobtool exposes it on create/update. Becauseboolis anintsubclass, it's excluded explicitly somax_turns: truefalls back to the default rather than silently capping a run at one turn.2. Budget stops recorded as partial, not hard failure (bug fix)
Previously, any result with
completed is Falsewas wrapped inRuntimeError, so a job that hit its iteration/turn budget but still produced a substantive multi-KB handoff report was stored aslast_status=failedwith the entire report buried inlast_error— indistinguishable from a turn-1 crash. This is the inverse over-correction of the mis-reporting fixed in #17855.Now, when
turn_exit_reasonstarts withmax_iterations_reachedorbudget_exhausted,failed is not True, and the final response is ≥ 300 chars (to reject the short boilerplatefinalize_turninjects when the post-budget summary call itself fails), the job is recorded as a success with a⚠️ PARTIAL — iteration budget hitbanner prepended to the report, which then flows through the normal success path. Genuine failures and empty/short results still raise as before.Related Issue
Related: #17855 (follow-on). That fix stopped API failures being mis-reported as
last_status=ok; this handles the inverse, where legitimate partial work was mis-reported as total failure. #17855 is closed, so this is not aFixes. No open issue currently tracks this.Type of Change
Changes Made
cron/scheduler.py—run_job: resolvemax_iterationsfrom a positive non-booljob.max_turnsbefore config/default; routemax_iterations_reached/budget_exhaustedstops with a ≥ 300-char report to partial success instead ofRuntimeError.cron/jobs.py—create_job(): optionalmax_turnsparameter, stored on the record only when a positive non-bool int (omitted otherwise, so existing records are unchanged).tools/cronjob_tools.py—cronjobtool:max_turnsparameter threaded to create/update + amax_turnsproperty in the tool schema.tests/cron/test_run_job_partial_budget.py— new: partial-success path, the ≥ 300-char floor, genuine-failure passthrough, and themax_turnsoverride (incl. bool/zero/negative/string guards).tests/cron/test_jobs.py—create_jobmax_turnspersistence/validation.tests/tools/test_cronjob_tools.py— tool create/updatemax_turnspassthrough.How to Test
pytest tests/cron/test_run_job_partial_budget.py tests/cron/test_jobs.py tests/tools/test_cronjob_tools.py -qpytest tests/cron/ -qmax_turns(e.g.3) and a task needing more turns. The run is stored aslast_status=okwith the report readable (prefixed⚠️ PARTIAL — iteration budget hit) instead oflast_status=failedwith the report buried inlast_error.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the relevant suites instead:tests/cron/+tests/tools/test_cronjob_tools.py= 527 passed. Did not run the fulltests/locally (unrelated collection errors from optional deps not installed in my env, e.g.mcp); CI covers the full suite.Documentation & Housekeeping
create_job+ tool schema description) — or N/Acli-config.yaml.exampleif I added/changed config keys — N/A (max_turnsis a per-job record field, not a top-level config key)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A