Timmy_Foundation/hermes-agent
15
0
Fork 0
Code Issues 208 Pull Requests 69 Actions 198 Packages Projects Releases Wiki Activity

fix(cron): include model/provider in deploy comparison #577

Closed
Rockachopa wants to merge 245 commits from am/375-1776166469 into main
pull from: am/375-1776166469
merge into: Timmy_Foundation:main
Conversation 4 Commits 245 Files Changed 227 +50977 -2487
Rockachopa commented 2026-04-14 11:35:31 +00:00
Owner
Copy Link

Fixes #375

_jobs_changed() now compares 4 fields: prompt, schedule, model, provider.
Model/provider-only YAML changes are no longer silently dropped.

Fixes #375 `_jobs_changed()` now compares 4 fields: prompt, schedule, model, provider. Model/provider-only YAML changes are no longer silently dropped.
Rockachopa added 1 commit 2026-04-14 11:35:32 +00:00
fix(cron): include model/provider in deploy comparison
Some checks failed
Forge CI / smoke-and-build (pull_request) Failing after 54s
Details
2e458b76ad 
Fixes #375

_jobs_changed() compares prompt, schedule, model, and provider.
Model/provider-only YAML changes are no longer silently dropped.
Rockachopa added 1 commit 2026-04-14 11:35:32 +00:00
fix(cron): include model/provider in deploy comparison
Some checks failed
Forge CI / smoke-and-build (pull_request) Failing after 54s
Details
2e458b76ad 
Fixes #375

_jobs_changed() compares prompt, schedule, model, and provider.
Model/provider-only YAML changes are no longer silently dropped.
Timmy requested changes 2026-04-14 12:13:35 +00:00
Dismissed
Timmy left a comment
Owner
Copy Link

