Skip to content

fix(cron): keep runtime-completed declarations manageable - #12

Closed
cirwel wants to merge 0 commit into
fix/cron-runtime-ownershipfrom
claude/github-pr-review-qem4b4
Closed

fix(cron): keep runtime-completed declarations manageable#12
cirwel wants to merge 0 commit into
fix/cron-runtime-ownershipfrom
claude/github-pr-review-qem4b4

Conversation

@cirwel

@cirwel cirwel commented Aug 1, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Addresses teknium1's review on NousResearch#75833 (targets the fix/cron-runtime-ownership branch so it folds into that PR).

The runtime-ownership work retains a repeat-exhausted job as a declaration but hides it behind a runtime_tombstone, and get_job / resolve_job_ref / list_jobs filtered those records unconditionally — so a completed declaration could not be listed, removed, edited, or revived through any supported surface. This PR adds explicit terminal-declaration management pathways, fixes the revival semantics so a revived job actually fires, and documents the definition/runtime split and paired-backup behavior.

Related Issue

Fixes the two review findings on NousResearch#75833 (hidden runtime tombstones; missing documentation for the definition/runtime split).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • cron/jobs.py
    • get_job, resolve_job_ref, list_jobs gain a keyword-only include_completed opt-in; tombstoned records surface with state: "completed" via _normalize_job_record.
    • remove_job resolves completed declarations; resume_job / trigger_job revive them — clearing the tombstone and resetting the repeat budget (_revive_updates) so the dispatch-limit guard can't immediately re-tombstone the fire. resume_job raises an actionable error for a completed past one-shot.
    • _reconcile_runtime_state: a definition-edit revival now also resets repeat.completed and clears claims — previously a prompt-only edit revived the job with an exhausted budget, and the next due scan re-tombstoned it (stale_dispatch_limit) without ever firing.
    • update_job re-reads the record after save so callers see reconciled state instead of a stale tombstone or repeat count.
  • tools/cronjob_tools.py
    • list gains include_completed (schema + handler); _format_job exposes completed_at / completed_reason.
    • The shared resolve now reaches terminal declarations; pause returns an actionable error for them; run revives before executing.
    • Post-run readbacks in _execute_job_now / the run action use include_completed=True — a finite one-shot tombstones itself inside mark_job_run, and the filtered readback misreported a successful final fire as success: false with an empty job record.
  • hermes_cli/cron.py: hermes cron list --all includes completed jobs (the [completed] render branch was previously unreachable); hermes cron edit resolves them.
  • Docs: website/docs/developer-guide/cron-internals.md (definition/runtime split, tombstone lifecycle and management pathways, paired backup/restore section), architecture.md (cron flow), user-guide/features/cron.md (job storage split, completed-jobs section, back-up-both-files warning), guides/cron-troubleshooting.md (runtime.db in the permissions check).
  • Tests: new tests/cron/test_tombstone_management.py — 10 end-to-end regressions covering list/get/resolve opt-ins, removal (definition + runtime row), revival via resume/trigger/edit (including the insta-retombstone trap and re-completion after revival), the no-op-update-keeps-completed contract, and the full tool-level list → pause-guard → resume → remove pathway.

How to Test

  1. scripts/run_tests.sh tests/cron tests/tools/test_cronjob_run_immediate.py tests/tools/test_cronjob_schema.py tests/hermes_cli/test_backup.py tests/hermes_cli/test_cron.py tests/hermes_cli/test_web_server_cron_profiles.py tests/gateway/test_api_server_jobs.py -q → 506 passed, 0 failed.
  2. Manual: create a one-shot (repeat 1), let it fire; hermes cron list hides it, hermes cron list --all shows it as [completed]; hermes cron resume <id> / run <id> revives it (repeat back to 0/1); hermes cron remove <id> deletes the declaration.
  3. ruff check, compileall, and scripts/check-windows-footguns.py pass on the changed files.

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 the affected suites and all tests pass (506 tests across cron/tools/backup/CLI/gateway job suites)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux 6.18

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 — N/A (no config keys changed)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — footgun checker passes
  • I've updated tool descriptions/schemas if I changed tool behavior — cronjob schema documents include_completed and completed-job management

Screenshots / Logs

=== Summary: 41 files, 506 tests passed, 0 failed (100% complete) in 17.4s (8 workers) ===

🤖 Generated with Claude Code

https://claude.ai/code/session_01UQr9bocSz9QnBak8rfh5Ng


Generated by Claude Code

cirwel commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

CI note: the Check contributors / check-attribution failure is pre-existing and not caused by this diff.

The check computes git merge-base origin/main HEAD, and this fork's main is at 49c3c2e — far behind the upstream main that fix/cron-runtime-ownership branched from. The scan therefore covers ~19.6k upstream commits (2,429 distinct author emails), and the 21 unmapped emails it reports (jquesnelle, nousbot, etc.) are all already present on the base branch — the identical scan over fix/cron-runtime-ownership alone yields the same set. The one email this PR's commit adds (noreply@anthropic.com) is in the check's ignore list (scripts/release.py).

Fix options outside this PR's scope: fast-forward the fork's main to upstream (the merge-base then collapses to the real fork point), or add the 21 mapping files via scripts/add_contributor.py. The check will pass upstream, where main is current.


Generated by Claude Code

@cirwel
cirwel marked this pull request as ready for review August 1, 2026 11:09
@cirwel
cirwel force-pushed the fix/cron-runtime-ownership branch 2 times, most recently from 7034a85 to f7dce23 Compare August 1, 2026 22:02
@cirwel

cirwel commented Aug 1, 2026

Copy link
Copy Markdown
Owner Author

Closing as superseded: the base branch (fix/cron-runtime-ownership) now carries an equivalent fix for the same sweeper finding — commits 7034a85 (terminal-management path + docs) and dd709c5 (review-driven coherence hardening, now f7dce23 after rebase) — developed independently and already posted to NousResearch#75833.

Harvested from this PR into the branch (thanks — these were real catches the other pass missed):

  • website/docs/developer-guide/architecture.md cron-flow diagram and website/docs/guides/cron-troubleshooting.md permissions section (both still described the single-file store)
  • isinstance(tombstone, dict) guards on tombstone reads
  • listing a completed job on the completed opt-in regardless of the disabled filter

Deliberate divergences from this PR's approach, for the record:

  • Revive semantics: this PR kept "any operator edit revives" and reset the repeat budget alongside the tombstone pop. The branch instead requires a cadence edit (schedule/repeat/enabled) to revive — a prompt-typo fix on a completed job should not silently re-arm unattended runs. Non-cadence edits persist but leave the job completed, and the response says so.
  • run on a completed job: this PR treats it as an explicit revive-and-fire; the branch returns an explicit terminal error pointing at update/remove, for the same reason — accidental resurrection of a finished job is the costlier failure mode for an unattended scheduler.

@cirwel cirwel closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant