diff --git a/cron/jobs.py b/cron/jobs.py index 6b3bc0e66f9..c5da32d44d5 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -645,6 +645,44 @@ def get_job(job_id: str) -> Optional[Dict[str, Any]]: 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]]: """List all jobs, optionally including disabled ones.""" 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]]: - """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( - job_id, + job["id"], { "enabled": False, "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]]: - """Resume a paused job and compute the next future run from now.""" - job = get_job(job_id) + """Resume a paused job and compute the next future run from now. Accepts a job ID or name.""" + job = resolve_job_ref(job_id) if not job: return None next_run_at = compute_next_run(job["schedule"]) return update_job( - job_id, + job["id"], { "enabled": True, "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]]: - """Schedule a job to run on the next scheduler tick.""" - job = get_job(job_id) + """Schedule a job to run on the next scheduler tick. Accepts a job ID or name.""" + job = resolve_job_ref(job_id) if not job: return None return update_job( - job_id, + job["id"], { "enabled": True, "state": "scheduled", @@ -751,14 +792,18 @@ def trigger_job(job_id: str) -> Optional[Dict[str, Any]]: 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() 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: save_jobs(jobs) # 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(): shutil.rmtree(job_output_dir) return True diff --git a/hermes_cli/cron.py b/hermes_cli/cron.py index adf4f0c0927..7bff9c6b87b 100644 --- a/hermes_cli/cron.py +++ b/hermes_cli/cron.py @@ -196,9 +196,15 @@ def cron_create(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: print(color(f"Job not found: {args.job_id}", Colors.RED)) return 1 diff --git a/tests/cron/test_jobs.py b/tests/cron/test_jobs.py index af42ca444b2..16c56cd6220 100644 --- a/tests/cron/test_jobs.py +++ b/tests/cron/test_jobs.py @@ -321,6 +321,93 @@ class TestPauseResumeJob: 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: def test_increments_completed(self, tmp_cron_dir): job = create_job(prompt="Test", schedule="every 1h") diff --git a/tools/cronjob_tools.py b/tools/cronjob_tools.py index e63b60047ac..3c29431484d 100644 --- a/tools/cronjob_tools.py +++ b/tools/cronjob_tools.py @@ -21,12 +21,14 @@ logger = logging.getLogger(__name__) sys.path.insert(0, str(Path(__file__).parent.parent)) from cron.jobs import ( + AmbiguousJobReference, create_job, get_job, list_jobs, parse_schedule, pause_job, remove_job, + resolve_job_ref, resume_job, trigger_job, update_job, @@ -393,12 +395,32 @@ def cronjob( if not job_id: return tool_error(f"job_id is required for action '{normalized}'", success=False) - job = get_job(job_id) - if not job: + try: + job = resolve_job_ref(job_id) + except AmbiguousJobReference as exc: 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, ) + 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": removed = remove_job(job_id)