From 32f0c34939f646e5d023513987cb782f32bd8093 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 28 Jul 2026 11:12:35 +0200 Subject: [PATCH] Establish workspace/config/ and multi-source config resolution Shareable configuration moves into /config/settings.yaml, a git-tracked subtree, and is merged beneath the two machine-local layers: settings.yaml < config.yaml < config.local.yaml. An absent subtree keeps the previous behaviour. A settings.yaml that is not a mapping now fails loudly instead of being merged as junk, a file that exists but cannot be read is reported as such rather than read as empty, and the YAML encoding is pinned so a mis-encoded file cannot decode differently on another machine. nerve doctor stays alive on a bad config so it can report it, and nerve backup captures the new subtree. Co-Authored-By: Claude --- docs/config.md | 57 +++- nerve/agent/engine.py | 5 +- nerve/backup.py | 10 +- nerve/bootstrap.py | 413 +++++++++++++++++++----- nerve/cli.py | 62 +++- nerve/config.py | 176 +++++++++-- nerve/gateway/routes/mcp_servers.py | 9 +- nerve/templates/config/settings.yaml | 46 +++ nerve/workspace.py | 52 ++++ tests/test_backup.py | 34 ++ tests/test_bootstrap.py | 388 ++++++++++++++++++++++- tests/test_config_sources.py | 449 +++++++++++++++++++++++++++ 12 files changed, 1558 insertions(+), 143 deletions(-) create mode 100644 nerve/templates/config/settings.yaml create mode 100644 tests/test_config_sources.py diff --git a/docs/config.md b/docs/config.md index 0c9da276..5dca3d59 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1,10 +1,49 @@ # Configuration Reference -Nerve uses two YAML config files: -- `config.yaml` — Template settings (version controlled) -- `config.local.yaml` — Secrets and personal overrides (gitignored) +Nerve merges configuration from up to three layers, lowest precedence first: + +1. `workspace/config/settings.yaml`: shareable, git-tracked settings that live + inside the workspace. This is the layer a remote config repo syncs. +2. `config.yaml`: machine-local base settings. +3. `config.local.yaml`: machine-local secrets and personal overrides (gitignored). + +Each layer is deep-merged over the one before it, so a machine can override a +shared setting locally. Lockdown mode drops both machine-local layers and leaves +the workspace as the only source of truth. With no `settings.yaml` present, only +`config.yaml` and `config.local.yaml` are read. + +The workspace location is resolved from `config.yaml` (or the default +`~/nerve-workspace`) *before* `settings.yaml` is read, so a `workspace:` key +inside `settings.yaml` is ignored. + +## Which layer a key belongs in + +`nerve init` splits its answers between the first two layers: + +| Layer | Gets | +|-------|------| +| `config.yaml` | `workspace`, `deployment`, `provider.aws_profile`, `gateway.ssl.*`, `proxy`, `docker`, `telegram.enabled`, `sync.gmail.accounts`, `external_agents`, `mcp_endpoint` | +| `settings.yaml` | `timezone`, `gateway.host`/`port`, `provider.type`/`aws_region` (incl. the region-scoped Bedrock model IDs), `agent.*`, `memory.*`, `sessions.*`, `sync.*`, `houseofagents.*`, quiet hours, `telegram.dm_policy`/`stream_mode` | + +The test is whether the value would be wrong on another machine: filesystem +paths, credential handles, whose mailboxes this person syncs, which agent +binaries this box has paired. Which provider the deployment uses and which port +it serves on are not in that category, so they are shared. Lockdown makes this +concrete: it drops `config.yaml`, so a key belongs there only if its declared +default is an acceptable answer on every locked box. + +Write each key to one layer only. `config.yaml` shadows `settings.yaml`, so a +shared value repeated in both leaves the tracked copy with no effect. + +Re-running `nerve init` regenerates `config.yaml` and `config.local.yaml` +wholesale. `settings.yaml` may be shared, so the wizard rewrites only the keys in +the table above and leaves the rest of the file alone; it prints what it added, +updated and removed. Each file is copied to `*.bak` first unless it is empty or +comments-only. Two things to expect: `settings.yaml` is rewritten with +`yaml.safe_dump`, which drops comments whenever anything changed (the previous +file is in `settings.yaml.bak`), and changing an answer overwrites whatever +someone else had under those keys, so review the diff before committing. -Values in `config.local.yaml` are deep-merged on top of `config.yaml`. Unknown keys are ignored but logged as warnings at startup (and shown by `nerve doctor`) so typos don't fail silently. @@ -15,9 +54,9 @@ TLS is off, not TLS with an empty certificate path. ## Environment Variable References -Any string value in `config.yaml` or `config.local.yaml` may reference an -environment variable, so secrets can come from the environment (or a secret -store) instead of being written into a file: +Any string value in any of the three layers may reference an environment +variable, so secrets can come from the environment (or a secret store) instead of +being written into a file: ```yaml anthropic_api_key: ${ANTHROPIC_API_KEY} # required: load fails if unset @@ -33,8 +72,8 @@ gateway: Only the braced `${...}` form is interpolated. A bare `$` is never touched, so bcrypt `password_hash` values (`$2b$...`), jwt secrets and connection strings -are safe as written. Interpolation runs once, after `config.local.yaml` is -merged on top, so either file may use references. +are safe as written. Interpolation runs once, after all three layers are merged, +so any of them may use references. Resolved values arrive as strings and are converted back to the field's declared type, including `int | None`, `list[int]` and `list[str]`. So `port: ${PORT}` and diff --git a/nerve/agent/engine.py b/nerve/agent/engine.py index 50b01368..576e1318 100644 --- a/nerve/agent/engine.py +++ b/nerve/agent/engine.py @@ -502,7 +502,10 @@ async def reload_mcp_config(self) -> list: Returns the list of McpServerConfig. """ from nerve.config import load_claude_code_plugins, load_mcp_servers - self._mcp_servers_cache = load_mcp_servers() + # Read from the daemon's actual config dir (not the process cwd) so a + # reload sees the same config.yaml + workspace/config/settings.yaml that + # startup loaded. + self._mcp_servers_cache = load_mcp_servers(self.config.config_dir) self._claude_code_plugins = load_claude_code_plugins() await self._sync_mcp_servers_to_db() logger.info( diff --git a/nerve/backup.py b/nerve/backup.py index 2c6488d1..9311adce 100644 --- a/nerve/backup.py +++ b/nerve/backup.py @@ -96,7 +96,15 @@ # take an *allowlist* of the small, irreplaceable "brain": identity markdown # at the root plus a few known directories. Extra excludes from config are # applied *within* these. -WORKSPACE_INCLUDE_DIRS: tuple[str, ...] = ("memory", "scripts", "skills") +# +# ``config`` carries the shareable settings and the cron jobs. It is tracked in +# the workspace repo, but so are ``skills`` and ``memory`` — "it is in git" has +# never been the rule here, and a backup taken to survive a lost machine is +# worth little if restoring it brings back no schedule. Nothing in it is secret +# by construction: values that matter are ``${ENV_VAR}`` references, and the +# real secrets live in the machine-local overlay that is already handled +# separately. +WORKSPACE_INCLUDE_DIRS: tuple[str, ...] = ("config", "memory", "scripts", "skills") WORKSPACE_INCLUDE_FILE_GLOBS: tuple[str, ...] = ("*.md",) # Directory names always pruned while walking the included workspace dirs. diff --git a/nerve/bootstrap.py b/nerve/bootstrap.py index 058db1a1..dee5cf50 100644 --- a/nerve/bootstrap.py +++ b/nerve/bootstrap.py @@ -7,6 +7,7 @@ from __future__ import annotations +import copy import json import os import secrets @@ -21,7 +22,12 @@ import yaml from nerve import paths -from nerve.workspace import initialize_workspace, install_bundled_skills +from nerve.config import _expand_path, _interpolate_str, workspace_settings_file +from nerve.workspace import ( + initialize_workspace, + install_bundled_skills, + install_config_scaffold, +) # --- Cron definitions for the wizard --- @@ -831,7 +837,7 @@ def _ensure_docker_files(self) -> None: if filepath.exists(): click.echo(f" {filename} already exists — skipping") continue - filepath.write_text(content.lstrip("\n")) + filepath.write_text(content.lstrip("\n"), encoding="utf-8") if filename == "docker-entrypoint.sh": try: os.chmod(filepath, 0o755) @@ -1626,7 +1632,7 @@ def _apply(self) -> None: # 1. Create workspace from templates click.echo(" Creating workspace...", nl=False) - ws_path = Path(os.path.expanduser(str(self.choices.workspace_path))) + ws_path = self._workspace_dir() created = initialize_workspace(ws_path, self.choices.mode) click.secho(" ✓", fg="green") @@ -1635,6 +1641,11 @@ def _apply(self) -> None: install_bundled_skills(ws_path) click.secho(" ✓", fg="green") + # 2b. Scaffold the git-syncable config subtree (workspace/config/) + click.echo(" Creating config subtree...", nl=False) + install_config_scaffold(ws_path) + click.secho(" ✓", fg="green") + # 3. Patch USER.md with name/timezone if provided (personal mode) if self.choices.mode == "personal" and self.choices.user_name: user_md = ws_path / "USER.md" @@ -1664,19 +1675,31 @@ def _apply(self) -> None: # must never silently destroy hand edits (model tweaks, paired # Telegram users, ...). backed_up = [] - for name in ("config.yaml", "config.local.yaml"): - existing = self.config_dir / name - if existing.exists(): - shutil.copy2(existing, existing.with_suffix(existing.suffix + ".bak")) - backed_up.append(f"{name}.bak") + for existing in ( + self.config_dir / "config.yaml", + self.config_dir / "config.local.yaml", + workspace_settings_file(self._workspace_dir()), + ): + if not existing.exists() or not _has_config_content(existing): + # install_config_scaffold has just created a comments-only + # settings.yaml, and it lives in a git-tracked directory — + # backing that up would leave a junk .bak in the repo on every + # fresh install, and claim to have rescued something. + continue + shutil.copy2(existing, existing.with_suffix(existing.suffix + ".bak")) + backed_up.append(f"{existing.name}.bak") if backed_up: click.echo(f" Backed up existing config → {', '.join(backed_up)}") - # 7. Write config.yaml + # 7. Write config.yaml (machine-local) + settings.yaml (portable) click.echo(" Writing config.yaml...", nl=False) self._write_config_yaml() click.secho(" ✓", fg="green") + click.echo(" Writing workspace config/settings.yaml...", nl=False) + self._write_workspace_settings() + click.secho(" ✓", fg="green") + # 8. Write config.local.yaml click.echo(" Writing config.local.yaml...", nl=False) self._write_config_local_yaml() @@ -1778,11 +1801,13 @@ def _resolve_jwt_secret(self) -> str: and stores it on ``self.choices``. If apply order changes in the future this method keeps the dependency explicit. """ - # The wizard generates this in _write_config_local_yaml. If - # that hasn't run yet, we generate it now and let the later - # config-write reuse it via ``self.choices.password``-style - # caching. In practice apply order calls _write_config_local - # before this method, so we'll have the secret already. + # The cache is not the usual path: _write_config_local_yaml puts the + # secret straight into the dict it dumps and never sets + # _jwt_secret_cache, so a normal `nerve init` falls through to the read + # below. That read pins its encoding because the except clause around it + # does not fail safe -- on a decode error it returns a newly generated + # secret that is not on disk, the external-agent MCP token is signed with + # that, and the daemon rejects every call the token makes. secret = getattr(self.choices, "_jwt_secret_cache", "") if secret: return secret @@ -1790,7 +1815,7 @@ def _resolve_jwt_secret(self) -> str: local = self.config_dir / "config.local.yaml" if local.exists(): try: - data = yaml.safe_load(local.read_text()) or {} + data = yaml.safe_load(local.read_text(encoding="utf-8")) or {} secret = (data.get("auth") or {}).get("jwt_secret", "") or "" except Exception: secret = "" @@ -1805,14 +1830,21 @@ def _compute_nerve_mcp_url(self) -> str: Uses the gateway scheme/host/port and MCP path from the configuration just written by the wizard. The trailing slash matters. + + Reads the merged layers rather than config.yaml alone. gateway.host and + .port are written to the tracked settings, so reading only the machine + file would build the URL from the declared defaults and ignore a port the + operator had set. """ raw: dict = {} - path = self.config_dir / "config.yaml" - if path.exists(): - try: - raw = yaml.safe_load(path.read_text()) or {} - except Exception: - raw = {} + try: + from nerve.config import _read_config_sources + + raw = _read_config_sources(self.config_dir) or {} + except Exception: + # A half-written or unloadable config must not sink `nerve init`; + # the per-key fallbacks below still produce a usable local URL. + raw = {} gateway = raw.get("gateway") or {} ssl = gateway.get("ssl") or {} scheme = "https" if ssl.get("cert") and ssl.get("key") else "http" @@ -1834,7 +1866,7 @@ def _record_external_agents_in_config_yaml(self, results) -> None: if not path.exists(): return try: - data = yaml.safe_load(path.read_text()) or {} + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} except Exception as e: click.secho( f" Warning: could not parse config.yaml to record external_agents: {e}", @@ -1858,7 +1890,7 @@ def _record_external_agents_in_config_yaml(self, results) -> None: endpoint = data.setdefault("mcp_endpoint", {}) endpoint["enabled"] = True endpoint.setdefault("path", "/mcp/v1") - path.write_text(yaml.safe_dump(data, sort_keys=False)) + path.write_text(yaml.safe_dump(data, sort_keys=False), encoding="utf-8") def _build_web_ui(self) -> None: """Build the web UI if not already built.""" @@ -1913,15 +1945,100 @@ def _build_web_ui(self) -> None: fg="yellow", ) - def _write_config_yaml(self) -> None: - """Write the base config.yaml.""" - ws = str(self.choices.workspace_path) - tz = self.choices.timezone + def _workspace_dir(self) -> Path: + """The workspace path, expanded the same way the config loader does. - config: dict[str, Any] = { - "workspace": ws, - "timezone": tz, + nerve/config.py resolves `workspace` with ${VAR} interpolation *and* + expandvars/expanduser. Expanding differently here would put + settings.yaml somewhere the loader never looks — the wizard would + report success and every portable setting would be silently lost. + """ + return _expand_workspace(str(self.choices.workspace_path)) + + @staticmethod + def _leaf_paths(d: dict[str, Any], prefix: str = "") -> dict[str, Any]: + """Flatten a nested dict to ``{"a.b": value}``. Lists are leaves.""" + out: dict[str, Any] = {} + for key, value in d.items(): + path = f"{prefix}{key}" + if isinstance(value, dict): + out.update(SetupWizard._leaf_paths(value, f"{path}.")) + else: + out[path] = value + return out + + @staticmethod + def _set_leaf(d: dict[str, Any], path: str, value: Any) -> None: + *parents, last = path.split(".") + node = d + for part in parents: + child = node.get(part) + if not isinstance(child, dict): + child = {} + node[part] = child + node = child + node[last] = value + + @staticmethod + def _del_leaf(d: dict[str, Any], path: str) -> bool: + """Remove ``path``, pruning any dict it leaves empty. True if removed.""" + *parents, last = path.split(".") + chain: list[tuple[dict[str, Any], str]] = [] + node = d + for part in parents: + child = node.get(part) + if not isinstance(child, dict): + return False + chain.append((node, part)) + node = child + if last not in node: + return False + del node[last] + for parent, key in reversed(chain): + if not parent[key]: + del parent[key] + return True + + # Keys that describe *this box* and must never travel with the workspace + # repo. Everything else the wizard decides is shared behaviour and belongs + # in the tracked settings layer, so `nerve config sync` and lockdown mean + # something on a default install instead of being no-ops. + # + # A key must appear in exactly one of the two dicts below. config.yaml + # shadows settings.yaml, so writing a portable value to both would make + # the tracked copy dead weight -- edit it and nothing happens. + + def _build_config_layers( + self, + ) -> tuple[dict[str, Any], dict[str, Any], list[str]]: + """Split the wizard's answers into (machine-local, portable, shadowed). + + ``shadowed`` lists dotted paths this run must *delete* from a + pre-existing settings.yaml rather than merely omit. The tracked file is + merge-preserving, so a key the wizard stops emitting would otherwise + remain at its old value. The case this covers is switching an install + away from Bedrock: ``provider.aws_region`` has to be removed, or the + tracked file keeps naming a region the box no longer uses. + """ + machine: dict[str, Any] = { + "workspace": str(self.choices.workspace_path), "deployment": self.choices.deployment, + } + shadowed: list[str] = [] + portable: dict[str, Any] = { + "timezone": self.choices.timezone, + # Shared, not machine-local: these describe the deployment, and a + # fleet needs one place to set them. The wizard only ever wrote the + # declared defaults here, so while they lived in config.yaml the + # tracked layer could not state them at all. A box needing a + # different port still overrides in config.yaml. + # + # host/port only. gateway.ssl.cert and .key are local filesystem + # paths and stay machine-local. + "gateway": { + "host": "0.0.0.0", + "port": 8900, + }, "agent": { "model": "claude-opus-5", "cron_model": "claude-sonnet-4-6", @@ -1931,10 +2048,6 @@ def _write_config_yaml(self) -> None: "effort": "max", "context_1m": True, }, - "gateway": { - "host": "0.0.0.0", - "port": 8900, - }, "quiet_start": "02:00", "quiet_end": "08:00", "memory": { @@ -1959,59 +2072,172 @@ def _write_config_yaml(self) -> None: } if self.choices.mode == "personal": - config["telegram"] = { - "enabled": bool(self.choices.telegram_bot_token), + # dm_policy/stream_mode are shared behaviour; `enabled` is not -- + # it is derived from whether *this* machine was given a bot token, + # and the token itself lives in config.local.yaml. + machine["telegram"] = {"enabled": bool(self.choices.telegram_bot_token)} + portable["telegram"] = { "dm_policy": "pairing", "stream_mode": "partial", } - config["sync"] = { + portable["sync"] = { "telegram": {"enabled": self.choices.telegram_sync}, - "gmail": { - "enabled": self.choices.gmail_sync, - "accounts": self.choices.gmail_accounts, - }, + "gmail": {"enabled": self.choices.gmail_sync}, "github": {"enabled": self.choices.github_sync}, "github_events": {"enabled": self.choices.github_sync}, # Disabled by default — requires an explicit list of repos to watch. "github_repos": {"enabled": False, "repos": []}, } + # Which sources to sync is shared policy; *whose* mailboxes is + # not. These are personal addresses in a file the docs tell you to + # commit, and they are per-person rather than per-team. + machine.setdefault("sync", {})["gmail"] = { + "accounts": self.choices.gmail_accounts + } - # Provider configuration + # Provider and region are shared; only the AWS profile is a local + # credential handle. While the whole block was machine-local, a locked + # box never read it and fell back to the declared default, becoming an + # `anthropic` instance and then failing for a missing API key that lived + # in config.local.yaml, which lockdown also ignores. + portable["provider"] = {"type": self.choices.provider_type} + if self.choices.aws_profile: + machine["provider"] = {"aws_profile": self.choices.aws_profile} if self.choices.provider_type == "bedrock": - config["provider"] = { - "type": "bedrock", - "aws_region": self.choices.aws_region, - } - if self.choices.aws_profile: - config["provider"]["aws_profile"] = self.choices.aws_profile - # Override model names to Bedrock inference-profile IDs. - # The prefix is geography-scoped (us./eu./apac.) and must match - # the configured region or every call 400s. + portable["provider"]["aws_region"] = self.choices.aws_region + # The prefix is geography-scoped (us./eu./apac.) and must match the + # configured region or every call 400s. Since the region is now in + # the tracked layer, these belong there too. geo = bedrock_geo_prefix(self.choices.aws_region) - config["agent"]["model"] = f"{geo}.anthropic.claude-opus-5" - config["agent"]["cron_model"] = f"{geo}.anthropic.claude-sonnet-4-6" - config["agent"]["title_model"] = f"{geo}.anthropic.claude-haiku-4-5-20251001-v1:0" - config["memory"]["recall_model"] = f"{geo}.anthropic.claude-sonnet-4-6" - config["memory"]["memorize_model"] = f"{geo}.anthropic.claude-sonnet-4-6" - config["memory"]["fast_model"] = f"{geo}.anthropic.claude-haiku-4-5-20251001-v1:0" + for section, key, value in ( + ("agent", "model", f"{geo}.anthropic.claude-opus-5"), + ("agent", "cron_model", f"{geo}.anthropic.claude-sonnet-4-6"), + ("agent", "title_model", f"{geo}.anthropic.claude-haiku-4-5-20251001-v1:0"), + ("memory", "recall_model", f"{geo}.anthropic.claude-sonnet-4-6"), + ("memory", "memorize_model", f"{geo}.anthropic.claude-sonnet-4-6"), + ("memory", "fast_model", f"{geo}.anthropic.claude-haiku-4-5-20251001-v1:0"), + ): + portable[section][key] = value + else: + # Switching away from Bedrock. The non-prefixed model names in the + # base dict above already overwrite the geo-prefixed ones, so only + # the region needs deleting. + shadowed.append("provider.aws_region") if self.choices.use_proxy: - config["proxy"] = { - "enabled": True, - "port": 8317, - } + # A local helper process on a local port. + machine["proxy"] = {"enabled": True, "port": 8317} if self.choices.deployment == "docker": - config["docker"] = { + machine["docker"] = { "extra_mounts": [], # e.g. ["~/code:/code", "~/projects:/projects"] } + return machine, portable, shadowed + + def _write_config_yaml(self) -> None: + """Write the machine-local base config.yaml.""" + machine, _portable, _shadowed = self._build_config_layers() config_path = self.config_dir / "config.yaml" - with open(config_path, "w") as f: - f.write("# Nerve — Configuration\n") - f.write("# Edit this file to customize Nerve's behavior.\n") - f.write("# Secrets (API keys, tokens) go in config.local.yaml.\n\n") - yaml.safe_dump(config, f, default_flow_style=False, sort_keys=False) + # encoding is pinned on every config read and write in this module, to + # match nerve.config, which pins it on the way in. Under an ASCII default + # encoding an unpinned open() raises on the em-dash in these headers, at + # the first write, before any user value is involved. A user value cannot + # trigger it: safe_dump escapes non-ASCII to \xNN, so the dumped body is + # always ASCII. + with open(config_path, "w", encoding="utf-8") as f: + f.write("# Nerve — Machine-local configuration\n") + f.write("# Settings specific to this box: workspace location,\n") + f.write("# bind address, deployment style, local credentials handles.\n") + f.write("# Shared behaviour lives in /config/settings.yaml,\n") + f.write("# which this file overrides. Secrets go in config.local.yaml.\n\n") + yaml.safe_dump(machine, f, default_flow_style=False, sort_keys=False) + + def _write_workspace_settings(self) -> None: + """Write the portable half into ``/config/settings.yaml``. + + Ownership model: the wizard owns the keys it generates and rewrites + them from this run's answers, exactly as it does for config.yaml. + Anything else in the file -- a team policy key, a hand-tuned setting + the wizard never emits -- is preserved untouched. + + The alternative ("existing always wins") sounds safer for a file that + may be shared through git, but it means re-running init after changing + an answer silently does nothing: the wizard prompts for a timezone, + prints a tick, and discards the answer because the key already exists. + Overwriting is recoverable -- there is a .bak, and the file is in a + repo where the diff is visible before anyone commits it. + """ + _machine, portable, shadowed = self._build_config_layers() + settings_path = workspace_settings_file(self._workspace_dir()) + settings_path.parent.mkdir(parents=True, exist_ok=True) + + raw = "" + existing: dict[str, Any] = {} + if settings_path.exists(): + raw = settings_path.read_text(encoding="utf-8") + try: + loaded = yaml.safe_load(raw) + except yaml.YAMLError as e: + click.secho( + f"\n Warning: {settings_path} is not valid YAML ({e}) —" + " leaving it alone.", + fg="yellow", + ) + return + if loaded is not None and not isinstance(loaded, dict): + click.secho( + f"\n Warning: {settings_path} is not a mapping —" + " leaving it alone.", + fg="yellow", + ) + return + existing = loaded or {} + + merged = copy.deepcopy(existing) + # Diff against the file as it was on disk, not against `merged` while + # it is being mutated. The two are equivalent -- no portable leaf path + # is a prefix of another, so a _set_leaf can only touch descendants of + # its own path -- but flattening once says what is meant and drops the + # per-key re-walk. + before_paths = self._leaf_paths(existing) + added, changed, removed = [], [], [] + for path, value in self._leaf_paths(portable).items(): + before = before_paths.get(path, _MISSING) + if before is _MISSING: + added.append(path) + elif before != value: + changed.append(f"{path}: {before!r} → {value!r}") + self._set_leaf(merged, path, value) + for path in shadowed: + if self._del_leaf(merged, path): + removed.append(path) + + if merged == existing: + return + + # safe_dump cannot round-trip comments, and this file is meant to be + # read by other people in a PR. Say so rather than quietly deleting + # someone's rationale. + if existing and any( + line.lstrip().startswith("#") + for line in raw.splitlines()[_SETTINGS_HEADER_LINES:] + ): + click.secho( + f"\n Note: comments in {settings_path.name} are not preserved" + " when it is regenerated (the previous file is in" + f" {settings_path.name}.bak).", + fg="yellow", + ) + + with open(settings_path, "w", encoding="utf-8") as f: + f.write(_SETTINGS_HEADER) + yaml.safe_dump(merged, f, default_flow_style=False, sort_keys=False) + + for label, items in (("added", added), ("updated", changed), + ("removed", removed)): + if items: + click.secho(f"\n {label}: {', '.join(items)}", dim=True) def _write_config_local_yaml(self) -> None: """Write config.local.yaml with secrets.""" @@ -2059,7 +2285,7 @@ def _write_config_local_yaml(self) -> None: local["auth"] = auth local_path = self.config_dir / "config.local.yaml" - with open(local_path, "w") as f: + with open(local_path, "w", encoding="utf-8") as f: f.write("# Nerve — Secrets (gitignored)\n") f.write("# API keys, tokens, and other sensitive configuration.\n\n") yaml.safe_dump(local, f, default_flow_style=False, sort_keys=False) @@ -2140,7 +2366,7 @@ def _write_cron_jobs(self) -> None: system_file = cron_dir / "system.yaml" system_file.parent.mkdir(parents=True, exist_ok=True) - with open(system_file, "w") as f: + with open(system_file, "w", encoding="utf-8") as f: f.write("# Nerve — System Cron Jobs\n") f.write("# Managed by 'nerve init'. Safe to re-generate.\n") f.write("# To add custom crons, use jobs.yaml instead.\n\n") @@ -2149,7 +2375,7 @@ def _write_cron_jobs(self) -> None: # Create empty jobs.yaml scaffold if it doesn't exist jobs_file = cron_dir / "jobs.yaml" if not jobs_file.exists(): - with open(jobs_file, "w") as f: + with open(jobs_file, "w", encoding="utf-8") as f: f.write("# Nerve — Custom Cron Jobs\n") f.write("# Add your own cron jobs here. Nerve will never overwrite this file.\n") f.write("# Format is the same as system.yaml — see it for examples.\n\n") @@ -2252,7 +2478,7 @@ def _done(self) -> None: click.echo(" nerve doctor Verify everything is set up") click.echo(" http://localhost:8900 Open the web UI") click.echo() - ws = os.path.expanduser(str(self.choices.workspace_path)) + ws = str(self._workspace_dir()) click.secho(f" Your workspace: {ws}", bold=True) click.echo(" Edit SOUL.md to customize Nerve's personality") click.echo(" Edit USER.md to tell Nerve about yourself") @@ -2427,6 +2653,51 @@ def _wrap_text(text: str, width: int = 51) -> list[str]: return lines or [""] +def _has_config_content(path: Path) -> bool: + """True if the file holds at least one key (not just comments/blank).""" + try: + data = yaml.safe_load(path.read_text(encoding="utf-8")) + except (OSError, yaml.YAMLError): + return True # can't tell — err towards keeping a backup + return bool(data) + + +def _expand_workspace(raw: str) -> Path: + """Expand a workspace path exactly as nerve.config resolves it. + + Delegates to ``_expand_path`` rather than repeating it. The previous + implementation reversed the order (``expanduser`` before ``expandvars``) and + did not strip, and both mattered: ``expanduser`` only expands a *leading* + ``~``, so a value carrying leading whitespace — a ``NERVE_WORKSPACE`` with a + trailing newline, say — was not expanded at all, and the wizard wrote + settings.yaml to a directory the loader never reads. + + Blank means unset, as it does for every other path setting, so it falls back + to the same default the loader uses. + """ + if "${" in raw: + raw = _interpolate_str(raw, []) + return _expand_path(raw) or paths.default_workspace() + + +_MISSING = object() + +# Regenerated verbatim on every write; anything below it is user content, so +# the comment-loss warning only fires for comments the operator added. +_SETTINGS_HEADER = """\ +# Nerve — Shared configuration +# Git-tracked and portable: this is the layer that syncs between machines and +# the one lockdown mode trusts. Machine-specific values belong in config.yaml, +# which overrides this file. Secrets belong in config.local.yaml or behind an +# ${ENV_VAR} reference — never here. +# +# `nerve init` owns the keys it generates and rewrites them on re-run. Keys it +# does not generate are left alone. + +""" +_SETTINGS_HEADER_LINES = _SETTINGS_HEADER.count("\n") + + # --- Docker file templates --- # Container-side locations, referenced by the Dockerfile, the compose mounts and diff --git a/nerve/cli.py b/nerve/cli.py index 1b3d3566..7b447429 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -170,6 +170,12 @@ def _docker_compose( return result.returncode +# 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"}) + + @click.group() @click.option( "--config-dir", "-c", type=click.Path(), default=None, @@ -186,16 +192,24 @@ def main(ctx: click.Context, config_dir: str | None, verbose: bool) -> None: resolved_dir, config_source = resolve_config_dir(config_dir) if not resolved_dir.is_absolute(): resolved_dir = resolved_dir.resolve() + config = None + config_error = None try: config = load_config(resolved_dir) except ConfigError as e: - # Render config errors (e.g. an unresolved required ${ENV_VAR}) as a - # clean message + non-zero exit instead of a raw traceback — this - # callback runs before every subcommand, including `nerve doctor`. - raise click.ClickException(str(e)) from e - set_config(config) + # 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. + 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) ctx.ensure_object(dict) ctx.obj["config"] = config + ctx.obj["config_error"] = config_error ctx.obj["config_dir"] = str(resolved_dir) ctx.obj["config_source"] = config_source ctx.obj["verbose"] = verbose @@ -219,7 +233,14 @@ def init(ctx: click.Context, if_needed: bool, non_interactive: bool, inside_dock if non_interactive: click.echo("Nerve is already configured. Skipping.") return - if not click.confirm("Nerve is already configured. Re-run setup? (Config files will be overwritten, workspace files won't.)"): + if not click.confirm( + "Nerve is already configured. Re-run setup? (config.yaml and " + "config.local.yaml are regenerated; workspace/config/settings.yaml " + "keeps any key the wizard doesn't generate; other workspace files " + "are untouched. Each of the three that holds any setting is copied " + "to *.bak first — an empty or comments-only file is skipped, since " + "there is nothing in it to lose.)" + ): return if non_interactive: @@ -761,10 +782,14 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s suffix = f" (via {source_label})" if source_label else "" has_base = (Path(config_dir) / "config.yaml").exists() has_local = (Path(config_dir) / "config.local.yaml").exists() - if has_base or has_local: + from nerve.config import workspace_settings_file + has_settings = workspace_settings_file(config.workspace).exists() + if has_base or has_local or has_settings: present = " + ".join( n for n, ok in ( - ("config.yaml", has_base), ("config.local.yaml", has_local), + ("config.yaml", has_base), + ("config.local.yaml", has_local), + ("workspace/config/settings.yaml", has_settings), ) if ok ) lines.append(f"[OK] Config: {config_dir} ({present}){suffix}") @@ -775,15 +800,12 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s "-c/--config-dir or NERVE_CONFIG_DIR" ) - # Unknown / misspelled config keys + # Unknown / misspelled config keys — validate the same merged view that + # load_config sees (workspace/config/settings.yaml + config.yaml + + # config.local.yaml), so typos in the shared settings layer are caught too. try: - from nerve.config import _deep_merge, validate_config_keys - merged: dict = {} - for name in ("config.yaml", "config.local.yaml"): - p = Path(config_dir or ".") / name - if p.exists(): - import yaml as _yaml - merged = _deep_merge(merged, _yaml.safe_load(p.read_text()) or {}) + from nerve.config import _read_config_sources, validate_config_keys + merged = _read_config_sources(Path(config_dir)) if config_dir else {} for w in validate_config_keys(merged): warnings.append(f"[WARN] config: {w}") except Exception: @@ -1030,6 +1052,14 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s def doctor(ctx: click.Context) -> None: """Check config, DB, API keys, and connectivity.""" 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("=" * 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']}") + ctx.exit(1) report = doctor_report( config, config_source=ctx.obj.get("config_source", ""), diff --git a/nerve/config.py b/nerve/config.py index ae5e2d11..8ae1fa6b 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -185,6 +185,136 @@ def _resolve_env_refs(merged: dict[str, Any]) -> dict[str, Any]: return resolved +# --- Multi-source config resolution ----------------------------------------- # +# +# Configuration is assembled from up to three layers, lowest precedence first: +# +# 1. workspace/config/settings.yaml — shareable, git-tracked settings (the +# portable surface synced from a remote workspace repo) +# 2. config.yaml — machine-local base (the historical file) +# 3. config.local.yaml — machine-local secrets / overrides +# +# ${ENV_VAR} interpolation is applied once at the end. This keeps existing +# single-file installs working unchanged (layer 1 absent → prior behavior) while +# letting shared settings live in the workspace. Lockdown mode (a later story) +# will drop layers 2/3 so only the tracked workspace layer applies. + + +def _read_yaml_mapping(path: Path, *, strict: bool = False) -> dict[str, Any]: + """Read a YAML file into a dict; ``{}`` if absent or empty. + + ``strict`` controls what happens when the file parses to something that + isn't a mapping — a list, a bare string, a number. Without it the file is + ignored with a warning, which is a bad failure mode for a config layer: + every key it was supposed to supply silently reverts to whatever the lower + layers said, and validation reports the bundle as fine. A truncated write, + a `yq` mishap, or a merge conflict resolved into a sequence all produce + exactly this shape. + + An *empty* file (or one that is nothing but comments) still yields ``{}`` + in both modes — the shipped ``settings.yaml`` scaffold is all comments and + parses to ``None``, so treating that as an error would break every fresh + install. + + Every way this can fail is raised as :class:`ConfigError` rather than + escaping as a parser or OS exception, so the callers that route around a + broken config (``nerve doctor``, ``config validate``, the reload paths) can + report it instead of showing a traceback. That includes the file being + unreadable: a mode-600 file owned by another user, or one deleted between + the ``exists()`` check and the open, is a config problem like any other and + deserves the same one-line message naming the path and the reason. + + The content is decoded as UTF-8 rather than in whatever encoding the locale + implies. The daemon usually runs under a service manager with ``LC_ALL=C``, + where the interpreter's default is ASCII unless it manages to coerce the + locale to C.UTF-8 — so on a box that lacks that locale, a settings file with + an accent in a name or a prompt hint refuses to load under the service + manager and loads fine from an interactive shell, which is a miserable thing + to debug. YAML is UTF-8 by spec, so pinning it is also the correct reading + of the file. + """ + if not path.exists(): + return {} + try: + with open(path, encoding="utf-8") as f: + data = yaml.safe_load(f) + except yaml.YAMLError as e: + raise ConfigError(f"Failed to parse {path}: {e}") from e + except (OSError, UnicodeDecodeError) as e: + raise ConfigError(f"Failed to read {path}: {e}") from e + if data is None: + return {} + if not isinstance(data, dict): + if strict: + raise ConfigError( + f"{path}: root must be a mapping of config keys, got " + f"{type(data).__name__}" + ) + logger.warning("config: %s is not a mapping — ignoring", path) + return {} + return data + + +def workspace_config_dir(workspace: Path) -> Path: + """The git-syncable config subtree inside a workspace (``/config``).""" + return Path(workspace) / "config" + + +def workspace_settings_file(workspace: Path) -> Path: + """Shareable settings file inside a workspace (``/config/settings.yaml``).""" + return workspace_config_dir(workspace) / "settings.yaml" + + +def _load_workspace_settings(workspace: Path) -> dict[str, Any]: + """Load ``workspace/config/settings.yaml``. + + The ``workspace`` key is stripped: the workspace location is resolved from + the machine-local config *before* this file is read, so honoring a + ``workspace`` value here would be circular. + + Read strictly. This is the layer that arrives from a shared repo, where + nobody is watching the daemon's log — a file of the wrong shape has to + fail loudly rather than evaporate into ``{}``. + """ + data = _read_yaml_mapping(workspace_settings_file(workspace), strict=True) + if "workspace" in data: + logger.warning( + "config: ignoring 'workspace' key in workspace/config/settings.yaml " + "(the workspace location must come from config.yaml or the default)" + ) + data = {k: v for k, v in data.items() if k != "workspace"} + return data + + +def _read_config_sources(config_dir: Path) -> dict[str, Any]: + """Merge all config layers for ``config_dir`` and resolve ${ENV_VAR} refs. + + Returns the fully-merged, env-interpolated config dict (untyped). Shared by + :func:`load_config` and :func:`load_mcp_servers` so both see the same view. + """ + base = _read_yaml_mapping(config_dir / "config.yaml") + local = _read_yaml_mapping(config_dir / "config.local.yaml") + + # The workspace path comes from the machine-local config (or the default), + # never from the tracked settings file — resolve it first. Best-effort + # ${VAR}/${VAR:-default} interpolation is applied so an env-based workspace + # path is honored when locating settings.yaml; an unresolved required ref is + # left intact here and surfaces later in the full _resolve_env_refs pass. + machine = _deep_merge(base, local) + ws_raw = machine.get("workspace") + if isinstance(ws_raw, str) and "${" in ws_raw: + ws_raw = _interpolate_str(ws_raw, []) + workspace = _expand_path(ws_raw) or paths.default_workspace() + + ws_settings = _load_workspace_settings(workspace) + + # Precedence, lowest first: workspace settings < config.yaml < config.local.yaml + merged = _deep_merge(ws_settings, base) + merged = _deep_merge(merged, local) + + return _resolve_env_refs(merged) + + @dataclass class SSLConfig: cert: Path | None = None @@ -1961,21 +2091,7 @@ def load_mcp_servers(config_dir: Path | None = None) -> list[McpServerConfig]: if config_dir is None: config_dir = Path.cwd() - base_path = config_dir / "config.yaml" - local_path = config_dir / "config.local.yaml" - - base: dict[str, Any] = {} - if base_path.exists(): - with open(base_path) as f: - base = yaml.safe_load(f) or {} - - local: dict[str, Any] = {} - if local_path.exists(): - with open(local_path) as f: - local = yaml.safe_load(f) or {} - - merged = _deep_merge(base, local) - merged = _resolve_env_refs(merged) + merged = _read_config_sources(config_dir) return _parse_mcp_servers(merged) @@ -2055,7 +2171,14 @@ def resolve_config_dir(explicit: str | Path | None = None) -> tuple[Path, str]: def load_config(config_dir: Path | None = None) -> NerveConfig: - """Load config from config.yaml + config.local.yaml in the given directory. + """Load and type the effective configuration. + + Merges (lowest→highest precedence) ``workspace/config/settings.yaml`` + (shareable, git-tracked), ``config.yaml`` (machine base), and + ``config.local.yaml`` (machine secrets/overrides), then resolves + ``${ENV_VAR}`` references. The workspace location is taken from the + machine-local config (or the default) before the tracked settings file is + read. If config_dir is None, the directory is resolved via the waterfall in :func:`resolve_config_dir` (flag/env/cwd/pointer), so commands behave the @@ -2064,24 +2187,9 @@ def load_config(config_dir: Path | None = None) -> NerveConfig: if config_dir is None: config_dir, _source = resolve_config_dir() - base_path = config_dir / "config.yaml" - local_path = config_dir / "config.local.yaml" - - base: dict[str, Any] = {} - if base_path.exists(): - with open(base_path) as f: - base = yaml.safe_load(f) or {} - - local: dict[str, Any] = {} - if local_path.exists(): - with open(local_path) as f: - local = yaml.safe_load(f) or {} - - merged = _deep_merge(base, local) - - # Resolve ${ENV_VAR} references before typing the config so secrets can be - # supplied from the environment rather than committed to tracked files. - merged = _resolve_env_refs(merged) + # Assemble config from workspace/config/settings.yaml + config.yaml + + # config.local.yaml (lowest→highest precedence) and resolve ${ENV_VAR} refs. + merged = _read_config_sources(config_dir) # Surface typos and stale keys instead of silently ignoring them. for warning in validate_config_keys(merged): diff --git a/nerve/gateway/routes/mcp_servers.py b/nerve/gateway/routes/mcp_servers.py index abec5091..dc531f37 100644 --- a/nerve/gateway/routes/mcp_servers.py +++ b/nerve/gateway/routes/mcp_servers.py @@ -46,7 +46,14 @@ async def get_mcp_server_usage( @router.post("/api/mcp-servers/reload") async def reload_mcp_servers(user: dict = Depends(require_auth)): """Re-read MCP server config from YAML files and refresh cache.""" + from nerve.config import ConfigError + deps = get_deps() - servers = await deps.engine.reload_mcp_config() + try: + servers = await deps.engine.reload_mcp_config() + except ConfigError as e: + # e.g. an unresolved required ${ENV_VAR} in config — report cleanly + # instead of a 500 so the caller sees what to fix. + raise HTTPException(status_code=400, detail=str(e)) from e stats = await deps.db.get_mcp_server_stats() return {"reloaded": len(servers), "servers": stats} diff --git a/nerve/templates/config/settings.yaml b/nerve/templates/config/settings.yaml new file mode 100644 index 00000000..6aa498ef --- /dev/null +++ b/nerve/templates/config/settings.yaml @@ -0,0 +1,46 @@ +# Nerve — shareable workspace configuration +# +# This file lives inside the workspace (workspace/config/settings.yaml) and is +# the portable, git-syncable home for settings you want to share across a team +# or a fleet of instances. It is merged UNDERNEATH the machine-local config.yaml +# and config.local.yaml, so a machine can still override anything here locally +# (until lockdown mode is enabled, which makes this the only source of truth). +# +# Precedence (lowest → highest): +# 1. this file (workspace/config/settings.yaml) ← shareable, tracked +# 2. config.yaml ← machine base +# 3. config.local.yaml ← machine secrets/overrides +# +# Keep SECRETS out of this file — it is meant to be committed to a shared repo. +# Reference them from the environment instead: +# anthropic_api_key: ${ANTHROPIC_API_KEY} +# some_optional: ${SOME_VAR:-default} +# +# The `workspace` key is NOT honored here (it would be circular — the workspace +# location is resolved from config.yaml or the default before this file loads). +# +# Everything below is commented out; uncomment and edit what you want to share. + +# timezone: America/New_York + +# Where the gateway listens. A box needing a different port overrides in its own +# config.yaml. Do not put gateway.ssl.cert/.key here: they are local filesystem +# paths, and a box without that file fails to start. +# gateway: +# host: 0.0.0.0 +# port: 8900 + +# Provider and region. Bedrock model IDs are geography-scoped to aws_region +# (us./eu./apac.) and must match it or every call is rejected, so they belong +# next to it. provider.aws_profile does not go here — it names a profile in one +# machine's AWS credentials file. +# provider: +# type: bedrock +# aws_region: eu-west-1 + +# agent: +# model: claude-opus-5 +# effort: high + +# notifications: +# channels: [telegram] diff --git a/nerve/workspace.py b/nerve/workspace.py index 301cbb15..d58777c7 100644 --- a/nerve/workspace.py +++ b/nerve/workspace.py @@ -125,6 +125,58 @@ def install_bundled_skills(workspace_path: Path) -> list[str]: return installed +def _get_config_scaffold_dir() -> Path | None: + """Resolve the shared config scaffold directory (nerve/templates/config/). + + Returns None if it doesn't exist. + """ + try: + ref = importlib.resources.files("nerve") / "templates" / "config" + cfg_path = Path(str(ref)) + if cfg_path.is_dir(): + return cfg_path + except (TypeError, FileNotFoundError): + pass + + cfg_path = Path(__file__).parent / "templates" / "config" + if cfg_path.is_dir(): + return cfg_path + + return None + + +def install_config_scaffold(workspace_path: Path) -> list[str]: + """Create the git-syncable ``workspace/config/`` subtree. + + Copies the shared scaffold (currently ``settings.yaml``) into + ``workspace/config/`` without overwriting anything that already exists. + Later stories add ``cron/`` and ``sources.yaml`` under the same subtree. + + Returns the list of files created (relative to the workspace). + """ + scaffold_dir = _get_config_scaffold_dir() + config_dir = workspace_path / "config" + config_dir.mkdir(parents=True, exist_ok=True) + + if scaffold_dir is None: + logger.debug("No config scaffold directory found — created empty config/") + return [] + + created: list[str] = [] + for src in sorted(scaffold_dir.iterdir()): + if not src.is_file(): + continue + dst = config_dir / src.name + if dst.exists(): + logger.debug("Skipping config/%s — already exists", src.name) + continue + shutil.copy2(src, dst) + logger.info("Created config/%s", src.name) + created.append(f"config/{src.name}") + + return created + + def initialize_workspace(workspace_path: Path, mode: str) -> list[str]: """Copy mode-appropriate template files into a workspace directory. diff --git a/tests/test_backup.py b/tests/test_backup.py index 1e9dee2b..74a779f2 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -267,6 +267,40 @@ def test_backup_excludes_junk(nerve_dir, workspace, config_dir, tmp_path): assert any(m.endswith("workspace/scripts/helper.py") for m in members) +def test_backup_captures_tracked_workspace_config( + nerve_dir, workspace, config_dir, tmp_path, +): + """The shareable config subtree has to survive a lost machine. + + Everything that decides *what the agent does on a schedule* lives here. A + bundle that restores the identity files and the skills but not the settings + or the cron jobs looks complete and is not, and the gap only shows up when + someone restores it. + """ + cfg = workspace / "config" + (cfg / "cron" / "gates").mkdir(parents=True) + (cfg / "settings.yaml").write_text("agent:\n name: nerve\n") + (cfg / "cron" / "jobs.yaml").write_text("jobs:\n - id: nightly\n") + (cfg / "cron" / "system.yaml").write_text("jobs:\n - id: cleanup\n") + (cfg / "cron" / "gates" / "quiet_hours.py").write_text("def gate():\n return True\n") + # Junk inside the subtree is still pruned like anywhere else. + (cfg / "cron" / "gates" / "__pycache__").mkdir() + (cfg / "cron" / "gates" / "__pycache__" / "quiet_hours.pyc").write_text("junk") + + out = tmp_path / "out" + result = backup_mod.create_backup(nerve_dir, workspace, out, config_dir=config_dir) + members = _bundle_members(result.path) + + for expected in ( + "workspace/config/settings.yaml", + "workspace/config/cron/jobs.yaml", + "workspace/config/cron/system.yaml", + "workspace/config/cron/gates/quiet_hours.py", + ): + assert any(m.endswith(expected) for m in members), f"missing {expected}" + assert "__pycache__" not in "\n".join(members) + + def test_workspace_extra_excludes(nerve_dir, workspace, config_dir, tmp_path): # Plant a file the user wants excluded via config glob. (workspace / "memory" / "diagram.png").write_text("PNG") diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index f1f2a553..cc237eaf 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -100,11 +100,17 @@ def test_creates_all_files(self, tmp_path: Path) -> None: with patch.dict(os.environ, env, clear=False): run_non_interactive(tmp_path) - # config.yaml exists + # config.yaml holds the machine-local half... assert (tmp_path / "config.yaml").exists() config = yaml.safe_load((tmp_path / "config.yaml").read_text()) - assert config["timezone"] == "Europe/London" assert config["workspace"] == str(tmp_path / "workspace") + # ...and the portable half lands in the git-tracked workspace layer, + # which is what `nerve config sync` and lockdown actually read. + settings = yaml.safe_load( + (tmp_path / "workspace" / "config" / "settings.yaml").read_text() + ) + assert settings["timezone"] == "Europe/London" + assert "timezone" not in config # config.local.yaml exists with API key assert (tmp_path / "config.local.yaml").exists() @@ -201,7 +207,11 @@ def test_apply_creates_files(self, tmp_path: Path) -> None: # Config content is valid YAML config = yaml.safe_load((tmp_path / "config.yaml").read_text()) - assert config["timezone"] == "US/Pacific" + assert config["workspace"] == str(tmp_path / "workspace") + settings = yaml.safe_load( + (tmp_path / "workspace" / "config" / "settings.yaml").read_text() + ) + assert settings["timezone"] == "US/Pacific" # Local config has keys local = yaml.safe_load((tmp_path / "config.local.yaml").read_text()) @@ -236,6 +246,25 @@ def test_if_needed_non_interactive(self, tmp_path: Path) -> None: assert result.exit_code == 0 assert (tmp_path / "config.local.yaml").exists() + def test_reinit_prompt_describes_the_backups_it_actually_makes( + self, configured_dir: Path + ) -> None: + """This prompt is how an operator decides whether re-running is safe. + + The wizard deliberately skips the ``.bak`` for an empty or + comments-only file — a freshly scaffolded settings.yaml has nothing to + lose and lives in a git-tracked directory. So a flat "all three are + backed up" is a promise the code does not keep, exactly where being + misled costs the most. + """ + result = CliRunner().invoke( + main, ["-c", str(configured_dir), "init"], input="n\n" + ) + assert result.exit_code == 0 + assert "*.bak" in result.output + assert "All three are backed up" not in result.output + assert "comments-only" in result.output + def test_non_interactive_fails_without_key(self, tmp_path: Path) -> None: """Non-interactive should fail without ANTHROPIC_API_KEY.""" (tmp_path).mkdir(exist_ok=True) @@ -841,11 +870,15 @@ def test_eu_region_writes_eu_models(self, tmp_path: Path, monkeypatch) -> None: with patch.dict(os.environ, env, clear=False): run_non_interactive(tmp_path) - config = yaml.safe_load((tmp_path / "config.yaml").read_text()) - assert config["provider"]["aws_region"] == "eu-central-1" - assert config["agent"]["model"].startswith("eu.anthropic.") - assert config["agent"]["cron_model"].startswith("eu.anthropic.") - assert config["memory"]["fast_model"].startswith("eu.anthropic.") + # The region and the model IDs derived from it are shared policy, so + # they land in the tracked layer and survive lockdown. + settings = yaml.safe_load( + (tmp_path / "ws" / "config" / "settings.yaml").read_text() + ) + assert settings["provider"]["aws_region"] == "eu-central-1" + assert settings["agent"]["model"].startswith("eu.anthropic.") + assert settings["agent"]["cron_model"].startswith("eu.anthropic.") + assert settings["memory"]["fast_model"].startswith("eu.anthropic.") def test_us_region_writes_us_models(self, tmp_path: Path, monkeypatch) -> None: monkeypatch.setenv("HOME", str(tmp_path / "home")) @@ -858,8 +891,10 @@ def test_us_region_writes_us_models(self, tmp_path: Path, monkeypatch) -> None: with patch.dict(os.environ, env, clear=False): run_non_interactive(tmp_path) - config = yaml.safe_load((tmp_path / "config.yaml").read_text()) - assert config["agent"]["model"].startswith("us.anthropic.") + settings = yaml.safe_load( + (tmp_path / "ws" / "config" / "settings.yaml").read_text() + ) + assert settings["agent"]["model"].startswith("us.anthropic.") class TestNonInteractiveTelegramAllowedUsers: @@ -1007,3 +1042,336 @@ def test_skipped_steps_keep_numbering(self, tmp_path: Path) -> None: wizard._do("mode", lambda: None) assert wizard._step_counter == 2 assert wizard._next_step("API") == "Step 3/10: API" + + +class TestWorkspaceExpansionMatchesTheLoader: + """The wizard must resolve `workspace` to the same directory as the loader. + + It decides where settings.yaml is written; the loader decides where it is + read. Any disagreement means `nerve init` reports success and every portable + setting is silently absent — and on a locked box, which has no other layer, + that leaves the instance running entirely on declared defaults. + """ + + @staticmethod + def _loader_answer(raw: str) -> Path: + """What nerve.config would resolve `workspace: ` to.""" + import nerve.config as cfgmod + from nerve import paths + + if "${" in raw: + raw = cfgmod._interpolate_str(raw, []) + return cfgmod._expand_path(raw) or paths.default_workspace() + + @pytest.mark.parametrize( + "raw", + [ + "~/nerve-workspace", + " ~/nerve-workspace", # leading space: expanduser is a no-op + "~/nerve-workspace\n", # e.g. NERVE_WORKSPACE from a heredoc + " /srv/nerve/ws ", + "${NERVE_TEST_WS}", + "$NERVE_TEST_WS", # bare form: needs expandvars first + "", # blank means unset + ], + ) + def test_agrees_with_the_loader(self, raw: str, monkeypatch) -> None: + monkeypatch.setenv("NERVE_TEST_WS", "~/from-env") + from nerve.bootstrap import _expand_workspace + + assert _expand_workspace(raw) == self._loader_answer(raw) + + def test_leading_whitespace_still_expands_the_home_directory( + self, monkeypatch, + ) -> None: + """The concrete failure: expanduser only expands a *leading* ``~``. + + Running it before the strip left ``Path(" ~/nerve-workspace")`` — a + relative path with a component literally named `` ~``. + """ + from nerve.bootstrap import _expand_workspace + + expanded = _expand_workspace(" ~/nerve-workspace") + assert expanded.is_absolute() + assert "~" not in str(expanded) + + def test_a_bare_env_var_holding_a_tilde_is_fully_expanded( + self, monkeypatch, + ) -> None: + """expandvars has to run before expanduser, not after. + + ``$WS`` expands to ``~/ws``, which then still needs expanduser. Doing it + the other way round ends at a literal tilde. + """ + monkeypatch.setenv("NERVE_TEST_WS", "~/ws") + from nerve.bootstrap import _expand_workspace + + expanded = _expand_workspace("$NERVE_TEST_WS") + assert expanded.is_absolute() + assert "~" not in str(expanded) + assert expanded.name == "ws" + + def test_the_wizard_writes_where_the_loader_reads(self, tmp_path: Path) -> None: + """End to end, with the whitespace that used to split the two apart.""" + from nerve.config import load_config + + ws = tmp_path / "ws" + wizard = SetupWizard(tmp_path) + wizard.choices.mode = "personal" + wizard.choices.anthropic_api_key = "sk-ant-api03-test" + wizard.choices.timezone = "Europe/Amsterdam" + # A trailing newline is what an env var out of a compose file or heredoc + # actually looks like. + wizard.choices.workspace_path = f"{ws}\n" + wizard._write_config_yaml() + wizard._write_workspace_settings() + + assert load_config(tmp_path).timezone == "Europe/Amsterdam" + + +class TestPortableSettingsSplit: + """`nerve init` must actually populate the tracked settings layer. + + Before the split the wizard wrote ~33 keys into machine-local config.yaml + and zero into settings.yaml, so `nerve config sync` and lockdown were + no-ops on a default install: adopting the shared layer meant hand-adding a + key to settings.yaml *and* deleting it from config.yaml, because + config.yaml shadows it. + """ + + def _wizard(self, tmp_path: Path, **choices: Any) -> SetupWizard: + w = SetupWizard(tmp_path) + w.choices.workspace_path = tmp_path / "workspace" + w.choices.mode = "personal" + w.choices.anthropic_api_key = "sk-ant-api03-test" + for k, v in choices.items(): + setattr(w.choices, k, v) + return w + + def test_layers_are_disjoint(self, tmp_path: Path) -> None: + """A key in both layers makes the tracked copy dead weight. + + config.yaml shadows settings.yaml, so a value written to both can be + edited in the shared repo forever with no effect. + """ + def leaves(d: dict, prefix: str = "") -> set[str]: + out: set[str] = set() + for k, v in d.items(): + path = f"{prefix}{k}" + if isinstance(v, dict): + out |= leaves(v, f"{path}.") + else: + out.add(path) + return out + + for kwargs in ( + {}, + {"provider_type": "bedrock", "aws_region": "eu-west-1"}, + {"use_proxy": True}, + {"deployment": "docker"}, + {"mode": "worker"}, + {"telegram_bot_token": "123:abc"}, + ): + machine, portable, _ = self._wizard(tmp_path, **kwargs)._build_config_layers() + overlap = leaves(machine) & leaves(portable) + assert not overlap, f"{kwargs} → written to both layers: {overlap}" + + def test_machine_layer_is_only_machine_things(self, tmp_path: Path) -> None: + machine, _portable, _shadowed = self._wizard(tmp_path)._build_config_layers() + assert set(machine) <= { + "workspace", "deployment", "gateway", "telegram", + "provider", "proxy", "docker", "agent", "memory", "sync", + } + assert "timezone" not in machine + assert "sessions" not in machine + # Which sources sync is shared policy; whose mailboxes is not. + assert set(machine.get("sync", {})) == {"gmail"} + assert set(machine["sync"]["gmail"]) == {"accounts"} + + def test_bedrock_models_travel_with_the_region(self, tmp_path: Path) -> None: + """Geography-scoped model IDs are shared, because the region is. + + They were machine-local on the grounds that a ``us.`` id is wrong for an + ``eu-`` box. The effect was that a locked instance fell back to the + non-prefixed declared defaults, which Bedrock rejects, and to an + ``anthropic`` provider, since that block was machine-local too. The + prefix is a function of the region alone, so tracking the region lets + the IDs be tracked with it. + """ + machine, portable, shadowed = self._wizard( + tmp_path, provider_type="bedrock", aws_region="eu-west-1" + )._build_config_layers() + assert portable["provider"]["type"] == "bedrock" + assert portable["provider"]["aws_region"] == "eu-west-1" + assert portable["agent"]["model"].startswith("eu.anthropic.") + assert portable["memory"]["fast_model"].startswith("eu.anthropic.") + assert "agent" not in machine + assert "memory" not in machine + # Non-model agent settings were portable already and stay that way. + assert portable["agent"]["thinking"] == "max" + + def test_only_the_aws_profile_stays_machine_local(self, tmp_path: Path) -> None: + """The profile names an entry in this box's AWS credentials file. + + Provider and region describe the deployment; the profile is the only part + of the block that cannot be shared. + """ + machine, portable, _ = self._wizard( + tmp_path, + provider_type="bedrock", + aws_region="eu-west-1", + aws_profile="nerve-prod", + )._build_config_layers() + assert machine["provider"] == {"aws_profile": "nerve-prod"} + assert "aws_profile" not in portable["provider"] + + def test_switching_off_bedrock_clears_the_tracked_region( + self, tmp_path: Path + ) -> None: + """settings.yaml is merge-preserving, so the region must be deleted. + + Omitting it would leave the tracked file naming a region the box no + longer uses, and under lockdown that stale value is the only one there. + """ + self._wizard( + tmp_path, provider_type="bedrock", aws_region="eu-west-1" + )._apply() + settings_path = tmp_path / "workspace" / "config" / "settings.yaml" + before = yaml.safe_load(settings_path.read_text()) + assert before["provider"]["aws_region"] == "eu-west-1" + assert before["agent"]["model"].startswith("eu.anthropic.") + + self._wizard(tmp_path, provider_type="anthropic")._apply() + + after = yaml.safe_load(settings_path.read_text()) + assert after["provider"]["type"] == "anthropic" + assert "aws_region" not in after.get("provider", {}) + # The non-prefixed names overwrite rather than needing deletion. + assert after["agent"]["model"] == "claude-opus-5" + + def test_gateway_host_and_port_are_shared(self, tmp_path: Path) -> None: + """The tracked layer has to be able to state the bind address. + + While these were machine-local the wizard only ever wrote the declared + defaults, so settings.yaml could not express them at all. + """ + machine, portable, _ = self._wizard(tmp_path)._build_config_layers() + assert portable["gateway"] == {"host": "0.0.0.0", "port": 8900} + assert "gateway" not in machine + + def test_reinit_applies_new_answers(self, tmp_path: Path) -> None: + """The wizard owns the keys it generates. + + "Existing always wins" sounds safer for a shared file, but it makes + re-running init a no-op: you are prompted for a timezone, shown a + tick, and the answer is discarded because the key already exists. + """ + self._wizard(tmp_path, timezone="Europe/Berlin")._apply() + settings_path = tmp_path / "workspace" / "config" / "settings.yaml" + assert yaml.safe_load(settings_path.read_text())["timezone"] == "Europe/Berlin" + + self._wizard(tmp_path, timezone="US/Pacific", gmail_sync=True)._apply() + after = yaml.safe_load(settings_path.read_text()) + assert after["timezone"] == "US/Pacific" + assert after["sync"]["gmail"]["enabled"] is True + + def test_reinit_preserves_keys_the_wizard_does_not_own(self, tmp_path: Path) -> None: + """A team policy key the wizard never emits must survive.""" + self._wizard(tmp_path)._apply() + settings_path = tmp_path / "workspace" / "config" / "settings.yaml" + edited = yaml.safe_load(settings_path.read_text()) + edited["team_only_key"] = "keep me" + edited["agent"]["cache_ttl"] = "1h" # real key, not wizard-generated + settings_path.write_text(yaml.safe_dump(edited), encoding="utf-8") + + self._wizard(tmp_path)._apply() + + after = yaml.safe_load(settings_path.read_text()) + assert after["team_only_key"] == "keep me" + assert after["agent"]["cache_ttl"] == "1h" + assert after["agent"]["thinking"] == "max" + + def test_switching_to_bedrock_rewrites_the_tracked_model_names( + self, tmp_path: Path + ) -> None: + """Re-running init replaces the names in place, in the shared file. + + The previous behaviour routed them to config.yaml and deleted them here, + which left a locked box with no model names at all: it fell back to the + non-prefixed declared defaults, which Bedrock rejects. + """ + self._wizard(tmp_path)._apply() + settings_path = tmp_path / "workspace" / "config" / "settings.yaml" + assert yaml.safe_load(settings_path.read_text())["agent"]["model"] == ( + "claude-opus-5" + ) + + self._wizard( + tmp_path, provider_type="bedrock", aws_region="eu-west-1" + )._apply() + + after = yaml.safe_load(settings_path.read_text()) + assert after["agent"]["model"].startswith("eu.anthropic.") + assert after["memory"]["recall_model"].startswith("eu.anthropic.") + assert after["provider"] == {"type": "bedrock", "aws_region": "eu-west-1"} + # And still nothing left in both layers. + machine = yaml.safe_load((tmp_path / "config.yaml").read_text()) + assert not ( + set(SetupWizard._leaf_paths(after)) + & set(SetupWizard._leaf_paths(machine)) + ) + + def test_fresh_install_leaves_no_bak_in_the_tracked_subtree( + self, tmp_path: Path + ) -> None: + """The scaffold is comments-only; backing it up drops junk in a git dir.""" + self._wizard(tmp_path)._apply() + ws_config = tmp_path / "workspace" / "config" + assert not (ws_config / "settings.yaml.bak").exists() + + def test_workspace_path_with_env_ref_lands_where_the_loader_looks( + self, tmp_path: Path, monkeypatch + ) -> None: + """The loader interpolates ${VAR} in `workspace`; the writer must too, + or settings.yaml goes to a literal './${VAR}/config' directory and + every portable setting is silently lost.""" + from nerve.config import load_config + + real_ws = tmp_path / "real-ws" + monkeypatch.setenv("MY_WS", str(real_ws)) + wizard = self._wizard(tmp_path, timezone="Asia/Tokyo") + wizard.choices.workspace_path = Path("${MY_WS}") + wizard._apply() + + assert (real_ws / "config" / "settings.yaml").exists() + assert load_config(tmp_path).timezone == "Asia/Tokyo" + + def test_reinit_backs_up_settings(self, tmp_path: Path) -> None: + wizard = self._wizard(tmp_path) + wizard._apply() + settings_path = tmp_path / "workspace" / "config" / "settings.yaml" + settings_path.write_text("timezone: UTC\n", encoding="utf-8") + self._wizard(tmp_path)._apply() + assert (settings_path.parent / "settings.yaml.bak").read_text() == "timezone: UTC\n" + + def test_malformed_settings_is_left_alone(self, tmp_path: Path) -> None: + """Don't destroy a file we can't parse — the operator needs to see it.""" + wizard = self._wizard(tmp_path) + ws_config = tmp_path / "workspace" / "config" + ws_config.mkdir(parents=True) + broken = "timezone: [unclosed\n" + (ws_config / "settings.yaml").write_text(broken, encoding="utf-8") + wizard._apply() + assert (ws_config / "settings.yaml").read_text() == broken + + def test_merged_result_still_loads(self, tmp_path: Path) -> None: + """The split must be invisible to the loader: same effective config.""" + from nerve.config import load_config + + self._wizard(tmp_path, timezone="US/Pacific")._apply() + cfg = load_config(tmp_path) + assert cfg.timezone == "US/Pacific" # from settings.yaml + assert cfg.workspace == tmp_path / "workspace" # from config.yaml + assert cfg.gateway.port == 8900 # from config.yaml + assert cfg.agent.thinking == "max" # from settings.yaml + assert cfg.sessions.max_sessions == 500 # from settings.yaml diff --git a/tests/test_config_sources.py b/tests/test_config_sources.py new file mode 100644 index 00000000..0185bca8 --- /dev/null +++ b/tests/test_config_sources.py @@ -0,0 +1,449 @@ +"""Tests for multi-source config resolution. + +workspace/config/settings.yaml (shareable) is merged underneath config.yaml +(machine base) and config.local.yaml (machine secrets/overrides). +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from nerve.config import ( + ConfigError, + load_config, + load_mcp_servers, + workspace_config_dir, + workspace_settings_file, +) +from nerve.workspace import install_config_scaffold + + +def _write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + +def _setup(tmp_path: Path): + """Return (config_dir, workspace) with the workspace wired into config.yaml.""" + config_dir = tmp_path / "cfg" + workspace = tmp_path / "ws" + config_dir.mkdir(parents=True, exist_ok=True) + workspace.mkdir(parents=True, exist_ok=True) + return config_dir, workspace + + +class TestMultiSourceMerge: + def test_workspace_settings_applied(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "timezone: Europe/Amsterdam\n") + + cfg = load_config(config_dir) + assert cfg.timezone == "Europe/Amsterdam" + + def test_config_yaml_overrides_workspace_settings(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write( + config_dir / "config.yaml", + f"workspace: {workspace}\ntimezone: America/New_York\n", + ) + _write(workspace_settings_file(workspace), "timezone: Europe/Amsterdam\n") + + cfg = load_config(config_dir) + # Machine config.yaml wins over shared settings. + assert cfg.timezone == "America/New_York" + + def test_local_overrides_everything(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write( + config_dir / "config.yaml", + f"workspace: {workspace}\ntimezone: America/New_York\n", + ) + _write(config_dir / "config.local.yaml", "timezone: UTC\n") + _write(workspace_settings_file(workspace), "timezone: Europe/Amsterdam\n") + + cfg = load_config(config_dir) + assert cfg.timezone == "UTC" + + def test_absent_settings_is_backwards_compatible(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write( + config_dir / "config.yaml", + f"workspace: {workspace}\ntimezone: America/New_York\n", + ) + # No workspace/config/settings.yaml at all. + cfg = load_config(config_dir) + assert cfg.timezone == "America/New_York" + + def test_workspace_key_in_settings_ignored(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + bogus = tmp_path / "bogus_ws" + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write( + workspace_settings_file(workspace), + f"workspace: {bogus}\ntimezone: Europe/Amsterdam\n", + ) + + cfg = load_config(config_dir) + # settings.yaml's workspace key is ignored (would be circular); the + # real workspace stays the one from config.yaml, and its other keys apply. + assert cfg.workspace == workspace + assert cfg.timezone == "Europe/Amsterdam" + + def test_settings_can_reference_env(self, tmp_path, monkeypatch): + config_dir, workspace = _setup(tmp_path) + monkeypatch.setenv("TZ_FROM_ENV", "Asia/Tokyo") + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "timezone: ${TZ_FROM_ENV}\n") + + cfg = load_config(config_dir) + assert cfg.timezone == "Asia/Tokyo" + + def test_nested_deep_merge_across_layers(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write( + config_dir / "config.yaml", + f"workspace: {workspace}\nagent:\n effort: high\n", + ) + _write( + workspace_settings_file(workspace), + "agent:\n model: claude-opus-4-8\n", + ) + cfg = load_config(config_dir) + # Nested dicts deep-merge across layers rather than clobbering. + assert cfg.agent.model == "claude-opus-4-8" # from settings + assert cfg.agent.effort == "high" # from config.yaml + + def test_workspace_path_env_default_locates_settings(self, tmp_path, monkeypatch): + config_dir = tmp_path / "cfg" + workspace = tmp_path / "wsx" + config_dir.mkdir(parents=True, exist_ok=True) + workspace.mkdir(parents=True, exist_ok=True) + monkeypatch.delenv("WS_DIR", raising=False) + # workspace path itself uses a ${VAR:-default} ref — the default must be + # honored when locating settings.yaml. + _write(config_dir / "config.yaml", f'workspace: "${{WS_DIR:-{workspace}}}"\n') + _write(workspace_settings_file(workspace), "timezone: Pacific/Auckland\n") + cfg = load_config(config_dir) + assert cfg.workspace == workspace + assert cfg.timezone == "Pacific/Auckland" + + def test_non_mapping_settings_is_an_error(self, tmp_path): + """A shared settings file of the wrong shape must not evaporate. + + Ignoring it means every key it was supposed to supply silently + reverts to the machine-local layers, with nothing but a log line — + and this is the layer that arrives from a remote repo, where nobody + is reading the daemon's log. A truncated write or a merge conflict + resolved into a sequence produces exactly this. + """ + config_dir, workspace = _setup(tmp_path) + _write( + config_dir / "config.yaml", + f"workspace: {workspace}\ntimezone: America/New_York\n", + ) + _write(workspace_settings_file(workspace), "- just\n- a\n- list\n") + with pytest.raises(ConfigError) as excinfo: + load_config(config_dir) + assert "must be a mapping" in str(excinfo.value) + assert "list" in str(excinfo.value) + + def test_comments_only_settings_is_fine(self, tmp_path): + """The shipped scaffold is 100% comments and parses to None. + + Treating "present but empty" as an error would break every fresh + install, so it stays permissive — only a wrong *shape* is an error. + """ + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write( + workspace_settings_file(workspace), + "# Shareable settings.\n# Uncomment what you need.\n", + ) + assert load_config(config_dir).workspace == workspace + + def test_empty_settings_file_is_fine(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "") + assert load_config(config_dir).workspace == workspace + + def test_malformed_settings_yaml_is_a_config_error(self, tmp_path): + """Not a raw yaml.ParserError — cli.py routes doctor/validate around + ConfigError specifically so they can report instead of crashing.""" + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "a: [1, 2\nb: {oops\n") + with pytest.raises(ConfigError) as excinfo: + load_config(config_dir) + assert "Failed to parse" in str(excinfo.value) + + def test_malformed_machine_config_is_a_config_error(self, tmp_path): + config_dir, _workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", "timezone: [unclosed\n") + with pytest.raises(ConfigError) as excinfo: + load_config(config_dir) + assert "Failed to parse" in str(excinfo.value) + + def test_non_mapping_machine_config_stays_lenient(self, tmp_path): + """config.yaml is local and hand-edited; the operator sees the warning + immediately. Only the layer that arrives over the wire is strict.""" + config_dir, _workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", "- not\n- a\n- mapping\n") + assert load_config(config_dir).timezone # loads with defaults + + # geteuid is POSIX-only and this runs at collection time, so an unguarded + # call is an error for the whole module rather than a skip for one test. + # Skipping where it is absent is also the right answer on the merits: a + # chmod(0o000) does not take read access away on Windows, so the test could + # not pass there even if it were collected. + @pytest.mark.skipif( + not hasattr(os, "geteuid") or os.geteuid() == 0, + reason="root (or a platform without POSIX uids) ignores file permissions", + ) + def test_unreadable_settings_is_a_config_error(self, tmp_path): + """A file we can see but cannot open is a config problem too. + + The layer exists, so the loader commits to reading it and gets an + OSError — wrong mode after a careless chmod, owned by another user, or + deleted between the exists() check and the open. Letting that escape + gives a PermissionError traceback out of the group callback, which + also takes down the commands you would use to fix it. + """ + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + settings = workspace_settings_file(workspace) + _write(settings, "timezone: Europe/Amsterdam\n") + settings.chmod(0o000) + try: + with pytest.raises(ConfigError) as excinfo: + load_config(config_dir) + finally: + settings.chmod(0o600) + # The path and the reason both have to survive — "cannot read config" + # with neither is not actionable. + assert str(settings) in str(excinfo.value) + assert "Permission denied" in str(excinfo.value) + + def test_non_utf8_settings_is_a_config_error(self, tmp_path): + """Latin-1 bytes in a UTF-8 file: still a report, not a traceback.""" + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + settings = workspace_settings_file(workspace) + settings.parent.mkdir(parents=True, exist_ok=True) + settings.write_bytes("timezone: Europe/Zürich\n".encode("latin-1")) + with pytest.raises(ConfigError) as excinfo: + load_config(config_dir) + assert str(settings) in str(excinfo.value) + + def test_non_ascii_settings_load_in_a_non_utf8_locale(self, tmp_path): + """The locale must not decide whether a valid UTF-8 file parses. + + Runs a real load under ``LC_ALL=C`` with PEP 538 locale coercion + disabled — the environment a daemon gets from a service manager on an + image with no C.UTF-8 locale. There the interpreter's default encoding + is ASCII, so an unpinned open() fails on the first accented byte: the + same config file loads from an interactive shell and refuses to load + under systemd. + """ + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write( + workspace_settings_file(workspace), + "timezone: Europe/Amsterdam\nsync:\n gmail:\n" + " prompt_hint: Grüße aus München — ça va?\n", + ) + proc = subprocess.run( + [ + sys.executable, "-c", + "import sys; from pathlib import Path;" + "from nerve.config import load_config;" + "print(load_config(Path(sys.argv[1])).sync.gmail.prompt_hint)", + str(config_dir), + ], + capture_output=True, text=True, encoding="utf-8", + env={ + **os.environ, + "LC_ALL": "C", + "LANG": "C", + "PYTHONCOERCECLOCALE": "0", # defeat PEP 538 C.UTF-8 coercion + "PYTHONUTF8": "0", # ...and PEP 540 UTF-8 mode + "PYTHONIOENCODING": "utf-8", # so we can read the answer back + # Beat the editable install: this checkout, not site-packages. + "PYTHONPATH": str(Path(__file__).resolve().parents[1]), + }, + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "Grüße aus München — ça va?" + + def test_init_writes_utf8_in_a_non_utf8_locale(self, tmp_path): + """The write side of the previous test: the wizard, not the loader. + + Every file `nerve init` emits starts with a comment header containing an + em-dash, so in this environment an unpinned open() raises on the first + write, before any user value is involved. A user value cannot trigger it: + safe_dump escapes non-ASCII to \\xNN, so the dumped body is always ASCII. + + The wizard also reads its own output back, under ``except Exception``. + A decode failure there does not abort the run — _resolve_jwt_secret + returns a different secret, signs the external-agent MCP token with it, + and the daemon rejects every call that token makes. The assertions below + therefore check the round-trip, not just that bytes were written. + """ + config_dir = tmp_path / "cfg" + config_dir.mkdir(parents=True, exist_ok=True) + proc = subprocess.run( + [ + sys.executable, "-c", + "import sys, yaml; from pathlib import Path;" + "from nerve.bootstrap import SetupWizard;" + "from nerve.config import load_config;" + "d = Path(sys.argv[1]);" + "w = SetupWizard(d);" + "w.choices.workspace_path = d / 'ws';" + "w.choices.mode = 'personal';" + "w.choices.timezone = 'Europe/Amsterdam';" + "w.choices.anthropic_api_key = 'sk-ant-api03-test';" + "w._write_config_yaml();" + "w._write_workspace_settings();" + "w._write_config_local_yaml();" + "w._write_cron_jobs();" + # The Dockerfile and entrypoint templates carry an em-dash too. + "w._ensure_docker_files();" + # Read-back paths: the secret recovered from disk must be the + # one on disk, and the URL must reflect the config just written. + "on_disk = yaml.safe_load(" + " (d / 'config.local.yaml').read_text(encoding='utf-8'));" + "print('JWT_MATCH'," + " w._resolve_jwt_secret()" + " == (on_disk.get('auth') or {}).get('jwt_secret'));" + "print('MCP_URL', w._compute_nerve_mcp_url());" + "print(load_config(d).timezone)", + str(config_dir), + ], + capture_output=True, text=True, encoding="utf-8", + env={ + **os.environ, + "LC_ALL": "C", + "LANG": "C", + "PYTHONCOERCECLOCALE": "0", + "PYTHONUTF8": "0", + "PYTHONIOENCODING": "utf-8", + # Keep the cron writer out of the real state dir. + "NERVE_HOME": str(tmp_path / "state"), + "PYTHONPATH": str(Path(__file__).resolve().parents[1]), + }, + ) + assert proc.returncode == 0, proc.stderr + out = proc.stdout + # Located rather than hardcoded: the cron directory moves from NERVE_HOME + # into the workspace later in this stack, and this file is carried up + # unchanged. + cron_files = sorted( + p + for root in (tmp_path / "state", config_dir / "ws") + if root.exists() + for p in root.rglob("*.yaml") + if p.name in {"system.yaml", "jobs.yaml"} + ) + assert [p.name for p in cron_files] == ["jobs.yaml", "system.yaml"], cron_files + # Written as UTF-8, not in the locale's encoding. The loader rejects a + # settings.yaml it cannot decode, so getting this wrong would produce a + # config the daemon refuses to load. + for path in ( + config_dir / "config.yaml", + config_dir / "config.local.yaml", + workspace_settings_file(config_dir / "ws"), + *cron_files, + ): + assert "—" in path.read_text(encoding="utf-8"), path + assert "JWT_MATCH True" in out, out + # host 0.0.0.0 is rewritten to localhost, so this is the configured + # gateway rather than the hardcoded 127.0.0.1 fallback a failed read + # would leave behind. + assert "MCP_URL http://localhost:8900/mcp/v1/" in out, out + # Last line, not the whole of stdout: the settings writer prints an + # "added: ..." summary of the keys it emitted ahead of this. + assert out.strip().splitlines()[-1] == "Europe/Amsterdam" + + +class TestMcpServersFromWorkspace: + def test_mcp_server_defined_in_workspace_settings(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write( + workspace_settings_file(workspace), + "mcp_servers:\n" + " shared:\n" + " type: http\n" + " url: https://example.com/mcp\n", + ) + servers = load_mcp_servers(config_dir) + assert any(s.name == "shared" for s in servers) + + +class TestConfigScaffold: + def test_scaffold_creates_settings(self, tmp_path): + created = install_config_scaffold(tmp_path) + assert (workspace_config_dir(tmp_path)).is_dir() + assert workspace_settings_file(tmp_path).exists() + assert "config/settings.yaml" in created + + def test_scaffold_is_idempotent_and_non_destructive(self, tmp_path): + install_config_scaffold(tmp_path) + # User edits their settings. + workspace_settings_file(tmp_path).write_text("timezone: UTC\n", encoding="utf-8") + created = install_config_scaffold(tmp_path) + assert created == [] # nothing re-created + assert workspace_settings_file(tmp_path).read_text() == "timezone: UTC\n" + + +class TestBrokenConfigIsRepairable: + """A settings.yaml that won't load must not take out the tools you'd use + to fix it. The group callback runs before every subcommand, so without an + exemption a bad shared file bricks `doctor` and `init` too.""" + + def _broken(self, tmp_path): + config_dir, workspace = _setup(tmp_path) + _write(config_dir / "config.yaml", f"workspace: {workspace}\n") + _write(workspace_settings_file(workspace), "- not\n- a\n- mapping\n") + return config_dir + + def test_doctor_reports_instead_of_crashing(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + result = CliRunner().invoke( + main, ["-c", str(self._broken(tmp_path)), "doctor"] + ) + assert result.exit_code == 1 + assert "must be a mapping" in result.output + assert "Traceback" not in result.output + + def test_init_still_runs(self, tmp_path): + """`nerve init` is the repair tool; it must not need a loadable config.""" + from click.testing import CliRunner + + from nerve.cli import main + + result = CliRunner().invoke( + main, ["-c", str(self._broken(tmp_path)), "init", "--if-needed"] + ) + assert "must be a mapping" not in result.output + assert "Traceback" not in result.output + + def test_other_commands_get_a_clean_error(self, tmp_path): + from click.testing import CliRunner + + from nerve.cli import main + + result = CliRunner().invoke( + main, ["-c", str(self._broken(tmp_path)), "status"] + ) + assert result.exit_code != 0 + assert "must be a mapping" in result.output + assert "Traceback" not in result.output