Skip to content

fix: normalize repeat field in update_job() to prevent TypeError (#15582) - #15682

Open
vominh1919 wants to merge 2 commits into
NousResearch:mainfrom
vominh1919:fix/cron-repeat-normalize
Open

fix: normalize repeat field in update_job() to prevent TypeError (#15582)#15682
vominh1919 wants to merge 2 commits into
NousResearch:mainfrom
vominh1919:fix/cron-repeat-normalize

Conversation

@vominh1919

Copy link
Copy Markdown
Contributor

Problem

update_job() merges updates via {**job, **updates}, which blindly overwrites the repeat dict structure {"times": N, "completed": 0} with a raw integer when the API passes repeat as an int.

This causes mark_job_run() to crash with TypeError: 'int' object is not subscriptable when it tries job["repeat"]["completed"].

Fix

Add repeat field normalization in update_job(), mirroring what create_job() already applies:

  • If repeat is int/float: convert to {"times": val, "completed": 0} dict
  • If repeat is a dict: ensure "times" and "completed" keys exist via setdefault

Before vs After

Scenario Before After
PATCH /api/jobs/{id} with repeat: 5 Corrupts state, mark_job_run() crashes Normalizes to {"times": 5, "completed": 0}

Fixes #15582

The /api/skills endpoint calls _find_all_skills() without a try/except.
If skill discovery raises (e.g. corrupted skills directory, permission
error), the endpoint returns a 500 Internal Server Error.

Wrap the call in try/except and return an empty list on failure,
matching the pattern used by _list_all_skills() in skills_config.py.

Fixes NousResearch#15486
update_job() merges updates directly via {**job, **updates}, which
overwrites the repeat dict structure {"times": N, "completed": 0}
with a raw integer when the API passes repeat as an int.

This causes mark_job_run() to crash with TypeError when it tries
to subscript the integer: job["repeat"]["completed"].

Add the same normalization that create_job() applies, converting
raw integers to the expected dict structure.

Fixes NousResearch#15582
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #15590 — same fix: normalize repeat integer to dict format in update_job(). Both address #15582.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label Apr 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

duplicate comment posted

@nwang783

Copy link
Copy Markdown

I checked #15590. The overlap is real: both PRs are fixing repeat normalization in update_job() for the same bug report.

Given that, I would prefer to consolidate onto one PR. Unless there's an important behavioral difference not captured in the current discussion, #15590 currently looks like the better place to land the fix rather than merging two variants.

@hanzckernel

Copy link
Copy Markdown
Contributor

Noting the overlap for consolidation: #18416 is now open as a broader tested fix for #15582 after checking this PR and #15590.

It keeps the same core normalization idea, but also covers:

  • repeat: null clearing
  • completed-count preservation when changing/clearing the repeat limit
  • invalid API update value rejection
  • legacy scalar repair on get_job, list_jobs, and mark_job_run
  • regression coverage across cron storage, API update validation, and the cronjob tool list path

If maintainers prefer one landing path, #18416 is intended to supersede this variant rather than merging two overlapping fixes.

@alt-glitch alt-glitch added comp/dashboard Web dashboard / control panel UI (dashboard/, landing) and removed duplicate This issue or pull request already exists comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for identifying the raw-form repeat mismatch. The premise remains valid on current main: cron/jobs.py:1220 blindly merges updates, while the dashboard accepts CronJobUpdate.updates: dict at hermes_cli/web_server.py:9935; mark_job_run() then calls .get() on repeat at cron/jobs.py:1412.

Problems

  • The new (int, float) branch accepts booleans and truncates floats. Because the API payload is untyped, validation must exclude bool and reject non-integer values.
  • The proposed {"times": val, "completed": 0} replacement resets completed runs when an existing job's limit is edited. Preserve the stored completed count.
  • Existing scalar records remain unsafe in mark_job_run() (cron/jobs.py:1412), claim_dispatch() (cron/jobs.py:1497), and due-job handling (cron/jobs.py:1816).
  • gh pr diff 15682 shows no regression tests, and commit d1d956da32a7 adds an unrelated /api/skills change for #15486.

Suggested changes

  • Add strict repeat validation and explicit null semantics at the update boundary, preserve completed counts, repair legacy records at every read/run boundary, and add storage/API regressions.
  • Split the skills-endpoint change for separate review.

This is an automated hermes-sweeper review.

Comment thread cron/jobs.py
elif isinstance(raw_repeat, dict):
raw_repeat.setdefault("times", None)
raw_repeat.setdefault("completed", 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CronJobUpdate.updates is an untyped dictionary on current main (hermes_cli/web_server.py:9935), so this also accepts True (bool is an int subclass) and truncates floats. Restrict the update contract to positive non-boolean integers, with explicit null semantics, and cover it at the API boundary.

Comment thread cron/jobs.py
raw_repeat.setdefault("completed", 0)

schedule_changed = "schedule" in updates

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This resets an already-running job's completed count whenever its repeat limit is edited, allowing previously consumed executions to recur. Preserve the existing job['repeat']['completed'] while replacing the limit.

@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #18416 (now open) is a broader, tested fix for the same bug (#15582) — it keeps the same repeat normalization but also handles repeat: null clearing and preserves the completed count. #15590 (the PR mentioned earlier) is closed. Flagging the overlap so a maintainer can consolidate onto the broadest fix; not marking a duplicate since #18416 is a superset (competing subset/superset PRs are related, not duplicates).

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@alt-glitch alt-glitch removed the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Jul 12, 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 comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API job repeat updates corrupt cron repeat state

5 participants