Skip to content

fix(cron): reject unknown fields in update_job instead of silently persisting (#67625) - #76831

Closed
Enough1122 wants to merge 3 commits into
NousResearch:mainfrom
Enough1122:fix/67625-update-job-validation
Closed

fix(cron): reject unknown fields in update_job instead of silently persisting (#67625)#76831
Enough1122 wants to merge 3 commits into
NousResearch:mainfrom
Enough1122:fix/67625-update-job-validation

Conversation

@Enough1122

Copy link
Copy Markdown
Contributor

Summary

update_job in cron/jobs.py merged update payloads unconditionally — the only guard was _IMMUTABLE_JOB_FIELDS = {"id"}. Any unknown key, including typos like promt instead of prompt, was silently persisted to jobs.json and reported as success via the API while the real field stayed unchanged — the illusion of a successful update.

Fix: add an _UPDATEABLE_JOB_FIELDS whitelist (the create_job() parameter set + schedule_display + the lifecycle fields enabled/state/paused_at/paused_reason/next_run_at that pause_job/resume_job/trigger_job persist through update_job). Unknown keys now raise ValueError listing the offending fields. The dashboard API handler already translates ValueError → HTTP 400, so typo'd payloads fail loudly on every surface (dashboard, CLI, cronjob tool).

Single-file change (+ 2 regression tests). No public API change.

NOT doing X: not rejecting fields that internal scheduler paths legitimately write (pause/resume/trigger keep working — covered by the existing lifecycle tests), not adding 422-vs-400 semantics — the existing ValueError→400 conversion is reused as-is.

Test plan

python -m pytest tests/cron/test_jobs.py -q   # 56 passed (53 + 3 new)
python -m pytest tests/tools/test_cronjob_tools.py -q  # 56 passed
# New: typo'd field raises ValueError and nothing is persisted;
# all 17 known fields accepted in one update.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation labels Aug 2, 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 isolating the unchecked storage merge; the premise is confirmed on current main: cron/jobs.py:1534 merges arbitrary update keys after only the immutable-id guard.

Problems

  • The whitelist added by 2578882ef5fa omits description. The linked #66786 discussion documents that description currently round-trips via this update path and that its feature work remains open, so this would turn existing behavior into a rejection.
  • The gateway REST path still silently drops unknown keys in mixed requests at gateway/platforms/api_server.py:5355; its current behavior is asserted by tests/gateway/test_api_server_jobs.py:189-213. That does not meet a uniform “unknown fields fail loudly” contract.

Suggested changes

  • Settle and test the supported update-field contract, including description compatibility.
  • Reject unknown raw REST keys before sanitization if the intended guarantee covers that API surface.

Automated hermes-sweeper review.

Comment thread cron/jobs.py
@teknium1 teknium1 added 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 Aug 2, 2026
…rsisting (NousResearch#67625)

update_job merged update payloads unconditionally (only 'id' was
guarded), so a typo like 'promt' was persisted to jobs.json and
reported as a successful update while the real prompt stayed unchanged.

Add an _UPDATEABLE_JOB_FIELDS whitelist (create_job parameters +
schedule_display + the lifecycle fields pause/resume/trigger persist) and
raise ValueError listing the unknown keys. Dashboard/API paths already
translate ValueError to HTTP 400, so typo'd payloads now fail loudly.
@Enough1122
Enough1122 force-pushed the fix/67625-update-job-validation branch from 2578882 to 6e18739 Compare August 2, 2026 15:21
…sResearch#76831)

description round-trips through update_job and is the compatibility basis
for the open NousResearch#66786 description feature. Without this entry, update_job
silently drops the field rather than accepting or loudly rejecting it.
@Enough1122

Copy link
Copy Markdown
Contributor Author

Closing as duplicate: #67660 (Ahmett101, opened 2 weeks earlier) fixes the same issue #67625 with the same whitelist strategy in cron/jobs.py, and its surface is broader (also covers the dashboard 422 path via hermes_cli/web_server.py + ACP session fix). Our branch was an independent narrower implementation of the same fix. Branch left for manual cleanup.

@Enough1122 Enough1122 closed this Aug 10, 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 needs-decision Awaiting maintainer decision before any implementation 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.

3 participants