diff --git a/config.example.yaml b/config.example.yaml index 75980c75..d688d508 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -125,13 +125,15 @@ memory: recall_model: claude-sonnet-4-6 # embed_model: text-embedding-3-small # Only needed with openai_api_key -# Cron — no keys needed. Cron config lives in /config/cron/ +# Cron — no path keys needed. Cron config lives in /config/cron/ # (system.yaml, jobs.yaml, gates/) and is resolved from the workspace, with a # fallback to the legacy ~/.nerve/cron for un-migrated installs. # # Do NOT pin system_file/jobs_file here unless you really mean it: an explicit # value wins outright, so it disables both the workspace location and the # fallback, and `nerve init` will keep regenerating a system.yaml nothing reads. +# +# Editing the cron files does not apply itself: POST /api/cron/reload does. # Workflow runs — budget-capped multi-agent jobs (Claude Workflow tool or # Codex Ultracode) in dedicated tracked sessions. Nerve meters real dollar diff --git a/docs/config.md b/docs/config.md index a43711e9..e5e7b97f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -97,6 +97,97 @@ type, including `int | None`, `list[int]` and `list[str]`. So `port: ${PORT}` an - Defaults are not re-scanned: `${A:-${B}}` yields the literal `${B}` when `A` is unset. Use a single reference instead. +## Validating Configuration + +`nerve config validate` checks a whole config bundle and exits non-zero on any +error, so it can gate a config repo in CI: + +```bash +nerve config validate # the active install's config +nerve config validate --workspace . # a checked-out config repo +nerve config validate --portable-only # ignore this machine's config.yaml layers +nerve config validate --strict-keys # unknown keys become errors +nerve config validate --strict-env # every ${ENV_VAR} must be set +``` + +It runs even when the config cannot otherwise load, so a missing secret does not +stop it. It reports an error for: + +- an unparseable or invalid cron file, a malformed `run_if` gate spec, or a bad + spec for a built-in gate; +- backend and codex misconfiguration; +- a schedule the daemon would not run as written, in `cron/*.yaml` or in + `sync..schedule`, named by job id. Either a 5-field crontab the + scheduler rejects (`99 * * * *`), which at run time the daemon only refuses + after the change has merged and synced, or a value that is neither a crontab + nor an interval (`hourly`, `@daily`), which silently becomes a fixed 2-hour + cadence. Write a crontab (`*/15 * * * *`) or an interval (`4h`, `30m`, + `1h30m`, `90s`); +- a path setting written `.` or `./`: `workspace`, `cron.jobs_file`, + `cron.system_file`, `cron.gate_plugins_dir`, `gateway.ssl.cert`, + `gateway.ssl.key`, `proxy.binary_path`, `proxy.auth_dir`, `proxy.log_file` or + `workflows.runs_dir`. A dot is a path like any other, so it aims the setting at + whatever directory the daemon was started in. Write the path out, or omit the + key to get the default — a blank value is *unset* (above) and is not an error. + This matters most for `cron.gate_plugins_dir`, whose `.py` files the daemon + imports and executes at startup and on every cron reload. + +Validation never imports those gate plugins, because checking them would mean +running the bundle it is judging. A `run_if` entry naming a gate type that is not +built in is therefore reported as a warning: validation can confirm neither that +the type exists nor that the spec's fields are right. Gate plugins are code, so +test them as code. + +Two checks are lenient by default, so that validating a live install does not cry +wolf: + +| Flag | Default | With the flag | +|------|---------|---------------| +| `--strict-keys` | An unknown or misspelled key is a warning. Covers config keys and the fields of a built-in gate's `run_if` spec. | Unknown keys are errors. | +| `--strict-env` | An unset `${ENV_VAR}` is info, since CI has no secrets. | Every reference must resolve. | + +Turn `--strict-keys` on in CI. A typo'd key is the most common config mistake and +the quietest, because nothing reads it. + +`--portable-only` judges the tracked `/config/settings.yaml` layer on +its own. Use it for a change headed to a shared repo: a local override can +otherwise mask an invalid shared value, and, more often, a broken local file can +fail a shared bundle that has nothing wrong with it. Pass `--workspace` with it, +because with the machine layers dropped there is nothing left to read the +workspace location from and it falls back to the default tree. It fails outright +if it opened no file under the workspace's `config/`, so an empty directory, a +`settings.yml` typo or a `settings.yaml` left at the repo root cannot pass as +clean. Every run names the layers it read, in absolute paths: + +``` +[info] portable layer: /home/you/config-repo/config/settings.yaml +[info] machine-local layers (config.yaml, config.local.yaml) not read: validating the portable workspace config on its own +``` + +Without `--portable-only` the second line names the machine-local files that were +overlaid instead, and the directory they came from. + +In CI, install nerve and run the same command: + +```yaml +jobs: + validate-config: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - uses: astral-sh/setup-uv@v6 + # nerve is not published to PyPI: the bare name is an unrelated project. + - run: uv pip install --system "nerve @ git+https://github.com/ClickHouse/nerve" + - run: nerve config validate --workspace . --portable-only --strict-keys +``` + +Installing from the default branch keeps the validator at or ahead of your +instance, which is the safe direction for `--strict-keys`: a newer validator +knows every key your instance reads. A key the validator accepts but the instance +is too old to read shows up as a startup warning on the instance and in `nerve +doctor`. If a key is ever retired upstream, `--strict-keys` flags it here first; +drop the key, or install the ref you deploy (`nerve @ git+...@v1.2.3`) instead. + ## Config Directory Resolution `nerve` commands locate the config directory via a waterfall, so they work diff --git a/docs/cron.md b/docs/cron.md index f44f9063..eaa3b8cc 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -121,7 +121,7 @@ prompt definition. | Field | Type | Required | Description | |-------|------|----------|-------------| | `id` | string | yes | Unique job identifier. `cleanup`, `wakeup_sweep` and anything starting with `source:` are **reserved by the daemon** — a job using one is skipped (with a warning naming it in the daemon log) and never scheduled, so rename it | -| `schedule` | string | yes | Crontab expression or interval (`2h`, `30m`, `1h30m`, `0.5h`) — see [Interval syntax](#interval-syntax). A 5-field crontab with an out-of-range field (`99 * * * *`) is **rejected**, never reinterpreted as an interval — reload returns `400` and startup skips that one job with an error in the daemon log | +| `schedule` | string | yes | Crontab expression or interval (`2h`, `30m`, `1h30m`, `0.5h`) — see [Interval syntax](#interval-syntax). A 5-field crontab with an out-of-range field (`99 * * * *`) is **rejected**, never reinterpreted as an interval — reload returns `400` and startup skips that one job with an error in the daemon log. Anything that names no usable interval (`hourly`, `@daily`, `1h junk`, or a zero like `0h`) silently becomes a 2-hour interval at run time; `nerve config validate` fails on both, which is the only warning you get about the second | | `prompt` | string | yes* | Message sent to the agent | | `prompt_file` | string | yes* | Path to a file containing the prompt (relative to the YAML's directory). Read fresh each run; shareable between jobs. *One of `prompt`/`prompt_file` is required | | `description` | string | no | Human-readable description | diff --git a/nerve/cli.py b/nerve/cli.py index 43d02438..cc758fc1 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -32,7 +32,6 @@ from nerve import paths from nerve.config import ( - ConfigError, RESUME_QUEUE_FILE, load_config, resolve_config_dir, @@ -173,7 +172,7 @@ def _docker_compose( # Commands that must keep working when the config itself is broken -- they are # how an operator diagnoses or repairs it. They receive ctx.obj["config"] = None # and ctx.obj["config_error"], and are responsible for reporting. -_SELF_DIAGNOSING_COMMANDS = frozenset({"doctor", "init"}) +_SELF_DIAGNOSING_COMMANDS = frozenset({"config", "doctor", "init"}) @click.group() @@ -196,17 +195,27 @@ def main(ctx: click.Context, config_dir: str | None, verbose: bool) -> None: config_error = None try: config = load_config(resolved_dir) - except ConfigError as e: + set_config(config) + except Exception as e: # This callback runs before *every* subcommand, so a config that won't # load would otherwise take down the commands you reach for to fix it. - # The self-diagnosing ones get config=None and report the problem - # themselves; everything else gets a clean message and a non-zero exit - # rather than a raw traceback. + # The self-diagnosing ones get config=None plus the message and report + # it themselves — for any failure at all, since a command whose job is to + # explain a broken config is the last place a traceback helps. + # + # Everything else gets a clean error and a non-zero exit, but only for + # the operator's own mistakes: ConfigError (malformed YAML, an + # unresolved required ${VAR}) and the plain ValueError that + # NerveConfig._validate_backend_config raises for a bad agent.backend or + # codex section — ConfigError subclasses ValueError, so one check covers + # both. Anything else means nerve itself is broken, not the config, and + # keeps its traceback: presenting an internal defect as "Error: " sends the operator to edit a file that was never at fault. if ctx.invoked_subcommand not in _SELF_DIAGNOSING_COMMANDS: - raise click.ClickException(str(e)) from e - config_error = e - if config is not None: - set_config(config) + if isinstance(e, ValueError): + raise click.ClickException(str(e)) from e + raise + config_error = str(e) ctx.ensure_object(dict) ctx.obj["config"] = config ctx.obj["config_error"] = config_error @@ -1066,11 +1075,12 @@ def doctor(ctx: click.Context) -> None: config = ctx.obj["config"] if config is None: # The config wouldn't load at all — that *is* the diagnosis. - click.secho("Nerve Doctor", bold=True) + click.echo("Nerve Doctor") click.echo("=" * 40) - click.secho(f"[ERR] Config could not be loaded: {ctx.obj['config_error']}", - fg="red") - click.echo(f" Config directory: {ctx.obj['config_dir']}") + click.echo(f"Config dir: {ctx.obj.get('config_dir')}") + click.secho( + f"[ERR] config failed to load: {ctx.obj.get('config_error')}", fg="red" + ) ctx.exit(1) report = doctor_report( config, @@ -1082,6 +1092,60 @@ def doctor(ctx: click.Context) -> None: ctx.exit(1) +@main.group(name="config") +def config_group() -> None: + """Configuration commands.""" + + +@config_group.command("validate") +@click.option( + "--workspace", "workspace", type=click.Path(), default=None, + help="Validate this workspace's config (e.g. a checked-out config repo: " + "--workspace .). Defaults to the resolved workspace.", +) +@click.option( + "--strict-env", is_flag=True, + help="Fail if any ${ENV_VAR} reference is unset. Default: allow (CI usually " + "has no secrets); unset refs are reported as info.", +) +@click.option( + "--strict-keys", is_flag=True, + help="Fail on unknown/misspelled config keys. Default: warn (a key from a " + "newer nerve shouldn't fail CI). Recommended in CI on a config repo, " + "where the validator is pinned to the deployed nerve version.", +) +@click.option( + "--portable-only", is_flag=True, + help="Ignore this machine's config.yaml / config.local.yaml and validate " + "only the portable workspace config — what a shared repo carries.", +) +@click.pass_context +def config_validate( + ctx: click.Context, workspace: str | None, strict_env: bool, strict_keys: bool, + portable_only: bool, +) -> None: + """Validate the configuration bundle. Non-zero exit on any error (CI-ready).""" + from nerve.config_validate import validate_config_bundle + + config_dir = Path(ctx.obj["config_dir"]) + result = validate_config_bundle( + config_dir, workspace_override=workspace, + strict_env=strict_env, strict_keys=strict_keys, + portable_only=portable_only, + ) + for msg in result.info: + click.echo(f"[info] {msg}") + for msg in result.warnings: + click.secho(f"[WARN] {msg}", fg="yellow") + for msg in result.errors: + click.secho(f"[ERR] {msg}", fg="red") + if result.ok: + click.secho("Config OK", fg="green") + else: + click.secho(f"Config invalid: {len(result.errors)} error(s)", fg="red") + ctx.exit(1) + + @main.group() def codex() -> None: """Inspect and authenticate the Codex integration.""" diff --git a/nerve/config_validate.py b/nerve/config_validate.py new file mode 100644 index 00000000..aee4d678 --- /dev/null +++ b/nerve/config_validate.py @@ -0,0 +1,688 @@ +"""Config-bundle validation for CI and ``nerve config validate``. + +Validates the merged configuration the way :func:`nerve.config.load_config` +assembles it (workspace/config/settings.yaml + config.yaml + config.local.yaml), +but **leniently** with respect to secrets: ``${ENV_VAR}`` references that are +unset are treated as valid placeholders (they aren't present in CI) rather than +hard failures — unless ``strict_env`` is requested. + +Structural problems are hard errors so a bad config PR fails CI before merge: an +unparseable or invalid cron file, a bad backend / codex config, a malformed cron +gate spec, a schedule the scheduler will not run as written +(:func:`_schedule_problem`), or a path setting written ``.`` — which aims nerve +at the daemon's working directory (:data:`_WORKING_DIR_PATH_KEYS`). Unknown / +misspelled top-level keys are warnings by default (a config carrying a key from +a newer nerve, or the shipped example, shouldn't fail CI) — pass ``strict_keys`` +to promote them to errors. + +**Validation never loads the bundle's own code.** Cron gate plugins are ordinary +``.py`` files that the daemon imports at startup; importing one to check it would +mean the bundle had already executed by the time we decided it was unfit — "an +invalid bundle is refused" is not a guarantee you can make about code you had to +run to judge. So validation does not load them at all, and it does not try to +divine what they declare either: a gate type it doesn't recognize is reported as +unverified, not accepted and not rejected. A plugin is code, and code is checked +by running it — that is the author's job, not the config validator's. + +Built-in gate specs *are* built (``build_gate``), which runs nerve's own +``from_config`` with values from the bundle. That is what type-checking a gate +spec means; those three bodies parse their arguments and have no side effects. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field, fields, is_dataclass +from pathlib import Path +from typing import Any + +from nerve import config as cfg + + +@dataclass +class ValidationResult: + errors: list[str] = field(default_factory=list) + warnings: list[str] = field(default_factory=list) + info: list[str] = field(default_factory=list) + + @property + def ok(self) -> bool: + return not self.errors + + +def validate_config_bundle( + config_dir: Path, + workspace_override: Path | str | None = None, + strict_env: bool = False, + strict_keys: bool = False, + portable_only: bool = False, +) -> ValidationResult: + """Validate the config bundle rooted at ``config_dir``. + + ``workspace_override`` forces the workspace location (e.g. a checked-out + config repo in CI: ``--workspace .``) instead of resolving it from + config.yaml. ``strict_env`` promotes unset ``${ENV_VAR}`` references from + info to errors. ``strict_keys`` promotes unknown/misspelled keys from + warnings to errors (off by default so a config carrying a key from a newer + nerve, or the shipped example, doesn't spuriously fail CI). + + ``portable_only`` drops the machine-local layers and judges the portable + workspace config alone — the right question for a change headed to a shared + repo, and the only way to get an answer that doesn't depend on the host + running the check. Pair it with ``workspace_override``: with no machine + config left to read the workspace location from, it falls back to the + default one. + """ + result = ValidationResult() + config_dir = Path(config_dir) + + # The two machine-local layers. Skipped under portable_only: an override on + # the validating host can otherwise mask an invalid shared value, and — the + # commoner failure — a broken local file condemns a shared bundle that has + # nothing wrong with it, blaming an error the bundle doesn't contain. + machine_paths = (config_dir / "config.yaml", config_dir / "config.local.yaml") + # ``None`` marks a layer that was not read — skipped, or unusable. + layers: list[dict[str, Any] | None] = [None, None] + if not portable_only: + layers = [_read_layer(p, result) for p in machine_paths] + overlaid = [ + p.name for p, layer in zip(machine_paths, layers) + if layer is not None and p.exists() + ] + base, local = layers[0] or {}, layers[1] or {} + machine = cfg._deep_merge(base, local) + + if workspace_override is not None: + workspace = Path(workspace_override).expanduser() + else: + # Resolve the workspace the same way load_config does, incl. best-effort + # ${VAR}/${VAR:-default} interpolation of the path itself. Under + # portable_only there is no machine config to read it from, so this + # falls back to the default location — pass workspace_override to point + # at a checkout. + ws_raw = machine.get("workspace") + if isinstance(ws_raw, str) and "${" in ws_raw: + ws_raw = cfg._interpolate_str(ws_raw, []) + workspace = cfg._expand_path(ws_raw) or cfg.paths.default_workspace() + + ws_settings = _read_workspace_settings(workspace, result) + merged = cfg._deep_merge(cfg._deep_merge(ws_settings, base), local) + # Before the pin below overwrites the bundle's own ``workspace`` value with + # the resolved one — that value is part of what is under review. + _validate_working_dir_paths(merged, result) + # Pin the workspace so cron paths resolve against the validated workspace. + merged["workspace"] = str(workspace) + + # Lenient env interpolation — collect unset refs without raising. + missing: list[str] = [] + merged = cfg._interpolate_env(merged, missing) + env_names = ", ".join(sorted(set(missing))) + if missing: + msg = f"references unset environment variable(s): {env_names}" + (result.errors if strict_env else result.info).append(msg) + + # Unknown / misspelled keys: warnings by default (forward-compat / example + # configs shouldn't fail CI), errors under --strict-keys. + key_problems = cfg.validate_config_keys(merged) + for w in key_problems: + bucket = result.errors if strict_keys else result.warnings + bucket.append(f"unknown or invalid config key: {w}") + if key_problems and strict_keys: + # The key set is this validator's own. Under --strict-keys that is the one + # check whose verdict can differ between two nerve versions, so a CI run + # that disagrees with the instance is a version difference rather than a + # bad config. Say it where the failure is read, or a config repo ends up + # blocked by a key its own instance understands with nothing to explain it. + result.info.append( + "unknown-key errors reflect the config keys this nerve version knows; " + "validate with the version you deploy" + ) + + # Typed construction surfaces backend / codex / structural validation errors. + # An unset ${VAR} left as a literal string can trip one of those checks on its + # own — "${NERVE_BACKEND}" is not a known backend name — which is a + # consequence of the missing variable, already reported, rather than a config + # bug. But that cannot excuse the whole construction: secrets are unset in CI, + # so "some variable is missing" is the normal state there, and downgrading + # every error under it disables this check exactly where it runs. So the + # failure is re-derived with the unresolved references out of the way, and only + # an error that disappears is charged to the environment. + config = None + try: + config = cfg.NerveConfig.from_dict(merged) + config.config_dir = config_dir + except Exception as e: # noqa: BLE001 — ConfigError / ValueError from validate() + residual = e + if missing and not strict_env: + residual = _error_without_unresolved(merged) + if residual is None: + result.warnings.append( + f"could not fully type-check config while env var(s) are unset " + f"({env_names}): {e}" + ) + else: + # The residual error rather than ``e``: the first failure may itself + # be env-caused, with a real one behind it. + result.errors.append(f"config error: {residual}") + + # Resolve the cron locations even if full typed construction failed above. + if config is not None: + cron_files = ( + ("system", config.cron.system_file), + ("jobs", config.cron.jobs_file), + ) + else: + base_cron = cfg._resolve_cron_dir(workspace) + cron_files = ( + ("system", base_cron / "system.yaml"), + ("jobs", base_cron / "jobs.yaml"), + ) + + _validate_cron(cron_files, result, strict_keys=strict_keys) + if config is not None: + # Source runners are scheduled from the same parser as cron jobs, so a + # typo'd sync schedule fails identically — and needs the same gate. + _validate_source_schedules(config, result) + # Files of the bundle that were actually opened, so "it validated" can be + # told from "it found nothing to validate". + portable_root = cfg.workspace_config_dir(workspace) + read = [ + p for p in (cfg.workspace_settings_file(workspace), *(p for _, p in cron_files)) + if p.is_file() and p.is_relative_to(portable_root) + ] + _note_layers( + config_dir, workspace, result, + portable_only=portable_only, + workspace_pinned=workspace_override is not None, + overlaid=overlaid, + portable_read=read, + ) + return result + + +def _read_layer(path: Path, result: ValidationResult) -> dict[str, Any] | None: + """Read one machine-local layer; ``None`` if it could not be used. + + These reads happen before anything is type-checked, so an unparseable layer + would otherwise escape as a traceback — the ugliest possible output for what + is the likeliest failure of all, a YAML typo. + + Read strictly. A layer that parses to the wrong shape — a list, from a merge + conflict resolved into a sequence, or a truncated write — is *dropped* in + lenient mode: every key it supplies silently reverts to the layer below, + including the ``workspace`` path, so validation walks off to a different + tree and reports it clean. ``load_config`` can't afford to fail there; + the validator exists to. + """ + try: + return cfg._read_yaml_mapping(path, strict=True) + except cfg.ConfigError as e: + result.errors.append(str(e)) + except OSError as e: + result.errors.append(f"cannot read {path}: {e}") + return None + + +def _read_workspace_settings(workspace: Path, result: ValidationResult) -> dict[str, Any]: + """Read the portable ``settings.yaml`` layer, reporting a bad file.""" + try: + return cfg._load_workspace_settings(workspace) + except cfg.ConfigError as e: + result.errors.append(str(e)) + except OSError as e: + result.errors.append( + f"cannot read {cfg.workspace_settings_file(workspace)}: {e}" + ) + return {} + + +def _error_without_unresolved(merged: dict[str, Any]) -> Exception | None: + """The construction error that is not an artifact of unset ``${VAR}`` refs. + + Rebuilds the config with every value that still carries a literal ``${...}`` + dropped, so those keys fall back to their defaults, and returns whatever it + still raises — ``None`` if it now builds, which means the missing variables + and nothing else caused the original failure. + + Dropping is what makes the question decidable without inventing values: a + default is a value nerve ships, so it cannot be the thing a check rejects, + and an error that survives is therefore stated in the bundle. The other + direction is deliberately out of scope: whether the value a *set* variable + would carry is valid cannot be known from the bundle, which is what + ``strict_env`` and the daemon's own load are for. + """ + try: + cfg.NerveConfig.from_dict(_without_unresolved(merged)) + except Exception as e: # noqa: BLE001 — same surface as the caller's + return e + return None + + +def _without_unresolved(value: Any) -> Any: + """*value* with every setting whose text still holds a ``${...}`` removed.""" + if isinstance(value, dict): + return { + k: _without_unresolved(v) + for k, v in value.items() if not _is_unresolved(v) + } + if isinstance(value, list): + return [_without_unresolved(v) for v in value if not _is_unresolved(v)] + return value + + +def _is_unresolved(value: Any) -> bool: + """True for a value whose own text still carries a ``${...}`` reference. + + Only reached on the interpolated config, where every reference that *could* + be resolved already was — so what is left is unset by definition. + """ + return isinstance(value, str) and "${" in value + + +#: Path settings that must not resolve to the process's working directory, paired +#: with what aiming each one there would actually do. +#: +#: The value that gets here is an explicit ``.``, ``./`` or ``./.``: those survive +#: :func:`nerve.config._expand_path` as the path they are, so the setting really +#: does mean "wherever the daemon was started". A *blank* value is not one of +#: these — ``_expand_path`` maps it to ``None`` and the field takes its documented +#: default, which is why nothing below applies to it. +#: +#: Nothing legitimate wants any of these aimed at a working directory, and a +#: bundle whose meaning depends on where the daemon was launched from is what a +#: config gate exists to stop, so each is a hard error regardless of strictness. +#: Kept in step with the config dataclasses by the coverage test in +#: tests/test_config_validate.py: a new ``Path`` setting has to be listed here or +#: exempted there. +_WORKING_DIR_PATH_KEYS: dict[tuple[str, ...], str] = { + ("workspace",): ( + "the entire workspace (settings.yaml, the cron config, memory, " + "everything nerve syncs) would be read from and written to whatever " + "directory the daemon was started in" + ), + ("cron", "gate_plugins_dir"): ( + "every *.py file in that directory is imported and executed by the " + "daemon, at startup and again on every cron reload, so this hands the " + "working directory's contents to nerve as code" + ), + ("cron", "jobs_file"): "cron would try to read a directory as its jobs file", + ("cron", "system_file"): ( + "cron would try to read a directory as its system-jobs file" + ), + ("gateway", "ssl", "cert"): ( + "TLS is 'on' precisely when cert and key are both set, so this turns it " + "on with a directory as the certificate file and the gateway fails to " + "bind at startup" + ), + ("gateway", "ssl", "key"): ( + "TLS is 'on' precisely when cert and key are both set, so this turns it " + "on with a directory as the private-key file and the gateway fails to " + "bind at startup" + ), + ("proxy", "binary_path"): ( + "a directory satisfies the exists-and-executable check that decides " + "whether to download the proxy, so nerve skips the download and tries " + "to exec the working directory" + ), + ("proxy", "auth_dir"): ( + "the proxy's credential store (its OAuth tokens) would be created in " + "whatever directory the daemon was started in" + ), + ("proxy", "log_file"): ( + "the proxy's output is opened for append at that path, which a directory " + "refuses, so the proxy does not start" + ), + ("workflows", "runs_dir"): ( + "every workflow run's journal directory would be created in whatever " + "directory the daemon was started in, and the API's containment check " + "on run paths would be rooted there too" + ), +} + + +def _validate_working_dir_paths( + merged: dict[str, Any], result: ValidationResult, +) -> None: + """Refuse path settings that resolve to the process's working directory. + + Checked on the raw bundle values rather than on the constructed config: by + the time these have become ``Path`` objects an explicit ``.`` is + indistinguishable from a deliberate choice, and the point is to name the + text the author wrote back to them. + """ + for key, consequence in _WORKING_DIR_PATH_KEYS.items(): + raw = _nested_get(merged, key) + if not isinstance(raw, str): + continue + # ``${VAR:-.}`` reaches the same directory, one indirection later. + # Expand best-effort as elsewhere here; an unset *required* ``${VAR}`` + # stays literal (so it isn't flagged) and is reported on its own. + value = cfg._interpolate_str(raw, []) if "${" in raw else raw + reason = _working_dir_reason(value) + if reason is None: + continue + shown = repr(raw) if value == raw else f"{raw!r} (expands to {value!r})" + result.errors.append( + f"{'.'.join(key)}: {shown} {reason} — {consequence}. Remove the key " + f"to use the default, or set an explicit path." + ) + + +def _working_dir_reason(value: str) -> str | None: + """Why *value* points at the process's working directory, or ``None``. + + Asked of ``_expand_path``, the function the daemon resolves these settings + with, so the two cannot disagree about which values reach a working + directory and which are the blank that means "use the default". + """ + if cfg._expand_path(value) == Path("."): # "." , "./" , "./." + return "resolves to the process's working directory" + return None + + +def _nested_get(d: dict[str, Any], key: tuple[str, ...]) -> Any: + """Follow a dotted key into nested mappings; ``None`` if the path breaks.""" + for part in key[:-1]: + d = d.get(part) + if not isinstance(d, dict): + return None + return d.get(key[-1]) + + +def _note_layers( + config_dir: Path, + workspace: Path, + result: ValidationResult, + *, + portable_only: bool, + workspace_pinned: bool, + overlaid: list[str], + portable_read: list[Path], +) -> None: + """Name every layer the verdict was reached on — always, including none. + + A report that lists nothing is ambiguous in three directions: an error may + have come from a file that only exists on the machine running the check, a + clean result may only be clean because a local override papered over it, + and — the one that makes a CI gate worthless — a run may have looked at a + workspace that isn't the tree under review, and passed because there was + nothing there. + + Paths are absolute on purpose: ``--workspace .`` would otherwise report + ``config/settings.yaml``, which answers none of the above. + """ + config_dir = config_dir.resolve() + settings = cfg.workspace_settings_file(workspace).resolve() + found = "" if settings.is_file() else " (not present)" + result.info.append(f"portable layer: {settings}{found}") + + if portable_only: + result.info.append( + "machine-local layers (config.yaml, config.local.yaml) not read: " + "validating the portable workspace config on its own" + ) + if not workspace_pinned: + # Dropping the machine layers also drops the workspace path they + # carried, so an unpinned run silently moves to the default tree. + result.warnings.append( + f"no workspace was given and machine-local config was not read, " + f"so the workspace fell back to the default ({workspace}) — pass " + f"a workspace to validate a specific checkout" + ) + if not portable_read: + # Asked to judge the portable layer, and not one of its files was + # opened. The usual cause is a layout mistake — settings.yaml at the + # repo root, a .yml extension, a config/ holding only placeholders — + # and passing here would be a gate that checked nothing, for good. + result.errors.append( + f"nothing to validate: no config files were found under " + f"{cfg.workspace_config_dir(workspace).resolve()} (looked for " + f"settings.yaml, cron/system.yaml, cron/jobs.yaml)" + ) + return + + present = [ + p.name for p in ( + config_dir / "config.yaml", config_dir / "config.local.yaml", + ) if p.exists() + ] + if not present: + result.info.append(f"no machine-local layers present in {config_dir}") + elif not overlaid: + # Present but unusable — the errors say why; don't claim they applied. + result.info.append( + f"machine-local layer(s) could not be used: " + f"{', '.join(present)} ({config_dir})" + ) + else: + note = ( + f"machine-local layer(s) overlaid on the portable config: " + f"{', '.join(overlaid)} ({config_dir})" + ) + if result.errors: + note += ( + " — an error in this run may originate there rather than in " + "the workspace config" + ) + result.info.append(note) + + +def _validate_cron( + cron_files, result: ValidationResult, *, strict_keys: bool = False, +) -> None: + """Validate cron files strictly, including gate specs (which build_gates + otherwise swallows).""" + from nerve.cron.gates import GATE_REGISTRY, GateConfigError, build_gate + from nerve.cron.jobs import load_jobs + + # One note per distinct unverifiable gate type, however many jobs use it. + reported: set[str] = set() + + for label, path in cron_files: + # Collect per-job failures rather than raising on the first: a job whose + # gate spec won't build is refused at run time (see build_gates), and + # stopping here would hide every other problem in the same file. + job_errors: list[str] = [] + try: + jobs = load_jobs( + path, strict=True, errors=job_errors, build_gates=False, + ) + except cfg.ConfigError as e: + result.errors.append(str(e)) + continue + except Exception as e: # noqa: BLE001 — job construction may raise oddly + result.errors.append(f"cron {label} ({path}): {e}") + continue + result.errors.extend(job_errors) + if path.exists(): + result.info.append(f"cron {label}: {len(jobs)} job(s) ({path})") + # The jobs that built are still worth checking spec by spec: load_jobs + # only reports the ones it could not construct at all, and a spec can be + # buildable yet wrong — an unknown field takes the default silently. + for job in jobs: + where = f"cron {label} job '{job.id}'" + problem = _schedule_problem(job.schedule) + if problem: + result.errors.append(f"{where}: {problem}") + for spec in _job_gate_specs(job, where, result): + problem = _gate_spec_problem(spec) + if problem: + result.errors.append(f"{where}: invalid gate {spec!r}: {problem}") + continue + gate_type = spec["type"] + cls = GATE_REGISTRY.get(gate_type) + if cls is None: + if gate_type in reported: + continue + # Could be a gate plugin's type, could be a typo — telling + # them apart means loading the plugin, i.e. running the + # bundle. Say which it can't confirm instead of guessing, + # and say what happens if nothing provides it. + reported.add(gate_type) + result.warnings.append( + f"{where}: gate type {gate_type!r} is not a built-in " + f"gate. A gate plugin may provide it; validation does " + f"not load plugins, so it can confirm neither the type " + f"nor this spec's fields. If nothing registers it at " + f"run time the job does not load at all — a reload is " + f"refused and startup skips the job" + ) + continue + try: + build_gate(spec) + except GateConfigError as e: + result.errors.append(f"{where}: invalid gate {spec}: {e}") + continue + # A misspelled field is the quiet failure: from_config reads the + # spec with .get(), so the typo takes the default and the gate + # silently checks something else. + unknown = sorted(set(spec) - {"type"} - cls.spec_keys) + if unknown and cls.spec_keys: + bucket = result.errors if strict_keys else result.warnings + bucket.append( + f"{where}: gate {gate_type!r} ignores unknown field(s) " + f"{', '.join(unknown)} (known: " + f"{', '.join(sorted(cls.spec_keys))})" + ) + + +def _schedule_problem(schedule: Any) -> str | None: + """Why the daemon would not run *schedule* as its author wrote it. + + Asked of the scheduler's own parser rather than re-derived here: a + validator that disagrees with the daemon is worse than no validator, + because it either fails configs that work or passes configs that don't. + + Two distinct mistakes, both silent without this check: + + * a 5-field crontab the scheduler rejects (``"99 * * * *"``) — the daemon + does refuse to schedule that job, but only once the change has merged + and synced, and only into a log line, with the instance left on its old + config until someone reads it; + * a string that is neither a crontab nor an interval (``"hourly"``, + ``"@daily"``) — nothing complains about this one *ever*: it falls back + to a fixed default and the job runs happily on a cadence nobody chose. + + Both are hard errors. Unlike an unknown config key, there is no + forward-compatibility case to protect: the accepted schedule forms are + read out of the scheduler code this validator is importing, so if nerve + learns a new one the answer here changes with it. And nothing the daemon + would honour is flagged — every schedule that yields the cadence its + author asked for either parses as a crontab or carries an h/m/s token. + """ + from nerve.cron.service import ( + InvalidScheduleError, + NotCrontabError, + _crontab_to_trigger, + _interval_seconds, + _parse_interval, + ) + + if not isinstance(schedule, str): + # YAML hands over whatever was written: `schedule: 4` is an int, and + # the scheduler's first move is `.split()`. That AttributeError escapes + # every handler the daemon has for a bad schedule and takes the whole + # cron service down with it, so it cannot reach one. + return ( + f"schedule must be a string, got {type(schedule).__name__} " + f"({schedule!r}) — quote it" + ) + if "${" in schedule: + # An unresolved ${VAR} is reported on its own; judging the literal text + # would report a second, invented error for the same cause. + return None + try: + _crontab_to_trigger(schedule) + except NotCrontabError: + pass # not a crontab — must then be an interval + except InvalidScheduleError as e: + return str(e) + else: + return None + + if _interval_seconds(schedule) is None: + return ( + f"schedule {schedule!r} is neither a 5-field crontab expression " + f"nor an interval like '4h', '30m' or '1h30m'. Nothing rejects it " + f"at run time — it falls back to a fixed " + f"{_parse_interval(schedule)}s, so it runs on a cadence nobody chose" + ) + return None + + +def _validate_source_schedules(config, result: ValidationResult) -> None: + """Check ``sync..schedule`` the same way as a cron job's. + + The service turns these into triggers through the same call, so they fail + the same way — and worse: a source that never syncs looks exactly like a + source with nothing new, so the wrong cadence here is invisible from the + outside for as long as it lasts. + """ + for f in fields(config.sync): + source = getattr(config.sync, f.name) + if not is_dataclass(source): + continue + schedule = getattr(source, "schedule", None) + if schedule is None: # e.g. codex sync, which has no schedule + continue + problem = _schedule_problem(schedule) + if problem: + result.errors.append(f"sync.{f.name}.schedule: {problem}") + + +def _job_gate_specs(job, where: str, result: ValidationResult) -> list: + """Every gate spec *job* will build, mirroring ``CronJob._build_gates``. + + The legacy ``skip_when_idle`` shorthand is turned into a ``messages`` gate + at load time, so it needs the same checking as ``run_if`` — and one shape + more. It takes a *list* of source names; a bare string is iterated into one + source per character, none of which matches anything, so the gate is never + satisfied and the job silently never runs again. + """ + specs: list = [] + if isinstance(job.run_if, list): + specs.extend(job.run_if) + else: + result.errors.append( + f"{where}: 'run_if' must be a list of gate specs, got " + f"{type(job.run_if).__name__}" + ) + + idle = job.skip_when_idle + # "Not set" is an empty *list* (or a bare key, which the loader turns into + # one) — not merely anything falsy. A falsy wrong shape such as + # ``skip_when_idle: {}`` is exactly the case worth reporting: it reads as a + # gate the author meant to have, and nothing can build one from it — the + # daemon refuses the job over it. + if not isinstance(idle, list) or not all(isinstance(s, str) for s in idle): + shown = repr(idle) if isinstance(idle, list) else type(idle).__name__ + result.errors.append( + f"{where}: 'skip_when_idle' must be a list of source names, got {shown}" + ) + return specs + if not idle: + return specs + specs.append({ + "type": "messages", + "sources": list(idle), + "consumer": job.idle_consumer, + }) + return specs + + +def _gate_spec_problem(spec) -> str | None: + """What is structurally wrong with one gate spec, if anything. + + Checked for every gate, built-in or not: the shape of a spec is config, and + getting it wrong is the common authoring mistake — no knowledge of the gate + itself is needed to catch it. + """ + if not isinstance(spec, dict): + return f"gate spec must be a mapping, got {type(spec).__name__}" + gate_type = spec.get("type") + if gate_type is None or gate_type == "": + return "gate spec missing required 'type' key" + if not isinstance(gate_type, str): + return f"gate 'type' must be a string, got {type(gate_type).__name__}" + if not gate_type.strip(): + return "gate spec 'type' is blank" + return None diff --git a/nerve/cron/gates.py b/nerve/cron/gates.py index 0b62287d..576b767f 100644 --- a/nerve/cron/gates.py +++ b/nerve/cron/gates.py @@ -79,6 +79,14 @@ class CronGate(ABC): #: Registry key — must match the ``type`` field in the YAML spec. type: ClassVar[str] = "" + #: Keys this gate reads from its spec, besides ``type``. Declaring them + #: lets ``nerve config validate`` catch a misspelled field, which is + #: otherwise invisible: ``from_config`` uses ``spec.get()``, so a typo just + #: takes the default and the gate quietly does something else. Empty means + #: "unknown" — no field checking — which is the right default for an + #: out-of-tree gate nothing has read. + spec_keys: ClassVar[frozenset[str]] = frozenset() + @abstractmethod async def is_satisfied(self, ctx: GateContext) -> bool: """Return True if the job is allowed to run under this condition.""" @@ -110,6 +118,11 @@ class MessagesGate(CronGate): """ type = "messages" + # "skip_when_idle"/"idle_consumer" are the legacy spellings from_config + # still accepts. + spec_keys = frozenset({ + "sources", "consumer", "skip_when_idle", "idle_consumer", + }) def __init__(self, sources: list[str], consumer: str = "inbox") -> None: if not sources: @@ -167,6 +180,7 @@ class TasksGate(CronGate): """ type = "tasks" + spec_keys = frozenset({"status", "tag", "min_count"}) def __init__( self, @@ -278,6 +292,7 @@ class GitHubPrActivityGate(CronGate): """ type = "github_pr_activity" + spec_keys = frozenset({"author", "force_run_after_hours"}) def __init__(self, author: str, force_run_after_hours: float = 8.0) -> None: if not author: @@ -449,6 +464,13 @@ def build_gate(spec: dict) -> CronGate: gate_type = spec.get("type") if not gate_type: raise GateConfigError("gate spec missing required 'type' key") + if not isinstance(gate_type, str): + # A YAML list or mapping under `type:` is unhashable, so the registry + # lookup below would raise TypeError — which build_gates does not catch, + # taking the whole job down instead of just this gate. + raise GateConfigError( + f"gate 'type' must be a string, got {type(gate_type).__name__}" + ) cls = GATE_REGISTRY.get(gate_type) if cls is None: known = ", ".join(sorted(GATE_REGISTRY)) or "(none)" diff --git a/nerve/cron/jobs.py b/nerve/cron/jobs.py index cfa9aae7..d28e3670 100644 --- a/nerve/cron/jobs.py +++ b/nerve/cron/jobs.py @@ -42,6 +42,15 @@ def describe_reserved_job_ids() -> str: ) +def _none_to_empty(value: Any) -> Any: + """``None`` becomes ``[]``; every other value is returned verbatim. + + A bare ``run_if:`` / ``skip_when_idle:`` key parses to None and means "no + gates". Nothing else does — see :meth:`CronJob.from_dict`. + """ + return [] if value is None else value + + @dataclass class CronJob: """A cron job definition.""" @@ -168,15 +177,30 @@ def _build_gates(self) -> list["CronGate"]: A spec that cannot be built raises out of __post_init__ and takes the whole job with it — see :func:`nerve.cron.gates.build_gates` for why that - beats quietly dropping the gate. The job id is added to the message here - because the spec alone doesn't say which YAML entry to go and fix, and - this text is what reaches the reload's 400. + beats quietly dropping the gate. The same goes for the two fields + themselves: both hold whatever the config said (from_dict normalizes only + a bare key), so either may be the wrong shape here, and a shape nobody + can read is not permission to run the job unguarded. + + Messages name the job and the offending value, because that is what the + operator has to go and fix — and for a spec, what reaches the reload's + 400. """ from nerve.cron.gates import GateConfigError, build_gates + if not isinstance(self.run_if, list): + raise GateConfigError( + f"Cron job {self.id!r}: 'run_if' must be a list of gate specs, " + f"got {self.run_if!r}" + ) specs: list[dict] = list(self.run_if) # Translate the legacy skip_when_idle shorthand into a messages gate # so old configs keep working without rewrites. + if not isinstance(self.skip_when_idle, list): + raise GateConfigError( + f"Cron job {self.id!r}: 'skip_when_idle' must be a list of " + f"source names, got {self.skip_when_idle!r}" + ) if self.skip_when_idle: specs.append({ "type": "messages", @@ -189,7 +213,18 @@ def _build_gates(self) -> list["CronGate"]: raise GateConfigError(f"Cron job {self.id!r}: {e}") from e @classmethod - def from_dict(cls, d: dict, base_dir: Path | None = None) -> CronJob: + def from_dict( + cls, d: dict, base_dir: Path | None = None, *, gates: bool = True, + ) -> CronJob: + """Build a job from its YAML mapping. + + ``gates=False`` attaches the gate specs without building them. Only + ``nerve config validate`` wants that: it never loads the bundle's gate + plugins, so every plugin-provided type looks unregistered to it, and + building would refuse jobs that are in fact fine. It checks the specs + itself instead — per type and per field — and reports an unrecognized + type as unverified rather than wrong. + """ job = cls( id=d["id"], schedule=d["schedule"], @@ -210,12 +245,27 @@ def from_dict(cls, d: dict, base_dir: Path | None = None) -> CronJob: catchup=d.get("catchup", True), enabled=d.get("enabled", True), lock=bool(d.get("lock", False)), - run_if=d.get("run_if", []), - skip_when_idle=d.get("skip_when_idle", []), + # A bare `run_if:` parses to None, which means "no gates" and must + # not reach gate construction. Only None is normalized: any other + # wrong shape (`run_if: {}`) is kept verbatim so validation can + # reject it, rather than silently reading as an ungated job. + # + # Held back when gates aren't being built, so __post_init__ has + # nothing to construct; the real values are attached below. + run_if=_none_to_empty(d.get("run_if")) if gates else [], + skip_when_idle=( + _none_to_empty(d.get("skip_when_idle")) if gates else [] + ), idle_consumer=d.get("idle_consumer", "inbox"), show_session_label=d.get("show_session_label", True), metadata=d.get("metadata", {}), ) + if not gates: + # Attached after construction, so ``gates`` stays empty while the + # specs are still there to be inspected. The pair is deliberately + # inconsistent and only validation ever sees it. + job.run_if = _none_to_empty(d.get("run_if")) + job.skip_when_idle = _none_to_empty(d.get("skip_when_idle")) if job.prompt_file: p = Path(job.prompt_file).expanduser() if not p.is_absolute() and base_dir is not None: @@ -224,7 +274,13 @@ def from_dict(cls, d: dict, base_dir: Path | None = None) -> CronJob: return job -def load_jobs(jobs_file: Path, strict: bool = False) -> list[CronJob]: +def load_jobs( + jobs_file: Path, + strict: bool = False, + errors: list[str] | None = None, + *, + build_gates: bool = True, +) -> list[CronJob]: """Load cron jobs from a YAML file. By default this is *tolerant*: a YAML parse failure or an invalid job entry @@ -233,6 +289,16 @@ def load_jobs(jobs_file: Path, strict: bool = False) -> list[CronJob]: instead — used by hot-reload so a malformed file is refused rather than silently unscheduling every job. A missing file is never an error (returns ``[]``) in either mode. + + Pass ``errors`` to collect per-job failures instead of raising or logging + them: each bad entry appends one message and is skipped, and the jobs that + did build are still returned. ``nerve config validate`` needs that, because + raising on the first bad job would hide every problem after it in the same + file, and its whole job is to list them all in one pass. File-level failures + still follow ``strict``. + + ``build_gates=False`` is passed straight to :meth:`CronJob.from_dict`; see + there for who wants it and why. """ if not jobs_file.exists(): logger.info("No cron jobs file at %s", jobs_file) @@ -261,14 +327,18 @@ def load_jobs(jobs_file: Path, strict: bool = False) -> list[CronJob]: jobs = [] for item in jobs_data: try: - jobs.append(CronJob.from_dict(item, base_dir=jobs_file.parent)) + jobs.append(CronJob.from_dict( + item, base_dir=jobs_file.parent, gates=build_gates, + )) except (KeyError, TypeError, ValueError) as e: + message = f"Invalid cron job in {jobs_file}: {item!r} — {e}" + if errors is not None: + errors.append(message) + continue logger.warning("Invalid cron job definition: %s — %s", item, e) if strict: from nerve.config import ConfigError - raise ConfigError( - f"Invalid cron job in {jobs_file}: {item!r} — {e}" - ) from e + raise ConfigError(message) from e logger.info("Loaded %d cron jobs from %s", len(jobs), jobs_file) return jobs diff --git a/nerve/cron/service.py b/nerve/cron/service.py index 8d501a94..c3fb0352 100644 --- a/nerve/cron/service.py +++ b/nerve/cron/service.py @@ -91,13 +91,15 @@ class InvalidScheduleError(ConfigError): """ -def _parse_interval(interval: str) -> int: - """Parse an interval string like '2h', '30m', '1h30m', '0.5h' into seconds. - - The whole string has to be a run of ```` tokens. Anything - else — ``hourly``, ``@daily``, ``???``, ``1h junk`` — is not an interval at - all and gets the 2h default, because the daemon has to keep running: a job - on a conservative cadence beats a scheduler that refused to start. +def _interval_seconds(interval: str) -> int | None: + """Seconds for an interval string like '2h', '30m', '1h30m', '0.5h'. + + ``None`` when the string names no usable interval: either it is not a run of + ```` tokens at all (``hourly``, ``@daily``, ``???``, + ``1h junk``), or it names zero (``0h``, or ``0.4s`` rounding down to it). + The daemon has to keep running either way and substitutes a default (see + :func:`_parse_interval`); config validation, which can still refuse the + file, needs to tell both cases apart from a real '2h'. """ import re # One token, and the same expression used to check that the string is @@ -117,7 +119,7 @@ def _parse_interval(interval: str) -> int: token = r"(\d+(?:\.\d+)?|\.\d+)([hms])" text = interval.strip().lower() if not re.fullmatch(rf"(?:{token}\s*)+", text): - return 7200 # Not an interval at all → default 2h + return None total = 0.0 for value, unit in re.findall(token, text): v = float(value) @@ -129,11 +131,20 @@ def _parse_interval(interval: str) -> int: total += v # Fractions mean what they say ("0.5h" is 1800s), rounded to the nearest # whole second because IntervalTrigger counts seconds and half a second of - # drift on a cadence measured in minutes is noise. Zero takes the default - # too — written ("0h") or rounded down to it ("0.4s") — since - # IntervalTrigger(seconds=0) is a fire-as-fast-as-you-can loop rather than - # a schedule. - return round(total) or 7200 # Default 2h + # drift on a cadence measured in minutes is noise. Zero is not a schedule — + # IntervalTrigger(seconds=0) is a fire-as-fast-as-you-can loop — so a zero, + # written or rounded down to, is reported as no interval at all. + return round(total) or None + + +def _parse_interval(interval: str) -> int: + """Parse an interval string like '2h', '30m', '1h30m', '0.5h' into seconds. + + Falls back to 2h when the string names no usable interval, because the + daemon has to keep running: a job on a conservative cadence beats a + scheduler that refuses to start. + """ + return _interval_seconds(interval) or 7200 # Default 2h # Unix crontab day-of-week numbering is 0=Sun..6=Sat (7 also means Sun). diff --git a/tests/test_config_sources.py b/tests/test_config_sources.py index 0185bca8..27fa9265 100644 --- a/tests/test_config_sources.py +++ b/tests/test_config_sources.py @@ -447,3 +447,41 @@ def test_other_commands_get_a_clean_error(self, tmp_path): assert result.exit_code != 0 assert "must be a mapping" in result.output assert "Traceback" not in result.output + + def test_a_bad_backend_value_is_a_clean_error_too(self, tmp_path): + """The other half of "the config is broken": typing is what rejects an + unknown agent.backend, and it raises a plain ValueError rather than + ConfigError. Re-raising that gave a traceback for a one-word typo.""" + from click.testing import CliRunner + + from nerve.cli import main + + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "agent:\n backend: bogus\n") + + result = CliRunner().invoke(main, ["-c", str(config_dir), "status"]) + + assert result.exit_code != 0 + assert "agent.backend" in result.output + assert "Traceback" not in result.output + assert not isinstance(result.exception, ValueError) + + def test_an_internal_error_keeps_its_traceback(self, tmp_path, monkeypatch): + """The clean-error path is for the operator's config, not for nerve's + own bugs: reporting a defect here as "Error: " sends + someone off to edit a file that was never at fault.""" + import nerve.cli as cli_mod + from click.testing import CliRunner + + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + + def _boom(_dir): + raise RuntimeError("internal defect") + + monkeypatch.setattr(cli_mod, "load_config", _boom) + + result = CliRunner().invoke(cli_mod.main, ["-c", str(config_dir), "status"]) + + assert isinstance(result.exception, RuntimeError) diff --git a/tests/test_config_validate.py b/tests/test_config_validate.py new file mode 100644 index 00000000..42a6197b --- /dev/null +++ b/tests/test_config_validate.py @@ -0,0 +1,1161 @@ +"""Tests for config-bundle validation: validate_config_bundle + CLI.""" + +import textwrap +from pathlib import Path + +import pytest +import yaml + +from nerve.config import NerveConfig +from nerve.config_validate import _WORKING_DIR_PATH_KEYS, validate_config_bundle +from nerve.cron.gates import GATE_REGISTRY + + +def _cfg(tmp_path: Path, base: str = "", workspace: Path | None = None) -> Path: + config_dir = tmp_path / "cfg" + config_dir.mkdir(parents=True, exist_ok=True) + ws = workspace or (tmp_path / "ws") + ws.mkdir(parents=True, exist_ok=True) + text = f"workspace: {ws}\n" + base + (config_dir / "config.yaml").write_text(text, encoding="utf-8") + return config_dir + + +def _settings(workspace: Path, body: str) -> None: + """Write the portable ``/config/settings.yaml`` layer.""" + d = workspace / "config" + d.mkdir(parents=True, exist_ok=True) + (d / "settings.yaml").write_text(body, encoding="utf-8") + + +def _jobs(workspace: Path, jobs: list[dict]) -> None: + cron = workspace / "config" / "cron" + cron.mkdir(parents=True, exist_ok=True) + (cron / "jobs.yaml").write_text(yaml.safe_dump({"jobs": jobs}), encoding="utf-8") + + +def _nested(key: tuple[str, ...], value) -> dict: + """``("a", "b"), 1`` -> ``{"a": {"b": 1}}`` — one config key, written out.""" + out = {key[-1]: value} + for part in reversed(key[:-1]): + out = {part: out} + return out + + +def _gate_plugin(workspace: Path, name: str, body: str) -> Path: + gates = workspace / "config" / "cron" / "gates" + gates.mkdir(parents=True, exist_ok=True) + path = gates / name + path.write_text(textwrap.dedent(body), encoding="utf-8") + return path + + +class TestValidateBundle: + def test_valid_bundle_ok(self, tmp_path): + result = validate_config_bundle(_cfg(tmp_path, "timezone: UTC\n")) + assert result.ok + assert result.errors == [] + + def test_unknown_key_is_warning_by_default(self, tmp_path): + result = validate_config_bundle(_cfg(tmp_path, "tiimezone: UTC\n")) + assert result.ok # forward-compat / example keys shouldn't fail CI + assert any("tiimezone" in w for w in result.warnings) + + def test_unknown_key_is_error_with_strict_keys(self, tmp_path): + result = validate_config_bundle( + _cfg(tmp_path, "tiimezone: UTC\n"), strict_keys=True + ) + assert not result.ok + assert any("tiimezone" in e for e in result.errors) + # The version the check reflects, so CI disagreeing with the instance + # reads as version skew rather than as a bad config. + assert any("this nerve version knows" in i for i in result.info) + + def test_no_version_note_without_unknown_keys(self, tmp_path): + result = validate_config_bundle( + _cfg(tmp_path, "timezone: UTC\n"), strict_keys=True + ) + assert result.ok + assert not any("this nerve version knows" in i for i in result.info) + + def test_backend_misconfig_is_error(self, tmp_path): + result = validate_config_bundle(_cfg(tmp_path, "agent:\n backend: bogus\n")) + assert not result.ok + assert any("backend" in e.lower() or "config error" in e for e in result.errors) + + def test_unset_env_elsewhere_does_not_excuse_a_real_config_error( + self, tmp_path, monkeypatch + ): + """A missing variable downgrades only the errors it causes. + + Secrets are unset in CI, so a bundle naming one would otherwise have + every construction error demoted to a warning — disabling this check in + the environment it exists for. + """ + monkeypatch.delenv("SECRET_X", raising=False) + result = validate_config_bundle( + _cfg(tmp_path, "anthropic_api_key: ${SECRET_X}\nagent:\n backend: bogus\n") + ) + assert not result.ok + assert any("bogus" in e for e in result.errors) + + def test_error_caused_by_the_unset_var_itself_stays_a_warning( + self, tmp_path, monkeypatch + ): + """The literal ``${VAR}`` is not a known backend name, but that says + nothing about the config — with the variable set it never happens.""" + monkeypatch.delenv("NERVE_BACKEND", raising=False) + result = validate_config_bundle( + _cfg(tmp_path, "agent:\n backend: ${NERVE_BACKEND}\n") + ) + assert result.ok + assert any("NERVE_BACKEND" in w for w in result.warnings) + + def test_env_caused_error_that_does_not_quote_the_value_stays_a_warning( + self, tmp_path, monkeypatch + ): + """Classification is by rebuilding without the unresolved refs, not by + looking for ``${`` in the message — this one never mentions the value.""" + monkeypatch.delenv("UC_REV", raising=False) + result = validate_config_bundle(_cfg(tmp_path, textwrap.dedent("""\ + agent: + backend: codex + codex: + ultracode: + revision: ${UC_REV} + """))) + assert result.ok + assert any("UC_REV" in w for w in result.warnings) + + def test_unset_env_on_numeric_field_does_not_hard_fail(self, tmp_path, monkeypatch): + """An unset ${VAR} on an int field must not be a hard error in lenient + mode — it is a consequence of the missing variable, already reported.""" + monkeypatch.delenv("IDLE_T", raising=False) + result = validate_config_bundle( + _cfg(tmp_path, "agent:\n cli_idle_timeout_seconds: ${IDLE_T}\n") + ) + assert result.ok + assert any("IDLE_T" in i for i in result.info) + + def test_strict_env_all_set_passes(self, tmp_path, monkeypatch): + monkeypatch.setenv("MY_KEY", "sk-real") + result = validate_config_bundle( + _cfg(tmp_path, "anthropic_api_key: ${MY_KEY}\n"), strict_env=True + ) + assert result.ok + + def test_unset_env_is_info_by_default(self, tmp_path, monkeypatch): + monkeypatch.delenv("SECRET_X", raising=False) + result = validate_config_bundle( + _cfg(tmp_path, "anthropic_api_key: ${SECRET_X}\n") + ) + assert result.ok # unset secret is fine in CI + assert any("SECRET_X" in i for i in result.info) + + def test_unset_env_is_error_when_strict(self, tmp_path, monkeypatch): + monkeypatch.delenv("SECRET_X", raising=False) + result = validate_config_bundle( + _cfg(tmp_path, "anthropic_api_key: ${SECRET_X}\n"), strict_env=True + ) + assert not result.ok + assert any("SECRET_X" in e for e in result.errors) + + def test_malformed_cron_is_error(self, tmp_path): + ws = tmp_path / "ws" + cron = ws / "config" / "cron" + cron.mkdir(parents=True) + (cron / "jobs.yaml").write_text("jobs: [ broken: yaml: here\n", encoding="utf-8") + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + assert not result.ok + assert any("jobs.yaml" in e or "parse" in e.lower() for e in result.errors) + + def test_invalid_job_is_error(self, tmp_path): + ws = tmp_path / "ws" + cron = ws / "config" / "cron" + cron.mkdir(parents=True) + # Missing prompt/prompt_file → CronJob.__post_init__ raises. + (cron / "jobs.yaml").write_text( + yaml.safe_dump({"jobs": [{"id": "bad", "schedule": "1h"}]}), + encoding="utf-8", + ) + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + assert not result.ok + + def test_valid_cron_reported(self, tmp_path): + ws = tmp_path / "ws" + cron = ws / "config" / "cron" + cron.mkdir(parents=True) + (cron / "jobs.yaml").write_text( + yaml.safe_dump({"jobs": [{"id": "ok", "schedule": "1h", "prompt": "hi"}]}), + encoding="utf-8", + ) + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + assert result.ok + assert any("1 job" in i for i in result.info) + + def test_workspace_override_reads_repo_settings(self, tmp_path): + """--workspace points validation at a checked-out config repo.""" + repo = tmp_path / "repo" + (repo / "config").mkdir(parents=True) + (repo / "config" / "settings.yaml").write_text( + "tiimezone: UTC\n", encoding="utf-8" # typo in the shared settings + ) + # config_dir has no config.yaml at all (just like a bare repo checkout) + config_dir = tmp_path / "empty" + config_dir.mkdir() + result = validate_config_bundle(config_dir, workspace_override=repo, strict_keys=True) + assert not result.ok + assert any("tiimezone" in e for e in result.errors) + + +class TestGatePluginsAreNotExecuted: + """Checking a bundle must not be the same act as running it. + + Gate plugins are ordinary ``.py`` files the daemon imports at startup. If + validation imported them too, then by the time it decided a bundle was + unfit, the bundle's Python would already have run — under the daemon's uid, + env and network — so "an invalid bundle is refused" would guarantee nothing. + """ + + #: A plugin that records the fact it ran, then declares a perfectly good gate. + MARKER_PLUGIN = ''' + import pathlib + pathlib.Path({marker!r}).write_text("executed") + + from nerve.cron.gates import CronGate + + class MarkerGate(CronGate): + type = "marker_test" + + async def is_satisfied(self, ctx): + return True + + def describe(self): + return "marker" + + @classmethod + def from_config(cls, spec): + return cls() + ''' + + def test_validating_does_not_run_plugin_code(self, tmp_path): + marker = tmp_path / "EXECUTED" + ws = tmp_path / "ws" + _gate_plugin(ws, "marker.py", self.MARKER_PLUGIN.format(marker=str(marker))) + _jobs(ws, [{ + "id": "g", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "marker_test"}], + }]) + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert not marker.exists(), "validation executed the candidate plugin" + assert "marker_test" not in GATE_REGISTRY + # The gate type can't be confirmed without loading the plugin, so it is + # reported — not accepted, and not failed. + assert result.ok, result.errors + assert any("marker_test" in w for w in result.warnings) + + +class TestWorkingDirectoryPaths: + """An explicit ``.`` is refused; a blank value is the documented default. + + ``.``, ``./`` and ``./.`` survive ``config._expand_path`` as the path they + are, so they really do point nerve at whichever directory the daemon happened + to be started in. The sharp one is ``cron.gate_plugins_dir``, which the daemon + imports every ``*.py`` from at startup and again on every reload: one + character turns the working directory into a source of code the daemon + executes. + + Blank is the opposite case and must not be reported at all. ``_expand_path`` + maps it to ``None`` before it can become ``Path(".")`` and the field takes its + documented default, so flagging it would fail a bundle that runs correctly — + ``jobs_file: ${CRON_JOBS_FILE:-}`` is the shape that turns up, and the docs + promote that idiom. + """ + + def _validate(self, tmp_path: Path, settings: str): + ws = tmp_path / "ws" + _settings(ws, settings) + return validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + def test_dot_gate_plugins_dir_is_error(self, tmp_path): + for value in (".", "./", "./."): + result = self._validate( + tmp_path, f"cron:\n gate_plugins_dir: '{value}'\n" + ) + [err] = [e for e in result.errors if "gate_plugins_dir" in e] + # The message has to say what the setting would *do*. "Invalid path" + # would leave the reader thinking they had a typo, not a + # code-execution path into the daemon. + assert "working directory" in err, value + assert "executed" in err, value + + def test_blank_gate_plugins_dir_is_accepted(self, tmp_path): + """Blank, whitespace-only and a bare key all mean the default.""" + for value in ("''", "' '", ""): + result = self._validate( + tmp_path, f"cron:\n gate_plugins_dir: {value}\n" + ) + assert result.ok, (value, result.errors) + + def test_dot_workspace_is_error(self, tmp_path): + """The worst one: the whole tree nerve reads and writes moves to the cwd. + + Flagged even though this run pins the workspace itself — the pin says + where to look for the checkout, it does not excuse the value in the + bundle. + """ + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + config_dir = tmp_path / "cfg" + config_dir.mkdir(parents=True, exist_ok=True) + (config_dir / "config.yaml").write_text("workspace: .\n", encoding="utf-8") + result = validate_config_bundle(config_dir, workspace_override=ws) + assert not result.ok + assert any(e.startswith("workspace: ") for e in result.errors) + + def test_blank_workspace_is_accepted(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + config_dir = tmp_path / "cfg" + config_dir.mkdir(parents=True, exist_ok=True) + (config_dir / "config.yaml").write_text("workspace: ''\n", encoding="utf-8") + result = validate_config_bundle(config_dir, workspace_override=ws) + assert result.ok, result.errors + + def test_dot_cron_file_names_the_key(self, tmp_path): + """These already fail — as ``Is a directory: '.'``, which names neither + the setting at fault nor what it meant.""" + for key in ("jobs_file", "system_file"): + result = self._validate(tmp_path, f"cron:\n {key}: '.'\n") + assert any(e.startswith(f"cron.{key}: ") for e in result.errors), key + + def test_blank_cron_file_resolves_to_the_default(self, tmp_path): + """The loader's contract, checked from the validator's side. + + A blank ``jobs_file`` loads ``/config/cron/jobs.yaml``; reporting it + would have the two disagree about the same bundle. + """ + for key in ("jobs_file", "system_file"): + result = self._validate(tmp_path, f"cron:\n {key}: ''\n") + assert result.ok, (key, result.errors) + + def test_explicit_path_is_accepted(self, tmp_path): + result = self._validate( + tmp_path, "cron:\n gate_plugins_dir: /opt/nerve/gates\n" + ) + assert result.ok, result.errors + + def test_unset_env_ref_in_a_path_is_not_flagged(self, tmp_path, monkeypatch): + """An unset ``${VAR}`` stays literal and is reported as a missing var — + the lenient CI case, not a working-directory setting.""" + monkeypatch.delenv("GATES_DIR", raising=False) + result = self._validate( + tmp_path, "cron:\n gate_plugins_dir: ${GATES_DIR}\n" + ) + assert result.ok, result.errors + assert any("GATES_DIR" in i for i in result.info) + + def test_env_default_expanding_to_empty_is_accepted(self, tmp_path, monkeypatch): + """``${VAR:-}`` is how the docs say to make a fleet setting optional, and + an unset variable then leaves the field on its default.""" + monkeypatch.delenv("GATES_DIR", raising=False) + result = self._validate( + tmp_path, "cron:\n gate_plugins_dir: ${GATES_DIR:-}\n" + ) + assert result.ok, result.errors + + def test_env_default_expanding_to_a_dot_is_error(self, tmp_path, monkeypatch): + """A gate that only read literals would wave this one through.""" + monkeypatch.delenv("GATES_DIR", raising=False) + result = self._validate( + tmp_path, "cron:\n gate_plugins_dir: ${GATES_DIR:-.}\n" + ) + assert not result.ok + [err] = [e for e in result.errors if "gate_plugins_dir" in e] + # Both texts, or the author cannot see which of the two is at fault. + assert "${GATES_DIR:-.}" in err + assert "expands to '.'" in err + + +def _declared_path_keys(klass, prefix: tuple[str, ...] = ()) -> list[tuple[str, ...]]: + """Dotted keys of every ``Path`` / ``Path | None`` field under *klass*. + + Read off ``typing.get_type_hints``, walking into nested config dataclasses + (including ones reached through ``X | None`` or ``list[X]``), so the answer + comes from what the config declares rather than from what any consumer of it + happens to handle. + """ + import dataclasses + import typing + + keys: list[tuple[str, ...]] = [] + hints = typing.get_type_hints(klass) + for f in dataclasses.fields(klass): + declared = hints.get(f.name) + key = (*prefix, f.name) + if declared is Path or declared == (Path | None): + keys.append(key) + continue + for arg in (declared, *typing.get_args(declared)): + if isinstance(arg, type) and dataclasses.is_dataclass(arg): + keys.extend(_declared_path_keys(arg, key)) + return keys + + +class TestWorkingDirPathCoverage: + """The rule is a hand-written literal; the settings it must cover are not. + + Every ``Path`` setting missing from ``_WORKING_DIR_PATH_KEYS`` is one where an + explicit ``.`` passes validation and lands the daemon on its working directory + anyway — which is how ``gateway.ssl.cert`` and ``proxy.auth_dir`` went + uncovered while the check's own tests stayed green. So the field set here is + derived from the config dataclasses' annotations, never from the rule under + test, which would only agree with itself. + """ + + # Path settings the working-directory rule cannot apply to, with the reason. + EXEMPT = { + # Not a bundle key at all: load_config overwrites it with the directory + # the config was read from, so no written value ever reaches the check. + ("config_dir",): "set by load_config, not by the config", + } + + def test_every_path_setting_is_covered_or_exempt(self): + found = _declared_path_keys(NerveConfig) + assert len(found) >= 11, ( + f"the walk reached only {len(found)} Path settings — did it break?" + ) + uncovered = sorted(set(found) - set(_WORKING_DIR_PATH_KEYS) - set(self.EXEMPT)) + assert not uncovered, ( + "Path settings that neither the working-directory rule nor the " + "exemptions above account for — add a consequence to " + "_WORKING_DIR_PATH_KEYS, or exempt it here with the reason:\n" + + "\n".join(".".join(k) for k in uncovered) + ) + + def test_the_rule_names_only_real_settings(self): + """A key that matches no field silently checks nothing forever.""" + found = set(_declared_path_keys(NerveConfig)) + unknown = sorted(set(_WORKING_DIR_PATH_KEYS) - found) + assert not unknown, ( + "_WORKING_DIR_PATH_KEYS entries that are not Path settings of the " + "config (renamed? misspelled?):\n" + + "\n".join(".".join(k) for k in unknown) + ) + + def test_every_covered_key_refuses_an_explicit_dot(self, tmp_path): + """The listing is only half of it; each key has to actually be read. + + ``gateway.ssl.cert`` is three levels deep and ``workspace`` is only ever + set in the machine layer, so a rule entry can be correct and still never + be reached. + """ + for i, key in enumerate(_WORKING_DIR_PATH_KEYS): + ws = tmp_path / f"ws{i}" + _settings(ws, "timezone: UTC\n") + config_dir = tmp_path / f"cfg{i}" + config_dir.mkdir(parents=True, exist_ok=True) + data = _nested(key, ".") + data.setdefault("workspace", str(ws)) + (config_dir / "config.yaml").write_text( + yaml.safe_dump(data), encoding="utf-8" + ) + result = validate_config_bundle(config_dir, workspace_override=ws) + dotted = ".".join(key) + assert any(e.startswith(f"{dotted}: ") for e in result.errors), ( + dotted, result.errors, + ) + + +class TestScheduleChecking: + """A schedule the daemon won't run as written must fail here, not there. + + This is the earliest and cheapest gate on the config repo. Everything it + misses is caught — at best — after the change has merged and synced, with + the instance stuck on its old config until someone reads a log line. + """ + + def _run(self, tmp_path, schedule, base=""): + ws = tmp_path / "ws" + _jobs(ws, [{"id": "j", "schedule": schedule, "prompt": "hi"}]) + return validate_config_bundle( + _cfg(tmp_path, base, workspace=ws), workspace_override=ws, + ) + + def test_bad_crontab_field_is_an_error(self, tmp_path): + result = self._run(tmp_path, "99 * * * *") + + assert not result.ok + assert any("99 * * * *" in e for e in result.errors) + + def test_bad_day_of_week_is_an_error(self, tmp_path): + result = self._run(tmp_path, "0 3 * * 9") + + assert not result.ok + + def test_the_error_names_the_job(self, tmp_path): + result = self._run(tmp_path, "99 * * * *") + + assert any("job 'j'" in e for e in result.errors) + + def test_every_bad_job_is_reported_not_just_the_first(self, tmp_path): + ws = tmp_path / "ws" + _jobs(ws, [ + {"id": "a", "schedule": "99 * * * *", "prompt": "hi"}, + {"id": "b", "schedule": "* 99 * * *", "prompt": "hi"}, + {"id": "c", "schedule": "0 3 * * *", "prompt": "hi"}, + ]) + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, + ) + + assert not result.ok + assert sum("job 'a'" in e or "job 'b'" in e for e in result.errors) == 2 + assert not any("job 'c'" in e for e in result.errors) + + def test_a_bad_schedule_does_not_hide_the_job_s_other_faults(self, tmp_path): + ws = tmp_path / "ws" + _jobs(ws, [{ + "id": "j", "schedule": "99 * * * *", "prompt": "hi", + "run_if": [{"type": "tasks", "min_count": "lots"}], + }]) + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, + ) + + assert any("99 * * * *" in e for e in result.errors) + assert any("min_count" in e for e in result.errors) + + @pytest.mark.parametrize( + "schedule", ["4h", "30m", "1h30m", "90s", "*/15 * * * *", "0 3 * * 1-5"], + ) + def test_valid_schedules_pass(self, tmp_path, schedule): + result = self._run(tmp_path, schedule) + + assert result.ok, result.errors + + @pytest.mark.parametrize("schedule", ["hourly", "every day", "@daily", "???"]) + def test_a_schedule_that_is_neither_form_is_an_error(self, tmp_path, schedule): + """Not a warning: the runtime never complains about these either. + + The interval parser finds no h/m/s token and substitutes its default, + so the job runs — just not on the cadence anyone wrote down. A warning + would leave `ok` True and merge it. + """ + result = self._run(tmp_path, schedule) + + assert not result.ok + assert any("neither" in e and repr(schedule) in e for e in result.errors) + + def test_a_non_string_schedule_is_an_error(self, tmp_path): + """`schedule: 4` reaches the scheduler as an int, whose first move is + .split() — an AttributeError no runtime handler catches.""" + result = self._run(tmp_path, 4) + + assert not result.ok + assert any("must be a string" in e for e in result.errors) + + def test_bad_source_schedule_is_an_error(self, tmp_path): + result = self._run( + tmp_path, "1h", "sync:\n gmail:\n schedule: '99 * * * *'\n", + ) + + assert not result.ok + assert any("sync.gmail.schedule" in e for e in result.errors) + + def test_valid_source_schedule_passes(self, tmp_path): + result = self._run( + tmp_path, "1h", "sync:\n telegram:\n schedule: '*/5 * * * *'\n", + ) + + assert result.ok, result.errors + + def test_unset_env_ref_in_a_schedule_is_not_flagged(self, tmp_path, monkeypatch): + """The missing variable is already reported; the literal ${VAR} text is + not a second, separate mistake.""" + monkeypatch.delenv("SYNC_SCHED", raising=False) + result = self._run( + tmp_path, "1h", "sync:\n gmail:\n schedule: ${SYNC_SCHED}\n", + ) + + assert result.ok, result.errors + assert any("SYNC_SCHED" in i for i in result.info) + + +class TestGateSpecChecking: + """Three cases, and only three: structure is always enforced, a built-in + type is checked in full, anything else is reported as unverified.""" + + def _run(self, tmp_path, run_if): + ws = tmp_path / "ws" + _jobs(ws, [{"id": "g", "schedule": "1h", "prompt": "hi", "run_if": run_if}]) + return validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + def test_builtin_gate_spec_is_checked_in_full(self, tmp_path): + result = self._run(tmp_path, [{"type": "tasks", "min_count": "lots"}]) + + assert not result.ok + assert any("min_count" in e for e in result.errors) + + def test_valid_builtin_gate_passes(self, tmp_path): + result = self._run(tmp_path, [{"type": "tasks", "status": "pending"}]) + + assert result.ok, result.errors + assert result.warnings == [] + + def test_spec_that_is_not_a_mapping_is_an_error(self, tmp_path): + result = self._run(tmp_path, ["tasks"]) + + assert not result.ok + assert any("mapping" in e for e in result.errors) + + def test_spec_without_a_type_is_an_error(self, tmp_path): + result = self._run(tmp_path, [{"status": "pending"}]) + + assert not result.ok + assert any("'type'" in e for e in result.errors) + + def test_non_string_type_is_an_error(self, tmp_path): + result = self._run(tmp_path, [{"type": 7}]) + + assert not result.ok + assert any("must be a string" in e for e in result.errors) + + def test_run_if_that_is_not_a_list_is_an_error(self, tmp_path): + result = self._run(tmp_path, "tasks") + + assert not result.ok + assert any("run_if" in e for e in result.errors) + + def test_plugin_provided_type_is_a_warning_not_an_error(self, tmp_path): + """A type a plugin supplies is legitimate and common; failing it would + make the gate unusable for anyone with a custom gate.""" + result = self._run(tmp_path, [{"type": "stale_tasks", "min_age_minutes": 60}]) + + assert result.ok, result.errors + assert any("stale_tasks" in w for w in result.warnings) + + def test_the_warning_states_the_runtime_consequence(self, tmp_path): + """Warning, not error, was a deliberate call — so the text is the whole + safety net. A typo'd built-in ('taks') costs the job: nothing registers + the type, so it does not load at all.""" + result = self._run(tmp_path, [{"type": "taks"}]) + + assert result.ok + assert any("does not load" in w for w in result.warnings) + + def test_blank_type_is_an_error(self, tmp_path): + result = self._run(tmp_path, [{"type": " "}]) + + assert not result.ok + + def test_unhashable_type_does_not_abort_the_rest_of_the_file(self, tmp_path): + """A list under `type:` is unhashable; the registry lookup used to raise + TypeError, failing the whole file and hiding every job after it.""" + ws = tmp_path / "ws" + _jobs(ws, [ + {"id": "first", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": ["tasks"]}]}, + {"id": "second", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "tasks", "min_count": "lots"}]}, + ]) + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert any("first" in e for e in result.errors) + assert any("second" in e and "min_count" in e for e in result.errors) + + def test_empty_run_if_key_is_not_an_error(self, tmp_path): + """`run_if:` with nothing under it parses to None and used to take the + whole job down with it.""" + result = self._run(tmp_path, None) + + assert result.ok, result.errors + + def test_a_falsy_wrong_shape_is_reported_not_treated_as_absent(self, tmp_path): + """`run_if: {}` is not "no gates". Normalizing every falsy value to [] + made a mistyped gate block indistinguishable from an ungated job: the + job ran unconditionally and this reported the bundle clean.""" + result = self._run(tmp_path, {}) + + assert not result.ok + assert any("run_if" in e and "dict" in e for e in result.errors) + + def test_a_scalar_run_if_does_not_abort_the_rest_of_the_file(self, tmp_path): + """A non-iterable under `run_if:` used to raise out of job construction, + failing the whole file and hiding every job after it.""" + ws = tmp_path / "ws" + _jobs(ws, [ + {"id": "first", "schedule": "1h", "prompt": "hi", "run_if": 0}, + {"id": "second", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "tasks", "min_count": "lots"}]}, + ]) + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert any("first" in e and "run_if" in e for e in result.errors) + assert any("second" in e and "min_count" in e for e in result.errors) + + def test_one_pass_reports_every_bad_job_in_the_file(self, tmp_path): + """Validation collects per-job construction failures instead of raising + on the first one. + + A job that cannot be constructed at all — no prompt, an invalid workflow + block — makes ``load_jobs(strict=True)`` raise, and validation answers a + raise by abandoning the file. So the first such job used to hide every + problem after it, including problems in jobs that are perfectly loadable. + One pass has to list everything, or fixing a bundle becomes a sequence of + CI rounds that each reveal one more line. + + Two failures that stop construction, then two that only surface once the + job exists, then a job with nothing wrong — all in one file. + """ + ws = tmp_path / "ws" + _jobs(ws, [ + {"id": "no-prompt", "schedule": "1h"}, + {"id": "bad-workflow", "schedule": "1h", + "workflow": {"engine": "claude", "prompt": "go", "budget_usd": 0}}, + {"id": "bad-gate", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "tasks", "min_count": "lots"}]}, + {"id": "bad-schedule", "schedule": "99 * * * *", "prompt": "hi"}, + {"id": "fine", "schedule": "1h", "prompt": "hi"}, + ]) + + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, + ) + + assert not result.ok + for job_id in ("no-prompt", "bad-workflow", "bad-gate", "bad-schedule"): + assert any(job_id in e for e in result.errors), ( + f"{job_id} went unreported — errors: {result.errors}" + ) + # The three that did construct are still counted, so a failure to build + # skips its job rather than derailing the file. + assert any("3 job(s)" in i for i in result.info), result.info + + def test_unknown_field_in_a_builtin_gate_spec_is_reported(self, tmp_path): + """The quietest gate mistake: from_config reads with .get(), so a typo + takes the default and the gate checks something else entirely.""" + result = self._run(tmp_path, [{"type": "tasks", "statuz": "pending"}]) + + assert result.ok # a warning by default, like every other unknown key + assert any("statuz" in w for w in result.warnings) + + def test_unknown_gate_field_is_an_error_under_strict_keys(self, tmp_path): + ws = tmp_path / "ws" + _jobs(ws, [{ + "id": "g", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "tasks", "statuz": "pending"}], + }]) + + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, strict_keys=True, + ) + + assert not result.ok + assert any("statuz" in e for e in result.errors) + + +class TestLegacyIdleShorthand: + """`skip_when_idle` is synthesized into a messages gate at load time, so it + needs the same checking as run_if — and one shape more.""" + + def _run(self, tmp_path, job_extra): + ws = tmp_path / "ws" + _jobs(ws, [{"id": "g", "schedule": "1h", "prompt": "hi", **job_extra}]) + return validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + def test_a_bare_string_is_an_error(self, tmp_path): + """`skip_when_idle: gmail` becomes sources ['g','m','a','i','l'] — no + source ever matches, so the job silently never runs again.""" + result = self._run(tmp_path, {"skip_when_idle": "gmail"}) + + assert not result.ok + assert any("skip_when_idle" in e for e in result.errors) + + def test_a_list_of_non_strings_is_an_error(self, tmp_path): + result = self._run(tmp_path, {"skip_when_idle": [1, 2]}) + + assert not result.ok + assert any("skip_when_idle" in e for e in result.errors) + + def test_a_proper_list_passes(self, tmp_path): + result = self._run(tmp_path, {"skip_when_idle": ["gmail"]}) + + assert result.ok, result.errors + assert result.warnings == [] + + def test_an_empty_list_is_not_an_error(self, tmp_path): + """An explicit empty list, like a bare key, means "don't check".""" + result = self._run(tmp_path, {"skip_when_idle": []}) + + assert result.ok, result.errors + + def test_a_falsy_wrong_shape_is_reported(self, tmp_path): + """`skip_when_idle: {}` used to be normalized to [] on the way in and + then skipped here as "not set" — the mistyped block disappeared twice + over, and the job ran with no idle check at all.""" + result = self._run(tmp_path, {"skip_when_idle": {}}) + + assert not result.ok + assert any("skip_when_idle" in e for e in result.errors) + + def test_one_warning_per_distinct_type(self, tmp_path): + ws = tmp_path / "ws" + _jobs(ws, [ + {"id": "a", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "custom_one"}]}, + {"id": "b", "schedule": "1h", "prompt": "hi", + "run_if": [{"type": "custom_one"}, {"type": "custom_two"}]}, + ]) + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert result.ok, result.errors + assert len([w for w in result.warnings if "custom_one" in w]) == 1 + assert len([w for w in result.warnings if "custom_two" in w]) == 1 + + +class TestBadLayerIsReportedNotRaised: + """Every layer is read before anything is type-checked, so a parse error + there has to come back as a validation error and not a traceback.""" + + def test_unparseable_config_yaml(self, tmp_path): + config_dir = _cfg(tmp_path) + (config_dir / "config.yaml").write_text("a: [1,\nb: }{\n", encoding="utf-8") + + result = validate_config_bundle(config_dir) + + assert not result.ok + assert any("config.yaml" in e for e in result.errors) + + def test_unparseable_config_local_yaml(self, tmp_path): + config_dir = _cfg(tmp_path) + (config_dir / "config.local.yaml").write_text("x: [1,\ny: }{\n", encoding="utf-8") + + result = validate_config_bundle(config_dir) + + assert not result.ok + assert any("config.local.yaml" in e for e in result.errors) + + def test_unparseable_settings_yaml(self, tmp_path): + """The CI case: a YAML typo in the shared, reviewed layer.""" + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: [claude\n") + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert not result.ok + assert any("settings.yaml" in e for e in result.errors) + + def test_settings_yaml_of_the_wrong_shape(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "- not\n- a mapping\n") + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert not result.ok + assert any("settings.yaml" in e for e in result.errors) + + def test_cli_prints_the_error_not_a_traceback(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: [claude\n") + config_dir = _cfg(tmp_path, workspace=ws) + + result = CliRunner().invoke( + main, + ["-c", str(config_dir), "config", "validate", "--workspace", str(ws)], + ) + + assert result.exit_code == 1 + assert "Traceback" not in result.output + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Config invalid" in result.output + + +class TestPortableOnly: + """A change headed for a shared repo has to be judged on what it carries, + not on what the machine reviewing it happens to have lying around.""" + + def test_machine_override_masks_a_shared_error_by_default(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: bogus\n") + config_dir = _cfg(tmp_path, "agent:\n backend: claude\n", workspace=ws) + + result = validate_config_bundle(config_dir, workspace_override=ws) + + assert result.ok # the local override wins the merge, as it does at load + + def test_portable_only_sees_the_shared_error(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: bogus\n") + config_dir = _cfg(tmp_path, "agent:\n backend: claude\n", workspace=ws) + + result = validate_config_bundle( + config_dir, workspace_override=ws, portable_only=True, + ) + + assert not result.ok + assert any("bogus" in e for e in result.errors) + + def test_portable_only_ignores_a_broken_machine_local_layer(self, tmp_path): + """The inverse, and the commoner one: a host whose own secrets file is + broken must still be able to tell that a shared bundle is fine.""" + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + config_dir = _cfg(tmp_path, workspace=ws) + (config_dir / "config.local.yaml").write_text("x: [1,\ny: }{\n", encoding="utf-8") + + overlaid = validate_config_bundle(config_dir, workspace_override=ws) + assert not overlaid.ok # today's behaviour: blames the incoming change + + portable = validate_config_bundle( + config_dir, workspace_override=ws, portable_only=True, + ) + assert portable.ok, portable.errors + + def test_overlaid_machine_layers_are_named_in_the_report(self, tmp_path): + ws = tmp_path / "ws" + config_dir = _cfg(tmp_path, workspace=ws) + (config_dir / "config.local.yaml").write_text("timezone: UTC\n", encoding="utf-8") + + result = validate_config_bundle(config_dir, workspace_override=ws) + + note = "\n".join(result.info) + assert "config.yaml" in note and "config.local.yaml" in note + + def test_portable_only_says_the_machine_layers_were_skipped(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, portable_only=True, + ) + + assert result.ok, result.errors + assert any("machine-local" in i for i in result.info) + + def test_portable_only_on_a_workspace_with_no_portable_config_fails(self, tmp_path): + """Even with the workspace named explicitly: judging the portable layer + of a tree that has none is a gate that checked nothing.""" + ws = tmp_path / "ws" + ws.mkdir() + + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, portable_only=True, + ) + + assert not result.ok + assert any("nothing to validate" in e for e in result.errors) + + def test_portable_only_fails_on_a_config_dir_that_holds_nothing_read(self, tmp_path): + """The day-one layout mistakes: an empty config/, a .yml extension, or + settings.yaml left at the repo root. The directory exists; the gate + still read nothing, and a permanently green CI job is the worst of the + available outcomes.""" + for name, place in ( + ("empty", None), + ("wrong-ext", "config/settings.yml"), + ("misplaced", "settings.yaml"), + ): + ws = tmp_path / name + (ws / "config").mkdir(parents=True) + if place: + (ws / place).write_text("agent:\n backend: bogus\n", encoding="utf-8") + + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, portable_only=True, + ) + + assert not result.ok, name + assert any("nothing to validate" in e for e in result.errors), name + + def test_portable_only_passes_on_a_workspace_with_only_cron(self, tmp_path): + """A repo may legitimately carry cron and no settings.yaml — that is a + bundle, and it was read.""" + ws = tmp_path / "ws" + _jobs(ws, [{"id": "ok", "schedule": "1h", "prompt": "hi"}]) + + result = validate_config_bundle( + _cfg(tmp_path, workspace=ws), workspace_override=ws, portable_only=True, + ) + + assert result.ok, result.errors + + def test_the_reported_paths_are_absolute(self, tmp_path, monkeypatch): + """`--workspace .` must not report `config/settings.yaml`, which + answers none of the questions the line exists to answer.""" + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + config_dir = _cfg(tmp_path, workspace=ws) + monkeypatch.chdir(ws) + + result = validate_config_bundle(config_dir, workspace_override=Path(".")) + + assert any(str(ws.resolve() / "config" / "settings.yaml") in i for i in result.info) + + +class TestWrongShapedLayer: + """A layer that parses to the wrong shape is dropped in lenient mode, and + everything it was supposed to supply reverts to the layer below — including + the workspace path, so validation walks off to a different tree.""" + + def test_machine_layer_that_is_a_list_is_an_error(self, tmp_path): + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: bogus\n") + config_dir = _cfg(tmp_path, workspace=ws) + (config_dir / "config.yaml").write_text("- a\n- b\n", encoding="utf-8") + + result = validate_config_bundle(config_dir) + + assert not result.ok + assert any("config.yaml" in e for e in result.errors) + + def test_local_layer_that_is_a_scalar_is_an_error(self, tmp_path): + config_dir = _cfg(tmp_path) + (config_dir / "config.local.yaml").write_text("just a string\n", encoding="utf-8") + + result = validate_config_bundle(config_dir) + + assert not result.ok + assert any("config.local.yaml" in e for e in result.errors) + + def test_an_unusable_layer_is_not_reported_as_overlaid(self, tmp_path): + config_dir = _cfg(tmp_path) + (config_dir / "config.yaml").write_text("- a\n", encoding="utf-8") + + result = validate_config_bundle(config_dir) + + assert not any("overlaid" in i for i in result.info) + + def test_the_workspace_that_was_read_is_always_named(self, tmp_path): + """Whatever the flags, the report has to say which tree it looked at.""" + ws = tmp_path / "ws" + _settings(ws, "timezone: UTC\n") + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert any(str(ws / "config" / "settings.yaml") in i for i in result.info) + + def test_a_missing_portable_layer_is_named_as_missing(self, tmp_path): + ws = tmp_path / "ws" + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) + + assert any("not present" in i for i in result.info) + + def test_portable_only_without_a_workspace_does_not_pass_silently( + self, tmp_path, monkeypatch, + ): + """The worst outcome for a CI gate: green, and it read nothing. Dropping + the machine layers also drops the workspace path they carried.""" + elsewhere = tmp_path / "not-the-checkout" + monkeypatch.setattr( + "nerve.paths.default_workspace", lambda: elsewhere, + ) + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: bogus\n") + + result = validate_config_bundle(_cfg(tmp_path, workspace=ws), portable_only=True) + + assert not result.ok + assert any("nothing to validate" in e for e in result.errors) + assert any(str(elsewhere) in w for w in result.warnings) + + def test_cli_portable_only_flag(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + ws = tmp_path / "ws" + _settings(ws, "agent:\n backend: bogus\n") + config_dir = _cfg(tmp_path, "agent:\n backend: claude\n", workspace=ws) + args = ["-c", str(config_dir), "config", "validate", "--workspace", str(ws)] + + assert CliRunner().invoke(main, args).exit_code == 0 + strict = CliRunner().invoke(main, [*args, "--portable-only"]) + assert strict.exit_code == 1 + assert "bogus" in strict.output + + +class TestFlagsAreDocumented: + """`--strict-keys` shipped undocumented while the docs claimed its behaviour + was the default. Keep every flag of the command visible in the docs.""" + + def test_every_option_appears_in_docs_config_md(self): + from nerve.cli import config_validate + + doc = (Path(__file__).resolve().parent.parent / "docs" / "config.md").read_text() + flags = [ + opt for param in config_validate.params + for opt in getattr(param, "opts", []) if opt.startswith("--") + ] + assert flags + assert [f for f in flags if f not in doc] == [] + + +class TestValidateCli: + def test_cli_ok_exit_zero(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + config_dir = _cfg(tmp_path, "timezone: UTC\n") + result = CliRunner().invoke(main, ["-c", str(config_dir), "config", "validate"]) + assert result.exit_code == 0, result.output + assert "Config OK" in result.output + + def test_cli_bad_exit_nonzero(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + # A genuine error (bad backend) fails regardless of key strictness. + config_dir = _cfg(tmp_path, "agent:\n backend: bogus\n") + result = CliRunner().invoke(main, ["-c", str(config_dir), "config", "validate"]) + assert result.exit_code != 0 + assert "[ERR]" in result.output + + def test_cli_strict_keys_flag(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + config_dir = _cfg(tmp_path, "tiimezone: UTC\n") + ok = CliRunner().invoke(main, ["-c", str(config_dir), "config", "validate"]) + assert ok.exit_code == 0 # warning only by default + strict = CliRunner().invoke( + main, ["-c", str(config_dir), "config", "validate", "--strict-keys"] + ) + assert strict.exit_code != 0 + + def test_cli_unset_env_does_not_crash_group(self, tmp_path, monkeypatch): + """The CI scenario: a required ${VAR} is unset, but `config validate` + must still run (the group callback must not hard-fail).""" + from click.testing import CliRunner + + from nerve.cli import main + + monkeypatch.delenv("MISSING_SECRET", raising=False) + config_dir = _cfg(tmp_path, "anthropic_api_key: ${MISSING_SECRET}\n") + result = CliRunner().invoke(main, ["-c", str(config_dir), "config", "validate"]) + assert result.exit_code == 0, result.output + assert "MISSING_SECRET" in result.output + + def test_cli_doctor_reports_unloadable_config(self, tmp_path, monkeypatch): + from click.testing import CliRunner + + from nerve.cli import main + + monkeypatch.delenv("MISSING_SECRET", raising=False) + config_dir = _cfg(tmp_path, "anthropic_api_key: ${MISSING_SECRET}\n") + result = CliRunner().invoke(main, ["-c", str(config_dir), "doctor"]) + assert result.exit_code == 1 + assert "MISSING_SECRET" in result.output + assert "Traceback" not in result.output diff --git a/tests/test_cron_gates.py b/tests/test_cron_gates.py index 1dbedb0f..3d9bb1e7 100644 --- a/tests/test_cron_gates.py +++ b/tests/test_cron_gates.py @@ -373,6 +373,40 @@ def test_from_dict_parses_run_if(self): assert len(job.gates) == 1 assert isinstance(job.gates[0], TasksGate) + def test_from_dict_normalizes_only_a_bare_key(self): + """A bare `run_if:` is None and means "no gates". Nothing else does: + a wrong shape has to survive on the job so `nerve config validate` can + reject it, instead of turning into an ungated job that looks correct.""" + bare = CronJob.from_dict({"id": "x", "schedule": "1h", "prompt": "p", + "run_if": None, "skip_when_idle": None}) + assert bare.run_if == [] and bare.skip_when_idle == [] + + # gates=False is how validation loads a bundle: the shape is kept intact + # for inspection rather than built (and refused). + for shape in ({}, "", 0, "tasks", {"type": "tasks"}): + job = CronJob.from_dict({ + "id": "x", "schedule": "1h", "prompt": "p", "run_if": shape, + }, gates=False) + assert job.run_if == shape, f"{shape!r} was normalized away" + job = CronJob.from_dict({ + "id": "x", "schedule": "1h", "prompt": "p", + "skip_when_idle": shape, + }, gates=False) + assert job.skip_when_idle == shape, f"{shape!r} was normalized away" + + def test_a_wrong_shape_is_refused_not_run_unguarded(self): + """__post_init__ builds the gates, so this drops the whole job — which is + the intent. A gate block nobody can read is not permission to run.""" + with pytest.raises(GateConfigError, match="run_if"): + CronJob.from_dict({ + "id": "x", "schedule": "1h", "prompt": "p", "run_if": 0, + }) + with pytest.raises(GateConfigError, match="skip_when_idle"): + CronJob.from_dict({ + "id": "x", "schedule": "1h", "prompt": "p", + "skip_when_idle": {}, + }) + # --------------------------------------------------------------------------- # GitHubPrActivityGate