Skip to content

fix(cron): run named jobs without unnecessary list scans - #83477

Open
fangliquanflq wants to merge 1 commit into
NousResearch:mainfrom
fangliquanflq:fix/cron-direct-name-resolution
Open

fix(cron): run named jobs without unnecessary list scans#83477
fangliquanflq wants to merge 1 commit into
NousResearch:mainfrom
fangliquanflq:fix/cron-direct-name-resolution

Conversation

@fangliquanflq

Copy link
Copy Markdown
Contributor

What does this PR do?

When a user asks Hermes to run, pause, or resume a scheduled job by its exact name, the tool guidance currently tells the model to list every job first. That unnecessary visual scan can make the model miss a job that the server could resolve deterministically. This change directs non-destructive actions to the existing exact-name resolver while preserving list-first ID verification for removal.

Symptom

A request to trigger a job by its exact name can produce an unnecessary action='list' call followed by a false claim that the job does not exist, even when the target is present in the returned list.

Impact

Users can fail to run, pause, or resume existing scheduled jobs by name. The extra list response also adds avoidable context and latency.

Bug Cause

Trigger: tools/cronjob_tools.py:1452 / the cronjob schema description for job-management actions

Causal chain:

  1. A user requests run, pause, or resume using an exact job name.
  2. The schema's unconditional "always list first" wording routes the model through a full job listing and visual name scan instead of the deterministic resolver.
  3. The model can miss the target and report that it does not exist rather than execute the requested action.

Why it is wrong: The server already accepts a job ID or exact case-insensitive name for these non-destructive actions and reports missing or ambiguous names explicitly.

Working sibling / contrast: Removal is destructive, so listing jobs and verifying the concrete ID remains the appropriate safe path.

Ruled out: The runtime resolver is not missing or nondeterministic; existing tool tests verify exact-name resolution, ambiguity handling, and not-found behavior. The defect is the broader schema guidance that discourages using that resolver.

Fix

The schema now tells run, pause, and resume callers to pass an exact job name directly and list only after a not-found or ambiguity response. Separate removal guidance still requires listing first and using a verified job ID. Runtime schema-contract tests cover both requirements.

Related Issue

Closes #83470

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/cronjob_tools.py - scope list-first safety guidance to removal and document exact-name resolution for non-destructive actions.
  • tests/cron/test_cronjob_schema.py - add runtime schema-contract coverage for direct name lookup and destructive removal safety.

How to Test

  1. Import CRONJOB_SCHEMA and verify its description directs run, pause, and resume to exact case-insensitive name resolution without a preliminary list call.
  2. Verify the removal-specific guidance still requires action='list', a verified ID, and no guessed deletion target.
  3. Run the focused tool and schema suites:
scripts/run_tests.sh tests/tools/test_cronjob_tools.py
scripts/run_tests.sh tests/cron/test_cronjob_schema.py

Expected result: 68 tool tests and 3 schema tests pass.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the repository test entry point on the relevant tests and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 10

Documentation & Housekeeping

  • Relevant documentation updates are N/A because the agent-facing schema is the documented behavior changed here
  • cli-config.yaml.example updates are N/A because no config keys changed
  • CONTRIBUTING.md and AGENTS.md updates are N/A because no architecture or workflow changed
  • I've considered cross-platform impact; this schema-only guidance behaves identically across platforms
  • I've updated tool descriptions/schemas for the changed tool guidance

Screenshots / Logs

N/A - focused automated tests exercise the imported runtime schema contract.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management labels Aug 10, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(cron): run named jobs without unnecessary list scans

  1. The schema promise matches the code: resolve_job_ref (cron/jobs.py:1817) resolves exact ID first, then case-insensitive name, raising AmbiguousJobReference on collisions — so the new "exact name (case-insensitive)" guidance is accurate. One thing to verify: the cronjob() handler (tools/cronjob_tools.py:1176) catches the not-found case with a friendly error, but confirm AmbiguousJobReference is also caught and surfaced as a helpful message listing the matching IDs (as the schema text promises "if ... ambiguous, the tool reports that") rather than propagating as an unhandled exception.

  2. Wording-locked tests: test_cronjob_schema_directs_non_destructive_actions_to_exact_name_lookup asserts exact phrases ("exact name", "case-insensitive", "do not call action='list' first") of the schema description — any rephrase of the guidance breaks the test with no behavior change. Per the repo's change-detector guidance, prefer asserting semantic intent or drop the wording pins.

  3. Same brittleness in test_cronjob_schema_keeps_list_first_safety_for_remove: description.index("for action='remove'") raises ValueError if that exact substring is ever reworded. A "for action='remove'" in description guard or a semantic check would be more robust.

  4. No blocking issues with the guidance change itself — keeping list-first for the destructive remove while allowing direct name resolution for run/pause/resume is a sensible split.

@fangliquanflq

fangliquanflq commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I checked each point against the current head and the original schema-test intent:

  1. Already addressed: cronjob() catches AmbiguousJobReference at tools/cronjob_tools.py:1175-1193 and returns success: false with the error plus structured matching IDs/names/schedules, so ambiguity does not escape as an unhandled exception. The not-found path remains the separate friendly response at lines 1194-1198.
  2. No change: these assertions guard the model-visible behavioral contract introduced by this PR—not a catalog/value snapshot. The exact concepts (exact-name lookup, case-insensitivity, the affected non-destructive actions, and explicitly skipping a preliminary list) are the behavior that prevents the reported unnecessary scan. Replacing them with runtime resolver tests would not cover that instructional contract; this file was originally added specifically to guard load-bearing description text for description-driven models (issue cronjob tool fails to include required schedule parameter when using Grok models #32427 / PR fix(cron): clarify schedule is required for create in tool schema #32448).
  3. No change: the index anchor intentionally scopes the destructive-action assertions to the removal paragraph. If that explicit removal guidance disappears or is reworded so the anchor is no longer present, the safety contract should fail. Adding a separate membership guard would only change the assertion error shape, not make the contract less wording-dependent.
  4. Agreed; no action needed. The list-first rule remains limited to removal, while run/pause/resume use direct deterministic resolution.

Verification: scripts/run_tests.sh tests/cron/test_cronjob_schema.py (3 passed) and scripts/run_tests.sh tests/cron/test_jobs.py -k 'resolve or ambiguous' (2 passed). All required checks on the current PR head are also successful.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Confirmed — AmbiguousJobReference handling and schema-test intent confirmed at head. No further blockers.

@fangliquanflq

Copy link
Copy Markdown
Contributor Author

Thanks for confirming. No further code changes are needed: the current head catches AmbiguousJobReference and returns the matching job IDs, while the schema tests intentionally protect the model-visible direct-name and removal-safety contracts. The previously reported focused tests passed (3 schema tests and 2 resolver/ambiguity tests), and the current required CI checks are successful.

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 P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tools/cronjob: schema's 'always list first' guidance over-generalizes from remove to run/pause/resume

3 participants