Skip to content

fix(cron): propagate pre-run script failure to job status - #20323

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-20301-cron-script-failure-status
Open

fix(cron): propagate pre-run script failure to job status#20323
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-20301-cron-script-failure-status

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 5, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

When a cron job's pre-run script exits non-zero, the error is injected into the agent prompt — but run_job() still reports success (status=ok). Operators see ok and assume the job ran correctly, when the data-collection step actually failed.

Root Cause

In run_job(), the pre-run script result (_ran_ok) is only used to build the prompt content. The function always returns True at the end, regardless of whether the script succeeded or failed.

Related Issue

N/A

Type of Change

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

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels May 5, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracking the cron status mismatch. The underlying bug is still present on current main: cron/scheduler.py:2632 receives a failed script result, while cron/scheduler.py:3244-3245 still returns success after an otherwise successful agent response.

Problems

  • The proposed return not _script_failed, output, final_response, None drops the failure detail. _process_job() builds failed-job delivery from error at cron/scheduler.py:3448, and mark_job_run() persists it as last_error at cron/jobs.py:1392-1394; this would yield an unknown-error delivery and no stored script failure.
  • The branch also includes unrelated changes. 6ef568c adds WHATSAPP_NPM_INSTALL_TIMEOUT as user-facing behavioral config, while AGENTS.md:102-107 requires config.yaml. 5dc3423 globally requires mutually exclusive patch-mode fields; current main intentionally keeps only mode globally required (tools/file_tools.py:2092, tests/tools/test_file_tools.py:607-613).

Suggested changes

  • Keep the focused cron fix, return a non-empty error derived from the script failure, and assert that error in the regression test.
  • Split/drop the unrelated WhatsApp and PATCH_SCHEMA commits.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) labels Jul 12, 2026
liuhao1024 and others added 2 commits July 13, 2026 07:28
When a cron job's pre-run script exits non-zero, the error was injected
into the agent prompt but the job was still reported as success (status=ok).
This hid silent data-collection failures from operators.

Fix: track script failure via _script_failed flag and return False when
the pre-run script fails, even if the agent runs successfully.

Regression tests added:
- test_script_failure_marks_job_as_failed: script exits 1 → job failed
- test_script_success_marks_job_as_ok: script exits 0 → job ok
- test_no_script_marks_job_as_ok: no script → job ok

Fixes NousResearch#20301
When the pre-run script fails, run_job() now returns a non-empty error
containing the script output instead of None. This ensures:

- mark_job_run() can persist the script failure as last_error
- _process_job() can deliver a meaningful failure message
- Operators see the actual script failure, not "unknown error"

Previously, the job was marked as failed (success=False) but error=None,
causing mark_job_run() to persist no last_error and _process_job() to
deliver a generic "unknown error" message.

Also drops unrelated commits from this PR (WhatsApp npm timeout
config and PATCH_SCHEMA changes) as requested by teknium1 review.

Responds to teknium1 review on PR NousResearch#20323.
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-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

3 participants