Review: Include model/provider in deploy comparison (#375)

This PR rewrites deploy-crons.py to add YAML-based deployment (--deploy) alongside the existing normalization (--normalize), and fixes the core bug where _jobs_changed() only compared prompt and schedule, silently dropping model/provider changes.

Looks good:

  • The core fix is correct: _jobs_changed() now compares 4 fields (prompt, schedule, model, provider) using _flat_model() and _flat_provider() helpers to normalize dict vs string model representations
  • The --deploy / --normalize mutual exclusion via add_mutually_exclusive_group is clean
  • normalize_job() is simplified without losing functionality
  • The YAML deploy flow (create/update/skip) with dry-run support is well-structured

Issues requiring changes:

1. Job matching key is fragile and will break on model/provider changes

The index key is f"{prompt}||{json.dumps(schedule, sort_keys=True)}". This means if a user changes only the model or provider in the YAML, the key still matches the existing job (same prompt + schedule), so it correctly triggers an update. Good. BUT if the user changes the prompt, the key won't match, so a new job is created instead of updating the existing one — leaving the old job orphaned. There's no mechanism to match by job name or ID, which would be more robust.

2. _parse_schedule fallback is incomplete

The fallback parser (when cron.jobs.parse_schedule can't be imported) only handles every Xm/Xh/Xd patterns. Standard cron expressions like 0 9 * * 1-5 would fall through to the else branch and be returned as {"kind": "cron", "expr": schedule}. This may not match the format produced by the real parse_schedule, causing key mismatches and duplicate job creation.

3. New jobs are created with created_at: None

When creating a new job, "created_at": None is hardcoded. This should be set to the current timestamp (datetime.now().isoformat() or similar).

4. No tests

This is a complete rewrite of the deploy script with new deployment logic. There should be tests for:

  • _jobs_changed() with various field change combinations
  • _flat_model() / _flat_provider() with dict and string model formats
  • normalize_job() edge cases
  • The deploy flow (create, update, skip scenarios)

5. Lost error handling from the original

The original normalize_jobs_file had try/except json.JSONDecodeError around JSON loading. The rewrite just calls json.load(f) without catching parse errors, which would produce an unhelpful traceback for corrupted files.

6. Readability regression

The original code had clear variable names and comments. The rewrite uses very compressed style (om, op, n, a, g, p). For a deployment tool that people run in production, readability matters.

## Review: Include model/provider in deploy comparison (#375) This PR rewrites `deploy-crons.py` to add YAML-based deployment (`--deploy`) alongside the existing normalization (`--normalize`), and fixes the core bug where `_jobs_changed()` only compared prompt and schedule, silently dropping model/provider changes. ### Looks good: - The core fix is correct: `_jobs_changed()` now compares 4 fields (prompt, schedule, model, provider) using `_flat_model()` and `_flat_provider()` helpers to normalize dict vs string model representations - The `--deploy` / `--normalize` mutual exclusion via `add_mutually_exclusive_group` is clean - `normalize_job()` is simplified without losing functionality - The YAML deploy flow (create/update/skip) with dry-run support is well-structured ### Issues requiring changes: **1. Job matching key is fragile and will break on model/provider changes** The index key is `f"{prompt}||{json.dumps(schedule, sort_keys=True)}"`. This means if a user changes only the model or provider in the YAML, the key still matches the existing job (same prompt + schedule), so it correctly triggers an update. Good. BUT if the user changes the prompt, the key won't match, so a new job is created instead of updating the existing one — leaving the old job orphaned. There's no mechanism to match by job name or ID, which would be more robust. **2. `_parse_schedule` fallback is incomplete** The fallback parser (when `cron.jobs.parse_schedule` can't be imported) only handles `every Xm/Xh/Xd` patterns. Standard cron expressions like `0 9 * * 1-5` would fall through to the else branch and be returned as `{"kind": "cron", "expr": schedule}`. This may not match the format produced by the real `parse_schedule`, causing key mismatches and duplicate job creation. **3. New jobs are created with `created_at: None`** When creating a new job, `"created_at": None` is hardcoded. This should be set to the current timestamp (`datetime.now().isoformat()` or similar). **4. No tests** This is a complete rewrite of the deploy script with new deployment logic. There should be tests for: - `_jobs_changed()` with various field change combinations - `_flat_model()` / `_flat_provider()` with dict and string model formats - `normalize_job()` edge cases - The deploy flow (create, update, skip scenarios) **5. Lost error handling from the original** The original `normalize_jobs_file` had `try/except json.JSONDecodeError` around JSON loading. The rewrite just calls `json.load(f)` without catching parse errors, which would produce an unhelpful traceback for corrupted files. **6. Readability regression** The original code had clear variable names and comments. The rewrite uses very compressed style (`om`, `op`, `n`, `a`, `g`, `p`). For a deployment tool that people run in production, readability matters.
claw-code requested changes 2026-04-14 16:09:49 +00:00
claw-code left a comment
Collaborator
Copy Link

Review: PR #577 — fix(cron): include model/provider in deploy comparison

This PR rewrites deploy-crons.py substantially — adding YAML-based deployment mode and fixing _jobs_changed() to compare model/provider fields in addition to prompt and schedule.

Issues:

  1. Full file rewrite for a small fix: The PR title says "include model/provider in deploy comparison" but the diff rewrites the entire deploy-crons.py (154 lines removed, 174 added). The core fix is just the _jobs_changed() function. This level of rewrite carries merge-conflict risk and makes review harder.
  2. _flat_model / _flat_provider helpers are a good abstraction for handling the dict-or-string model field.
  3. _parse_schedule fallback has a simplistic parser (int(dur[:-1])) that will throw ValueError on unexpected input like every 1.5h or every foo. Needs error handling.
  4. HAS_YAML import guard is good, but the deploy path silently fails if YAML is not installed — should this be a hard dependency for deploy mode?
  5. No tests for the new deploy-from-YAML workflow or _jobs_changed() comparison logic.

The fix for #375 (model/provider comparison) is correct and needed, but the scope creep from a 4-field comparison fix to a full rewrite of the deploy script is concerning.

## Review: PR #577 — fix(cron): include model/provider in deploy comparison This PR rewrites `deploy-crons.py` substantially — adding YAML-based deployment mode and fixing `_jobs_changed()` to compare model/provider fields in addition to prompt and schedule. **Issues:** 1. **Full file rewrite for a small fix:** The PR title says "include model/provider in deploy comparison" but the diff rewrites the entire `deploy-crons.py` (154 lines removed, 174 added). The core fix is just the `_jobs_changed()` function. This level of rewrite carries merge-conflict risk and makes review harder. 2. **`_flat_model` / `_flat_provider` helpers** are a good abstraction for handling the dict-or-string model field. 3. **`_parse_schedule` fallback** has a simplistic parser (`int(dur[:-1])`) that will throw `ValueError` on unexpected input like `every 1.5h` or `every foo`. Needs error handling. 4. **`HAS_YAML` import guard** is good, but the deploy path silently fails if YAML is not installed — should this be a hard dependency for deploy mode? 5. **No tests** for the new deploy-from-YAML workflow or `_jobs_changed()` comparison logic. The fix for #375 (model/provider comparison) is correct and needed, but the scope creep from a 4-field comparison fix to a full rewrite of the deploy script is concerning.
Timmy requested changes 2026-04-14 20:14:37 +00:00
Timmy left a comment
Owner
Copy Link

Duplicate PR — This is an older version. The latest iteration is PR #607. Please close this PR and consolidate work there.

This version includes model/provider in deploy comparison but #607 adds improved deploy-sync guard validation with more comprehensive kwarg checking and better error messages.

**Duplicate PR** — This is an older version. The latest iteration is PR #607. Please close this PR and consolidate work there. This version includes model/provider in deploy comparison but #607 adds improved deploy-sync guard validation with more comprehensive kwarg checking and better error messages.
Timmy closed this pull request 2026-04-14 20:35:56 +00:00
Timmy commented 2026-04-14 20:35:58 +00:00
Owner
Copy Link

Archived after upstream sync to NousResearch/hermes-agent.

Branch unknown preserved — use git log unknown to review work, cherry-pick if still relevant against current codebase.

Archived after upstream sync to NousResearch/hermes-agent. Branch `unknown` preserved — use `git log unknown` to review work, cherry-pick if still relevant against current codebase.
Some checks failed
Forge CI / smoke-and-build (pull_request) Failing after 54s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
claw-code
Timmy
Labels
Clear labels
CI
Continuous integration, runners, workflow issues
 QA
Quality assurance, testing, production audit
 assigned-claw-code
Queued for Code Claw (qwen/openrouter)
 assigned-kimi
Task assigned to KimiClaw for processing
 audit-finding
batch-pipeline
batch-pipeline label
 blocked
Blocked by external dependency or merge conflict
 bug
claw-code-done
Code Claw completed this task
 claw-code-in-progress
Code Claw is actively working
 critical
duplicate
Duplicate of another issue
 epic
Epic - large feature with multiple sub-tasks
 epic:memory
Unified Memory Architecture epic
 gaming
Gaming agent capabilities
 infra
Infrastructure, VPS, DNS, deployment
 kimi-done
KimiClaw has completed this task
 kimi-in-progress
KimiClaw is actively working on this
 mcp
MCP (Model Context Protocol) tools & servers
 morrowind
Morrowind Agent gameplay & MCP integration
 needs-review
PR or issue requires reviewer sign-off before merge/close
 ops
p0-critical
p1-important
phase:1-activate
Phase 1: Activate existing systems
 phase:2-operational
Phase 2: Operational memory bridge
 phase:3-episodic
Phase 3: Proactive episodic recall
 phase:4-hygiene
Phase 4: Memory hygiene and quality
 phase:5-integration
Phase 5: Integration and testing
 poka-yoke
research
security
Security hardening, vulnerability fixes
 shared-infra
Shared infrastructure across projects (Playground + LPM)
 stale
No activity, pending triage or closure
 throughput-10x
throughput-10x label
 token-masterplan
token-masterplan label
 tool-evaluation
velocity-engine
Auto-generated by velocity engine
 wont-fix
Closed as intentionally not fixing — explicit descope
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Rockachopa Timmy allegro antigravity bezalel claude claw-code codex-agent ezra gemini google grok hermes kimi manus perplexity sonnet
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Timmy_Foundation/hermes-agent#577
Delete Branch "am/375-1776166469"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?