mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-27 06:11:40 +00:00
feat(cron): support name-based lookup for job operations
Cron mutation operations (run/pause/resume/remove) and 'hermes cron edit'
now accept a job name in addition to the hex ID, with case-insensitive
matching. Before this, 'hermes cron run my_job_name' died with
'Job with ID my_job_name not found' and forced the user to look up the
hex ID first.
The original PR matched by name but silently picked the first match when
two jobs shared a name. This version refuses to act on an ambiguous name
and surfaces every matching job (id, name, schedule, next_run_at) so the
caller can pick a specific ID.
- cron/jobs.py:
- get_job() stays ID-only (preserves existing call-site semantics for
web_server/api_server/curator/scheduler/test code that always passes
real IDs).
- resolve_job_ref() is the new name-or-ID resolver, used by pause/
resume/trigger/remove_job. Exact ID match wins over a name match
even if a different job's name happens to equal that ID. Ambiguous
name match raises AmbiguousJobReference with all candidate IDs.
- tools/cronjob_tools.py: dispatch site uses resolve_job_ref, surfaces
ambiguous matches as a structured error with the matching IDs.
- hermes_cli/cron.py: 'cron edit' uses resolve_job_ref so editing by
name works and ambiguous names are reported with IDs.
- tests/cron/test_jobs.py: new TestResolveJobRef covering ID match,
case-insensitive name match, ID-wins-over-name, ambiguous refusal,
and that pause/resume/trigger/remove all refuse on ambiguity.
Closes #2627
This commit is contained in:
parent
05d9f641c0
commit
6682f91b80
4 changed files with 176 additions and 16 deletions
67
cron/jobs.py
67
cron/jobs.py
|
|
@ -645,6 +645,44 @@ def get_job(job_id: str) -> Optional[Dict[str, Any]]:
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
class AmbiguousJobReference(LookupError):
|
||||||
|
"""Raised when a job name matches more than one job."""
|
||||||
|
|
||||||
|
def __init__(self, ref: str, matches: List[Dict[str, Any]]):
|
||||||
|
self.ref = ref
|
||||||
|
self.matches = matches
|
||||||
|
ids = ", ".join(m["id"] for m in matches)
|
||||||
|
super().__init__(
|
||||||
|
f"Job name '{ref}' is ambiguous — matches {len(matches)} jobs: {ids}. "
|
||||||
|
f"Use the job ID instead."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def resolve_job_ref(ref: str) -> Optional[Dict[str, Any]]:
|
||||||
|
"""Resolve a job reference (ID or name) to a job record.
|
||||||
|
|
||||||
|
- Exact ID match wins (works even if a different job's name equals this ID).
|
||||||
|
- Otherwise, case-insensitive name match.
|
||||||
|
- If a name matches more than one job, raises AmbiguousJobReference so the
|
||||||
|
caller can surface the matching IDs rather than silently picking one.
|
||||||
|
"""
|
||||||
|
if not ref:
|
||||||
|
return None
|
||||||
|
jobs = load_jobs()
|
||||||
|
for job in jobs:
|
||||||
|
if job["id"] == ref:
|
||||||
|
return _normalize_job_record(job)
|
||||||
|
ref_lower = ref.lower()
|
||||||
|
name_matches = [j for j in jobs if (j.get("name") or "").lower() == ref_lower]
|
||||||
|
if not name_matches:
|
||||||
|
return None
|
||||||
|
if len(name_matches) > 1:
|
||||||
|
raise AmbiguousJobReference(
|
||||||
|
ref, [_normalize_job_record(j) for j in name_matches]
|
||||||
|
)
|
||||||
|
return _normalize_job_record(name_matches[0])
|
||||||
|
|
||||||
|
|
||||||
def list_jobs(include_disabled: bool = False) -> List[Dict[str, Any]]:
|
def list_jobs(include_disabled: bool = False) -> List[Dict[str, Any]]:
|
||||||
"""List all jobs, optionally including disabled ones."""
|
"""List all jobs, optionally including disabled ones."""
|
||||||
jobs = [_normalize_job_record(j) for j in load_jobs()]
|
jobs = [_normalize_job_record(j) for j in load_jobs()]
|
||||||
|
|
@ -702,9 +740,12 @@ def update_job(job_id: str, updates: Dict[str, Any]) -> Optional[Dict[str, Any]]
|
||||||
|
|
||||||
|
|
||||||
def pause_job(job_id: str, reason: Optional[str] = None) -> Optional[Dict[str, Any]]:
|
def pause_job(job_id: str, reason: Optional[str] = None) -> Optional[Dict[str, Any]]:
|
||||||
"""Pause a job without deleting it."""
|
"""Pause a job without deleting it. Accepts a job ID or name."""
|
||||||
|
job = resolve_job_ref(job_id)
|
||||||
|
if not job:
|
||||||
|
return None
|
||||||
return update_job(
|
return update_job(
|
||||||
job_id,
|
job["id"],
|
||||||
{
|
{
|
||||||
"enabled": False,
|
"enabled": False,
|
||||||
"state": "paused",
|
"state": "paused",
|
||||||
|
|
@ -715,14 +756,14 @@ def pause_job(job_id: str, reason: Optional[str] = None) -> Optional[Dict[str, A
|
||||||
|
|
||||||
|
|
||||||
def resume_job(job_id: str) -> Optional[Dict[str, Any]]:
|
def resume_job(job_id: str) -> Optional[Dict[str, Any]]:
|
||||||
"""Resume a paused job and compute the next future run from now."""
|
"""Resume a paused job and compute the next future run from now. Accepts a job ID or name."""
|
||||||
job = get_job(job_id)
|
job = resolve_job_ref(job_id)
|
||||||
if not job:
|
if not job:
|
||||||
return None
|
return None
|
||||||
|
|
||||||
next_run_at = compute_next_run(job["schedule"])
|
next_run_at = compute_next_run(job["schedule"])
|
||||||
return update_job(
|
return update_job(
|
||||||
job_id,
|
job["id"],
|
||||||
{
|
{
|
||||||
"enabled": True,
|
"enabled": True,
|
||||||
"state": "scheduled",
|
"state": "scheduled",
|
||||||
|
|
@ -734,12 +775,12 @@ def resume_job(job_id: str) -> Optional[Dict[str, Any]]:
|
||||||
|
|
||||||
|
|
||||||
def trigger_job(job_id: str) -> Optional[Dict[str, Any]]:
|
def trigger_job(job_id: str) -> Optional[Dict[str, Any]]:
|
||||||
"""Schedule a job to run on the next scheduler tick."""
|
"""Schedule a job to run on the next scheduler tick. Accepts a job ID or name."""
|
||||||
job = get_job(job_id)
|
job = resolve_job_ref(job_id)
|
||||||
if not job:
|
if not job:
|
||||||
return None
|
return None
|
||||||
return update_job(
|
return update_job(
|
||||||
job_id,
|
job["id"],
|
||||||
{
|
{
|
||||||
"enabled": True,
|
"enabled": True,
|
||||||
"state": "scheduled",
|
"state": "scheduled",
|
||||||
|
|
@ -751,14 +792,18 @@ def trigger_job(job_id: str) -> Optional[Dict[str, Any]]:
|
||||||
|
|
||||||
|
|
||||||
def remove_job(job_id: str) -> bool:
|
def remove_job(job_id: str) -> bool:
|
||||||
"""Remove a job by ID."""
|
"""Remove a job by ID or name."""
|
||||||
|
job = resolve_job_ref(job_id)
|
||||||
|
if not job:
|
||||||
|
return False
|
||||||
|
canonical_id = job["id"]
|
||||||
jobs = load_jobs()
|
jobs = load_jobs()
|
||||||
original_len = len(jobs)
|
original_len = len(jobs)
|
||||||
jobs = [j for j in jobs if j["id"] != job_id]
|
jobs = [j for j in jobs if j["id"] != canonical_id]
|
||||||
if len(jobs) < original_len:
|
if len(jobs) < original_len:
|
||||||
save_jobs(jobs)
|
save_jobs(jobs)
|
||||||
# Clean up output directory to prevent orphaned dirs accumulating
|
# Clean up output directory to prevent orphaned dirs accumulating
|
||||||
job_output_dir = OUTPUT_DIR / job_id
|
job_output_dir = OUTPUT_DIR / canonical_id
|
||||||
if job_output_dir.exists():
|
if job_output_dir.exists():
|
||||||
shutil.rmtree(job_output_dir)
|
shutil.rmtree(job_output_dir)
|
||||||
return True
|
return True
|
||||||
|
|
|
||||||
|
|
@ -196,9 +196,15 @@ def cron_create(args):
|
||||||
|
|
||||||
|
|
||||||
def cron_edit(args):
|
def cron_edit(args):
|
||||||
from cron.jobs import get_job
|
from cron.jobs import AmbiguousJobReference, resolve_job_ref
|
||||||
|
|
||||||
job = get_job(args.job_id)
|
try:
|
||||||
|
job = resolve_job_ref(args.job_id)
|
||||||
|
except AmbiguousJobReference as exc:
|
||||||
|
print(color(str(exc), Colors.RED))
|
||||||
|
for m in exc.matches:
|
||||||
|
print(f" {m['id']} (name: {m.get('name')!r})")
|
||||||
|
return 1
|
||||||
if not job:
|
if not job:
|
||||||
print(color(f"Job not found: {args.job_id}", Colors.RED))
|
print(color(f"Job not found: {args.job_id}", Colors.RED))
|
||||||
return 1
|
return 1
|
||||||
|
|
|
||||||
|
|
@ -321,6 +321,93 @@ class TestPauseResumeJob:
|
||||||
assert resumed["paused_reason"] is None
|
assert resumed["paused_reason"] is None
|
||||||
|
|
||||||
|
|
||||||
|
class TestResolveJobRef:
|
||||||
|
"""Name-based job lookup for CLI/tool callers (PR #2627, @buntingszn)."""
|
||||||
|
|
||||||
|
def test_resolve_by_exact_id(self, tmp_cron_dir):
|
||||||
|
from cron.jobs import resolve_job_ref
|
||||||
|
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
assert resolve_job_ref(job["id"])["id"] == job["id"]
|
||||||
|
|
||||||
|
def test_resolve_by_name(self, tmp_cron_dir):
|
||||||
|
from cron.jobs import resolve_job_ref
|
||||||
|
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
assert resolve_job_ref("alpha")["id"] == job["id"]
|
||||||
|
|
||||||
|
def test_resolve_by_name_case_insensitive(self, tmp_cron_dir):
|
||||||
|
from cron.jobs import resolve_job_ref
|
||||||
|
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="MyJob")
|
||||||
|
assert resolve_job_ref("myjob")["id"] == job["id"]
|
||||||
|
assert resolve_job_ref("MYJOB")["id"] == job["id"]
|
||||||
|
|
||||||
|
def test_resolve_returns_none_when_not_found(self, tmp_cron_dir):
|
||||||
|
from cron.jobs import resolve_job_ref
|
||||||
|
|
||||||
|
create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
assert resolve_job_ref("does-not-exist") is None
|
||||||
|
assert resolve_job_ref("") is None
|
||||||
|
|
||||||
|
def test_resolve_id_wins_over_name(self, tmp_cron_dir):
|
||||||
|
"""If a job's name happens to equal another job's ID, ID match wins."""
|
||||||
|
from cron.jobs import resolve_job_ref
|
||||||
|
|
||||||
|
j1 = create_job(prompt="A", schedule="1h")
|
||||||
|
# Create a second job whose name is j1's ID
|
||||||
|
j2 = create_job(prompt="B", schedule="1h", name=j1["id"])
|
||||||
|
# Looking up j1["id"] must return j1, not the colliding-name job j2
|
||||||
|
assert resolve_job_ref(j1["id"])["id"] == j1["id"]
|
||||||
|
assert resolve_job_ref(j1["id"])["id"] != j2["id"]
|
||||||
|
|
||||||
|
def test_resolve_ambiguous_name_raises(self, tmp_cron_dir):
|
||||||
|
"""Two jobs sharing a name → refuse to pick, surface both IDs."""
|
||||||
|
from cron.jobs import AmbiguousJobReference, resolve_job_ref
|
||||||
|
|
||||||
|
j1 = create_job(prompt="A", schedule="1h", name="dup")
|
||||||
|
j2 = create_job(prompt="B", schedule="1h", name="dup")
|
||||||
|
with pytest.raises(AmbiguousJobReference) as exc_info:
|
||||||
|
resolve_job_ref("dup")
|
||||||
|
ids = {m["id"] for m in exc_info.value.matches}
|
||||||
|
assert ids == {j1["id"], j2["id"]}
|
||||||
|
# Error message mentions both IDs so the user can pick one
|
||||||
|
assert j1["id"] in str(exc_info.value)
|
||||||
|
assert j2["id"] in str(exc_info.value)
|
||||||
|
|
||||||
|
def test_trigger_by_name(self, tmp_cron_dir):
|
||||||
|
from cron.jobs import trigger_job
|
||||||
|
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
result = trigger_job("alpha")
|
||||||
|
assert result is not None
|
||||||
|
assert result["id"] == job["id"]
|
||||||
|
|
||||||
|
def test_pause_by_name(self, tmp_cron_dir):
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
result = pause_job("alpha", reason="manual")
|
||||||
|
assert result is not None
|
||||||
|
assert result["id"] == job["id"]
|
||||||
|
assert result["state"] == "paused"
|
||||||
|
|
||||||
|
def test_remove_by_name(self, tmp_cron_dir):
|
||||||
|
job = create_job(prompt="A", schedule="1h", name="alpha")
|
||||||
|
assert remove_job("alpha") is True
|
||||||
|
assert get_job(job["id"]) is None
|
||||||
|
|
||||||
|
def test_mutations_refuse_ambiguous_name(self, tmp_cron_dir):
|
||||||
|
"""pause/resume/trigger/remove must refuse to act on an ambiguous name."""
|
||||||
|
from cron.jobs import AmbiguousJobReference, trigger_job
|
||||||
|
|
||||||
|
create_job(prompt="A", schedule="1h", name="dup")
|
||||||
|
create_job(prompt="B", schedule="1h", name="dup")
|
||||||
|
for fn in (pause_job, resume_job, trigger_job):
|
||||||
|
with pytest.raises(AmbiguousJobReference):
|
||||||
|
fn("dup")
|
||||||
|
with pytest.raises(AmbiguousJobReference):
|
||||||
|
remove_job("dup")
|
||||||
|
|
||||||
|
|
||||||
class TestMarkJobRun:
|
class TestMarkJobRun:
|
||||||
def test_increments_completed(self, tmp_cron_dir):
|
def test_increments_completed(self, tmp_cron_dir):
|
||||||
job = create_job(prompt="Test", schedule="every 1h")
|
job = create_job(prompt="Test", schedule="every 1h")
|
||||||
|
|
|
||||||
|
|
@ -21,12 +21,14 @@ logger = logging.getLogger(__name__)
|
||||||
sys.path.insert(0, str(Path(__file__).parent.parent))
|
sys.path.insert(0, str(Path(__file__).parent.parent))
|
||||||
|
|
||||||
from cron.jobs import (
|
from cron.jobs import (
|
||||||
|
AmbiguousJobReference,
|
||||||
create_job,
|
create_job,
|
||||||
get_job,
|
get_job,
|
||||||
list_jobs,
|
list_jobs,
|
||||||
parse_schedule,
|
parse_schedule,
|
||||||
pause_job,
|
pause_job,
|
||||||
remove_job,
|
remove_job,
|
||||||
|
resolve_job_ref,
|
||||||
resume_job,
|
resume_job,
|
||||||
trigger_job,
|
trigger_job,
|
||||||
update_job,
|
update_job,
|
||||||
|
|
@ -393,12 +395,32 @@ def cronjob(
|
||||||
if not job_id:
|
if not job_id:
|
||||||
return tool_error(f"job_id is required for action '{normalized}'", success=False)
|
return tool_error(f"job_id is required for action '{normalized}'", success=False)
|
||||||
|
|
||||||
job = get_job(job_id)
|
try:
|
||||||
if not job:
|
job = resolve_job_ref(job_id)
|
||||||
|
except AmbiguousJobReference as exc:
|
||||||
return json.dumps(
|
return json.dumps(
|
||||||
{"success": False, "error": f"Job with ID '{job_id}' not found. Use cronjob(action='list') to inspect jobs."},
|
{
|
||||||
|
"success": False,
|
||||||
|
"error": str(exc),
|
||||||
|
"matches": [
|
||||||
|
{
|
||||||
|
"id": m["id"],
|
||||||
|
"name": m.get("name"),
|
||||||
|
"schedule": m.get("schedule_display"),
|
||||||
|
"next_run_at": m.get("next_run_at"),
|
||||||
|
}
|
||||||
|
for m in exc.matches
|
||||||
|
],
|
||||||
|
},
|
||||||
indent=2,
|
indent=2,
|
||||||
)
|
)
|
||||||
|
if not job:
|
||||||
|
return json.dumps(
|
||||||
|
{"success": False, "error": f"Job with ID or name '{job_id}' not found. Use cronjob(action='list') to inspect jobs."},
|
||||||
|
indent=2,
|
||||||
|
)
|
||||||
|
# Resolve to canonical ID (supports name-based lookup)
|
||||||
|
job_id = job["id"]
|
||||||
|
|
||||||
if normalized == "remove":
|
if normalized == "remove":
|
||||||
removed = remove_job(job_id)
|
removed = remove_job(job_id)
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue