diff --git a/docs/cron.md b/docs/cron.md index 9b9cf8b6..4771d7ae 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -69,6 +69,34 @@ Notes: - `show_session_label` is **restart-only**: changing it and reloading has no effect until the daemon restarts. +### Turning a job on and off + +Each cron job has an on/off switch in the web UI (**Cron Jobs** — in the job list +and on the selected job's card). It is a thin wrapper over the reload above: + +`POST /api/cron/jobs//enable` · `POST /api/cron/jobs//disable` + +Both write `enabled:` for that one job in whichever file it was loaded from, then +reload, so the change survives a restart instead of lasting until the next one. + +- Only the flag is rewritten — the scalar after `enabled:`, or one inserted line. + Comments, key order and formatting elsewhere in the file are preserved, and a + job that never had an `enabled:` key does not gain one until you toggle it. +- The write and the reload are **atomic together**. If the reload is refused — + because of another job's typo, say — the file is put back, so it can never + claim a job is off while the scheduler keeps firing it. +- A job defined in both files is edited in the one that **won** the merge + (`jobs.yaml`), leaving the shadowed `system.yaml` copy alone. +- **Source runners have no switch.** They exist because their `sync` section is + enabled; turn them off there. +- Under [lockdown](config.md#lockdown-remote-only-read-only) the cron files are + reviewed config, so the switch is disabled and the API answers `403`. Change + `enabled:` in the config repo and open a PR. + +Failures come back as `404` (no such job), `403` (lockdown) or `400` — the last +covering a file this edit cannot be expressed in, such as a job written in flow +style (`- {id: x, ...}`) that has no `enabled:` key to overwrite. + ## Job Definition ```yaml diff --git a/nerve/cron/jobs.py b/nerve/cron/jobs.py index c43592d2..bd5ae13a 100644 --- a/nerve/cron/jobs.py +++ b/nerve/cron/jobs.py @@ -344,6 +344,199 @@ def load_jobs( return jobs +class JobEditError(Exception): + """A cron file could not be edited in place, with the reason why. + + Carries an operator-facing message: every raise site names the file and what + about it defeated the edit, because the answer is always "go and change that + line by hand" and the message is the only thing that says which line. + """ + + +def _find_jobs_sequence(root: yaml.Node, jobs_file: Path) -> yaml.SequenceNode: + """The node holding the job list, for either shape :func:`load_jobs` accepts. + + ``jobs:`` under a mapping is the documented form; a bare top-level list is + the one old installs still have (see :func:`load_jobs`). Both are read at + startup, so both have to be editable — supporting only the first would give + those installs a toggle that returns success and changes nothing. + """ + if isinstance(root, yaml.SequenceNode): + return root + if isinstance(root, yaml.MappingNode): + for key, value in root.value: + if isinstance(key, yaml.ScalarNode) and key.value == "jobs": + if not isinstance(value, yaml.SequenceNode): + raise JobEditError( + f"{jobs_file}: 'jobs' is not a list" + ) + return value + raise JobEditError(f"{jobs_file}: no 'jobs' list to edit") + + +def _mapping_entry( + mapping: yaml.MappingNode, name: str, +) -> tuple[yaml.ScalarNode, yaml.Node] | None: + """The ``(key, value)`` node pair for *name*, or ``None`` if absent.""" + for key, value in mapping.value: + if isinstance(key, yaml.ScalarNode) and key.value == name: + return key, value + return None + + +def set_job_enabled_in_file( + jobs_file: Path, job_id: str, enabled: bool, +) -> bool: + """Flip one job's ``enabled`` flag in *jobs_file*. Returns whether it changed. + + Rewrites the smallest span of text that carries the answer — the scalar after + ``enabled:``, or a single inserted line — rather than re-dumping the + document. :func:`save_jobs` is the other option and the wrong one for a + toggle: ``safe_dump`` cannot round-trip comments, so pausing one job would + silently delete every note in a hand-written cron file and materialize each + omitted default as an explicit key. The same reasoning, and the same choice, + as the settings writer in :mod:`nerve.bootstrap`. + + Locating the span is left to the parser. ``yaml.compose`` gives every node + the offsets it occupied in the source, so the job is found by structure and + the edit lands at a real position — where matching ``enabled:`` by text or + indentation would be guessing, and would eventually guess on a job whose + prompt happens to contain the word. + + The result is parsed and compared against the original before anything is + written: the requested flag must have moved and nothing else may have. A + surgical text edit that type-checks is still a text edit, and the cheap + structural check is what separates "the file now says what I meant" from + "the old value is gone". + + Raises :class:`JobEditError` if the file can't be parsed, holds no such job, + or is shaped so the flag can't be placed. + """ + try: + text = jobs_file.read_text(encoding="utf-8") + except OSError as e: + raise JobEditError(f"Cannot read {jobs_file}: {e}") from e + + try: + root = yaml.compose(text) + except yaml.YAMLError as e: + raise JobEditError(f"Cannot parse {jobs_file}: {e}") from e + if root is None: + raise JobEditError(f"{jobs_file} is empty") + + sequence = _find_jobs_sequence(root, jobs_file) + + target: yaml.MappingNode | None = None + for item in sequence.value: + if not isinstance(item, yaml.MappingNode): + continue + found = _mapping_entry(item, "id") + if ( + found is not None + and isinstance(found[1], yaml.ScalarNode) + and found[1].value == job_id + ): + target = item + break + if target is None: + raise JobEditError(f"{jobs_file} defines no job {job_id!r}") + + literal = "true" if enabled else "false" + existing = _mapping_entry(target, "enabled") + + if existing is not None: + _, value_node = existing + if not isinstance(value_node, yaml.ScalarNode): + raise JobEditError( + f"{jobs_file}: job {job_id!r} has a non-scalar 'enabled' value " + "— edit it by hand" + ) + start, end = value_node.start_mark.index, value_node.end_mark.index + # Anything after `enabled:` on that line — a trailing comment most + # often — sits outside the scalar's span and survives untouched. + new_text = text[:start] + literal + text[end:] + else: + # No flag to overwrite, so one is added. It goes directly beneath `id:`, + # at the column the mapping's keys already use: the first key of a + # sequence item shares its line with the `- `, so taking the indent from + # `id` rather than from that line is what keeps the insert aligned with + # its siblings instead of with the dash. + if target.flow_style: + raise JobEditError( + f"{jobs_file}: job {job_id!r} is written in flow style " + "({...}) and has no 'enabled' key — add one by hand" + ) + id_key, id_value = _mapping_entry(target, "id") # type: ignore[misc] + indent = " " * id_key.start_mark.column + lines = text.splitlines(keepends=True) + at = id_value.end_mark.line + 1 + # A file whose last line has no newline would otherwise get the new key + # appended to it. + if at > 0 and lines[at - 1] and not lines[at - 1].endswith("\n"): + lines[at - 1] += "\n" + lines.insert(at, f"{indent}enabled: {literal}\n") + new_text = "".join(lines) + + if new_text == text: + return False + + _verify_only_enabled_changed(text, new_text, job_id, enabled, jobs_file) + + try: + jobs_file.write_text(new_text, encoding="utf-8") + except OSError as e: + raise JobEditError(f"Cannot write {jobs_file}: {e}") from e + return True + + +def _verify_only_enabled_changed( + before: str, after: str, job_id: str, enabled: bool, jobs_file: Path, +) -> None: + """Refuse an edit that did anything besides set *job_id*'s flag. + + Compares the two documents as data: the target job's ``enabled`` must now be + *enabled*, and every other key of every job — including the rest of the + target's — must be identical. A span computed from stale offsets, a second + job sharing the id, a truncating write: each shows up here as a diff nobody + asked for, before the file is touched. + """ + def jobs_of(text: str) -> list[dict]: + data = yaml.safe_load(text) or {} + raw = data.get("jobs", []) if isinstance(data, dict) else data + return [j for j in raw if isinstance(j, dict)] if isinstance(raw, list) else [] + + try: + old_jobs, new_jobs = jobs_of(before), jobs_of(after) + except yaml.YAMLError as e: + raise JobEditError( + f"{jobs_file}: editing job {job_id!r} would leave unparseable " + f"YAML ({e}) — file not written" + ) from e + + if len(old_jobs) != len(new_jobs): + raise JobEditError( + f"{jobs_file}: editing job {job_id!r} changed the job count " + f"({len(old_jobs)} → {len(new_jobs)}) — file not written" + ) + + for old, new in zip(old_jobs, new_jobs): + is_target = old.get("id") == job_id + if is_target and new.get("enabled") is not enabled: + raise JobEditError( + f"{jobs_file}: setting job {job_id!r} to enabled={enabled} " + f"produced {new.get('enabled')!r} — file not written" + ) + stripped_old = {k: v for k, v in old.items() if k != "enabled"} + stripped_new = {k: v for k, v in new.items() if k != "enabled"} + if stripped_old != stripped_new or ( + not is_target and old.get("enabled") != new.get("enabled") + ): + raise JobEditError( + f"{jobs_file}: editing job {job_id!r} would also change job " + f"{old.get('id')!r} — file not written" + ) + + def save_jobs(jobs: list[CronJob], jobs_file: Path) -> None: """Save cron jobs to a YAML file. diff --git a/nerve/cron/service.py b/nerve/cron/service.py index a1ccdd5a..7e48355c 100644 --- a/nerve/cron/service.py +++ b/nerve/cron/service.py @@ -9,6 +9,7 @@ import json import logging from datetime import datetime, timezone, tzinfo +from pathlib import Path from typing import TYPE_CHECKING from zoneinfo import ZoneInfo @@ -21,9 +22,11 @@ from nerve.config import ConfigError, NerveConfig from nerve.cron.jobs import ( CronJob, + JobEditError, describe_reserved_job_ids, is_reserved_job_id, load_jobs, + set_job_enabled_in_file, ) from nerve.db import Database @@ -427,6 +430,101 @@ async def _reload_locked(self) -> dict: GATE_REGISTRY.update(gates_before) raise + def job_source_file(self, job: CronJob) -> Path: + """The cron file *job* was loaded from, and the one a toggle writes. + + Follows the merge in :meth:`_load_merged_jobs`: a user job shadowing a + system job of the same id is the one that got scheduled, so it is also + the one an edit has to land in. Writing the system file there would + change the copy that loses and leave the running job as it was. + """ + if job.metadata.get("_source") == "system": + return self.config.cron.system_file + return self.config.cron.jobs_file + + def toggle_refusal(self, job: CronJob) -> str | None: + """Why this job's enabled flag can't be toggled from the API, or ``None``. + + Reported per job by :meth:`list_jobs` so the UI can render the control as + unavailable and say why, instead of offering a switch that fails when + pressed. Only lockdown refuses today, and it refuses by path: on a + migrated install the cron files are inside the reviewed config subtree, + where the answer is a reviewed PR rather than a live write. + """ + from nerve.config import tracked_config_write_refusal + + return tracked_config_write_refusal(self.job_source_file(job)) + + async def set_job_enabled(self, job_id: str, enabled: bool) -> dict: + """Persist *job_id*'s enabled flag, then apply it to the scheduler. + + The pair has to be atomic in both directions, because the file is what + the next reload and the next restart read, and the scheduler is what + actually fires. So the write and the reload happen under the reload lock, + and a reload that refuses puts the file back: the alternative is a job + the YAML calls disabled that goes on running until something else + reloads, which is the one outcome an off switch may not produce. + + A reload can refuse for reasons that have nothing to do with this job — + another job's schedule typo, a malformed file — and that is exactly when + the rollback earns its place. ``enabled`` is applied by + :meth:`_reload_from_disk` like any other field: newly disabled jobs are + unscheduled, newly enabled ones are given a trigger. + + Returns this job's resulting state, whether the file needed changing, and + the reload summary under ``reload``. Raises ``ValueError`` for an unknown + job, :class:`JobEditError` if the file cannot carry the change, and + :class:`nerve.config.LockdownError` if it is tracked config. + """ + from nerve.config import ensure_path_not_tracked_config + + job = next((j for j in self._jobs if j.id == job_id), None) + if job is None: + raise ValueError(f"No such cron job: {job_id!r}") + + target = self.job_source_file(job) + ensure_path_not_tracked_config(target, f"toggle cron job {job_id!r} in") + + async with self._reload_lock: + try: + before = target.read_text(encoding="utf-8") + except OSError as e: + raise JobEditError(f"Cannot read {target}: {e}") from e + + changed = set_job_enabled_in_file(target, job_id, enabled) + try: + summary = await self._reload_locked() + except BaseException: + # BaseException so a cancellation rolls back too — same reason + # _reload_locked restores the gate registry on one. + if changed: + try: + target.write_text(before, encoding="utf-8") + except OSError: + logger.exception( + "Cron toggle of %s failed and %s could not be " + "restored — the file now says enabled=%s while the " + "scheduler is unchanged", + job_id, target, enabled, + ) + raise + + logger.info( + "Cron job %s %s via API (%s)", + job_id, "enabled" if enabled else "disabled", + "file updated" if changed else "file already said so", + ) + return { + "job_id": job_id, + "enabled": enabled, + "changed": changed, + "file": str(target), + # Nested, not spread: reload()'s summary has an "enabled" of its own + # and it means the *count* of scheduled jobs. Spread, it would quietly + # replace this job's new state with a number. + "reload": summary, + } + async def _reload_from_disk( self, gates_before: dict[str, type["CronGate"]], ) -> dict: @@ -1457,6 +1555,7 @@ async def list_jobs(self) -> list[dict]: "gates": [gate.describe() for gate in job.gates], "next_run": next_run.isoformat() if next_run else None, "last_session_id": last_session_id, + "toggle_refusal": self.toggle_refusal(job), }) # Include source runners @@ -1475,6 +1574,12 @@ async def list_jobs(self) -> list[dict]: "enabled": True, "next_run": next_run.isoformat() if next_run else None, "last_session_id": None, + # A source runner exists because its sync section is configured + # and enabled; there is no per-runner flag in the cron files for + # a toggle to write. + "toggle_refusal": ( + "Source runners follow the sync config, not cron YAML" + ), }) return result diff --git a/nerve/gateway/routes/cron.py b/nerve/gateway/routes/cron.py index 49a9f1f0..7b8ac850 100644 --- a/nerve/gateway/routes/cron.py +++ b/nerve/gateway/routes/cron.py @@ -61,6 +61,47 @@ async def trigger_cron_job(job_id: str, user: dict = Depends(require_auth)): raise HTTPException(status_code=404, detail=str(e)) +@router.post("/api/cron/jobs/{job_id}/enable") +async def enable_cron_job(job_id: str, user: dict = Depends(require_auth)): + """Enable a cron job in its cron file and schedule it, without a restart.""" + return await _set_cron_job_enabled(job_id, True) + + +@router.post("/api/cron/jobs/{job_id}/disable") +async def disable_cron_job(job_id: str, user: dict = Depends(require_auth)): + """Disable a cron job in its cron file and unschedule it, without a restart.""" + return await _set_cron_job_enabled(job_id, False) + + +async def _set_cron_job_enabled(job_id: str, enabled: bool) -> dict: + """Shared body of the two toggle routes. + + Status codes follow what the caller can do about it: 404 for a job that is + not there, 403 when lockdown reserves the file for a reviewed PR, and 400 for + a file this edit cannot be expressed in (flow style, no ``jobs`` list) or a + reload the new config cannot satisfy. None of the three is worth a retry — + each is fixed by editing YAML. + """ + from nerve.config import ConfigError, LockdownError + from nerve.cron.jobs import JobEditError + from nerve.gateway.server import _cron_service + + if not _cron_service: + raise HTTPException(status_code=503, detail="Cron service not available") + + try: + return await _cron_service.set_job_enabled(job_id, enabled) + except LockdownError as e: + raise HTTPException(status_code=403, detail=str(e)) from e + # Before the bare ValueError below, not after: ConfigError subclasses + # ValueError (and InvalidScheduleError subclasses ConfigError), so the + # broad clause first would report a schedule typo as a missing job. + except (JobEditError, ConfigError) as e: + raise HTTPException(status_code=400, detail=str(e)) from e + except ValueError as e: + raise HTTPException(status_code=404, detail=str(e)) from e + + @router.post("/api/cron/jobs/{job_id}/rotate") async def rotate_cron_session(job_id: str, user: dict = Depends(require_auth)): """Force-rotate a persistent cron session's context.""" diff --git a/tests/test_cron_reload.py b/tests/test_cron_reload.py index 98429263..0dfa98a5 100644 --- a/tests/test_cron_reload.py +++ b/tests/test_cron_reload.py @@ -12,7 +12,7 @@ import yaml from nerve.cron.service import CronService -from nerve.cron.jobs import CronJob, is_reserved_job_id +from nerve.cron.jobs import CronJob, is_reserved_job_id, load_jobs def _write_jobs(path: Path, jobs: list[dict]) -> None: @@ -936,6 +936,366 @@ async def test_400_on_invalid_schedule(self, monkeypatch): assert "typo" in ei.value.detail +# A cron file as people actually write them: comments above, beside and between +# the jobs, a block-scalar prompt, and a flag that is absent on one job and +# present on another. Every one of those is something a full re-dump destroys. +_COMMENTED_FILE = """\ +# Parked templates — switch on when needed. +jobs: + # Proposes only; it never acts. + - id: planner + schedule: "0 9 * * *" # every morning + prompt: | + Review the board. + Mention enabled: true to be awkward. + enabled: false # flip me + + - id: sweeper + schedule: 2h + prompt: Sweep. +""" + + +class TestSetJobEnabledInFile: + """The YAML edit itself, independent of any scheduler.""" + + def test_flips_an_existing_flag_and_keeps_every_comment(self, tmp_path): + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text(_COMMENTED_FILE, encoding="utf-8") + + assert set_job_enabled_in_file(f, "planner", True) is True + out = f.read_text(encoding="utf-8") + for comment in ( + "# Parked templates — switch on when needed.", + "# Proposes only; it never acts.", + "# every morning", + "# flip me", + ): + assert comment in out, f"lost {comment!r}" + # The flag changed, and the comment that trailed it stayed put. + assert "enabled: true # flip me" in out + assert {j.id: j.enabled for j in load_jobs(f)} == { + "planner": True, "sweeper": True, + } + + def test_a_prompt_mentioning_the_key_is_not_the_key(self, tmp_path): + """The edit is located by parsing, so prose about `enabled:` is safe. + + This is the case that decides between parsing and pattern-matching: a + text search for the flag finds the prompt line first and corrupts the + prompt while reporting success. + """ + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text(_COMMENTED_FILE, encoding="utf-8") + set_job_enabled_in_file(f, "planner", True) + + planner = next(j for j in load_jobs(f) if j.id == "planner") + assert "Mention enabled: true to be awkward." in planner.prompt + assert planner.enabled is True + + def test_inserts_the_flag_when_the_job_has_none(self, tmp_path): + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text(_COMMENTED_FILE, encoding="utf-8") + + assert set_job_enabled_in_file(f, "sweeper", False) is True + assert {j.id: j.enabled for j in load_jobs(f)} == { + "planner": False, "sweeper": False, + } + # Aligned with its siblings, not with the `- ` that opens the item. + assert "\n enabled: false\n" in f.read_text(encoding="utf-8") + + def test_already_in_that_state_is_not_a_write(self, tmp_path): + """Reports no change and leaves the bytes alone, so the caller can skip + the reload and the rollback has nothing to undo.""" + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text(_COMMENTED_FILE, encoding="utf-8") + before = f.read_text(encoding="utf-8") + + assert set_job_enabled_in_file(f, "planner", False) is False + assert f.read_text(encoding="utf-8") == before + + def test_quoted_flag_becomes_a_real_boolean(self, tmp_path): + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text( + 'jobs:\n - id: q\n schedule: 1h\n prompt: hi\n' + ' enabled: "true"\n', + encoding="utf-8", + ) + set_job_enabled_in_file(f, "q", False) + assert load_jobs(f)[0].enabled is False + + def test_edits_a_bare_top_level_list(self, tmp_path): + """The shape old installs still have — see load_jobs' compat branch.""" + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text("- id: old\n schedule: 1h\n prompt: hi\n", encoding="utf-8") + assert set_job_enabled_in_file(f, "old", False) is True + assert load_jobs(f)[0].enabled is False + + def test_file_without_a_trailing_newline(self, tmp_path): + from nerve.cron.jobs import set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text("jobs:\n - id: t\n schedule: 1h\n prompt: hi", encoding="utf-8") + assert set_job_enabled_in_file(f, "t", False) is True + assert load_jobs(f)[0].enabled is False + + def test_replaces_a_flow_style_flag_but_refuses_to_add_one(self, tmp_path): + from nerve.cron.jobs import JobEditError, set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text( + "jobs:\n - {id: f, schedule: 1h, prompt: hi, enabled: true}\n", + encoding="utf-8", + ) + assert set_job_enabled_in_file(f, "f", False) is True + assert load_jobs(f)[0].enabled is False + + # With no key to overwrite there is no line to insert into, so it says so + # instead of guessing. + f.write_text( + "jobs:\n - {id: f, schedule: 1h, prompt: hi}\n", encoding="utf-8", + ) + with pytest.raises(JobEditError, match="flow style"): + set_job_enabled_in_file(f, "f", False) + + @pytest.mark.parametrize("content,match", [ + ("", "empty"), + ("crons:\n - id: x\n", "no 'jobs' list"), + ("jobs: {}\n", "not a list"), + ("jobs:\n - id: other\n schedule: 1h\n prompt: hi\n", "no job"), + ]) + def test_refuses_what_it_cannot_edit(self, tmp_path, content, match): + from nerve.cron.jobs import JobEditError, set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + f.write_text(content, encoding="utf-8") + with pytest.raises(JobEditError, match=match): + set_job_enabled_in_file(f, "wanted", True) + + def test_unparseable_file_is_refused_not_overwritten(self, tmp_path): + from nerve.cron.jobs import JobEditError, set_job_enabled_in_file + + f = tmp_path / "jobs.yaml" + broken = "jobs:\n - id: x\n bad indent: [\n" + f.write_text(broken, encoding="utf-8") + with pytest.raises(JobEditError, match="Cannot parse"): + set_job_enabled_in_file(f, "x", True) + assert f.read_text(encoding="utf-8") == broken + + +class TestSetJobEnabled: + """The service method: persist, apply, and roll back together.""" + + @pytest.mark.asyncio + async def test_disable_writes_the_file_and_unschedules(self, svc): + service, jobs_file = svc + _write_jobs(jobs_file, [_job_dict("j1"), _job_dict("j2")]) + await service.reload() + assert service.scheduler.get_job("j1") is not None + + result = await service.set_job_enabled("j1", False) + assert result["enabled"] is False and result["changed"] is True + + # Both halves: the file for the next restart, the scheduler for now. + assert {j.id: j.enabled for j in load_jobs(jobs_file)} == { + "j1": False, "j2": True, + } + assert service.scheduler.get_job("j1") is None + assert service.scheduler.get_job("j2") is not None + + # j2 keeps the *absence* of the key, rather than being handed an + # explicit `enabled: true` it never had. A re-dump of the document would + # have materialised one on every job in the file. + raw = {j["id"]: j for j in yaml.safe_load(jobs_file.read_text())["jobs"]} + assert raw["j1"]["enabled"] is False + assert "enabled" not in raw["j2"] + + @pytest.mark.asyncio + async def test_enable_writes_the_file_and_schedules(self, svc): + service, jobs_file = svc + _write_jobs(jobs_file, [_job_dict("j1", enabled=False)]) + await service.reload() + assert service.scheduler.get_job("j1") is None + + result = await service.set_job_enabled("j1", True) + assert result["enabled"] is True + assert service.scheduler.get_job("j1") is not None + assert load_jobs(jobs_file)[0].enabled is True + + @pytest.mark.asyncio + async def test_unknown_job_raises_value_error(self, svc): + service, _ = svc + with pytest.raises(ValueError, match="No such cron job"): + await service.set_job_enabled("nope", False) + + @pytest.mark.asyncio + async def test_response_enabled_is_the_flag_not_the_reload_count(self, svc): + """`reload()`'s summary has an `enabled` too, and it is a count. + + Spreading the summary into this response instead of nesting it replaces + the job's new state with the number of scheduled jobs — same key, wholly + different meaning, and a caller reading `enabled` gets a plausible + integer rather than the flag it asked to set. + """ + service, jobs_file = svc + _write_jobs(jobs_file, [_job_dict("j1"), _job_dict("j2")]) + await service.reload() + + result = await service.set_job_enabled("j1", False) + assert result["enabled"] is False + assert result["reload"]["enabled"] == 1 # j2 is still scheduled + + @pytest.mark.asyncio + async def test_a_refused_reload_puts_the_file_back(self, svc, monkeypatch): + """The rollback that keeps the two halves from disagreeing. + + A reload can fail for reasons that have nothing to do with the job being + toggled. If the file kept the new flag, it would claim the job is off + while the scheduler goes on firing it until something else reloads — + exactly the outcome an off switch may not produce. + """ + service, jobs_file = svc + _write_jobs(jobs_file, [_job_dict("j1")]) + await service.reload() + before = jobs_file.read_text(encoding="utf-8") + + from nerve.cron.service import InvalidScheduleError + monkeypatch.setattr( + service, "_reload_locked", + AsyncMock(side_effect=InvalidScheduleError("someone else's typo")), + ) + with pytest.raises(InvalidScheduleError): + await service.set_job_enabled("j1", False) + + assert jobs_file.read_text(encoding="utf-8") == before + assert service.scheduler.get_job("j1") is not None + + @pytest.mark.asyncio + async def test_toggle_targets_the_file_the_job_came_from(self, svc, tmp_path): + """A user job shadowing a system one is the copy that got scheduled, so + it is the copy the edit has to land in.""" + service, jobs_file = svc + system_file = service.config.cron.system_file + _write_jobs(system_file, [_job_dict("shared", schedule="4h")]) + _write_jobs(jobs_file, [_job_dict("shared", schedule="1h")]) + await service.reload() + + await service.set_job_enabled("shared", False) + + assert load_jobs(jobs_file)[0].enabled is False + # The losing copy is untouched. + assert load_jobs(system_file)[0].enabled is True + + @pytest.mark.asyncio + async def test_toggles_a_system_job_in_the_system_file(self, svc): + service, jobs_file = svc + system_file = service.config.cron.system_file + _write_jobs(system_file, [_job_dict("sys")]) + _write_jobs(jobs_file, [_job_dict("mine")]) + await service.reload() + + await service.set_job_enabled("sys", False) + assert load_jobs(system_file)[0].enabled is False + assert load_jobs(jobs_file)[0].enabled is True + + @pytest.mark.asyncio + async def test_list_jobs_reports_no_refusal_when_unlocked(self, svc): + service, jobs_file = svc + _write_jobs(jobs_file, [_job_dict("j1")]) + await service.reload() + listed = await service.list_jobs() + assert listed[0]["toggle_refusal"] is None + + +class TestToggleRoutes: + @pytest.mark.asyncio + async def test_enable_and_disable_call_through(self, monkeypatch): + import nerve.gateway.server as srv + + from nerve.gateway.routes.cron import disable_cron_job, enable_cron_job + + fake = MagicMock() + fake.set_job_enabled = AsyncMock(return_value={"job_id": "j1"}) + monkeypatch.setattr(srv, "_cron_service", fake, raising=False) + + await enable_cron_job("j1", user={}) + assert fake.set_job_enabled.await_args.args == ("j1", True) + await disable_cron_job("j1", user={}) + assert fake.set_job_enabled.await_args.args == ("j1", False) + + @pytest.mark.asyncio + async def test_503_when_no_service(self, monkeypatch): + import nerve.gateway.server as srv + from fastapi import HTTPException + + from nerve.gateway.routes.cron import disable_cron_job + + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + with pytest.raises(HTTPException) as ei: + await disable_cron_job("j1", user={}) + assert ei.value.status_code == 503 + + @pytest.mark.asyncio + @pytest.mark.parametrize("error,status", [ + (ValueError("No such cron job: 'j1'"), 404), + (None, 403), # LockdownError, built in the test body + (None, 400), # JobEditError / ConfigError, ditto + ]) + async def test_status_codes(self, monkeypatch, error, status): + import nerve.gateway.server as srv + from fastapi import HTTPException + + from nerve.config import ConfigError, LockdownError + from nerve.gateway.routes.cron import disable_cron_job + + if status == 403: + error = LockdownError("cannot write tracked config") + elif status == 400: + error = ConfigError("bad cron file") + + fake = MagicMock() + fake.set_job_enabled = AsyncMock(side_effect=error) + monkeypatch.setattr(srv, "_cron_service", fake, raising=False) + with pytest.raises(HTTPException) as ei: + await disable_cron_job("j1", user={}) + assert ei.value.status_code == status + + @pytest.mark.asyncio + async def test_schedule_typo_is_400_not_404(self, monkeypatch): + """ConfigError subclasses ValueError, so clause order decides this one. + + With the broad ValueError clause first, a reload refused over somebody + else's schedule typo would come back as "no such job". + """ + import nerve.gateway.server as srv + from fastapi import HTTPException + + from nerve.cron.service import InvalidScheduleError + from nerve.gateway.routes.cron import disable_cron_job + + fake = MagicMock() + fake.set_job_enabled = AsyncMock( + side_effect=InvalidScheduleError("Cron job 'other': bad minute"), + ) + monkeypatch.setattr(srv, "_cron_service", fake, raising=False) + with pytest.raises(HTTPException) as ei: + await disable_cron_job("j1", user={}) + assert ei.value.status_code == 400 + assert "other" in ei.value.detail + + class TestReservedIds: @pytest.mark.parametrize("job_id", [ "cleanup", "wakeup_sweep", diff --git a/tests/test_lockdown.py b/tests/test_lockdown.py index 4a2c3ab0..32f7eb68 100644 --- a/tests/test_lockdown.py +++ b/tests/test_lockdown.py @@ -1476,6 +1476,39 @@ def test_save_jobs_is_guarded(self, tmp_path, monkeypatch): with pytest.raises(LockdownError): save_jobs([], ws / "config" / "cron" / "jobs.yaml") + @pytest.mark.asyncio + async def test_cron_toggle_is_guarded_and_leaves_the_file_alone( + self, tmp_path, monkeypatch, + ): + """The UI's off switch writes tracked cron config, so lockdown owns it. + + Asserts the file as well as the raise: a guard that refuses after writing + is not a guard, and writing the flag is the whole point of the method. + """ + from unittest.mock import AsyncMock, MagicMock + + from nerve.cron.jobs import CronJob + from nerve.cron.service import CronService + + ws = tmp_path / "ws" + cron_dir = ws / "config" / "cron" + cron_dir.mkdir(parents=True) + jobs_file = cron_dir / "jobs.yaml" + original = "jobs:\n - id: j1\n schedule: 1h\n prompt: hi\n" + jobs_file.write_text(original, encoding="utf-8") + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + + config = MagicMock() + config.timezone = "UTC" + config.cron.jobs_file = jobs_file + config.cron.system_file = cron_dir / "system.yaml" + service = CronService(config, AsyncMock(), AsyncMock()) + service._jobs = [CronJob(id="j1", schedule="1h", prompt="hi")] + + with pytest.raises(LockdownError): + await service.set_job_enabled("j1", False) + assert jobs_file.read_text(encoding="utf-8") == original + @pytest.mark.asyncio async def test_task_route_cannot_be_pointed_at_tracked_config(self, tmp_path, monkeypatch): """``task["file_path"]`` is a stored path joined to the workspace. Today diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 0c7e89ad..46e9a0c7 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -630,6 +630,18 @@ export const api = { request(`/cron/jobs/${encodeURIComponent(jobId)}/trigger`, { method: 'POST' }), rotateCronJob: (jobId: string) => request(`/cron/jobs/${encodeURIComponent(jobId)}/rotate`, { method: 'POST' }), + setCronJobEnabled: (jobId: string, enabled: boolean) => + request<{ + job_id: string; + enabled: boolean; + /** False when the file already said so and only the reload ran. */ + changed: boolean; + file: string; + reload: { added: string[]; removed: string[]; updated: string[] }; + }>( + `/cron/jobs/${encodeURIComponent(jobId)}/${enabled ? 'enable' : 'disable'}`, + { method: 'POST' }, + ), // Skills listSkills: () => request<{ skills: any[] }>('/skills'), diff --git a/web/src/components/Cron/CronSidebar.tsx b/web/src/components/Cron/CronSidebar.tsx index 061eaca8..0e62adbb 100644 --- a/web/src/components/Cron/CronSidebar.tsx +++ b/web/src/components/Cron/CronSidebar.tsx @@ -1,7 +1,7 @@ import { Timer } from 'lucide-react'; import { useCronStore } from '../../stores/cronStore'; import { chatPath, jobLabel } from './utils'; -import { ChatLink, TriggerButton, JobTypeIcon } from './controls'; +import { ChatLink, EnabledSwitch, TriggerButton, JobTypeIcon } from './controls'; export function CronSidebar({ inDrawer = false, onSelect }: { inDrawer?: boolean; @@ -66,6 +66,20 @@ export function CronSidebar({ inDrawer = false, onSelect }: { )} + {/* + Cron jobs only — a source runner has no flag in the cron files, + so all it could show here is a permanently locked switch. + Revealed on hover like the others while the job is on, but kept + visible once it is off: that row is already dimmed, and a + switch you have to hover to find is a poor way back on. + */} + {job.type === 'cron' && ( + + + + )} diff --git a/web/src/components/Cron/JobInfoCard.tsx b/web/src/components/Cron/JobInfoCard.tsx index 0a35c2d1..d8bf44bb 100644 --- a/web/src/components/Cron/JobInfoCard.tsx +++ b/web/src/components/Cron/JobInfoCard.tsx @@ -1,7 +1,10 @@ import { Filter, FileText } from 'lucide-react'; import type { CronJob } from '../../stores/cronStore'; import { formatRelativeTime, formatSchedule } from './utils'; -import { ChatLink, JobTypeBadge, JobTypeIcon, RotateButton, TriggerButton } from './controls'; +import { + ChatLink, EnabledSwitch, JobTypeBadge, JobTypeIcon, RotateButton, + ToggleError, TriggerButton, +} from './controls'; export function JobInfoCard({ job }: { job: CronJob }) { return ( @@ -28,9 +31,13 @@ export function JobInfoCard({ job }: { job: CronJob }) { {job.last_session_id && } {job.enabled && job.session_mode === 'persistent' && } {job.enabled && } + {/* Last, and never hidden by `enabled`: it is the way back on. */} + + +
Schedule
diff --git a/web/src/components/Cron/controls.tsx b/web/src/components/Cron/controls.tsx index da6fd14d..4900e931 100644 --- a/web/src/components/Cron/controls.tsx +++ b/web/src/components/Cron/controls.tsx @@ -1,9 +1,9 @@ import { Link } from 'react-router-dom'; import { RotateCw, Play, Loader2, Clock, Inbox, MessageSquare, - CheckCircle2, XCircle, + CheckCircle2, XCircle, Lock, } from 'lucide-react'; -import { useCronStore } from '../../stores/cronStore'; +import { useCronStore, type CronJob } from '../../stores/cronStore'; import { chatPath } from './utils'; export function JobTypeIcon({ type }: { type: string }) { @@ -83,6 +83,66 @@ export function TriggerButton({ jobId, small = false }: { jobId: string; small?: ); } +/** + * On/off switch for a cron job's `enabled` flag. + * + * Same track-and-knob shape as the skills toggle, so the two read as one + * control rather than two conventions. Rendered as a real `switch` with + * `aria-checked` because that is what it is; the visual is unchanged. + * + * A job the server says cannot be toggled gets a locked, non-interactive + * switch whose tooltip is the server's reason — a control that is visibly + * unavailable beats one that looks live and fails on click. + */ +export function EnabledSwitch({ job }: { job: CronJob }) { + const { toggling, setJobEnabled } = useCronStore(); + const pending = toggling === job.id; + const refusal = job.toggle_refusal; + const locked = Boolean(refusal); + + const label = locked + ? refusal! + : `${job.enabled ? 'Disable' : 'Enable'} ${job.id}`; + + return ( + + + {locked && + ); +} + +/** The reason the last toggle of *jobId* was rejected, if it was. */ +export function ToggleError({ jobId }: { jobId: string }) { + const message = useCronStore(s => s.toggleErrors[jobId]); + if (!message) return null; + return ( +
+ + {message} +
+ ); +} + export function RotateButton({ jobId }: { jobId: string }) { const { rotating, rotateSession } = useCronStore(); const isRotating = rotating === jobId; diff --git a/web/src/stores/cronStore.test.ts b/web/src/stores/cronStore.test.ts new file mode 100644 index 00000000..2c5538cf --- /dev/null +++ b/web/src/stores/cronStore.test.ts @@ -0,0 +1,114 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { CronJob } from './cronStore'; + +vi.mock('../api/client', () => ({ + api: { + setCronJobEnabled: vi.fn(), + listCronJobs: vi.fn(), + getCronLogs: vi.fn(), + }, +})); + +const { api } = await import('../api/client'); +const { useCronStore } = await import('./cronStore'); + +function job(id: string, enabled = true, extra: Partial = {}): CronJob { + return { + id, type: 'cron', schedule: '1h', description: '', enabled, + next_run: enabled ? '2026-08-19T12:00:00Z' : null, + toggle_refusal: null, + ...extra, + }; +} + +/** What `request()` throws for a non-2xx response: ": ". */ +function httpError(status: number, detail: string): Error { + return new Error(`${status}: ${JSON.stringify({ detail })}`); +} + +beforeEach(() => { + // clearAllMocks resets calls but keeps implementations, so a rejection set by + // one test would leak into the next. Re-arm both mocks explicitly. + vi.clearAllMocks(); + useCronStore.setState({ + jobs: [job('planner'), job('sweeper', false)], + toggling: null, + toggleErrors: {}, + }); + vi.mocked(api.listCronJobs).mockResolvedValue({ jobs: [] }); + vi.mocked(api.setCronJobEnabled).mockResolvedValue({ + job_id: 'planner', enabled: false, changed: true, + file: '/ws/config/cron/jobs.yaml', + reload: { added: [], removed: [], updated: [] }, + }); +}); + +describe('setJobEnabled', () => { + it('calls the API with the requested state and reloads the list', async () => { + vi.mocked(api.listCronJobs).mockResolvedValue({ + jobs: [job('planner', false), job('sweeper', false)], + }); + + await useCronStore.getState().setJobEnabled('planner', false); + + expect(api.setCronJobEnabled).toHaveBeenCalledWith('planner', false); + // Reloaded rather than patched locally: the server also recomputed + // next_run, which a local flip would leave pointing at a stale time. + expect(api.listCronJobs).toHaveBeenCalled(); + expect(useCronStore.getState().jobs.find(j => j.id === 'planner')!.enabled) + .toBe(false); + expect(useCronStore.getState().toggling).toBeNull(); + }); + + it('records the server detail when the toggle is refused', async () => { + vi.mocked(api.setCronJobEnabled).mockRejectedValue( + httpError(403, 'Cannot toggle cron job in /ws/config/cron/jobs.yaml: tracked config'), + ); + + await useCronStore.getState().setJobEnabled('planner', false); + + // The detail, not the raw "403: {...}" envelope — a lockdown refusal is an + // expected answer and has to be readable. + expect(useCronStore.getState().toggleErrors.planner) + .toBe('Cannot toggle cron job in /ws/config/cron/jobs.yaml: tracked config'); + expect(useCronStore.getState().toggling).toBeNull(); + // The switch must not appear to have moved. + expect(api.listCronJobs).not.toHaveBeenCalled(); + }); + + it('clears a previous error once a retry succeeds', async () => { + useCronStore.setState({ toggleErrors: { planner: 'old failure' } }); + + await useCronStore.getState().setJobEnabled('planner', false); + + expect(useCronStore.getState().toggleErrors.planner).toBeUndefined(); + }); + + it('keeps other jobs’ errors when one job is retried', async () => { + useCronStore.setState({ toggleErrors: { planner: 'mine', sweeper: 'theirs' } }); + + await useCronStore.getState().setJobEnabled('planner', true); + + const { toggleErrors } = useCronStore.getState(); + expect(toggleErrors.planner).toBeUndefined(); + expect(toggleErrors.sweeper).toBe('theirs'); + }); + + it('falls back to the whole message when the body is not FastAPI JSON', async () => { + vi.mocked(api.setCronJobEnabled).mockRejectedValue( + new Error('502: gateway'), + ); + + await useCronStore.getState().setJobEnabled('planner', false); + + expect(useCronStore.getState().toggleErrors.planner).toBe('gateway'); + }); + + it('surfaces a non-HTTP failure rather than swallowing it', async () => { + vi.mocked(api.setCronJobEnabled).mockRejectedValue(new TypeError('network down')); + + await useCronStore.getState().setJobEnabled('planner', false); + + expect(useCronStore.getState().toggleErrors.planner).toBe('network down'); + }); +}); diff --git a/web/src/stores/cronStore.ts b/web/src/stores/cronStore.ts index 3f2d9fb4..4c25a073 100644 --- a/web/src/stores/cronStore.ts +++ b/web/src/stores/cronStore.ts @@ -17,6 +17,12 @@ export interface CronJob { next_run: string | null; /** Most recently active chat session for this job (cron:{id}[:{run}]). */ last_session_id?: string | null; + /** + * Why this job's enabled flag cannot be toggled here, or null when it can. + * Set for source runners (they have no flag in the cron files) and under + * lockdown (the cron file is reviewed config, so the change belongs in a PR). + */ + toggle_refusal?: string | null; } export interface CronLog { @@ -40,6 +46,14 @@ interface CronState { loading: boolean; triggering: string | null; rotating: string | null; + toggling: string | null; + /** + * Why the last toggle failed, keyed by job id. A rejected toggle has to say + * so: the switch springs back to where it was, which on its own is + * indistinguishable from a click that never registered — and a 403 under + * lockdown is an expected answer, not a bug. + */ + toggleErrors: Record; loadJobs: () => Promise; loadLogs: (offset?: number) => Promise; @@ -47,9 +61,29 @@ interface CronState { selectJob: (jobId: string | null) => void; triggerJob: (jobId: string) => Promise; rotateSession: (jobId: string) => Promise; + setJobEnabled: (jobId: string, enabled: boolean) => Promise; refresh: () => Promise; } +/** + * The human-readable half of a failed `request()`, which throws + * `Error(": ")` where the body is FastAPI's + * `{"detail": "..."}`. Falls back to the whole message when it is not that + * shape, so an unexpected failure is still shown rather than swallowed. + */ +function errorDetail(e: unknown): string { + const raw = e instanceof Error ? e.message : String(e); + const match = raw.match(/^\d+:\s*([\s\S]*)$/); + if (!match) return raw; + try { + const parsed = JSON.parse(match[1]); + if (parsed && typeof parsed.detail === 'string') return parsed.detail; + } catch { + // Not JSON — fall through to the raw body. + } + return match[1] || raw; +} + export const useCronStore = create((set, get) => ({ jobs: [], logs: [], @@ -59,6 +93,8 @@ export const useCronStore = create((set, get) => ({ loading: false, triggering: null, rotating: null, + toggling: null, + toggleErrors: {}, loadJobs: async () => { try { @@ -119,6 +155,27 @@ export const useCronStore = create((set, get) => ({ } }, + setJobEnabled: async (jobId: string, enabled: boolean) => { + // Clear this job's previous complaint up front, so a retry that succeeds + // doesn't leave the old message sitting under a switch that now works. + set(s => { + const remaining = { ...s.toggleErrors }; + delete remaining[jobId]; + return { toggling: jobId, toggleErrors: remaining }; + }); + try { + await api.setCronJobEnabled(jobId, enabled); + // Reload the list rather than patching the flag locally: the server also + // recomputed next_run, and a disabled job has none. + await get().loadJobs(); + } catch (e) { + console.error('Failed to toggle cron job:', e); + set(s => ({ toggleErrors: { ...s.toggleErrors, [jobId]: errorDetail(e) } })); + } finally { + set({ toggling: null }); + } + }, + refresh: async () => { await Promise.all([get().loadJobs(), get().loadLogs()]); },