fix(cronjob): accept bare model string in schema and _resolve_model_override - #68587
fix(cronjob): accept bare model string in schema and _resolve_model_override#68587webtecnica wants to merge 2 commits into
Conversation
…verride Issue NousResearch#68380 — cronjob action=update silently drops . The JSON schema declared as an object ({provider, model}) while the Python handler expected a flat string. When an agent sent a flat model string (the shape the handler reads), schema validation stripped it, and returned (None, None) for non-dict inputs — the model was silently lost. Fix (two parts): 1. Schema: Change from {provider, model} object to a flat string. Add top-level and string params, matching the function signature and the NousResearch#59031 guardrail syntax. 2. _resolve_model_override: Accept bare model strings in addition to dicts. A bare string is treated as model name with no provider override. Whitespace-stripped; empty strings return (None, None). Regression tests (5): bare string, whitespace-stripped, empty string, dict path unchanged, None unchanged.
22cfdf2 to
41909eb
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the earlier schema/handler mismatch. Current main has since adopted a different, deliberate model: per-job inference pins are user-owned rather than agent-callable.
Problems
tools/cronjob_tools.py:1083-1087intentionally ignores agent-suppliedmodel,provider, andbase_url, so restoring the schema fields in this PR would let an agent redirect unattended spend again.tests/tools/test_cronjob_tools.py:543-608already verifies both schema exclusion and preservation of existing user-owned pins. The helper this PR extends was removed byd464ae3652cee225bc667c6b64caf749bfb55965as part of that design.
Suggested changes
- Please target the supported pinning surfaces instead: dashboard,
hermes cron create/edit --model/--provider,jobs.json, or the cron-fleetcron.model/cron.model_providerconfiguration added ind464ae3652.
Automated hermes-sweeper review.
| @@ -1023,19 +1033,16 @@ def cronjob( | |||
| "description": "Optional ordered list of skill names to load before executing the cron prompt. On update, pass an empty array to clear attached skills." | |||
There was a problem hiding this comment.
Current main deliberately removed agent-facing model/provider/base_url pins in d464ae3652: tools/cronjob_tools.py:1083-1087 now ignores those arguments so an agent cannot redirect unattended spend. Please do not restore this schema surface; use the user-owned CLI/dashboard pinning paths instead.
SummaryTwo PRs address the former schema/handler mismatch in #68380: #68397 adds bare-string resolver compatibility and helper tests, while #68587 additionally exposes flat Related pull requests
Duplicates#68397 and #68587 substantially duplicate the bare-string resolver change and helper tests; #68587 is the broader superseding diff because it also changes the exposed schema. Suggested consolidationClose #68587 as already implemented on main through the alternative resolution in Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I68380(["issue #68380 (closed)"])
subgraph Dup68397 ["PRs duplicating each other"]
P68397["PR #68397 (closed)"]
P68587["PR #68587 (open)"]
end
P68587 -->|best fix| I68380
class I68380 closed
class P68397 closed
class P68587 open
class P68587 best
class P68587 target
click I68380 "https://github.com/NousResearch/hermes-agent/issues/68380"
click P68397 "https://github.com/NousResearch/hermes-agent/pull/68397"
click P68587 "https://github.com/NousResearch/hermes-agent/pull/68587"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 9 kB of PR diffs, 8 kB of issue/PR text, 3 kB of discussion (4 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Thanks for the work here @webtecnica — closing this one on policy grounds rather than code quality. PR #73532 deliberately removed |
Fixes #68380
Problem
cronjob action=updatesilently drops themodelfield when the LLM sends a flat string (e.g."gpt-4") instead of the declared object shape{"model": "gpt-4"}. The provider updates fine because it is a top-level string, but_resolve_model_overrideguards onisinstance(model_obj, dict)and returns(None, None)for strings — the model value is lost entirely while the tool reports success.Fix (two parts)
Schema: Change
modelfrom{provider, model}object to a flat string. Add top-levelproviderandbase_urlstring params, matching the function signature and the UX: model drift guardrail silently breaks all unpinned cron jobs on model change #59031 guardrail's advertised syntax._resolve_model_override: Accept both shapes:
{"model": "...", "provider": "..."}— existing object shape (unchanged)"gpt-4"— bare string treated as model name, no provider overrideRegression tests (5)
Added 5 new tests to
TestResolveModelOverride(8 total):test_bare_string_model_returns_none_provider—"anthropic/claude-haiku-4.5"→(None, "anthropic/claude-haiku-4.5")test_bare_string_model_whitespace_stripped—" gpt-4 "→(None, "gpt-4")test_bare_string_model_empty_returns_none_none—" "→(None, None)test_dict_model_still_works— dict path unaffectedtest_none_returns_none_none— None path unaffectedAll 82 tests in
tests/tools/test_cronjob_tools.pypass.