fix(cron): don't attribute no_agent script failures to a provider - #77648
fix(cron): don't attribute no_agent script failures to a provider#77648Yanir-R wants to merge 1 commit into
Conversation
`_summarize_cron_failure_for_delivery` classifies a failed job by
substring-matching the error prose — "timed out", "429",
`authenticat|authoriz` — and maps any hit onto a provider-shaped
explanation, without consulting the job's execution mode.
A `no_agent` job IS its script: `run_job` short-circuits it before any
model is reached. Provider timeouts, rate limits, auth errors and
fallback chains are therefore structurally impossible for it, yet those
branches are tested first.
`_run_job_script` reports a timeout as "Script timed out after {n}s:
{path}". That contains "timed out", so a shell script exceeding its
timeout is delivered to chat as:
⚠️ Cron 'x' failed: provider timeout. Fallback chain was exhausted
or unavailable.
for a job that never opened a socket, sending the reader to inspect
model routing while the actual fault is a shell script. "429" or
"authentication" appearing anywhere in a script's output misfires the
same way.
Gate the three provider branches on `not job.get("no_agent")` and let
script jobs fall through to the existing generic cleaner, which already
reports the real error and names the script. No new message text.
The auth branch carries a word-boundary guard so "oauth" and "4015" do
not trip it, which addresses one substring false-positive; gating on
mode removes the remaining class for script jobs.
Tests: the summarizer had no direct coverage — the only test referencing
it patches it out and asserts on its arguments. Adds parametrized cases
pinning both directions: script jobs are never blamed on a provider
(including when their output contains "429" or "authentication"), and
agent-mode jobs keep the existing provider summaries unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Duplicate of #70977 — same fix at the same site: gate the provider-classification branches in |
|
Code Review: #77648 Verdict: Approve File symlink normalization security: adds path resolution checks. Good hardening. LGTM - Reviewed diff. Changes are sound. |
…composed no_agent gate The cherry-picked tests predate #85508's honest fallback-chain phrasing and each other: assertions pinned the old 'exhausted or unavailable' literal and #83188's no_agent fallback-note behavior, which #77648's mode gate supersedes (no provider classification at all for no_agent jobs). Assert the composed contract instead.
|
Merged via PR #85536 (rebase-merge) — your commit was cherry-picked onto current main with your authorship preserved in git log. Yours was the earliest of three independent fixes for this bug (#79451, #81629 followed), and the mode-gate approach with the strongest test coverage — the parametrized "script prose must never pick the blamed subsystem" cases landed as you wrote them. Thanks! |
…composed no_agent gate The cherry-picked tests predate NousResearch#85508's honest fallback-chain phrasing and each other: assertions pinned the old 'exhausted or unavailable' literal and NousResearch#83188's no_agent fallback-note behavior, which NousResearch#77648's mode gate supersedes (no provider classification at all for no_agent jobs). Assert the composed contract instead.
…composed no_agent gate The cherry-picked tests predate NousResearch#85508's honest fallback-chain phrasing and each other: assertions pinned the old 'exhausted or unavailable' literal and NousResearch#83188's no_agent fallback-note behavior, which NousResearch#77648's mode gate supersedes (no provider classification at all for no_agent jobs). Assert the composed contract instead.
Problem
_summarize_cron_failure_for_deliveryincron/scheduler.pyclassifies a failed cron job by substring-matching the error prose —"timed out","429",authenticat|authoriz— and maps any hit onto a provider-shaped explanation. It never consults the job's execution mode, which is present in the samejobdict it is passed.A
no_agentjob is its script:run_jobshort-circuits it before any model is reached ("no LLM involvement"). Provider timeouts, rate limits, auth errors and fallback chains are therefore structurally impossible for such a job — yet those branches are tested first.Reproduction
_run_job_scriptreports a timeout asf"Script timed out after {script_timeout}s: {path}". That string contains"timed out", so a shell script exceeding its timeout is delivered to chat as:for a job that never opened a socket. The operator is sent to inspect model routing and fallback configuration while the actual fault is a shell script.
"429"or"authentication"appearing anywhere in a script's output misfires the same way.Fix
Gate the three provider branches on
not job.get("no_agent")and let script jobs fall through to the existing generic cleaner, which already reports the real error and names the script. No new message text — the correct output already existed on the fall-through path:Agent-mode jobs are unaffected.
The auth branch already carries a word-boundary guard so
"oauth"and"4015"do not trip it, which addresses one substring false-positive; gating on mode removes the remaining class for script jobs.Tests
The summarizer had no direct coverage — the only test referencing it (
tests/cron/test_shutdown_interrupt.py) patches it out and asserts on its call arguments, so the classification logic itself was never exercised.Adds parametrized cases to
tests/cron/test_cron_no_agent.pypinning both directions:"429","authentication"or"ReadTimeout"provider timeout/provider rate limit/provider authentication errorsummaries unchangedNotes
No change to what data reaches the delivery channel beyond routing these errors to the existing generic path: script stdout/stderr already passes through
redact_sensitive_text, and the timeout error is a fixed string plus the script path.