Skip to content

fix(cron): preserve latest execution truth - #70890

Open
MrD1az wants to merge 2 commits into
NousResearch:mainfrom
MrD1az:fix/cron-preserve-latest-execution
Open

MrD1az wants to merge 2 commits into
NousResearch:mainfrom
MrD1az:fix/cron-preserve-latest-execution

Conversation

@MrD1az

@MrD1az MrD1az commented Jul 24, 2026

Copy link
Copy Markdown

What does this PR do?

_format_job() drops the latest_execution field when it builds a job's public shape, so consumers only see the historical completion summary (last_status, last_run). When a newer attempt is claimed, running, failed or in an unknown state, that summary still reports the previous outcome — a stale last_status="ok" reads as the result of the newer attempt.

This adds latest_execution alongside the existing summary fields, so the occurrence ledger travels with the job and a caller can tell "the last completed run was ok" apart from "the current attempt is ok".

Four lines plus a regression test. No behavior change for callers that ignore the field.

Related Issue

No existing issue — this surfaced while reading cron job state in a dashboard, where a job that was mid-failure kept presenting as healthy.

Searched before filing: no open or merged PR mentions latest_execution. The closest existing work is #48072 (feat(cron): 3-state last_status), which reshapes the summary field. This PR does not touch last_status and does not conflict with that approach — it exposes the ledger that already exists in the job dict.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/cronjob_tools.py_format_job() now carries latest_execution through to the formatted job (+4 lines, including the comment explaining the stale-summary trap).
  • tests/cron/test_execution_ledger.py — adds test_cronjob_formatter_exposes_newer_unknown_attempt_over_stale_success, which constructs a job whose last_status is ok while a newer attempt is unknown, and asserts the formatter exposes the newer attempt (+40 lines).

How to Test

  1. Take a cron job whose most recent completed run succeeded, then start a new attempt and leave it claimed/unknown (kill the worker, or construct the job dict directly as the test does).
  2. Read the job through _format_job(). Before this change the result reports last_status="ok" and nothing about the newer attempt.
  3. After this change, latest_execution is present and describes the newer attempt.

Automated:

pytest tests/cron/ -q          # 748 passed
pytest tests/cron/test_execution_ledger.py -q

Falsifier — reverting only the tools/cronjob_tools.py hunk makes the new test fail with KeyError at test_execution_ledger.py:359, so the test genuinely covers the fix rather than passing either way.

Note on the checklist below: I ran the cron suite (pytest tests/cron/ -q, 748 passed) rather than the whole tests/ tree, because the full run can touch a real HERMES_HOME on this machine and I did not want it reaching a live install. Happy to post a full-suite run from a clean environment if you want it before merge.

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 — ran pytest tests/cron/ -q (748 passed); see the note above
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.5.0, arm64), Python 3.13.13

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (N/A: internal formatter, comment added inline)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (N/A: no config keys)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A (N/A)
  • I've considered cross-platform impact (Windows, macOS) — or N/A (N/A: pure dict passthrough, no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A (N/A: adds a field to an existing payload, no schema/description change)

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

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the stale-summary distinction and adding a focused regression test. The list-path fix is valid on current main, but the execution truth is still inconsistent across cron-tool response paths.

Problems

  • tools/cronjob_tools.py:590 copies latest_execution only if the input already has it. Main enriches only list_jobs() results (cron/jobs.py:1491-1504), while get_job() and update_job() return plain job records (cron/jobs.py:1444-1450, cron/jobs.py:1602-1604). The formatted pause/resume/update/run responses can therefore return latest_execution: null despite an existing newer ledger record.

Suggested changes

  • Route every formatted cron-tool job response through a shared ledger-enrichment path, and add a non-list action regression test.

Automated hermes-sweeper review.

Comment thread tools/cronjob_tools.py Outdated
# Keep the occurrence ledger beside the historical completion summary.
# Without it, a prior last_status="ok" can be mistaken for the result
# of a newer claimed/running/failed/unknown attempt.
"latest_execution": job.get("latest_execution"),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list_jobs() is the only current supplier of this field (cron/jobs.py:1491-1504). Other _format_job() callers receive records from get_job()/update_job(), which do not attach ledger state, so those responses emit null even when an execution exists. Please use a shared enriched-job path for all formatted action responses.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in commit 3169e489. The list result retains the existing batch enrichment, while action responses now lazily resolve ledger truth through the shared _format_job() seam. The list and pause regressions plus related cron checks passed: 103 tests passed. The full canonical suite was unavailable because the shared Hermes environment selected by scripts/run_tests.sh lacks pytest.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants