Skip to content

fix(cron): whitelist cron update_job fields so typos don't silently persist (#67625) - #67660

Open
Ahmett101 wants to merge 2 commits into
NousResearch:mainfrom
Ahmett101:fix/67625-clean
Open

fix(cron): whitelist cron update_job fields so typos don't silently persist (#67625)#67660
Ahmett101 wants to merge 2 commits into
NousResearch:mainfrom
Ahmett101:fix/67625-clean

Conversation

@Ahmett101

Copy link
Copy Markdown
Contributor

Summary

Reopened as a clean branch — the previous fix/67625-cron-update-whitelist branch had grown a stale ACP commit from an unrelated worktree. This branch (fix/67625-clean) carries only the cron whitelist change.

Adds an allow-list of valid update fields to cron/jobs.py::update_job and the dashboard adapter _normalize_dashboard_cron_updates. Unknown keys (promt typo, stale-client custom keys) now raise ValueError (cron layer) or HTTP 422 (dashboard) instead of silently being persisted and silently failing the actual intended update. Sanctioned escape valve: metadata: { ... } for legitimate integration bookkeeping.

Changes

  • cron/jobs.py: new _VALID_JOB_FIELDS whitelist + update_job rejects any unknown key with ValueError. The existing immutable-id guard is preserved as a distinct 400-class error.
  • hermes_cli/web_server.py: new _DASHBOARD_ALLOWED_UPDATE_FIELDS constant; _normalize_dashboard_cron_updates returns 422 listing every offending field, before the IPC round-trip. id is allowed as a payload key so the immutable guard's 400 remains the actionable error.
  • tests/cron/test_jobs.py: 3 new regression tests (typo, multi-unknown, metadata escape valve).
  • tests/hermes_cli/test_web_server_cron_profiles.py: 1 new dashboard test (422 with offending key listed, original field untouched).

How to Test

pytest tests/cron/test_jobs.py tests/hermes_cli/test_web_server_cron_profiles.py -q

166/166 passed

Checklist

  • Tests pass — 166
  • Follows Conventional Commits
  • Changes scoped to this fix only
  • Cross-platform impact assessed (Linux / macOS / WSL2 / Windows / Termux) — none
  • profile-safe paths used
  • .env not used for non-credential settings

Risk & Impact: Low. Strict validation: update payloads with typos or stale-client custom keys now raise ValueError / 422. External integrations relying on ad-hoc keys must move to whitelisted fields or use metadata.

Type: Bug fix
Closes: #67625

@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #67625 and #66881. The live patch combines independent cron-validation and ACP session-listing fixes; please choose a split or combined review.

@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 addressing the core and dashboard pass-through paths; the current-main premise is valid (cron/jobs.py:1486, hermes_cli/web_server.py:12034).

Problems

  • The gateway REST update route remains a silent-partial-update path: gateway/platforms/api_server.py:5301 filters unknown keys, and tests/gateway/test_api_server_jobs.py:358-371 currently expects HTTP 200 for a request containing name plus unknown fields. This PR does not change that route.
  • The dashboard allow-list is described as mirroring the core allow-list, but the diff accepts attach_to_session in core and omits it in the dashboard. This is the linked #67706 concern; align the contracts or explicitly document the dashboard subset.
  • metadata is described as a dict escape valve, but the proposed guard only allow-lists it; the existing merge at cron/jobs.py:1486 will persist a scalar/list too.
  • The diff also includes the separate ACP fix from a7a21b2c6f95; this matches the existing request to split or explicitly conduct a combined review.

Suggested changes

  • Reject unknown fields before the gateway API sanitization and add a mixed valid-plus-typo regression test.
  • Reconcile/document the dashboard and core field sets, validate metadata as a dict, and add negative coverage.

Automated hermes-sweeper review.

Comment thread hermes_cli/web_server.py
)


_DASHBOARD_ALLOWED_UPDATE_FIELDS = frozenset({

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 is documented as mirroring the core allow-list, but the diff omits attach_to_session, which the core list accepts. Linked issue #67706 tracks that drift; please either share one canonical contract or document this as an intentional dashboard-only subset.

Comment thread cron/jobs.py
"last_delivery_error", "deliver", "origin", "attach_to_session",
"provider_snapshot", "model_snapshot", "created_at",
"created_by_user_id",
# Fragmentation of arbitrary bookkeeping. Kept opaque downstream

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.

metadata is documented here as a dict escape valve, but the subsequent guard only checks its key. Please reject non-dict metadata before the existing {**job, **updates} merge can persist a scalar or list.

Comment thread acp_adapter/session.py
# Collect in-memory sessions first.
with self._lock:
seen_ids = set(self._sessions.keys())
seen_ids = set()

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 is the independent #66881 ACP behavior change, not part of the cron validation fix. The existing maintainer comment requested a split or an explicit combined review; please separate it so the cron fix can be evaluated independently.

@strzhao

strzhao commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@Ahmett101 — nice fix on #67625; the metadata escape hatch is a thoughtful touch. The sweeper's medium review matched gaps I'd been looking at too, so I worked through them on top of your branch (rebased onto current main). I'm not trying to jump the queue — happy to hand all of it back as a diff if you're still on this; only pushing forward if you'd like to hand it off.

Mapping to the sweeper's four Problems:

  1. Gateway REST (api_server.py _handle_update_job) — the third silent-partial path: it filtered unknown keys via _UPDATE_ALLOWED_FIELDS but still returned 200, so typos were silently dropped. Now 422 with the keys named. test_update_job_rejects_unknown_fields was asserting 200 despite its name → fixed to 422 + _cron_update never called, plus a happy-path test.
  2. Dashboard drift_DASHBOARD_ALLOWED_UPDATE_FIELDS missed attach_to_session (your core list has it). Added it, documented the dashboard as a deliberate subset of cron.jobs._VALID_JOB_FIELDS (form doesn't expose internal state like next_run_at/*_snapshot), guarded by a subset-assertion test so future drift fails loudly.
  3. metadata type — whitelist let it through but {**job,**updates} persisted scalar/list the same way. Added isinstance(dict) guard + test.
  4. ACP split — took only your 857fb756 cron commit; #66881 stays separate.

110 cron/gateway/dashboard tests + ruff green, on local branch fix/cron-update-field-validation-salvage. If you'd like me to push it as a salvages #67660 PR with full credit to you, say the word; otherwise I'll leave it with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter comp/cli CLI entry point, hermes_cli/, setup wizard 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.

Bug: update_job silently persists unknown keys with no validation

4 participants