From 9cf71b27135ba61451aa7b54b20a2d89628db334 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 28 Jul 2026 11:12:41 +0200 Subject: [PATCH 1/2] =?UTF-8?q?nerve=20config=20validate=20=E2=80=94=20CI-?= =?UTF-8?q?ready=20config=20bundle=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hard-failing validation over a whole bundle — unknown keys, cron job and gate parsing, source sanity and ${VAR} resolvability — with a non-zero exit so it drops straight into a config repo's CI. Validation reads and parses; it never loads the bundle's own code. Importing a gate plugin to check it would mean the bundle had already executed by the time we decided it was unfit, so an unrecognized gate type is reported as unverified rather than accepted or rejected. Built-in gate specs are built, which is what type-checking a gate spec means. Two silent-acceptance gaps are closed. A blank path setting is refused with a message saying what it would actually do: cron.gate_plugins_dir set to "" is not "no directory" but the daemon's working directory, every .py in which is imported and executed at startup and on every cron reload. And a schedule the scheduler will not run now fails CI — a bad crontab, a string that is neither crontab nor interval, a zero interval, or a non-string that would raise past every handler the daemon has. Source schedules were not checked at all and now are, through the same helper the daemon uses, so the validator and the runtime cannot disagree about what is valid. Co-Authored-By: Claude --- config.example.yaml | 4 +- docs/config.md | 89 +++ docs/cron.md | 2 +- nerve/cli.py | 92 ++- nerve/config_validate.py | 656 +++++++++++++++++++++ nerve/cron/gates.py | 22 + nerve/cron/jobs.py | 92 ++- nerve/cron/service.py | 37 +- tests/test_config_sources.py | 38 ++ tests/test_config_validate.py | 1029 +++++++++++++++++++++++++++++++++ tests/test_cron_gates.py | 34 ++ 11 files changed, 2055 insertions(+), 40 deletions(-) create mode 100644 nerve/config_validate.py create mode 100644 tests/test_config_validate.py 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..58ac724e 100644 --- a/docs/config.md +++ b/docs/config.md @@ -97,6 +97,95 @@ 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 blank `workspace`, `cron.jobs_file`, `cron.system_file` or + `cron.gate_plugins_dir`. An empty value reads as "unset, use the default", but + `Path("")` is `Path(".")`, so it resolves to whatever directory the daemon was + started in. Omit the key instead, and never set it to `''`, `.` or `./`. 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..e3405bee --- /dev/null +++ b/nerve/config_validate.py @@ -0,0 +1,656 @@ +"""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 left blank — which is not "unset" +but 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 decide *where nerve reads its config and its code from*, +#: paired with what pointing them at the working directory would actually do. +#: +#: ``Path("")`` is ``Path(".")``, so ``key: ''`` in a bundle does not mean "unset, +#: use the default" the way it reads — it means the directory the daemon happened +#: to be started in. Nothing legitimate wants any of these aimed there, and a +#: value that means something other than what it looks like is precisely what a +#: config gate exists to stop, so each is a hard error regardless of strictness. +_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" + ), +} + + +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:-}`` is the same empty string, 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``.""" + if not value.strip(): + # Includes whitespace-only, which is a relative path *inside* the + # working directory rather than the directory itself — same mistake, + # same answer. + return ( + 'is not "no path": an empty value resolves against the process\'s ' + "working directory" + ) + 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..9018523b --- /dev/null +++ b/tests/test_config_validate.py @@ -0,0 +1,1029 @@ +"""Tests for config-bundle validation: validate_config_bundle + CLI.""" + +import textwrap +from pathlib import Path + +import pytest +import yaml + +from nerve.config_validate import 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 _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: + """A blank path setting is not "unset": ``Path("")`` is ``Path(".")``. + + So ``key: ''`` does not fall back to the default the way it reads — it points + 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 missing character turns the + working directory into a source of code the daemon executes. Nothing + legitimate wants any of these aimed there, so validation refuses them + outright rather than passing a bundle whose meaning depends on a cwd. + """ + + 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_empty_gate_plugins_dir_is_error(self, tmp_path): + result = self._validate(tmp_path, "cron:\n gate_plugins_dir: ''\n") + assert not result.ok + [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 + assert "executed" in err + + def test_dot_gate_plugins_dir_is_error(self, tmp_path): + """Spelling the working directory out loud is the same setting.""" + for value in (".", "./"): + result = self._validate( + tmp_path, f"cron:\n gate_plugins_dir: '{value}'\n" + ) + assert any("gate_plugins_dir" in e for e in result.errors), value + + def test_whitespace_only_gate_plugins_dir_is_error(self, tmp_path): + result = self._validate(tmp_path, "cron:\n gate_plugins_dir: ' '\n") + assert any("gate_plugins_dir" in e for e in result.errors) + + def test_empty_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_empty_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_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_error(self, tmp_path, monkeypatch): + """``${VAR:-}`` with VAR unset is the same empty string, one indirection + later, and a gate that only reads literals would wave it through.""" + monkeypatch.delenv("GATES_DIR", raising=False) + result = self._validate( + tmp_path, "cron:\n gate_plugins_dir: ${GATES_DIR:-}\n" + ) + assert not result.ok + assert any("gate_plugins_dir" in e for e in 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 From 282647562fcc2ad489a4e12faa1c574498308fc2 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 30 Jul 2026 16:19:39 +0200 Subject: [PATCH 2/2] Review: let the loader own what a blank path setting means MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validator refused a blank `workspace`, `cron.jobs_file`, `cron.system_file` or `cron.gate_plugins_dir` on the premise that `Path("")` is `Path(".")`, so a blank value "means the directory the daemon happened to be started in". It does not. `_expand_path` maps blank to None before it can become `Path(".")`, and the field takes its documented default — which the validator's own run demonstrated, logging the resolved default path and then erroring to say the value resolved to the working directory. So the same bundle loaded correctly and failed CI. The shape that hits it is `jobs_file: ${CRON_JOBS_FILE:-}`, and docs/config.md promotes `${VAR:-default}` as the fleet idiom. `docs/config.md` also carried both readings, in two places, and the tests pinned both contracts with `TestBlankPathSettingsMeanUnset` and `TestWorkingDirectoryPaths` green side by side. Blank is no longer reported. The explicit `.` / `./` / `./.` check stays a hard error, because those do survive the loader as the path they are. The rule was also a hand-written four-key literal against fourteen path settings, so `gateway.ssl.cert` accepted a dot that switches TLS on with a directory as the certificate. The keys are now derived from the config dataclasses: one test walks every `Path`-annotated field and fails if it is neither covered nor exempted with a reason, a second fails if the rule names a field the config does not declare, and a third asserts each covered key actually produces an error, so a key cannot be listed and never read. Six settings that check turned up are now covered. Co-Authored-By: Claude --- docs/config.md | 14 ++- nerve/config_validate.py | 72 ++++++++---- tests/test_config_validate.py | 202 ++++++++++++++++++++++++++++------ 3 files changed, 227 insertions(+), 61 deletions(-) diff --git a/docs/config.md b/docs/config.md index 58ac724e..e5e7b97f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -123,12 +123,14 @@ stop it. It reports an error for: 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 blank `workspace`, `cron.jobs_file`, `cron.system_file` or - `cron.gate_plugins_dir`. An empty value reads as "unset, use the default", but - `Path("")` is `Path(".")`, so it resolves to whatever directory the daemon was - started in. Omit the key instead, and never set it to `''`, `.` or `./`. This - matters most for `cron.gate_plugins_dir`, whose `.py` files the daemon imports - and executes at startup and on every cron reload. +- 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 diff --git a/nerve/config_validate.py b/nerve/config_validate.py index e3405bee..aee4d678 100644 --- a/nerve/config_validate.py +++ b/nerve/config_validate.py @@ -9,8 +9,8 @@ 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 left blank — which is not "unset" -but the daemon's working directory (:data:`_WORKING_DIR_PATH_KEYS`). Unknown / +(: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. @@ -278,14 +278,21 @@ def _is_unresolved(value: Any) -> bool: return isinstance(value, str) and "${" in value -#: Path settings that decide *where nerve reads its config and its code from*, -#: paired with what pointing them at the working directory would actually do. +#: Path settings that must not resolve to the process's working directory, paired +#: with what aiming each one there would actually do. #: -#: ``Path("")`` is ``Path(".")``, so ``key: ''`` in a bundle does not mean "unset, -#: use the default" the way it reads — it means the directory the daemon happened -#: to be started in. Nothing legitimate wants any of these aimed there, and a -#: value that means something other than what it looks like is precisely what a +#: 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, " @@ -301,6 +308,34 @@ def _is_unresolved(value: Any) -> bool: ("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" + ), } @@ -318,9 +353,9 @@ def _validate_working_dir_paths( raw = _nested_get(merged, key) if not isinstance(raw, str): continue - # ``${VAR:-}`` is the same empty string, 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. + # ``${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: @@ -333,15 +368,12 @@ def _validate_working_dir_paths( def _working_dir_reason(value: str) -> str | None: - """Why *value* points at the process's working directory, or ``None``.""" - if not value.strip(): - # Includes whitespace-only, which is a relative path *inside* the - # working directory rather than the directory itself — same mistake, - # same answer. - return ( - 'is not "no path": an empty value resolves against the process\'s ' - "working directory" - ) + """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 diff --git a/tests/test_config_validate.py b/tests/test_config_validate.py index 9018523b..42a6197b 100644 --- a/tests/test_config_validate.py +++ b/tests/test_config_validate.py @@ -6,7 +6,8 @@ import pytest import yaml -from nerve.config_validate import validate_config_bundle +from nerve.config import NerveConfig +from nerve.config_validate import _WORKING_DIR_PATH_KEYS, validate_config_bundle from nerve.cron.gates import GATE_REGISTRY @@ -33,6 +34,14 @@ def _jobs(workspace: Path, jobs: list[dict]) -> None: (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) @@ -249,15 +258,20 @@ def test_validating_does_not_run_plugin_code(self, tmp_path): class TestWorkingDirectoryPaths: - """A blank path setting is not "unset": ``Path("")`` is ``Path(".")``. - - So ``key: ''`` does not fall back to the default the way it reads — it points - 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 missing character turns the - working directory into a source of code the daemon executes. Nothing - legitimate wants any of these aimed there, so validation refuses them - outright rather than passing a bundle whose meaning depends on a cwd. + """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): @@ -265,29 +279,27 @@ def _validate(self, tmp_path: Path, settings: str): _settings(ws, settings) return validate_config_bundle(_cfg(tmp_path, workspace=ws), workspace_override=ws) - def test_empty_gate_plugins_dir_is_error(self, tmp_path): - result = self._validate(tmp_path, "cron:\n gate_plugins_dir: ''\n") - assert not result.ok - [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 - assert "executed" in err - def test_dot_gate_plugins_dir_is_error(self, tmp_path): - """Spelling the working directory out loud is the same setting.""" - for value in (".", "./"): + for value in (".", "./", "./."): result = self._validate( tmp_path, f"cron:\n gate_plugins_dir: '{value}'\n" ) - assert any("gate_plugins_dir" in e for e in result.errors), value - - def test_whitespace_only_gate_plugins_dir_is_error(self, tmp_path): - result = self._validate(tmp_path, "cron:\n gate_plugins_dir: ' '\n") - assert any("gate_plugins_dir" in e for e in result.errors) + [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_empty_workspace_is_error(self, tmp_path): + 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 @@ -298,18 +310,37 @@ def test_empty_workspace_is_error(self, tmp_path): _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") + (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_empty_cron_file_names_the_key(self, tmp_path): + 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") + 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" @@ -326,15 +357,116 @@ def test_unset_env_ref_in_a_path_is_not_flagged(self, tmp_path, monkeypatch): assert result.ok, result.errors assert any("GATES_DIR" in i for i in result.info) - def test_env_default_expanding_to_empty_is_error(self, tmp_path, monkeypatch): - """``${VAR:-}`` with VAR unset is the same empty string, one indirection - later, and a gate that only reads literals would wave it through.""" + 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 - assert any("gate_plugins_dir" in e for e in result.errors) + [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: