Skip to content

fix(cron): skip provider-error classification for no_agent job failures - #70977

Closed
brian717 wants to merge 1 commit into
NousResearch:mainfrom
brian717:fix/cron-no-agent-provider-auth-false-match-70908
Closed

fix(cron): skip provider-error classification for no_agent job failures#70977
brian717 wants to merge 1 commit into
NousResearch:mainfrom
brian717:fix/cron-no-agent-provider-auth-false-match-70908

Conversation

@brian717

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a misleading cron notification. no_agent=True jobs run a bash script with no LLM or provider involved. When such a script exits non-zero, its entire stdout is handed to _summarize_cron_failure_for_delivery as the error string. The provider classification branches in that function match bare tokens (401, 403, 429, "timeout") anywhere in the text, so ordinary script output that just happens to mention a status code, for example a passing auth-gate test printing "returns 401", got reported to the user as a "provider authentication error" even though no provider was ever called.

The fix gates all three provider classification branches (rate limit, timeout, authentication) behind not job.get("no_agent"). Script-only jobs now fall through to the generic failure summary, which reflects what actually broke. Agent jobs are untouched, so real provider errors are still classified exactly as before.

I widened the guard to cover 429 and timeout in addition to the 401/403 case from the issue because they share the same root cause: bare tokens in script stdout being read as provider diagnostics. This matches the issue's preferred option A ("skip the provider-error classification for no_agent jobs entirely").

Related Issue

Fixes #70908

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/scheduler.py: wrap the rate-limit, timeout, and authentication classification branches in _summarize_cron_failure_for_delivery inside if not job.get("no_agent"):. Pure re-indentation of the existing branches plus a comment explaining why; agent-job output is unchanged.
  • tests/cron/test_summarize_cron_failure.py: new test module. Four cases assert a no_agent job whose stdout mentions 401/403/429/timeout is not classified as a provider error, and three regression cases assert agent jobs (including a legacy job dict with no no_agent key) still classify 401/429 as before.

How to Test

  1. Reproduce on main: build a job dict with no_agent=True and pass stdout containing a bare 401 to _summarize_cron_failure_for_delivery. It returns "provider authentication error" even though no provider is involved.

  2. With this change, the same input falls through to the generic summary and no longer mentions a provider.

  3. Run the suite:

    pytest tests/cron/test_summarize_cron_failure.py tests/cron/test_run_one_job.py tests/cron/test_cron_no_agent.py tests/cron/test_shutdown_interrupt.py -q
    

    51 passed. The four no_agent tests fail on main and pass with the fix.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected cron tests and they pass
  • I've added tests for my changes
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • No documentation changes needed (behavior fix, no config keys or tool schemas touched) - N/A
  • No config keys added or changed - N/A
  • No architecture or workflow changes - N/A
  • Cross-platform impact considered: this is provider-agnostic string classification with no OS-specific behavior
  • No tool descriptions or schemas changed - N/A

no_agent cron jobs run a bash script with no LLM/provider involved. When
such a script exits non-zero, its entire stdout is passed to
_summarize_cron_failure_for_delivery as the error string. The provider
classification branches matched bare 401/403/429/"timeout" tokens anywhere
in that text, so ordinary script output that merely mentioned a status code
(e.g. a passing auth-gate test printing "returns 401") produced a misleading
"provider authentication error" notification for a failure unrelated to any
provider.

Gate all three provider-classification branches (rate limit, timeout, auth)
behind `not job.get("no_agent")` so script-only jobs fall through to the
generic summary. Agent jobs are unaffected.

Fixes NousResearch#70908
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Jul 24, 2026

yinkev commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Validated this earlier implementation against current main rather than keeping the competing #71494 open. The patch applies cleanly, and the complete focused set from the PR (test_summarize_cron_failure, test_run_one_job, test_cron_no_agent, test_shutdown_interrupt) passes 51/51 with Ruff and diff hygiene clean. Its production change fully subsumes #71494 and its regression coverage is broader, so I am closing #71494 in favor of this PR.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. Current main still has the reported mismatch: cron/scheduler.py:105130 applies provider classification to every failure string, while cron/scheduler.py:27762841 makes no_agent a script-only path and returns failed-script output as the error. Failed runs then enter this helper at cron/scheduler.py:3989.

The proposed guard matches that execution boundary, preserves classification for agent jobs and legacy job dictionaries, and the added tests cover the relevant token classes.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
@jeff-mettel

Copy link
Copy Markdown
Contributor

#74112 implemented the same production change — the no_agent-gated branches of _summarize_cron_failure_for_delivery in cron/scheduler.py — and was closed as a duplicate of this PR on 2026-07-30. Posting the cross-reference here so the closed PR is discoverable from this thread.

Diffing the two test suites (10 cases there vs. 7 here), three cases covered there have no equivalent on this branch:

Case (name on #74112) What it locks
test_agent_job_with_prerun_script_keeps_provider_wording an agent job that also has a script still classifies a genuine provider timeout as a provider error — catches any future "simplification" of the gate from no_agent to job.get("script")
test_script_job_without_script_field_still_avoids_provider_wording a no_agent job with no script field still avoids provider framing
test_agent_job_weekly_usage_limit_message_unchanged weekly-usage-limit classification on the agent path, not covered by the 429 case

They can be cherry-picked from jeff-mettel:fix/cron-script-failure-message (#74112), or filed as a follow-up PR against this branch or against main once this merges — whichever the author and maintainers prefer. This supersedes the follow-up question left open on #74112.


Filed by an AI agent (Claude Fable 5) operating autonomously on @jeff-mettel's behalf. The test-suite delta was verified by diffing both PRs (gh pr diff) before posting.

@ThoriumLabsDev

Copy link
Copy Markdown

Independent validation against current origin/main at 89c14ae (2026-08-08): the PR applies cleanly; 30 focused cron tests passed across summarize/run/no_agent/shutdown coverage; ruff check passed on the changed files; and git diff --check passed. This also matches a reproduced no_agent upstream-API failure that was being mislabeled as a provider/fallback failure. I intentionally did not open a duplicate PR.

@alt-glitch alt-glitch added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P2 Medium — degraded but workaround exists and removed P3 Low — cosmetic, nice to have labels Aug 8, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing with credit — the same no_agent mode gate landed on main via PR #85536 (cherry-picked from #77648, which reimplemented the identical approach later). Your Jul 24 submission predates it; together with #60593 (Jul 8, the earliest) this fix was independently submitted five times, which tells us the bug was well and truly real. Sorry the first-submitter credit in the merge notes went to a later PR — our sweep missed the earlier cluster. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cron failure summarizer false-matches 401/403 in no_agent script stdout → misleading 'provider authentication error' notification

6 participants