fix(cron): whitelist cron update_job fields so typos don't silently persist (#67625) - #67646
Closed
Ahmett101 wants to merge 2 commits into
Closed
fix(cron): whitelist cron update_job fields so typos don't silently persist (#67625)#67646Ahmett101 wants to merge 2 commits into
Ahmett101 wants to merge 2 commits into
Conversation
Collaborator
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
update_jobincron/jobs.pyand the dashboard adapter_normalize_dashboard_cron_updatessilently persisted any key in the update payload — including typos likepromtinstead ofprompt. The API returned 200 and persisted the garbage key; downstream consumers then depended on it. Stale-client custom fields accumulated in jobs.json with no audit trail.This change rejects unknown keys with a clear error instead of silently storing them. Existing valid fields continue to work; an explicit
metadatafield is reserved for integration-specific bookkeeping so users have a sanctioned place to keep extension data instead of inventing top-level keys.Changes
cron/jobs.py: add_VALID_JOB_FIELDSwhitelist (all valid job attrs +description,attach_to_session,provider_snapshot,model_snapshot,metadata);update_jobnow raisesValueError("Cron job update contains unknown field(s): ..."). The existing immutable-idguard is preserved as a distinct 400-class error.hermes_cli/web_server.py: add_DASHBOARD_ALLOWED_UPDATE_FIELDSmirroring the whitelist;_normalize_dashboard_cron_updatesrejects unknown keys with a 422 listing every offending field, before the IPC round-trip.idis allowed as a payload key here so the existing immutable guard's 400 remains actionable.tests/cron/test_jobs.py: 3 new regression tests — typo reject, multiple-unknown listing, metadata escape-valve.tests/hermes_cli/test_web_server_cron_profiles.py: 1 new test — dashboard surfaces a 422 with the offending key listed, original field untouched.How to Test
pytest tests/cron/test_jobs.py -q
144/144 passed (3 new: test_update_job_rejects_unknown_field_typo, test_update_job_rejects_multiple_unknown_keys, test_update_job_accepts_explicit_metadata)
pytest tests/hermes_cli/test_web_server_cron_profiles.py -q
22/22 passed (1 new: test_update_cron_job_rejects_unknown_field_as_422)
Decision rationale (issue is needs-decision)
Three modes were on the table in the issue: warn-first, reject-only, strip-and-proceed — and whether ad-hoc keys deserve an explicit metadata vent. Reject-only is the strict choice the issue favours for the typo class, and a preserved metadata escape valve keeps the legitimate-integration use case working without letting accidental keys slip in. Lowest ambiguity: caller learns immediately when a key is unknown, but has a sanctioned place for genuine extension data.
Checklist
Risk & Impact: Low. Strict validation: previously-passing update payloads with typos or stale-client custom keys will now raise ValueError (caller sees 4xx instead of false-positive 200). External integrations relying on ad-hoc keys must either update to whitelisted fields or move their extension data under metadata. Issue explicitly flags this as a breaking-change risk and asks for the audit — this PR documents the reject semantics clearly so the audit can compare against the whitelist.
Type: Bug fix
Closes: #67625