fix(cron): summarize no-agent script failures accurately - #60593
fix(cron): summarize no-agent script failures accurately#60593cpahgw-rgb wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for fixing the no-agent classification path. The underlying issue is present on current main: run_one_job() routes failed jobs through _summarize_cron_failure_for_delivery() (cron/scheduler.py:3448), whose generic timeout branch reports a provider fallback failure (cron/scheduler.py:74-79), while no-agent subprocess timeouts originate as Script timed out after ... (cron/scheduler.py:2131-2132).
Problems
- The new broad
timeoutmatch atcron/scheduler.py:75also classifies ordinary nonzero script failures as scheduler timeouts when their stderr contains that word._run_job_script()includes arbitrary stderr in nonzero-exit errors (cron/scheduler.py:2121-2127), so this can produce an inaccurate watchdog timeout alert.
Suggested changes
- Narrow the new timeout case to the exact
TimeoutExpiredmessage emitted atcron/scheduler.py:2131-2132; keep other no-agent failures as generic script failures. - Add a regression case for a nonzero script error containing
timeoutthat must not receive the timeout-specific wording.
Automated hermes-sweeper review.
| if len(cleaned) > 180: | ||
| cleaned = cleaned[:177].rstrip() + "..." | ||
| if "script timed out" in lower or "timed out" in lower or "timeout" in lower: | ||
| return ( |
There was a problem hiding this comment.
This broad match also catches a nonzero script exit whose stderr merely says request timeout: _run_job_script() preserves arbitrary stderr in its error at current cron/scheduler.py:2121-2127. Please restrict this to the exact Script timed out after <seconds>s: message emitted for TimeoutExpired, so those failures retain the generic watchdog-failure summary.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the same root cause: no_agent cron failures bypass providers but are currently summarized by provider-oriented heuristics. No Verify verdict is recorded for either PR; the diffs differ in that #60593 adds timeout-specific wording via broad substring matching, while #63178 consistently classifies every no_agent failure as a script failure.
Related pull requests
- #60593
related— (+36/-0) — needs revision: The diff correctly movesno_agenthandling ahead of provider/API classification, but its broad"timed out"/"timeout"matching can misclassify an ordinary nonzero script error whose stderr merely contains that text. Despite the keep_open review on #60593, this should not be merged as-is: the contributor review explicitly requires matching the exactScript timed out after ...message and adding a regression test for nonzero stderr containingtimeout. - #63178 [closed]
duplicate— (+31/-0) — focused alternative, closed as superseded by #64474: The diff handlesno_agentfirst and always reports a source-accurate script failure, with parameterized coverage for timeout-, rate-limit-, and authentication-like script output, thereby avoiding the false timeout classification present in #60593. It remains relevant as the narrower reference implementation, although the supersession claim comes from a non-contributor discussion note and no Verify verdict is recorded.
Duplicates
#60593 and #63178 address the same no-agent failure-classification bug, but they are not exact duplicates: #60593 attempts a distinct timeout-specific message with an overbroad match, whereas #63178 uses uniform script-failure wording and broader false-provider regression coverage.
Suggested consolidation
Do not merge #60593 as-is. Consolidate on the focused #63178 behavior in its reported successor #64474, or revise #60593 to satisfy its keep_open contributor review by recognizing only the exact subprocess-timeout message and testing nonzero stderr containing timeout; once that requirement is covered on the active PR, #60593 can be closed as the overlapping implementation, while #63178 remains closed as superseded.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup60593 ["PRs duplicating each other"]
P60593["PR #60593 (open)"]
P63178["PR #63178 (closed)"]
end
class P60593 open
class P63178 closed
class P60593 target
click P60593 "https://github.com/NousResearch/hermes-agent/pull/60593"
click P63178 "https://github.com/NousResearch/hermes-agent/pull/63178"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 1 kB of issue/PR text, 1 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Closing with credit — and a credit correction. This bug (no_agent script failures mislabeled as provider/fallback failures) was fixed on main via PR #85536, which cherry-picked #77648. Reviewing after the merge, we found your PR was the EARLIEST submission of this fix — Jul 8, more than three weeks ahead of the PR that got first-submitter credit. Our pre-merge duplicate sweep missed it because of phrasing differences; that's on us, and we're sorry the credit note in #85536 named the wrong first submitter. Your diagnosis and approach were correct. Thanks! |
Summary
no_agentcron failures as script/watchdog failures before provider/API classification.Testing
HERMES_HOME=$(mktemp -d) uv run --frozen pytest tests/cron/test_cron_no_agent.py -q