diff --git a/docs/config.md b/docs/config.md index e5e7b97f..258ca1cb 100644 --- a/docs/config.md +++ b/docs/config.md @@ -18,11 +18,12 @@ inside `settings.yaml` is ignored. ## Which layer a key belongs in -`nerve init` splits its answers between the first two layers: +The first two layers divide as below. `nerve init` writes its answers this way, +and migration splits a legacy `config.yaml` on the same table: | Layer | Gets | |-------|------| -| `config.yaml` | `workspace`, `deployment`, `provider.aws_profile`, `gateway.ssl.*`, `proxy`, `docker`, `telegram.enabled`, `sync.gmail.accounts`, `external_agents`, `mcp_endpoint` | +| `config.yaml` | `workspace`, `deployment`, `provider.aws_profile`, `gateway.ssl.*`, `proxy`, `docker`, `telegram.enabled`, `sync.gmail.accounts`, `external_agents`, `mcp_endpoint`, `workflows.runs_dir` | | `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 @@ -188,6 +189,73 @@ 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. +## Migrating an Existing Install + +Installs from before the workspace-config layout are migrated automatically and +idempotently on `nerve upgrade` and daemon start. To run it by hand: + +```bash +nerve migrate --dry-run # show what would change +nerve migrate # apply +``` + +Nothing is deleted and the effective configuration does not change; values are +only relocated. Originals are renamed to `*.migrated` breadcrumbs, and an existing +breadcrumb is never overwritten. + +- `config.yaml` is split rather than copied. Shareable keys go to the git-tracked + `workspace/config/settings.yaml`; secret values go to machine-local + `config.local.yaml`, replaced with `${ENV_VAR}` placeholders; the machine-local + keys from the table above are rewritten into `config.yaml`, so a certificate + path or an AWS profile handle never travels with the workspace repo. The split + follows the table at whatever depth a key is listed, so `gateway.ssl.*` stays + local while `gateway.host`/`port` move across. Migration prints which keys it + kept. The `workspace` path is the exception that stays in `config.local.yaml`: + it is written before anything is consumed, so an interrupted migration cannot + leave the instance unable to find its workspace. +- The legacy `~/.nerve/cron/*` moves to `workspace/config/cron/*`, including any + `prompts/` referenced by `prompt_file`. +- `config.local.yaml`, the rewritten `config.yaml` and the breadcrumb can all hold + plaintext credentials, so all three are written `0600`. `settings.yaml` is + written normally, so a restrictive `umask` is honored. + +A value is treated as secret when any of these hold: + +- The key name looks like one: `*api_key*`, `*api_hash*`, `api_id`, `*token*`, + `*secret*`, `password*`, `jwt`, `authorization`, `bearer`, `oauth`, + `*access_key*`, `*private_key*`, `dsn`, `pw`, `pat`, `session_string`, + `webhook_url`. A public identifier is not a secret: `client_id` is left alone, + `client_secret` is not. +- The value's shape is a credential whatever the key is called: `sk-…`, `ghp_…`, + `xox…`, `user:password@host`, `?token=…`, `Bearer …`. This fires only when the + whole value is the credential, so a category description that mentions + `postgres://user:pass@host` stays put. +- It sits inside an `env` or `headers` block, including inside lists, where MCP + `headers` entries live. +- It is the argument after a flag that names a credential, so both + `args: ["--token=abc123"]` and the split `args: ["--token", "abc123"]` are + caught. The split form is what `npx` and `uvx` MCP servers use, and the value + on its own is unrecognizable. + +If one item in a list is a secret, the whole list moves, because a merge replaces +a list rather than combining it element-wise. Migration reports this, and the copy +left in `settings.yaml` has no further effect. + +Secret detection is best-effort, so **review `workspace/config/settings.yaml` +before committing it to a shared repo**, then run `nerve config validate` to +confirm the bundle is well-formed. Migration also lists any value it left in the +tracked file that still looks like a credential, for you to judge. + +Migration only runs on a pre-refactor `config.yaml`, meaning one that holds +shareable settings rather than only this box's. A `config.yaml` written by +`nerve init` — or left behind by an earlier migration — holds only machine-local +keys (workspace, certificate paths, provider handles), so it is left in place even +when the workspace has no `settings.yaml`. Migration is also a no-op once +`settings.yaml` carries real keys; the `nerve init` scaffold is all comments and +counts as empty. A `config.yaml` with shareable keys sitting next to a populated +`settings.yaml` is reported, because it overrides the tracked file, and you move +those keys across by hand. + ## Config Directory Resolution `nerve` commands locate the config directory via a waterfall, so they work diff --git a/nerve/agent/backends/codex/ultracode.py b/nerve/agent/backends/codex/ultracode.py index 47ddc2f3..ac91dd69 100644 --- a/nerve/agent/backends/codex/ultracode.py +++ b/nerve/agent/backends/codex/ultracode.py @@ -21,6 +21,8 @@ from pathlib import Path from typing import Any +from nerve.utils.fs import atomic_write_text + logger = logging.getLogger(__name__) _MARKETPLACE = "nerve-ultracode" @@ -93,15 +95,6 @@ def managed_dir(home: str | Path) -> Path: return Path(home).expanduser() / "nerve-managed" / "ultracode" -def _atomic_write(path: Path, content: str, mode: int = 0o600) -> None: - path.parent.mkdir(parents=True, exist_ok=True) - tmp = path.with_name(f".{path.name}.{os.getpid()}.tmp") - tmp.write_text(content, encoding="utf-8") - os.chmod(tmp, mode) - tmp.replace(path) - os.chmod(path, mode) - - def materialize_worker_wrapper(home: str | Path) -> Path: """Write an owner-only executable pointing at Nerve's wrapper module.""" path = managed_dir(home) / "codex-worker" @@ -110,7 +103,7 @@ def materialize_worker_wrapper(home: str | Path) -> Path: "from nerve.agent.backends.codex.worker_wrapper import main\n" "raise SystemExit(main())\n" ) - _atomic_write(path, script, mode=0o700) + atomic_write_text(path, script, mode=0o700) return path @@ -123,7 +116,7 @@ def _replace_overlay(path: Path, old: str, new: str) -> None: f"Pinned Ultracode source drifted; policy overlay does not match {path}" ) mode = path.stat().st_mode & 0o777 - _atomic_write(path, content.replace(old, new), mode=mode) + atomic_write_text(path, content.replace(old, new), mode=mode) def _sha256(path: Path) -> str: @@ -156,7 +149,7 @@ def apply_policy_overlay(config: Any, plugin_root: str | Path) -> dict[str, Any] "scripts/ultracode-script-runner.js": _sha256(script_runner), }, } - _atomic_write( + atomic_write_text( managed_dir(config.codex.home_dir) / "policy-overlay.json", json.dumps(marker, indent=2, sort_keys=True) + "\n", ) @@ -318,7 +311,7 @@ async def ensure_installed(config: Any) -> dict[str, Any]: root = managed_dir(config.codex.home_dir) / "marketplace" manifest_path = root / ".agents" / "plugins" / "marketplace.json" - _atomic_write( + atomic_write_text( manifest_path, json.dumps(_marketplace_manifest(config), indent=2) + "\n", ) diff --git a/nerve/cli.py b/nerve/cli.py index cc758fc1..f09c6519 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -172,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({"config", "doctor", "init"}) +_SELF_DIAGNOSING_COMMANDS = frozenset({"config", "doctor", "init", "migrate"}) @click.group() @@ -276,6 +276,25 @@ def start(ctx: click.Context, foreground: bool) -> None: config_dir = Path(ctx.obj["config_dir"]) config = ctx.obj["config"] + # Migrate a legacy install to the workspace/config layout if needed + # (idempotent, best-effort). Non-destructive — originals kept as *.migrated. + if config is not None: + from nerve.migrate import maybe_migrate + report = maybe_migrate(config_dir, workspace=getattr(config, "workspace", None)) + if report is not None and report.did_anything: + # The config in hand was loaded from the old locations, and migration + # has just moved the files out from under it — cron in particular now + # lives in the workspace, so the stale object resolves to job files + # that no longer exist and a --foreground run would schedule nothing. + try: + config = load_config(config_dir) + except Exception as e: + raise click.ClickException( + f"Config could not be reloaded after migration: {e}" + ) from e + set_config(config) + ctx.obj["config"] = config + # Detect fresh install — offer to run setup wizard from nerve.bootstrap import is_fresh_install if is_fresh_install(config_dir): @@ -726,6 +745,47 @@ def upgrade(ctx: click.Context, no_frontend: bool, no_deps: bool, no_pull: bool) if rc != 0: raise click.ClickException("npm run build failed") + # Migrate a legacy config layout to workspace/config (idempotent). + if config is not None: + from nerve.migrate import maybe_migrate + report = maybe_migrate( + Path(ctx.obj["config_dir"]), workspace=getattr(config, "workspace", None) + ) + if report and report.did_anything: + click.echo("\nMigrated config to the workspace layout:") + for action in report.actions: + click.echo(f" - {action}") + if report.error: + # The steps above did apply. Saying so beats a bare success + # message, because the rest has to be finished by hand or by a + # re-run, and the tracked file already exists either way. + click.secho( + f" Migration did not finish: {report.error}\n" + " What is listed above was applied; re-run `nerve migrate` " + "once the cause is fixed.", + fg="yellow", + ) + if report.secrets_moved: + click.echo( + f" (scrubbed {len(report.secrets_moved)} secret(s) into " + "config.local.yaml; review workspace/config/settings.yaml " + "before committing)" + ) + if report.machine_local_kept: + click.echo( + f" ({len(report.machine_local_kept)} machine-local key(s) kept " + "in config.yaml instead of published: " + f"{', '.join(report.machine_local_kept)})" + ) + if report.suspect_values: + click.secho( + f" ({len(report.suspect_values)} value(s) left in settings.yaml " + f"still look like credentials: {', '.join(report.suspect_values)})", + fg="yellow", + ) + for warning in report.warnings: + click.secho(f" Note: {warning}", fg="yellow") + click.echo("\nUpgrade complete. Restart Nerve for changes to take effect:") click.echo(" nerve restart") @@ -1092,6 +1152,56 @@ def doctor(ctx: click.Context) -> None: ctx.exit(1) +@main.command() +@click.option("--dry-run", is_flag=True, help="Show what would change without writing.") +@click.pass_context +def migrate(ctx: click.Context, dry_run: bool) -> None: + """Migrate a legacy config layout into the workspace/config subtree. + + Splits config.yaml: shareable keys → workspace/config/settings.yaml (secrets + scrubbed into config.local.yaml as ${ENV_VAR} refs), machine-local keys stay + in config.yaml. Also moves ~/.nerve/cron → workspace/config/cron. + Non-destructive (originals kept as *.migrated) and idempotent. + """ + from nerve.migrate import migrate as run_migrate + + config = ctx.obj["config"] + config_dir = Path(ctx.obj["config_dir"]) + workspace = getattr(config, "workspace", None) if config is not None else None + try: + report = run_migrate(config_dir, workspace=workspace, dry_run=dry_run) + except Exception as e: # e.g. a malformed config.local.yaml + raise click.ClickException(f"Migration failed: {e}") from e + + if not report.did_anything: + click.secho("Nothing to migrate — already on the workspace layout.", fg="green") + for warning in report.warnings: + click.secho(f" Note: {warning}", fg="yellow") + return + prefix = "[dry-run] would " if dry_run else "" + for action in report.actions: + click.echo(f" {prefix}{action}") + for warning in report.warnings: + click.secho(f"\n Note: {warning}", fg="yellow") + if report.secrets_moved: + click.echo(f"\n Secrets scrubbed to config.local.yaml: {', '.join(report.secrets_moved)}") + if report.machine_local_kept: + click.echo( + "\n Machine-local keys kept in config.yaml: " + f"{', '.join(report.machine_local_kept)}" + ) + if report.suspect_values: + click.secho( + f"\n {len(report.suspect_values)} value(s) headed for settings.yaml still " + f"look like credentials — review: {', '.join(report.suspect_values)}", + fg="yellow", + ) + if dry_run: + click.secho("\nDry run — no changes written.", fg="yellow") + else: + click.secho("\nMigration complete. Review workspace/config/settings.yaml before committing.", fg="green") + + @main.group(name="config") def config_group() -> None: """Configuration commands.""" diff --git a/nerve/migrate.py b/nerve/migrate.py new file mode 100644 index 00000000..0820edf3 --- /dev/null +++ b/nerve/migrate.py @@ -0,0 +1,840 @@ +"""One-time migration of legacy config into the git-syncable workspace subtree. + +Moves an existing install from the pre-refactor layout: + + /config.yaml (shareable + secrets, gitignored) + /config.local.yaml (secrets, gitignored) + ~/.nerve/cron/{jobs,system}.yaml (cron config) + +to the workspace-centric layout: + + /config/settings.yaml (shareable, git-tracked — SCRUBBED) + /config.yaml (machine-local base — REWRITTEN) + /config.local.yaml (secrets, machine-local) + /config/cron/* (cron config, git-tracked) + +Design (confirmed with the user): + +* **Auto on upgrade / daemon start**, via :func:`maybe_migrate` — idempotent, + a no-op once migrated. Also exposed as ``nerve migrate [--dry-run]``. +* **Only a legacy monolith is migrated.** A ``config.yaml`` holding nothing but + the keys the split layout keeps machine-local is left exactly where it is, + however empty the workspace's ``settings.yaml`` looks — see + :func:`_has_portable_content`. +* **Copy + keep as backup** — originals are never deleted; ``config.yaml`` and + the legacy cron files are renamed to ``*.migrated`` breadcrumbs, and the new + location wins. An existing breadcrumb is never overwritten. +* **Split, don't copy** — a legacy ``config.yaml`` holds both halves, so the keys + :data:`_MACHINE_LOCAL_PATHS` names are rewritten into a fresh ``config.yaml`` + and never reach the tracked file. A certificate path, an AWS profile handle or + a mount list is right on exactly one box; syncing it to the rest of a fleet + points them all at something that isn't there. +* **Auto-scrub secrets** — before writing the *tracked* ``settings.yaml``, secret + values are moved into machine-local ``config.local.yaml`` and replaced with + ``${ENV_VAR}`` placeholders. Scrubbed: values under secret-looking keys + (see :data:`_SECRET_KEY_RE`), values whose *shape* is a credential whatever the + key is called (``sk-…``, ``ghp_…``, ``user:pass@host``, ``?token=…``), *every* + value inside ``env`` / ``headers`` mappings (where arbitrarily-named secrets + live, e.g. MCP ``Authorization`` headers), and secrets nested inside lists. + Migration prints exactly what it moved, plus anything left behind that still + looks credential-shaped; heuristics can't be exhaustive, so **review + settings.yaml before committing**. + +Never destructive and never raises out of :func:`maybe_migrate` (best-effort on +startup). + +**Isolating a migration.** Three roots are read and written, and only two of +them are obvious from the call: + +* ``config_dir`` — the first argument. +* the workspace — the ``workspace`` argument; when omitted it is resolved from + the machine-local config files, falling back to ``paths.default_workspace()``. +* the legacy cron directory — the ``legacy_cron_dir`` argument; when omitted it + is ``paths.cron_dir()``, i.e. under ``NERVE_HOME``. + +Passing ``workspace=`` alone therefore does **not** sandbox anything: the cron +half still reads, copies and *renames* files under the real state directory. +Callers that must not touch the machine — tests, tooling, a dry run against +someone else's tree — have to pass ``legacy_cron_dir=`` as well (or set +``NERVE_HOME``). +""" + +from __future__ import annotations + +import logging +import os +import re +import shutil +from dataclasses import dataclass, field +from pathlib import Path + +import yaml + +from nerve import paths +from nerve.config import ( + _deep_merge, + _expand_path, + _read_yaml_mapping, + workspace_config_dir, + workspace_settings_file, +) +from nerve.utils.fs import atomic_write_text + +logger = logging.getLogger(__name__) + +# Anything holding a credential is owner-only, including the breadcrumb copy of +# the pre-migration config — it still has every secret in plaintext. +_SECRET_FILE_MODE = 0o600 + +# Leaf key names whose values are treated as secrets and scrubbed out of the +# tracked settings file. Matched against the *normalized* key (see +# :func:`_normalize_key`), and every alternative has to cover whole +# underscore-separated runs: a substring match would read ``max_tokens`` (a +# size) as a token and ``client_idle_timeout_minutes`` (a duration) as a client +# id. Keys ending in "_env" hold an env-var *name* — a reference, not a secret — +# and are left alone. +# +# ``client_id`` is deliberately absent: an OAuth client id is public by design, +# and scrubbing it turns a shareable value into a required ``${VAR}`` that no +# other machine can resolve. ``client_secret`` is caught by ``secret``. +_SECRET_KEY_RE = re.compile( + r"(?:^|_)(?:" + r"api_?key|api_?hash|api_?id|access_?key|private_?key|secret_?key" + r"|secret|token|password|passwd|passphrase|credentials?|jwt|authorization" + r"|bearer|oauth|pw|pat|dsn" + r"|session_?(?:string|token|secret|id)" + r"|webhook_?url" + r")(?:_|$)" +) + +# Numbers are credentials far less often than strings are — a config is full of +# sizes, ports and timeouts under names that brush against the list above. So a +# non-string leaf is only scrubbed when the key *ends* in one of these, which +# keeps ``telegram.api_id`` (half of a Telegram credential pair, and an int) +# while leaving ``max_tokens`` and ``default_token_budget`` in place. +_NUMERIC_SECRET_KEY_RE = re.compile( + r"(?:^|_)(?:api_?id|app_?id|api_?key|api_?hash|access_?key|secret|token" + r"|password|passwd|pw|pat)$" +) + +# Values whose *shape* is a credential, whatever the key is called. This is the +# half a key-name list can never do: a key pasted into an ``args`` list, a token +# in a URL's query string, a password inside a DSN. The lookbehind stops the +# provider prefixes from matching mid-word (``task-management-system`` is not an +# ``sk-`` key). +_SECRET_VALUE_RE = re.compile( + r"(? bool: + return self.migrated_config or self.migrated_cron + + +def _env_name(path: tuple[str, ...]) -> str: + """Derive an ENV_VAR name from a dotted config path (auth.jwt_secret → + AUTH_JWT_SECRET).""" + joined = "_".join(path) + return re.sub(r"[^A-Za-z0-9]+", "_", joined).strip("_").upper() + + +def _normalize_key(key) -> str: + """``apiKey`` / ``API-KEY`` / ``api.key`` → ``api_key``. + + Config written by hand (and MCP server blocks copied from vendor docs) uses + every spelling; normalizing once means the pattern lists only have to know + about one. + """ + camel_split = re.sub(r"(?<=[a-z0-9])(?=[A-Z])", "_", str(key)) + return re.sub(r"[^a-z0-9]+", "_", camel_split.lower()).strip("_") + + +def _value_looks_secret(value: str) -> bool: + if _SECRET_VALUE_RE.search(value) or _BEARER_VALUE_RE.search(value): + return True + return not any(c.isspace() for c in value) and bool(_SECRET_VALUE_OPAQUE_RE.search(value)) + + +def _is_cli_flag(item) -> bool: + """True for an argv item that is a flag name rather than a value.""" + return isinstance(item, str) and item.startswith("-") + + +def _is_secret_leaf(key, value, path: tuple[str, ...], force: bool) -> bool: + """True if this leaf should be moved out of the tracked file. + + ``force`` marks a leaf inside a subtree that is sensitive by definition + (``env`` / ``headers``), where the key names are the user's own and tell us + nothing. + """ + # Scalars only. A bool is never a credential, and containers are walked by + # the caller. + if isinstance(value, bool) or not isinstance(value, (str, int, float)): + return False + if isinstance(value, str) and (not value or _FULL_ENV_REF_RE.match(value)): + return False + if ".".join(path) in _SCRUB_EXCLUDE_PATHS: + return False + norm = _normalize_key(key) + if force: + return True + if norm.endswith("_env"): + return False # holds an env-var name, not a secret + if isinstance(value, str): + return bool(_SECRET_KEY_RE.search(norm)) or _value_looks_secret(value) + return bool(_NUMERIC_SECRET_KEY_RE.search(norm)) + + +def _scrub_secrets( + data: dict, path: tuple[str, ...] = (), force: bool = False +) -> tuple[dict, dict, list[str]]: + """Split a config dict into (tracked, secrets, moved_paths). + + ``tracked`` is safe to commit (secret leaf values replaced with + ``${ENV_VAR}``). ``secrets`` is the parallel structure holding the real + values, to be merged into config.local.yaml. ``force`` scrubs every scalar + leaf regardless of key name (used inside sensitive subtrees like env/headers). + """ + tracked: dict = {} + secrets: dict = {} + moved: list[str] = [] + for key, value in data.items(): + p = path + (str(key),) + child_force = force or (_normalize_key(key) in _SENSITIVE_SUBTREE_KEYS) + if isinstance(value, dict): + t, s, m = _scrub_secrets(value, p, force=child_force) + tracked[key] = t + if s: + secrets[key] = s + moved.extend(m) + elif isinstance(value, list): + t_list, has_secret = [], False + after_secret_flag = False + for i, item in enumerate(value): + item_path = p + (str(i),) + if isinstance(item, dict): + t, s, m = _scrub_secrets(item, item_path, force=child_force) + t_list.append(t) + if s or m: + has_secret = True + moved.extend(m) + after_secret_flag = False + continue + # A scalar list item has no key of its own, so it is judged by + # the list's key, by its own shape, or by the item in front of + # it — which is how ``headers: ["Authorization: Bearer …"]``, + # ``args: ["--api-key=…"]`` and ``args: ["--token", "…"]`` are + # all caught. + positional = after_secret_flag and not _is_cli_flag(item) + if _is_secret_leaf(key, item, item_path, child_force or positional): + t_list.append("${" + _env_name(item_path) + "}") + has_secret = True + moved.append(".".join(item_path)) + after_secret_flag = False + else: + t_list.append(item) + # Only a flag arms the next item. Another flag does not: + # ``--token --verbose`` is a malformed command line, not a + # token whose value is ``--verbose``. + after_secret_flag = isinstance(item, str) and bool( + _SECRET_FLAG_RE.match(item) + ) + tracked[key] = t_list + if has_secret: + # The whole real list goes to the overlay, not just the secret + # items. Merging replaces a list rather than combining it + # element-wise, so there is no way for the overlay to supply + # item 3 and let the tracked file keep items 1 and 2 — it is all + # or nothing, and the tracked copy is inert from here on. + # :func:`_relocated_lists` surfaces that so it isn't a surprise. + secrets[key] = value + elif _is_secret_leaf(key, value, p, force): + tracked[key] = "${" + _env_name(p) + "}" + secrets[key] = value + moved.append(".".join(p)) + else: + tracked[key] = value + return tracked, secrets, moved + + +def _suspect_values(data, path: tuple[str, ...] = ()) -> list[str]: + """Dotted paths of tracked values that still look credential-shaped. + + Nothing is moved on this signal — it is too weak to act on and too strong to + swallow. Reporting it is the difference between "scrubbed 3 secrets" (which + a user reads as "and that was all of them") and a prompt to look at the four + opaque strings the key-name rules had no opinion about. + """ + out: list[str] = [] + items = data.items() if isinstance(data, dict) else enumerate(data) + for key, value in items: + p = path + (str(key),) + if isinstance(value, (dict, list)): + out.extend(_suspect_values(value, p)) + elif isinstance(value, str) and _OPAQUE_VALUE_RE.match(value): + if any(c.isdigit() for c in value) and any(c.isalpha() for c in value): + out.append(".".join(p)) + return out + + +def _relocated_lists(secrets: dict, path: tuple[str, ...] = ()) -> list[str]: + """Dotted paths of lists moved to the overlay whole because one item was a + secret. The copy left in the tracked file no longer has any effect.""" + out: list[str] = [] + for key, value in secrets.items(): + p = path + (str(key),) + if isinstance(value, list): + out.append(".".join(p)) + elif isinstance(value, dict): + out.extend(_relocated_lists(value, p)) + return out + + +def _leaf_paths(data, path: tuple[str, ...] = ()) -> list[str]: + """Dotted paths of every value in a config mapping. Lists count as leaves — + the split layout routes a whole list one way or the other.""" + out: list[str] = [] + for key, value in data.items(): + p = path + (str(key),) + if isinstance(value, dict) and value: + out.extend(_leaf_paths(value, p)) + else: + out.append(".".join(p)) + return out + + +def _is_machine_local(dotted: str) -> bool: + return any( + dotted == known or dotted.startswith(known + ".") for known in _MACHINE_LOCAL_PATHS + ) + + +def _partition_machine_local(data: dict, path: tuple[str, ...] = ()) -> tuple[dict, dict]: + """Split a config mapping into (portable, machine_local). + + The split happens at whatever depth the path is listed at, not at the top + level: ``gateway.ssl`` takes the ``ssl`` subtree and leaves ``gateway.host`` + and ``gateway.port`` behind. Moving whole top-level keys instead would drag + the bind address off to the machine layer along with the certificate paths, + and lockdown never reads that layer. + + A subtree emptied by the split is dropped rather than left behind as + ``gateway: {}``. An empty mapping in the source is preserved as written — + there is nothing under it to be machine-local. + """ + portable: dict = {} + machine: dict = {} + for key, value in data.items(): + p = path + (str(key),) + if _is_machine_local(".".join(p)): + machine[key] = value + elif isinstance(value, dict) and value: + sub_portable, sub_machine = _partition_machine_local(value, p) + if sub_portable: + portable[key] = sub_portable + if sub_machine: + machine[key] = sub_machine + else: + portable[key] = value + return portable, machine + + +def _has_portable_content(raw: dict) -> bool: + """True if ``config.yaml`` holds anything the shareable layer should own. + + The positive test for "this is a legacy monolith". Emptiness of + ``settings.yaml`` can't answer it: a workspace loses its settings file by + being repointed, moved, emptied, or interrupted mid-init, and in every one of + those cases the ``config.yaml`` sitting next to it is the deliberately + machine-local half — the last thing that should be copied into a file the + docs tell you to commit and push. + """ + return any(not _is_machine_local(p) for p in _leaf_paths(raw)) + + +def _resolve_workspace(config_dir: Path) -> Path: + machine = _deep_merge( + _read_yaml_mapping(config_dir / "config.yaml"), + _read_yaml_mapping(config_dir / "config.local.yaml"), + ) + return _expand_path(machine.get("workspace")) or paths.default_workspace() + + +def _breadcrumb_path(original: Path) -> Path: + """A free ``*.migrated`` name next to ``original``. + + ``Path.rename`` silently overwrites on POSIX (and raises on Windows), and the + cron half of the migration can run more than once — so a fixed suffix could + destroy the only surviving copy of an earlier original. + """ + candidate = original.with_name(original.name + ".migrated") + n = 1 + while candidate.exists(): + candidate = original.with_name(f"{original.name}.migrated.{n}") + n += 1 + return candidate + + +def _restrict(path: Path) -> None: + """Make a file owner-only, best-effort (some filesystems have no modes).""" + try: + os.chmod(path, _SECRET_FILE_MODE) + except OSError as e: + logger.warning("Could not restrict permissions on %s: %s", path, e) + + +def migrate( + config_dir: Path, + workspace: Path | None = None, + dry_run: bool = False, + legacy_cron_dir: Path | None = None, + report: MigrationReport | None = None, +) -> MigrationReport: + """Perform the migration for ``config_dir``. Idempotent; safe to re-run. + + ``workspace`` defaults to the one the machine-local config names, and + ``legacy_cron_dir`` to ``paths.cron_dir()``. Both have to be supplied to + confine the migration to a given tree — see the module docstring. + + Pass ``report`` to keep hold of it if this raises. Migration is not one + transaction: the config half can commit and the cron half then fail on a + directory it cannot write, and a caller that only sees the exception has no + way to know the files already moved. + """ + config_dir = Path(config_dir) + workspace = Path(workspace) if workspace is not None else _resolve_workspace(config_dir) + legacy_cron = Path(legacy_cron_dir) if legacy_cron_dir is not None else paths.cron_dir() + report = MigrationReport(dry_run=dry_run) if report is None else report + + _migrate_config_yaml(config_dir, workspace, report) + _migrate_cron(workspace, legacy_cron, report) + return report + + +def _settings_has_content(settings: Path) -> bool: + """True if the tracked settings file already carries configuration. + + Existence is not the test: ``nerve init`` scaffolds a comments-only + ``settings.yaml``, which parses to an empty mapping. Treating that as + "already migrated" made migration a permanent no-op for every install + created after the scaffold shipped. + + A file that won't parse — or parses to something that isn't a mapping — + counts as content: migration must never overwrite what it cannot read. + """ + if not settings.exists(): + return False + try: + return bool(_read_yaml_mapping(settings, strict=True)) + except Exception: # noqa: BLE001 — unreadable is "leave it alone" + return True + + +def _migrate_config_yaml(config_dir: Path, workspace: Path, report: MigrationReport) -> None: + config_yaml = config_dir / "config.yaml" + settings = workspace_settings_file(workspace) + + if not config_yaml.exists(): + return + + raw = _read_yaml_mapping(config_yaml) + + if _settings_has_content(settings): + # Already on the split layout. If config.yaml still carries shareable + # keys they silently mask the tracked file, which is also what an + # interrupted migration leaves behind — the two states are + # indistinguishable on disk, so say so rather than guess. + if _has_portable_content(raw): + report.warnings.append( + f"{config_yaml} is still present and overrides {settings}; " + "move any shared settings across and remove it" + ) + return + + if not _has_portable_content(raw): + return # a machine-local config.yaml from the split layout, not a legacy one + + # The workspace location is machine-local — it must not live in the tracked + # settings file (circular). It is the one machine-local key that does not go + # back into config.yaml with the rest: config.local.yaml is written before + # anything is consumed, so no interruption can lose it. Losing it is the only + # unrecoverable outcome here — the instance would resolve the *default* + # workspace and read no settings.yaml at all. + ws_value = raw.pop("workspace", None) + + # ``lockdown`` is honored only in the tracked settings file — the machine + # layers deliberately ignore it so that a local edit cannot unlock or + # fake-lock an instance (see :func:`nerve.config._load_layers`). Copying it + # across would promote a flag that has never had any effect into the one file + # where it is authoritative, and locking drops the machine layers entirely, + # including the config.local.yaml this migration just moved the secrets into: + # the next start would fail on an unresolved ${VAR} it used to resolve. So it + # is dropped, which preserves what the box does today, and reported. + if raw.pop("lockdown", None) is not None: + report.warnings.append( + f"dropped 'lockdown' from {config_yaml}: it was being ignored there and " + f"is honored only in {settings}, where it also stops the machine-local " + "layers from being read. Set it there deliberately to lock this instance" + ) + + # Split before scrubbing, so a machine-local value never reaches the tracked + # file even as a ${VAR} placeholder. The machine half is left unscrubbed: + # config.yaml is machine-local and gitignored, exactly like the overlay the + # placeholders would point at, so scrubbing it would only add indirection — + # which is why it is written owner-only below. + portable, machine_local = _partition_machine_local(raw) + + tracked, secrets, moved = _scrub_secrets(portable) + + # Everything bound for the machine-local overlay: scrubbed secrets + the + # workspace path (not a secret, but machine-specific). + local_additions = dict(secrets) + if ws_value is not None: + local_additions["workspace"] = ws_value + + local_path = config_dir / "config.local.yaml" + backup = _breadcrumb_path(config_yaml) + kept = _leaf_paths(machine_local) + + report.migrated_config = True + report.secrets_moved.extend(moved) + report.machine_local_kept.extend(kept) + report.suspect_values.extend(_suspect_values(tracked)) + if local_additions: + report.actions.append(f"moved secrets + workspace path into {local_path}") + report.actions.append(f"config.yaml → {settings} (scrubbed {len(moved)} secret(s))") + if kept: + report.actions.append(f"kept {len(kept)} machine-local key(s) in {config_yaml}") + report.actions.append(f"config.yaml → {backup} (backup)") + for dotted in _relocated_lists(secrets): + report.warnings.append( + f"{dotted} contained a secret, so the whole list moved to " + f"{local_path.name} — a list can't be half-overridden, and the copy " + "left in settings.yaml no longer has any effect" + ) + + if report.dry_run: + return + + # Order matters, and every write is atomic (temp file + rename), so an + # interruption at any point leaves a working install: + # + # 1. the local overlay first, so the tracked file never references secrets + # that exist nowhere on this machine; + # 2. the tracked settings file; + # 3. rename config.yaml away; + # 4. write the machine-local half back to config.yaml. + # + # Stopping between 2 and 3 leaves config.yaml still shadowing an already + # complete settings.yaml — the instance keeps working, and re-running is a + # no-op. Renaming earlier would invert that: a crash before the settings + # file existed would take every non-secret setting out of the live config + # with no way to retry. + # + # Steps 3 and 4 cannot be one operation — they are the same path — so an + # interruption between them leaves the machine half only in the breadcrumb, + # to be copied back by hand. That window holds nothing that stops the + # instance from loading: the workspace path went to config.local.yaml at + # step 1. + if local_additions: + existing_local = _read_yaml_mapping(local_path) + # Existing local values win — never clobber a value the operator already + # placed there. + merged_local = _deep_merge(local_additions, existing_local) + atomic_write_text( + local_path, + "# Nerve — machine-local secrets & overrides (gitignored).\n\n" + + yaml.safe_dump(merged_local, default_flow_style=False, sort_keys=False), + mode=_SECRET_FILE_MODE, + ) + + atomic_write_text( + settings, + "# Nerve — shareable workspace configuration (migrated).\n" + "# Secrets were moved to config.local.yaml and replaced with\n" + "# ${ENV_VAR} placeholders. Safe to commit.\n\n" + + yaml.safe_dump(tracked, default_flow_style=False, sort_keys=False), + # The shareable file gets whatever mode an ordinary write would have + # produced. Forcing it open would override a restrictive umask on a file + # that scrubbing is not guaranteed to have emptied of credentials. + mode=None, + ) + + # Rename the original as a breadcrumb so it no longer overrides settings.yaml. + # It keeps every secret in plaintext — unscrubbed, unlike the file we just + # locked down — so tighten it first, then move it. + _restrict(config_yaml) + config_yaml.rename(backup) + + if machine_local: + # Owner-only, unlike the tracked file: this half is unscrubbed, and the + # subtrees that land in it are where a paired agent's token or a local + # service credential lives. + atomic_write_text( + config_yaml, + "# Nerve — machine-local configuration (migrated).\n" + "# The half of the old config.yaml that describes this box:\n" + "# filesystem paths, credential handles, what this machine has\n" + "# paired. Not for a shared repo. Shareable settings are in\n" + "# /config/settings.yaml, which this file overrides.\n\n" + + yaml.safe_dump(machine_local, default_flow_style=False, sort_keys=False), + mode=_SECRET_FILE_MODE, + ) + + +def _migrate_cron(workspace: Path, legacy: Path, report: MigrationReport) -> None: + """Copy the legacy cron directory into the workspace, one file at a time. + + Per file rather than all-or-nothing, because the two sides legitimately + overlap: ``nerve init`` writes ``system.yaml`` and an empty ``gates/`` into + the workspace while copying only ``jobs.yaml`` across (see + :mod:`nerve.bootstrap`). A directory-level "the workspace already has cron + config" test therefore skipped the remainder permanently, and the legacy + directory stops being consulted as soon as the workspace has job files (see + :func:`nerve.config._resolve_cron_dir`) — so ``gates/*.py`` and any + ``prompts/`` a job names by relative path were stranded where nothing reads + them, and custom gates quietly stopped loading. Losing a gate is not a soft + failure either: an unknown gate ``type`` takes the whole job with it. + + A file already in the workspace is never overwritten; that copy is the + reviewed one. The exception worth reporting is a legacy *job* file that loses + to one already in place, because it is real cron config that now runs + nowhere and migration cannot merge two sets of jobs. + """ + if not legacy.is_dir(): + return + ws_cron = workspace_config_dir(workspace) / "cron" + job_files = ("system.yaml", "jobs.yaml") + + copy: list[Path] = [] + shadowed: list[Path] = [] + for src in sorted(legacy.rglob("*")): + if src.is_dir(): + continue + rel = src.relative_to(legacy) + if any(".migrated" in Path(part).suffixes for part in rel.parts): + continue # a breadcrumb from an earlier pass, not cron content + if not (ws_cron / rel).exists(): + copy.append(rel) + elif rel.parent == Path(".") and rel.name in job_files: + shadowed.append(rel) + + # Reported whether or not anything else moves, the way a leftover config.yaml + # is: nothing here can fix it, and the jobs in it are not running. + for rel in shadowed: + report.warnings.append( + f"{legacy / rel} still holds cron jobs, but {ws_cron / rel} is already " + "present and wins; move across what you need and remove it" + ) + if not copy: + return + + report.migrated_cron = True + report.actions.append( + f"cron {legacy}/* → {ws_cron}/ ({len(copy)} file(s), originals kept as *.migrated)" + ) + + if report.dry_run: + return + + for rel in copy: + dst = ws_cron / rel + dst.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(legacy / rel, dst) + # Breadcrumb only the job files that actually moved, so the legacy directory + # stops reading as "has jobs". Renaming one that lost to a workspace copy + # would hide the operator's only copy of it. + for name in job_files: + if Path(name) in copy: + (legacy / name).rename(_breadcrumb_path(legacy / name)) + + +def is_migrated( + config_dir: Path, + workspace: Path | None = None, + legacy_cron_dir: Path | None = None, +) -> bool: + """True if there is nothing left to migrate for ``config_dir``.""" + return not migrate( + config_dir, workspace=workspace, dry_run=True, legacy_cron_dir=legacy_cron_dir + ).did_anything + + +def maybe_migrate( + config_dir: Path, + workspace: Path | None = None, + legacy_cron_dir: Path | None = None, +) -> MigrationReport | None: + """Run migration if needed. Best-effort: never raises (called on startup). + + Returns the report if migration ran, else None. A failure partway still + returns what was applied, rather than reading as "nothing happened": the + config half commits before the cron half runs, so an exception can leave the + tracked settings file written, the secrets relocated and ``config.yaml`` + renamed away. Callers need that — ``nerve start`` has to reload the config it + is holding, and ``nerve upgrade`` has to print the review prompt for a + tracked file that now exists. + """ + report = MigrationReport(dry_run=False) + try: + migrate( + config_dir, + workspace=workspace, + dry_run=False, + legacy_cron_dir=legacy_cron_dir, + report=report, + ) + except Exception as e: # noqa: BLE001 — must never break upgrade/startup + report.error = str(e) + if report.did_anything: + logger.warning( + "Config migration failed partway and is half-applied (%s). Already " + "done: %s", + e, + "; ".join(report.actions), + ) + else: + logger.warning("Config migration skipped due to error: %s", e) + if report.did_anything: + if not report.error: + logger.info( + "Migrated config to the workspace layout: %s", + "; ".join(report.actions), + ) + if report.suspect_values: + logger.warning( + "Migration left %d value(s) in the tracked settings file that look " + "like credentials — review before committing: %s", + len(report.suspect_values), + ", ".join(report.suspect_values), + ) + for warning in report.warnings: + logger.warning("Config migration: %s", warning) + return report diff --git a/nerve/utils/fs.py b/nerve/utils/fs.py new file mode 100644 index 00000000..c76b6a27 --- /dev/null +++ b/nerve/utils/fs.py @@ -0,0 +1,118 @@ +"""Filesystem helpers shared by the writers that must not lose or leak data. + +Kept dependency-free (stdlib only) so any module can import it. +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + + +def atomic_write_text(path: Path | str, content: str, *, mode: int | None = 0o600) -> None: + """Replace ``path`` with ``content`` in one step, at ``mode``. + + ``mode=None`` means "whatever ``open(path, 'w')`` would have left behind": + a file that already exists keeps the permissions it already has, and a new + one is created at ``0o666`` minus the process umask. Use it for files that + are meant to be shared; pass an explicit mode for anything holding a + credential. + + Three properties the obvious ``open(path, "w")`` does not have: + + * **A crash never truncates the destination.** The bytes go to a temp file + in the same directory, are flushed and fsynced, and only then does + ``os.replace`` swap them in — an atomic rename on POSIX and on Windows. + An interrupted write (power loss, ENOSPC, SIGKILL) leaves either the + complete old file or the complete new one, never a half of either. + * **The permissions are in place before the content is.** The temp file is + created owner-only by ``mkstemp`` and chmod'd *before* the first write, + so a secret is never briefly readable at the process umask. Chmod'ing + after the write is a race, however short. + * **The rename itself is durable.** The parent directory is fsynced too, + so a crash right after the call cannot resurrect the old file. + + One thing it does *not* preserve: hard links. Swapping in a new file gives + the destination a new inode, so any other name for the old one keeps the + old content. Symlinks are followed (the link's target is rewritten, as a + plain write would do), but a hard-linked destination cannot be updated in + place and atomically at the same time. + + ``path``'s parent is created if missing. The temp file is removed if + anything goes wrong, so a failed write leaves no debris. + """ + # Follow symlinks to the real destination. Otherwise a config file the user + # linked into a dotfiles repo would be *replaced* by a regular file and the + # repo copy would silently keep the old content. + path = Path(os.path.realpath(path)) + path.parent.mkdir(parents=True, exist_ok=True) + if mode is None: + # Only a *new* file takes its mode from the umask; rewriting an existing + # one through ``open(path, "w")`` leaves the permissions alone. This + # helper writes a fresh inode and renames it into place, so the old mode + # has to be carried across by hand or every write silently re-permissions + # the file: a 0o600 config widened to 0o666 under a lax umask, or a + # deliberately group-readable one clamped to 0o600 under a strict one. + # The widening direction is the dangerous one — these are the files that + # hold credentials. + mode = _existing_file_mode(path) + if mode is None: + mode = _default_file_mode() + fd, tmp_name = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp") + tmp = Path(tmp_name) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + try: + os.fchmod(f.fileno(), mode) + except AttributeError: # no fchmod on Windows + os.chmod(tmp, mode) + f.write(content) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, path) + except BaseException: + tmp.unlink(missing_ok=True) + raise + _fsync_dir(path.parent) + + +def _existing_file_mode(path: Path) -> int | None: + """The permission bits already on ``path``, or ``None`` if there is no file. + + Only the low nine bits travel. Set-user-ID, set-group-ID and the sticky bit + belong to the inode being discarded, and re-applying them to a file whose + contents this process just wrote is how an ordinary config write turns into + a privilege hand-out. + """ + try: + return os.stat(path).st_mode & 0o777 + except OSError: + return None # absent, or unstattable — fall back to what open() would do + + +def _default_file_mode() -> int: + """The mode ``open()`` would create a file with, honoring the umask. + + There is no getter for the umask, only a setter that returns the previous + value — so it has to be read by writing it. The value parked in the window + is deliberately restrictive: if another thread creates a file in those few + instructions, it lands too tight rather than too loose. + """ + current = os.umask(0o077) + os.umask(current) + return 0o666 & ~current + + +def _fsync_dir(directory: Path) -> None: + """Flush a directory entry, ignoring platforms that can't open one.""" + try: + fd = os.open(directory, os.O_RDONLY) + except OSError: + return # Windows, and some network filesystems + try: + os.fsync(fd) + except OSError: + pass + finally: + os.close(fd) diff --git a/tests/test_migrate.py b/tests/test_migrate.py new file mode 100644 index 00000000..a99b218d --- /dev/null +++ b/tests/test_migrate.py @@ -0,0 +1,1234 @@ +"""Tests for legacy → workspace/config migration.""" + +import os +import re +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml +from click.testing import CliRunner + +import nerve.migrate +from nerve import paths +from nerve.config import load_config, workspace_settings_file +from nerve.cron.jobs import load_jobs +from nerve.migrate import ( + _scrub_secrets, + _suspect_values, + is_migrated, + maybe_migrate, + migrate, +) + + +def _legacy_install(tmp_path, config_yaml: str, local_yaml: str | None = None): + config_dir = tmp_path / "cfg" + workspace = tmp_path / "ws" + config_dir.mkdir(parents=True) + workspace.mkdir(parents=True) + (config_dir / "config.yaml").write_text( + f"workspace: {workspace}\n" + config_yaml, encoding="utf-8" + ) + if local_yaml is not None: + (config_dir / "config.local.yaml").write_text(local_yaml, encoding="utf-8") + return config_dir, workspace + + +def _legacy_cron(tmp_path, files: dict[str, str]) -> Path: + """A stand-in for the machine-global cron dir, inside the test's tmp tree.""" + d = tmp_path / "legacy-cron" + d.mkdir(parents=True, exist_ok=True) + for name, body in files.items(): + (d / name).write_text(body, encoding="utf-8") + return d + + +def _mode(path: Path) -> int: + return path.stat().st_mode & 0o777 + + +def _snapshot(root: Path) -> dict[str, bytes | None]: + """Every path under ``root`` and its contents (``None`` for directories).""" + return { + str(p.relative_to(root)): None if p.is_dir() else p.read_bytes() + for p in sorted(root.rglob("*")) + } + + +@pytest.fixture +def permissive_umask(): + """Run with the common umask 022, under which a plain write lands 0644.""" + old = os.umask(0o022) + yield + os.umask(old) + + +class TestScrubSecrets: + def test_moves_secret_keys(self): + tracked, secrets, moved = _scrub_secrets({ + "anthropic_api_key": "sk-real", + "auth": {"jwt_secret": "hunter2", "password_hash": "$2b$12$x"}, + "timezone": "UTC", + }) + assert tracked["anthropic_api_key"] == "${ANTHROPIC_API_KEY}" + assert tracked["auth"]["jwt_secret"] == "${AUTH_JWT_SECRET}" + assert tracked["auth"]["password_hash"] == "${AUTH_PASSWORD_HASH}" + assert tracked["timezone"] == "UTC" + assert secrets["anthropic_api_key"] == "sk-real" + assert secrets["auth"]["jwt_secret"] == "hunter2" + assert set(moved) == {"anthropic_api_key", "auth.jwt_secret", "auth.password_hash"} + + def test_leaves_env_refs_and_env_name_keys(self): + tracked, secrets, moved = _scrub_secrets({ + "openai_api_key": "${OPENAI_API_KEY}", # already a ref + "api_key_env": "OPENAI_API_KEY", # a name, not a secret + }) + assert tracked["openai_api_key"] == "${OPENAI_API_KEY}" + assert tracked["api_key_env"] == "OPENAI_API_KEY" + assert secrets == {} + + def test_sizes_and_timeouts_are_not_credentials(self): + """Names that brush against the secret vocabulary but hold quantities.""" + tracked, secrets, moved = _scrub_secrets({ + "max_tokens": 4096, + "default_token_budget": 120000, + "client_idle_timeout_minutes": 30, + "telegram_chat_id": 12345, + }) + assert secrets == {} + assert tracked["max_tokens"] == 4096 + assert tracked["default_token_budget"] == 120000 + assert tracked["client_idle_timeout_minutes"] == 30 + assert tracked["telegram_chat_id"] == 12345 + + def test_numeric_credential_scrubbed(self): + """Half a credential pair is no better than none: Telegram's api_id is an + int, and scrubbing only its api_hash sibling left it in the clear.""" + tracked, secrets, moved = _scrub_secrets({ + "telegram": {"api_id": 2040123, "api_hash": "0123456789abcdef"} + }) + assert tracked["telegram"]["api_id"] == "${TELEGRAM_API_ID}" + assert secrets["telegram"]["api_id"] == 2040123 + assert moved == ["telegram.api_id", "telegram.api_hash"] + + def test_partial_env_ref_is_still_scrubbed(self): + """One ${} reference anywhere used to exempt the whole value.""" + tracked, secrets, _ = _scrub_secrets({ + "github": {"token": "ghp_realsecretvalue${SUFFIX}"} + }) + assert tracked["github"]["token"] == "${GITHUB_TOKEN}" + assert secrets["github"]["token"] == "ghp_realsecretvalue${SUFFIX}" + + def test_scalar_list_items_scrubbed(self): + """A list of strings is a leaf too — inside a sensitive subtree every + item is a secret, and elsewhere the value's own shape decides.""" + tracked, secrets, moved = _scrub_secrets({ + "mcp": { + "headers": ["Authorization: Bearer ghp_AAAABBBBCCCCDDDDEEEE"], + "args": ["--stdio", "--api-key=sk-proj-AAAABBBBCCCCDDDDEEEE"], + } + }) + assert tracked["mcp"]["headers"] == ["${MCP_HEADERS_0}"] + assert tracked["mcp"]["args"] == ["--stdio", "${MCP_ARGS_1}"] + # Lists don't deep-merge — the overlay carries the real list wholesale. + assert secrets["mcp"]["args"][1] == "--api-key=sk-proj-AAAABBBBCCCCDDDDEEEE" + assert set(moved) == {"mcp.headers.0", "mcp.args.1"} + + def test_credential_after_a_flag_is_scrubbed(self): + """``--token`` and its value are separate argv items, which is the form + ``npx``/``uvx`` MCP servers are configured with. The value on its own has + no vendor prefix and no separator to match on, so the flag in front of it + is what marks it — otherwise it goes to the tracked file in plaintext.""" + tracked, secrets, moved = _scrub_secrets({ + "mcp_servers": [ + { + "name": "s", + "command": "npx", + "args": ["-y", "srv", "--token", "tok_abcdefghijklmnop"], + }, + ] + }) + assert moved == ["mcp_servers.0.args.3"] + assert tracked["mcp_servers"][0]["args"] == [ + "-y", "srv", "--token", "${MCP_SERVERS_0_ARGS_3}", + ] + assert secrets["mcp_servers"][0]["args"][3] == "tok_abcdefghijklmnop" + + def test_a_flag_does_not_make_a_following_flag_a_secret(self): + """``--token --verbose`` is a malformed command line, not a token whose + value is ``--verbose``. Scrubbing it would put a required ``${VAR}`` + where a switch used to be, and the server would stop starting.""" + tracked, _, moved = _scrub_secrets( + {"args": ["--token", "--verbose", "--port", "8080"]} + ) + assert moved == [] + assert tracked["args"] == ["--token", "--verbose", "--port", "8080"] + + def test_only_a_credential_naming_flag_arms_the_next_item(self): + """The flag has to name a credential. Every argv list has values after + flags, and treating all of them as secrets would empty the tracked file + into the overlay.""" + tracked, _, moved = _scrub_secrets( + {"args": ["-y", "server-name", "--port", "8080", "--model", "opus"]} + ) + assert moved == [] + assert tracked["args"] == ["-y", "server-name", "--port", "8080", "--model", "opus"] + + def test_credential_shaped_value_under_innocuous_key(self): + tracked, secrets, moved = _scrub_secrets({ + "database": {"url": "postgres://admin:hunter2@db.internal:5432/nerve"}, + "feed": {"url": "https://example.com/all?token=abcdef1234567890"}, + "repo": {"url": "https://github.com/owner/repo.git"}, + }) + assert tracked["database"]["url"] == "${DATABASE_URL}" + assert tracked["feed"]["url"] == "${FEED_URL}" + assert tracked["repo"]["url"] == "https://github.com/owner/repo.git" + + def test_alternate_key_spellings(self): + tracked, secrets, _ = _scrub_secrets({"apiKey": "abc", "API-TOKEN": "def"}) + assert tracked["apiKey"] == "${APIKEY}" + assert tracked["API-TOKEN"] == "${API_TOKEN}" + + def test_sqlite_dsn_not_scrubbed(self): + """"dsn" usually means a credential, but this one is a local file path.""" + tracked, secrets, _ = _scrub_secrets( + {"memory": {"sqlite_dsn": "sqlite:////home/me/.nerve/memu.sqlite"}} + ) + assert tracked["memory"]["sqlite_dsn"].startswith("sqlite:") + assert secrets == {} + + def test_paths_are_not_pats(self): + """Segment-anchored matching: ``pat`` must not swallow ``*_path``.""" + tracked, secrets, _ = _scrub_secrets({ + "archive_path": "/var/lib/nerve", + "redact_patterns": ["sk-[a-z]+"], + "public_key": "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5", + }) + assert secrets == {} + + def test_public_identifiers_are_not_scrubbed(self): + """An OAuth client id is public by design. Scrubbing it turns a + shareable value into a required ${VAR} that hard-fails load_config on + the second machine — the one the tracked layer exists for.""" + tracked, secrets, _ = _scrub_secrets({ + "external_agents": {"codex": {"client_id": "app-12345", "client_secret": "shh"}} + }) + codex = tracked["external_agents"]["codex"] + assert codex["client_id"] == "app-12345" + assert codex["client_secret"].startswith("${") + + def test_prose_mentioning_a_credential_is_not_scrubbed(self): + """Category descriptions and help text are shareable. Only a value that + *is* a credential counts, not a sentence that mentions the shape of one + — the wizard generates descriptions like these.""" + tracked, secrets, _ = _scrub_secrets({ + "memory": {"categories": [ + {"name": "infra", + "description": "Connection strings like postgres://user:pass@host and " + "CLI flags such as --api-key=YOURKEYHERE live here"}, + ]}, + "help": "Append ?token=YOURTOKEN to the URL to authenticate", + }) + assert secrets == {} + assert "postgres://user:pass@host" in tracked["memory"]["categories"][0]["description"] + assert tracked["help"].startswith("Append ?token=") + + def test_suspect_values_flags_what_the_rules_missed(self): + """No rule matches an opaque blob under an innocuous key — but the + operator gets told about it rather than a bare "scrubbed 0 secrets".""" + tracked, _, _ = _scrub_secrets({ + "analytics": {"project": "5f4dcc3b5aa765d61d8327deb882cf99"}, + "agent": {"model": "claude-haiku-4-5-20251001"}, + "timezone": "America/New_York", + }) + assert _suspect_values(tracked) == ["analytics.project"] + + def test_authorization_header_scrubbed(self): + tracked, secrets, moved = _scrub_secrets({ + "mcp_servers": {"gh": {"headers": {"Authorization": "Bearer sk-leak"}}} + }) + hdr = tracked["mcp_servers"]["gh"]["headers"]["Authorization"] + assert hdr.startswith("${") and "sk-leak" not in hdr + assert secrets["mcp_servers"]["gh"]["headers"]["Authorization"] == "Bearer sk-leak" + + def test_env_subtree_scrubbed_regardless_of_key_name(self): + tracked, secrets, moved = _scrub_secrets({ + "mcp_servers": {"x": {"env": {"GH_PAT": "ghp_leak", "OPENAI_KEY": "sk-leak"}}} + }) + env = tracked["mcp_servers"]["x"]["env"] + assert env["GH_PAT"].startswith("${") and env["OPENAI_KEY"].startswith("${") + assert secrets["mcp_servers"]["x"]["env"]["GH_PAT"] == "ghp_leak" + + def test_secret_inside_list_scrubbed(self): + tracked, secrets, moved = _scrub_secrets({ + "external_agents": {"targets": [{"name": "codex", "token": "tok-leak"}]} + }) + assert tracked["external_agents"]["targets"][0]["token"].startswith("${") + # Full real list preserved in the overlay (lists don't deep-merge). + assert secrets["external_agents"]["targets"] == [ + {"name": "codex", "token": "tok-leak"} + ] + + def test_api_hash_scrubbed(self): + tracked, secrets, _ = _scrub_secrets({"sync": {"telegram": {"api_hash": "H"}}}) + assert tracked["sync"]["telegram"]["api_hash"].startswith("${") + + def test_proxy_api_key_not_scrubbed(self): + # A fixed local-loopback token — must stay in the shared file. + tracked, secrets, _ = _scrub_secrets({"proxy": {"api_key": "sk-nerve-local-proxy"}}) + assert tracked["proxy"]["api_key"] == "sk-nerve-local-proxy" + assert secrets == {} + + +class TestMigrateConfig: + def test_creates_scrubbed_settings_and_moves_secrets(self, tmp_path): + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\nanthropic_api_key: sk-secret\n", + ) + report = migrate(config_dir, workspace=workspace) + assert report.migrated_config + + settings = yaml.safe_load(workspace_settings_file(workspace).read_text()) + assert settings["timezone"] == "UTC" + assert settings["anthropic_api_key"] == "${ANTHROPIC_API_KEY}" + assert "workspace" not in settings # stripped (circular) + + local = yaml.safe_load((config_dir / "config.local.yaml").read_text()) + assert local["anthropic_api_key"] == "sk-secret" + + # Original kept as a breadcrumb; config.yaml no longer present. + assert (config_dir / "config.yaml.migrated").exists() + assert not (config_dir / "config.yaml").exists() + + def test_effective_config_unchanged_after_migration(self, tmp_path): + """The whole point: values are preserved, just relocated.""" + config_dir, workspace = _legacy_install( + tmp_path, "timezone: Europe/Berlin\nanthropic_api_key: sk-xyz\n" + ) + before = load_config(config_dir) + migrate(config_dir, workspace=workspace) + after = load_config(config_dir) + assert after.timezone == before.timezone == "Europe/Berlin" + assert after.anthropic_api_key == before.anthropic_api_key == "sk-xyz" + + def test_lockdown_is_not_promoted_into_the_tracked_file(self, tmp_path): + """``lockdown`` is ignored in config.yaml deliberately, so that a local + edit cannot unlock or fake-lock an instance. Copying it into settings.yaml + would hand a flag that has never had any effect the authority to drop the + machine layers — including the config.local.yaml this migration just moved + the secrets into, leaving the ${VAR}s in the tracked file resolving to + nothing and the instance refusing to load.""" + config_dir, workspace = _legacy_install( + tmp_path, "timezone: UTC\nlockdown: true\nanthropic_api_key: sk-xyz\n" + ) + report = migrate(config_dir, workspace=workspace) + + settings = yaml.safe_load(workspace_settings_file(workspace).read_text()) + local = yaml.safe_load((config_dir / "config.local.yaml").read_text()) + assert "lockdown" not in settings + assert "lockdown" not in local + assert any("lockdown" in w for w in report.warnings) + # Dropping it preserves what the box already did, so the install loads. + assert load_config(config_dir).anthropic_api_key == "sk-xyz" + + def test_existing_local_secret_not_clobbered(self, tmp_path): + config_dir, workspace = _legacy_install( + tmp_path, + "anthropic_api_key: sk-from-config\n", + local_yaml="openai_api_key: sk-existing-local\n", + ) + migrate(config_dir, workspace=workspace) + local = yaml.safe_load((config_dir / "config.local.yaml").read_text()) + assert local["openai_api_key"] == "sk-existing-local" # preserved + assert local["anthropic_api_key"] == "sk-from-config" # added + + def test_idempotent(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + assert migrate(config_dir, workspace=workspace).did_anything + # Second run: settings.yaml now exists → no-op. + assert not migrate(config_dir, workspace=workspace).did_anything + assert is_migrated(config_dir, workspace=workspace) + + def test_settings_with_content_not_migrated(self, tmp_path): + """A workspace whose settings file already carries configuration is left + alone — migration must not rename config.yaml out from under it.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + (workspace / "config").mkdir(parents=True) + workspace_settings_file(workspace).write_text("timezone: UTC\n", encoding="utf-8") + report = migrate(config_dir, workspace=workspace) + assert not report.migrated_config + assert (config_dir / "config.yaml").exists() # untouched + + def test_comments_only_scaffold_does_not_block_migration(self, tmp_path): + """``nerve init`` scaffolds a settings.yaml that is nothing but comments. + Treating its mere existence as "already migrated" made migration a + permanent no-op for every install created after the scaffold shipped.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: Europe/Berlin\n") + (workspace / "config").mkdir(parents=True) + workspace_settings_file(workspace).write_text( + "# Nerve — shareable workspace configuration.\n" + "# Uncomment what you want to share.\n" + "# timezone: UTC\n", + encoding="utf-8", + ) + report = migrate(config_dir, workspace=workspace) + assert report.migrated_config + settings = yaml.safe_load(workspace_settings_file(workspace).read_text()) + assert settings["timezone"] == "Europe/Berlin" + assert (config_dir / "config.yaml.migrated").exists() + + def test_unreadable_settings_blocks_migration(self, tmp_path): + """Never overwrite a tracked file we can't parse.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + (workspace / "config").mkdir(parents=True) + workspace_settings_file(workspace).write_text("{ not: valid: yaml\n", encoding="utf-8") + report = migrate(config_dir, workspace=workspace) + assert not report.migrated_config + assert (config_dir / "config.yaml").exists() + + def test_dry_run_writes_nothing(self, tmp_path): + config_dir, workspace = _legacy_install( + tmp_path, "anthropic_api_key: sk-secret\n" + ) + legacy_cron = _legacy_cron(tmp_path, {"jobs.yaml": "jobs: []\n"}) + before = _snapshot(tmp_path) + + report = migrate( + config_dir, workspace=workspace, dry_run=True, legacy_cron_dir=legacy_cron + ) + + assert report.migrated_config and report.migrated_cron # would do both + # Not one byte, and not one directory — the workspace config subtree in + # particular must not be conjured into existence by a dry run. + assert _snapshot(tmp_path) == before + assert not (workspace / "config").exists() + + +class TestMigratePermissions: + """The migrated files carry plaintext secrets; the modes have to say so.""" + + def test_new_local_overlay_is_owner_only(self, tmp_path, permissive_umask): + config_dir, workspace = _legacy_install( + tmp_path, "telegram:\n bot_token: 12345:AAisasecret\n" + ) + migrate(config_dir, workspace=workspace) + assert _mode(config_dir / "config.local.yaml") == 0o600 + + def test_backup_of_the_unscrubbed_original_is_owner_only(self, tmp_path, permissive_umask): + """The breadcrumb still holds *every* secret, unscrubbed, right next to + the file we bothered to lock down.""" + config_dir, workspace = _legacy_install( + tmp_path, "auth:\n jwt_secret: hunter2\n" + ) + os.chmod(config_dir / "config.yaml", 0o644) + migrate(config_dir, workspace=workspace) + backup = config_dir / "config.yaml.migrated" + assert "hunter2" in backup.read_text() # unscrubbed, as designed + assert _mode(backup) == 0o600 + + def test_pre_existing_wide_open_local_file_is_tightened(self, tmp_path, permissive_umask): + config_dir, workspace = _legacy_install( + tmp_path, "anthropic_api_key: sk-a\n", local_yaml="openai_api_key: sk-b\n" + ) + os.chmod(config_dir / "config.local.yaml", 0o644) + migrate(config_dir, workspace=workspace) + assert _mode(config_dir / "config.local.yaml") == 0o600 + + @pytest.mark.parametrize("umask,expected", [(0o022, 0o644), (0o077, 0o600), (0o002, 0o664)]) + def test_tracked_settings_follow_the_umask(self, tmp_path, umask, expected): + """The shareable file is readable from a checkout, but an operator who + set a restrictive umask does not get it widened behind their back — and + scrubbing is explicitly not guaranteed to have emptied it of secrets.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + old = os.umask(umask) + try: + migrate(config_dir, workspace=workspace) + finally: + os.umask(old) + assert _mode(workspace_settings_file(workspace)) == expected + + def test_scaffolded_settings_file_keeps_its_own_permissions(self, tmp_path, permissive_umask): + """``nerve init`` scaffolds a comments-only settings.yaml, which migration + is free to fill in — but filling it in is a *rewrite*, and a rewrite of an + existing file does not re-permission it. An operator who locked the + scaffold down must not have it opened up by a migration that happened to + run under a laxer umask.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + settings = workspace_settings_file(workspace) + settings.parent.mkdir(parents=True, exist_ok=True) + settings.write_text("# Shared settings. No keys yet.\n", encoding="utf-8") + os.chmod(settings, 0o600) + + migrate(config_dir, workspace=workspace) + + assert yaml.safe_load(settings.read_text())["timezone"] == "UTC" + assert _mode(settings) == 0o600 + + def test_secret_files_ignore_a_permissive_umask(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "anthropic_api_key: sk-a\n") + old = os.umask(0o000) + try: + migrate(config_dir, workspace=workspace) + finally: + os.umask(old) + assert _mode(config_dir / "config.local.yaml") == 0o600 + assert _mode(config_dir / "config.yaml.migrated") == 0o600 + + +class TestMigrateCrashSafety: + def test_pre_existing_local_secrets_survive_a_failed_write(self, tmp_path, monkeypatch): + """config.local.yaml holds secrets that exist in no backup — the wizard's + API key, OAuth token and password hash. A migration that dies partway + must not be what removes them.""" + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\nanthropic_api_key: sk-from-config\n", + local_yaml="openai_api_key: sk-only-copy\nclaude_oauth_token: tok-only-copy\n", + ) + real_write = nerve.migrate.atomic_write_text + + def fail_on_settings(path, content, **kwargs): + if Path(path) == workspace_settings_file(workspace): + raise OSError("No space left on device") + return real_write(path, content, **kwargs) + + monkeypatch.setattr(nerve.migrate, "atomic_write_text", fail_on_settings) + with pytest.raises(OSError): + migrate(config_dir, workspace=workspace) + monkeypatch.undo() + + local = yaml.safe_load((config_dir / "config.local.yaml").read_text()) + assert local["openai_api_key"] == "sk-only-copy" + assert local["claude_oauth_token"] == "tok-only-copy" + + # And the install is still in a state that a retry can finish, because + # the source file was not renamed away first. + assert (config_dir / "config.yaml").exists() + assert migrate(config_dir, workspace=workspace).migrated_config + local = yaml.safe_load((config_dir / "config.local.yaml").read_text()) + assert local["openai_api_key"] == "sk-only-copy" + assert local["anthropic_api_key"] == "sk-from-config" + + def test_settings_is_written_before_the_source_is_renamed(self, tmp_path, monkeypatch): + """Losing config.yaml before its replacement exists would drop every + non-secret setting out of the live config, unrecoverably.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + seen = [] + real_rename = Path.rename + + def spy_rename(self, target): + seen.append(workspace_settings_file(workspace).exists()) + return real_rename(self, target) + + monkeypatch.setattr(Path, "rename", spy_rename) + migrate(config_dir, workspace=workspace) + assert seen == [True] + + def test_wholesale_list_relocation_is_reported(self, tmp_path): + """One secret in a list sends the whole list to the overlay, because a + merge replaces a list rather than combining it element-wise. The copy + left in settings.yaml is inert from then on — editing it does nothing, + so the operator has to be told.""" + config_dir, workspace = _legacy_install( + tmp_path, + "mcp_servers:\n" + " targets:\n" + " - name: gh\n" + " token: ghp_AAAABBBBCCCCDDDDEEEEFFFF\n", + ) + report = migrate(config_dir, workspace=workspace) + assert any("mcp_servers.targets" in w and "no longer has any effect" in w + for w in report.warnings) + + def test_existing_breadcrumb_is_never_overwritten(self, tmp_path): + """The cron half re-runs whenever the legacy dir has jobs again, so a + fixed .migrated suffix could destroy the only copy of the first one.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = _legacy_cron(tmp_path, {"jobs.yaml": "jobs: [] # first\n"}) + migrate(config_dir, workspace=workspace, legacy_cron_dir=legacy_cron) + + # A later run of the daemon writes jobs to the legacy dir again, and the + # workspace copy is gone (moved, or a fresh checkout). + (legacy_cron / "jobs.yaml").write_text("jobs: [] # second\n", encoding="utf-8") + (workspace / "config" / "cron" / "jobs.yaml").unlink() + migrate(config_dir, workspace=workspace, legacy_cron_dir=legacy_cron) + + assert "first" in (legacy_cron / "jobs.yaml.migrated").read_text() + assert "second" in (legacy_cron / "jobs.yaml.migrated.1").read_text() + # ...and the breadcrumbs stay in the legacy dir. They're inert, but the + # workspace cron dir is headed for a shared repo. + assert not list((workspace / "config" / "cron").glob("*.migrated*")) + + +class TestAlreadySplitInstall: + """A ``config.yaml`` written by the split layout holds only what must stay on + this box. Copying it into the shareable file would publish exactly the + values the split exists to keep local — and an absent or emptied + ``settings.yaml`` is not evidence to the contrary, since a workspace loses + one by being repointed, moved, or interrupted mid-init.""" + + # Only the genuinely local half: certificate paths, a credential handle, + # whose mailboxes, this box's helper process and mounts. gateway.host/port + # and provider.type/aws_region are deliberately absent — they are shared + # policy now, so a config.yaml holding them has something to migrate. + MACHINE_ONLY = """ +deployment: server +gateway: + ssl: + cert: /etc/nerve/tls/cert.pem + key: /etc/nerve/tls/key.pem +provider: + aws_profile: nerve-prod +sync: + gmail: + accounts: + - alex@example.com +proxy: + enabled: true + port: 8317 +docker: + extra_mounts: [] +""" + + def test_machine_local_config_is_not_migrated(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, self.MACHINE_ONLY) + report = migrate(config_dir, workspace=workspace) + assert not report.migrated_config + assert (config_dir / "config.yaml").exists() + assert not workspace_settings_file(workspace).exists() + + def test_one_shareable_key_is_enough_to_migrate(self, tmp_path): + """The test is "is there anything here the shared layer should own", not + "does this look tidy" — a legacy monolith always has something.""" + config_dir, workspace = _legacy_install(tmp_path, self.MACHINE_ONLY + "timezone: UTC\n") + report = migrate(config_dir, workspace=workspace) + assert report.migrated_config + settings = yaml.safe_load(workspace_settings_file(workspace).read_text()) + assert settings["timezone"] == "UTC" + + def test_a_workflow_journal_dir_does_not_make_the_machine_half_look_legacy( + self, tmp_path, + ): + """A runs directory alone is not a reason to migrate. + + The wizard never writes this section, so the coverage test above cannot + see it. Left uncovered, one absolute path is enough to make a post-split + machine-local ``config.yaml`` read as a legacy monolith — and then the + whole machine half, provider and gateway included, is copied into the + file the docs tell you to commit. + """ + machine_half = self.MACHINE_ONLY + ( + "workflows:\n" + " runs_dir: /srv/box-42/workflow-runs\n" + ) + config_dir, workspace = _legacy_install(tmp_path, machine_half) + report = migrate(config_dir, workspace=workspace) + + assert not report.migrated_config + assert (config_dir / "config.yaml").exists() + assert not workspace_settings_file(workspace).exists() + + def test_machine_local_paths_cover_what_the_wizard_writes(self): + """Keeps the list honest: if init starts routing another key to the + machine layer, migration has to learn about it in the same change.""" + from nerve.bootstrap import SetupWizard + + uncovered = set() + for mode in ("personal", "worker"): + for provider in ("anthropic", "bedrock"): + wizard = SetupWizard(Path("/nonexistent")) + wizard.choices.mode = mode + wizard.choices.provider_type = provider + wizard.choices.aws_region = "us-east-1" + wizard.choices.aws_profile = "nerve-prod" + wizard.choices.use_proxy = True + wizard.choices.deployment = "docker" + machine, _portable, _shadowed = wizard._build_config_layers() + uncovered |= { + p for p in nerve.migrate._leaf_paths(machine) + if not nerve.migrate._is_machine_local(p) + } + assert not uncovered, ( + "the wizard keeps these machine-local but migration would copy them " + f"into the git-tracked settings file: {sorted(uncovered)}" + ) + + def test_machine_local_paths_claim_nothing_the_wizard_shares(self): + """The direction nothing checked, and which had gone wrong. + + A path listed in _MACHINE_LOCAL_PATHS is one migration leaves behind in + config.yaml, so an entry the wizard treats as shareable is not a harmless + over-listing: it strands that value in a layer lockdown does not read. + ``gateway`` and ``provider`` were listed as whole subtrees after the + wizard had already moved host/port and type/aws_region to the tracked + file. + """ + from nerve.bootstrap import SetupWizard + + overclaimed = set() + for provider in ("anthropic", "bedrock"): + wizard = SetupWizard(Path("/nonexistent")) + wizard.choices.mode = "personal" + wizard.choices.provider_type = provider + wizard.choices.aws_region = "us-east-1" + wizard.choices.aws_profile = "nerve-prod" + wizard.choices.use_proxy = True + wizard.choices.deployment = "docker" + _machine, portable, _shadowed = wizard._build_config_layers() + overclaimed |= { + p for p in nerve.migrate._leaf_paths(portable) + if nerve.migrate._is_machine_local(p) + } + assert not overclaimed, ( + "the wizard shares these but migration would strand them in " + f"config.yaml, where lockdown never reads them: {sorted(overclaimed)}" + ) + + def test_a_legacy_bind_port_and_provider_are_migrated(self, tmp_path): + """Values the old classification stranded must now move across. + + A pre-split install with a non-default port, or one running on Bedrock, + had both judged machine-local. Migration left them in config.yaml, so + turning on lockdown reverted the box to 0.0.0.0:8900 and the Anthropic + API with no indication that it had. + """ + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\n" + "gateway:\n host: 127.0.0.1\n port: 9100\n" + " ssl:\n cert: /etc/nerve/tls/cert.pem\n" + "provider:\n type: bedrock\n aws_region: eu-west-1\n" + " aws_profile: nerve-prod\n", + ) + report = migrate(config_dir, workspace=workspace) + assert report.migrated_config + + settings = yaml.safe_load( + workspace_settings_file(workspace).read_text(encoding="utf-8") + ) + assert settings["gateway"]["host"] == "127.0.0.1" + assert settings["gateway"]["port"] == 9100 + assert settings["provider"]["type"] == "bedrock" + assert settings["provider"]["aws_region"] == "eu-west-1" + + def test_machine_local_keys_are_not_published(self, tmp_path): + """A legacy monolith is split on ``_MACHINE_LOCAL_PATHS``, not copied. + + The machine half is rewritten into config.yaml and never reaches the + git-tracked settings file: syncing a settings.yaml that names a + certificate path present on one box makes every other box's gateway fail + to bind. The split has to happen at the depth the path is listed at — + ``gateway.ssl`` moving must not take ``gateway.port`` with it, which would + strand the bind port in a layer lockdown does not read. + """ + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\n" + "gateway:\n port: 9100\n ssl:\n cert: /etc/nerve/tls/cert.pem\n" + "provider:\n type: bedrock\n aws_profile: nerve-prod\n", + ) + report = migrate(config_dir, workspace=workspace) + settings = yaml.safe_load( + workspace_settings_file(workspace).read_text(encoding="utf-8") + ) + assert "ssl" not in settings["gateway"] + assert "aws_profile" not in settings["provider"] + assert settings["gateway"]["port"] == 9100 # portable sibling + assert settings["provider"]["type"] == "bedrock" # portable sibling + + machine = yaml.safe_load((config_dir / "config.yaml").read_text(encoding="utf-8")) + assert machine["gateway"]["ssl"]["cert"] == "/etc/nerve/tls/cert.pem" + assert machine["provider"]["aws_profile"] == "nerve-prod" + assert "timezone" not in machine + assert sorted(report.machine_local_kept) == [ + "gateway.ssl.cert", "provider.aws_profile", + ] + + def test_the_split_preserves_the_effective_config(self, tmp_path): + """Relocated, not dropped: config.yaml is a layer the loader still reads, + so every machine-local value has to survive the split.""" + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\n" + "gateway:\n port: 9100\n" + " ssl:\n cert: /etc/nerve/tls/cert.pem\n key: /etc/nerve/tls/key.pem\n" + "docker:\n extra_mounts: ['~/code:/code']\n" + "workflows:\n runs_dir: /srv/box-42/workflow-runs\n", + ) + before = load_config(config_dir) + migrate(config_dir, workspace=workspace) + after = load_config(config_dir) + + assert after.gateway.ssl.cert == before.gateway.ssl.cert + assert after.gateway.ssl.key == before.gateway.ssl.key + assert after.gateway.port == before.gateway.port == 9100 + assert after.docker.extra_mounts == before.docker.extra_mounts + assert after.workflows.runs_dir == before.workflows.runs_dir + + def test_the_rewritten_config_yaml_is_owner_only(self, tmp_path, permissive_umask): + """It is the unscrubbed half — an ``external_agents`` target's token and a + local service's credentials are in the subtrees that land here.""" + config_dir, workspace = _legacy_install( + tmp_path, + "timezone: UTC\n" + "external_agents:\n targets:\n - name: codex\n token: tok-secret\n", + ) + migrate(config_dir, workspace=workspace) + config_yaml = config_dir / "config.yaml" + assert _mode(config_yaml) == 0o600 + assert "tok-secret" in config_yaml.read_text(encoding="utf-8") + assert "tok-secret" not in workspace_settings_file(workspace).read_text() + + def test_the_split_half_does_not_look_legacy_on_a_re_run(self, tmp_path): + """The file migration leaves behind is a post-split config.yaml, so a + second pass has to read it as nothing to do rather than as a monolith + shadowing the tracked file.""" + config_dir, workspace = _legacy_install(tmp_path, self.MACHINE_ONLY + "timezone: UTC\n") + assert migrate(config_dir, workspace=workspace).migrated_config + + again = migrate(config_dir, workspace=workspace) + assert not again.did_anything + assert again.warnings == [] + assert is_migrated(config_dir, workspace=workspace) + + def test_no_config_yaml_is_written_when_there_is_no_machine_half(self, tmp_path): + """A wholly portable legacy config leaves nothing behind to shadow the + tracked file.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + report = migrate(config_dir, workspace=workspace) + assert report.machine_local_kept == [] + assert not (config_dir / "config.yaml").exists() + + def test_the_external_agents_block_is_machine_local_too(self, tmp_path): + """A second writer appends to config.yaml after the wizard has paired + this box's external agents, so it has to be covered as well.""" + from types import SimpleNamespace + + from nerve.bootstrap import SetupWizard + + (tmp_path / "config.yaml").write_text("deployment: server\n", encoding="utf-8") + wizard = SetupWizard(tmp_path) + wizard._record_external_agents_in_config_yaml([SimpleNamespace(agent="codex")]) + + raw = yaml.safe_load((tmp_path / "config.yaml").read_text()) + assert "external_agents" in raw # the writer really did run + assert not nerve.migrate._has_portable_content(raw) + + def test_lingering_config_yaml_is_reported_not_ignored(self, tmp_path): + """What an interruption at the rename leaves behind, and what a + hand-managed install can look like. Indistinguishable on disk, so say + so instead of guessing.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + (workspace / "config").mkdir(parents=True) + workspace_settings_file(workspace).write_text("timezone: Europe/Berlin\n", encoding="utf-8") + + report = migrate(config_dir, workspace=workspace) + + assert not report.migrated_config + assert any("still present" in w for w in report.warnings) + + def test_no_warning_for_an_ordinary_split_install(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, self.MACHINE_ONLY) + (workspace / "config").mkdir(parents=True) + workspace_settings_file(workspace).write_text("timezone: UTC\n", encoding="utf-8") + assert migrate(config_dir, workspace=workspace).warnings == [] + + +_DOCS_CONFIG_MD = Path(__file__).resolve().parents[1] / "docs" / "config.md" +_LAYER_TABLE_HEADING = "## Which layer a key belongs in" +# A sentinel rather than getattr's default of None: a setting whose default *is* +# None (gateway.ssl.cert) has to read as present. +_MISSING = object() + + +def _documented_machine_local_paths() -> set[str]: + """The ``config.yaml`` row of the layer table in ``docs/config.md``. + + Parsed out of the document rather than restated here. A copy of the table + kept in this file would agree with the test forever and with the docs never, + which is the failure mode the guard exists to catch. + """ + text = _DOCS_CONFIG_MD.read_text(encoding="utf-8") + parts = text.split(_LAYER_TABLE_HEADING, 1) + assert len(parts) == 2, ( + f"{_LAYER_TABLE_HEADING!r} is no longer in {_DOCS_CONFIG_MD} — the layer " + "table moved or was renamed, and this guard is now checking nothing" + ) + rows = [ + line for line in parts[1].splitlines() if line.startswith("| `config.yaml` |") + ] + assert len(rows) == 1, ( + f"expected exactly one `config.yaml` row under {_LAYER_TABLE_HEADING!r}, " + f"found {len(rows)}" + ) + tokens = re.findall(r"`([^`]+)`", rows[0].split("|")[2]) + assert tokens, f"the `config.yaml` row names no keys: {rows[0]}" + + paths = set() + for token in tokens: + # ``gateway.ssl.*`` names a subtree; the code lists its root. + assert re.fullmatch(r"[a-z_]+(?:\.[a-z_]+)*(?:\.\*)?", token), ( + f"{token!r} in the layer table is not a plain dotted path. The " + "shorthand the settings.yaml row uses (`a`/`b` for two siblings) " + "would be read as one key here, so spell it out instead" + ) + paths.add(token[:-2] if token.endswith(".*") else token) + return paths + + +class TestMachineLocalPathsMatchTheDocs: + """The layer table in ``docs/config.md`` states which layer owns a key; + ``_MACHINE_LOCAL_PATHS`` is what migration and the wizard actually do. The + list's comment claims to mirror the table and nothing checked it. + + Both directions matter now that the list decides what migration publishes. A + key the table calls machine-local but the list omits is copied into a file the + docs tell you to commit; one the list claims but the table shares is stranded + in a layer lockdown never reads. + """ + + def test_the_list_and_the_table_agree(self): + documented = _documented_machine_local_paths() + assert len(documented) >= 10, ( + f"the parse found only {len(documented)} keys in the layer table — " + "did the table's formatting change?" + ) + listed = set(nerve.migrate._MACHINE_LOCAL_PATHS) + assert documented == listed, ( + "docs/config.md's `config.yaml` row and _MACHINE_LOCAL_PATHS " + "disagree.\n" + f" in the docs, not in the code: {sorted(documented - listed)}\n" + f" in the code, not in the docs: {sorted(listed - documented)}" + ) + + def test_every_listed_path_names_a_real_setting(self): + """A renamed or misspelled entry covers nothing and says nothing. + + It matches no key, so ``_has_portable_content`` counts the value as + shareable and the split hands it to the tracked file — while the list + still reads as though that key were accounted for. + """ + from nerve.config import NerveConfig + + defaults = NerveConfig() + unresolved = [] + for dotted in sorted(nerve.migrate._MACHINE_LOCAL_PATHS): + node = defaults + for part in dotted.split("."): + node = getattr(node, part, _MISSING) + if node is _MISSING: + unresolved.append(dotted) + break + assert not unresolved, ( + "_MACHINE_LOCAL_PATHS entries that resolve to no setting on a default " + f"NerveConfig (renamed? misspelled?): {unresolved}" + ) + + +class TestMigrationIsolation: + """``workspace=`` alone never sandboxed the cron half — it resolved its + source from the machine-global state dir and *renamed* files there.""" + + def test_scoped_migration_leaves_the_machine_cron_dir_untouched(self, tmp_path): + machine_cron = paths.cron_dir() + machine_cron.mkdir(parents=True, exist_ok=True) + (machine_cron / "jobs.yaml").write_text("jobs: [] # real\n", encoding="utf-8") + (machine_cron / "system.yaml").write_text("jobs: [] # real\n", encoding="utf-8") + + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = _legacy_cron(tmp_path, {"jobs.yaml": "jobs: [] # scoped\n"}) + report = migrate(config_dir, workspace=workspace, legacy_cron_dir=legacy_cron) + + assert report.migrated_cron + assert (machine_cron / "jobs.yaml").exists() + assert (machine_cron / "system.yaml").exists() + assert not list(machine_cron.glob("*.migrated")) + # The scoped directory is the one that moved. + assert "scoped" in (workspace / "config" / "cron" / "jobs.yaml").read_text() + + def test_override_threads_through_the_dry_run_helpers(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = _legacy_cron(tmp_path, {"jobs.yaml": "jobs: []\n"}) + empty = tmp_path / "no-cron-here" + assert not is_migrated(config_dir, workspace=workspace, legacy_cron_dir=legacy_cron) + maybe_migrate(config_dir, workspace=workspace, legacy_cron_dir=empty) + # Config migrated, cron did not — the override was honored by both. + assert workspace_settings_file(workspace).exists() + assert not (workspace / "config" / "cron").exists() + assert (legacy_cron / "jobs.yaml").exists() + + +class TestPlantedSecretCorpus: + """A legacy config carrying one of every plausible secret shape. What + matters is the bytes that land in the git-tracked file.""" + + CORPUS = """ +timezone: UTC +anthropic_api_key: sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGGHHHH +auth: + jwt_secret: hunter2hunter2hunter2 +github: + token: "ghp_MixedRefAAAABBBBCCCCDDDDEEEE${GH_SUFFIX}" +slack: + webhook_url: https://hooks.slack.com/services/T000/B000/XXXXXXXXXXXXXXXXXXXX +database: + url: postgres://admin:sup3rs3cr3t@db.internal:5432/nerve +mcp_servers: + gh: + args: ["--stdio", "--api-key=sk-proj-AAAABBBBCCCCDDDDEEEEFFFF"] + headers: ["Authorization: Bearer ghp_AAAABBBBCCCCDDDDEEEEFFFFGGGG"] + feed: + url: https://feeds.example.com/all?token=abcdef1234567890abcdef +telegram_sync: + api_id: 2040123 + api_hash: 0123456789abcdef0123456789abcdef +sentry: + dsn: https://0123456789abcdef0123456789abcdef@o12345.ingest.sentry.io/9876 +smtp: + host: smtp.example.com + pw: correct-horse-battery-staple + pat: pat_AAAABBBBCCCCDDDDEEEE +telethon: + session_string: 1BVtsOHYBu4XyZAbCdEfGhIjKlMnOpQrStUvWx0123456789 +worker: + env: ["GH_PAT=ghp_WWWWXXXXYYYYZZZZAAAABBBB"] +""" + + PLANTED = [ + "sk-ant-api03-AAAABBBBCCCCDDDDEEEEFFFFGGGGHHHH", + "hunter2hunter2hunter2", + "ghp_MixedRefAAAABBBBCCCCDDDDEEEE", + "T000/B000/XXXXXXXXXXXXXXXXXXXX", + "sup3rs3cr3t", + "sk-proj-AAAABBBBCCCCDDDDEEEEFFFF", + "ghp_AAAABBBBCCCCDDDDEEEEFFFFGGGG", + "token=abcdef1234567890abcdef", + "2040123", + "0123456789abcdef0123456789abcdef", + "correct-horse-battery-staple", + "pat_AAAABBBBCCCCDDDDEEEE", + "1BVtsOHYBu4XyZAbCdEfGhIjKlMnOpQrStUvWx0123456789", + "ghp_WWWWXXXXYYYYZZZZAAAABBBB", + ] + + def test_no_planted_secret_reaches_the_tracked_file(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, self.CORPUS) + migrate(config_dir, workspace=workspace) + + tracked = workspace_settings_file(workspace).read_text() + leaked = [s for s in self.PLANTED if s in tracked] + assert not leaked, f"{len(leaked)} secret(s) leaked into settings.yaml: {leaked}" + + def test_every_planted_secret_is_still_readable_after_migration(self, tmp_path): + """Scrubbing must relocate, not destroy — the daemon still resolves the + real values from the machine-local overlay.""" + config_dir, workspace = _legacy_install(tmp_path, self.CORPUS) + migrate(config_dir, workspace=workspace) + + overlay = (config_dir / "config.local.yaml").read_text() + missing = [s for s in self.PLANTED if s not in overlay] + assert not missing, f"lost in migration: {missing}" + + +class TestMigrateCron: + def test_copies_legacy_cron_to_workspace(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + legacy_cron.mkdir(parents=True, exist_ok=True) + (legacy_cron / "jobs.yaml").write_text( + "jobs:\n - id: mine\n schedule: 1h\n prompt: hi\n", encoding="utf-8" + ) + report = migrate(config_dir, workspace=workspace) + assert report.migrated_cron + + ws_jobs = workspace / "config" / "cron" / "jobs.yaml" + assert ws_jobs.exists() + assert "mine" in ws_jobs.read_text() + # Original kept as breadcrumb. + assert (legacy_cron / "jobs.yaml.migrated").exists() + + def test_copies_cron_prompts_dir(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + (legacy_cron / "prompts").mkdir(parents=True, exist_ok=True) + (legacy_cron / "prompts" / "daily.md").write_text("do it", encoding="utf-8") + (legacy_cron / "jobs.yaml").write_text( + "jobs:\n - id: j\n schedule: 1h\n prompt_file: prompts/daily.md\n", + encoding="utf-8", + ) + migrate(config_dir, workspace=workspace) + # The referenced prompt file came along, so the relative path still resolves. + assert (workspace / "config" / "cron" / "prompts" / "daily.md").exists() + + def test_no_cron_migration_when_workspace_has_jobs(self, tmp_path): + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + legacy_cron.mkdir(parents=True, exist_ok=True) + (legacy_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + ws_cron = workspace / "config" / "cron" + ws_cron.mkdir(parents=True) + (ws_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + report = migrate(config_dir, workspace=workspace) + assert not report.migrated_cron + assert not (legacy_cron / "jobs.yaml.migrated").exists() + # Silence would leave the operator with two job files, one of which has + # stopped running: the workspace copy wins and the legacy dir is dropped. + assert any("still holds cron jobs" in w for w in report.warnings) + + def test_gates_and_prompts_migrate_past_the_files_nerve_init_wrote(self, tmp_path): + """``nerve init`` writes ``system.yaml`` and an empty ``gates/`` into the + workspace and copies only ``jobs.yaml`` across. A directory-level "the + workspace already has cron config" test therefore skipped everything + else permanently — and the legacy dir is never read again once the + workspace has job files, so custom gates stopped loading. A job whose + gate ``type`` no longer resolves does not run at all.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + (legacy_cron / "gates").mkdir(parents=True, exist_ok=True) + (legacy_cron / "gates" / "stale_tasks.py").write_text("CUSTOM = 1\n", encoding="utf-8") + (legacy_cron / "prompts").mkdir(parents=True, exist_ok=True) + (legacy_cron / "prompts" / "daily.md").write_text("do it", encoding="utf-8") + (legacy_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + + ws_cron = workspace / "config" / "cron" + (ws_cron / "gates").mkdir(parents=True) # the init placeholder + (ws_cron / "system.yaml").write_text("jobs: []\n", encoding="utf-8") + (ws_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + + report = migrate(config_dir, workspace=workspace) + + assert report.migrated_cron + assert (ws_cron / "gates" / "stale_tasks.py").read_text() == "CUSTOM = 1\n" + assert (ws_cron / "prompts" / "daily.md").exists() + + def test_an_existing_workspace_file_is_kept_and_the_rest_still_moves(self, tmp_path): + """Never overwrite: the workspace copy is the reviewed one. That has to + hold per file rather than per directory, or one same-named gate blocks + every other gate in the directory.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + (legacy_cron / "gates").mkdir(parents=True, exist_ok=True) + (legacy_cron / "gates" / "shared.py").write_text("LEGACY = 1\n", encoding="utf-8") + (legacy_cron / "gates" / "only_legacy.py").write_text("OTHER = 1\n", encoding="utf-8") + (legacy_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + + ws_cron = workspace / "config" / "cron" + (ws_cron / "gates").mkdir(parents=True) + (ws_cron / "gates" / "shared.py").write_text("REVIEWED = 1\n", encoding="utf-8") + + migrate(config_dir, workspace=workspace) + + assert (ws_cron / "gates" / "shared.py").read_text() == "REVIEWED = 1\n" + assert (ws_cron / "gates" / "only_legacy.py").read_text() == "OTHER = 1\n" + + def test_a_job_file_that_did_not_move_is_not_breadcrumbed(self, tmp_path): + """Renaming a legacy ``jobs.yaml`` that lost to a workspace copy would + hide the operator's only copy of those jobs behind a ``.migrated`` + suffix, while the jobs themselves are already not running.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + (legacy_cron / "gates").mkdir(parents=True, exist_ok=True) + (legacy_cron / "gates" / "g.py").write_text("G = 1\n", encoding="utf-8") + (legacy_cron / "jobs.yaml").write_text( + "jobs:\n - id: legacy-only\n schedule: 1h\n prompt: hi\n", encoding="utf-8" + ) + ws_cron = workspace / "config" / "cron" + ws_cron.mkdir(parents=True) + (ws_cron / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + + report = migrate(config_dir, workspace=workspace) + + assert (ws_cron / "gates" / "g.py").exists() # the gate still moved + assert (legacy_cron / "jobs.yaml").exists() # ...and this stayed put + assert not (legacy_cron / "jobs.yaml.migrated").exists() + assert "legacy-only" in (legacy_cron / "jobs.yaml").read_text() + assert any("still holds cron jobs" in w for w in report.warnings) + + def test_gates_migrate_even_with_no_legacy_job_files(self, tmp_path): + """Job files are not the gate for the directory: an install that pinned + ``cron.jobs_file`` elsewhere still keeps its gate plugins here, and they + are imported from whichever cron dir resolution picks.""" + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + (legacy_cron / "gates").mkdir(parents=True, exist_ok=True) + (legacy_cron / "gates" / "g.py").write_text("G = 1\n", encoding="utf-8") + + report = migrate(config_dir, workspace=workspace) + + assert report.migrated_cron + assert (workspace / "config" / "cron" / "gates" / "g.py").exists() + + +class TestMaybeMigrate: + def test_never_raises(self, tmp_path): + # A config_dir with nothing — must not raise. + result = maybe_migrate(tmp_path / "nonexistent") + assert result is None or not result.did_anything + + def test_a_failure_partway_still_reports_what_was_applied(self, tmp_path, monkeypatch): + """The config half commits before the cron half runs, so an exception can + leave settings.yaml written, the secrets relocated and config.yaml renamed + away. Reporting that as "nothing happened" is what made a half-applied + migration a silent one: ``upgrade`` printed no review prompt for a tracked + file that now exists and may hold an unscrubbed credential, and ``start`` + skipped the reload it needs because the files moved underneath it.""" + config_dir, workspace = _legacy_install( + tmp_path, "timezone: UTC\nanthropic_api_key: sk-xyz\n" + ) + legacy_cron = _legacy_cron(tmp_path, {"jobs.yaml": "jobs: []\n"}) + + def boom(src, dst, *args, **kwargs): + raise OSError("Read-only file system") + + monkeypatch.setattr(nerve.migrate.shutil, "copy2", boom) + report = maybe_migrate(config_dir, workspace=workspace, legacy_cron_dir=legacy_cron) + + assert report is not None + assert report.did_anything + assert report.error and "Read-only file system" in report.error + # ...and what it reports really did happen. + assert workspace_settings_file(workspace).exists() + assert (config_dir / "config.yaml.migrated").exists() + assert report.secrets_moved == ["anthropic_api_key"] + + +class TestStartAfterMigration: + def test_foreground_start_gets_the_post_migration_config(self, tmp_path): + """``start`` loads config, migrates, then serves. Migration moves cron + into the workspace and renames the legacy files away, so the object + loaded beforehand points at job files that no longer exist — and + ``--foreground`` is the deployed path, where that means the daemon runs + for its whole lifetime with nothing scheduled.""" + from nerve.cli import start + + config_dir, workspace = _legacy_install(tmp_path, "timezone: UTC\n") + legacy_cron = paths.cron_dir() + legacy_cron.mkdir(parents=True, exist_ok=True) + (legacy_cron / "jobs.yaml").write_text( + "jobs:\n - id: mine\n schedule: 1h\n prompt: hi\n", encoding="utf-8" + ) + + stale = load_config(config_dir) + assert stale.cron.jobs_file == legacy_cron / "jobs.yaml" + + served = {} + with ( + patch("nerve.gateway.server.run_server", side_effect=lambda c: served.update(config=c)), + patch("nerve.cli._get_daemon_status", return_value=(False, None)), + patch("nerve.cli._write_pid"), + patch("nerve.cli._remove_pid"), + patch("nerve.bootstrap.is_fresh_install", return_value=False), + ): + result = CliRunner().invoke( + start, + ["--foreground"], + obj={"config": stale, "config_dir": str(config_dir), "verbose": False}, + standalone_mode=False, + ) + + assert result.exit_code == 0, result.output + config = served["config"] + assert config.cron.jobs_file == workspace / "config" / "cron" / "jobs.yaml" + assert [j.id for j in load_jobs(config.cron.jobs_file)] == ["mine"] diff --git a/tests/test_utils_fs.py b/tests/test_utils_fs.py new file mode 100644 index 00000000..170edfde --- /dev/null +++ b/tests/test_utils_fs.py @@ -0,0 +1,184 @@ +"""Tests for the shared atomic file writer.""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from nerve.utils.fs import atomic_write_text + + +def _mode(path: Path) -> int: + return path.stat().st_mode & 0o777 + + +@pytest.fixture +def permissive_umask(): + old = os.umask(0o022) + yield + os.umask(old) + + +def test_writes_content_and_creates_parents(tmp_path): + target = tmp_path / "deep" / "nested" / "file.yaml" + atomic_write_text(target, "hello\n") + assert target.read_text(encoding="utf-8") == "hello\n" + + +def test_mode_is_applied_regardless_of_umask(tmp_path, permissive_umask): + target = tmp_path / "secrets.yaml" + atomic_write_text(target, "token: x\n") + assert _mode(target) == 0o600 + + public = tmp_path / "shared.yaml" + atomic_write_text(public, "timezone: UTC\n", mode=0o644) + assert _mode(public) == 0o644 + + +@pytest.mark.parametrize("umask,expected", [(0o022, 0o644), (0o077, 0o600), (0o002, 0o664)]) +def test_mode_none_on_a_new_file_matches_a_plain_write(tmp_path, umask, expected): + """``mode=None`` is for shareable files: same permissions ``open()`` would + have given them, so a restrictive umask is not overridden.""" + old = os.umask(umask) + try: + atomic_write_text(tmp_path / "shared.yaml", "timezone: UTC\n", mode=None) + reference = tmp_path / "reference.yaml" + with open(reference, "w") as f: + f.write("timezone: UTC\n") + finally: + os.umask(old) + assert _mode(tmp_path / "shared.yaml") == expected == _mode(reference) + + +@pytest.mark.parametrize("existing,umask", [(0o600, 0o000), (0o644, 0o077), (0o640, 0o022)]) +def test_mode_none_keeps_an_existing_files_permissions(tmp_path, existing, umask): + """``open(path, "w")`` re-permissions nothing — only a *new* file takes its + mode from the umask. Deriving the mode from the umask on every write would + widen a 0o600 file to 0o666 under a lax umask, silently publishing a file + that had been locked down, and clamp a group-readable one under a strict + one. The rename this helper does must not change what the mode would be.""" + target = tmp_path / "shared.yaml" + reference = tmp_path / "reference.yaml" + for p in (target, reference): + p.write_text("old\n", encoding="utf-8") + os.chmod(p, existing) + + old = os.umask(umask) + try: + atomic_write_text(target, "new\n", mode=None) + with open(reference, "w") as f: + f.write("new\n") + finally: + os.umask(old) + + assert _mode(target) == existing == _mode(reference) + assert target.read_text(encoding="utf-8") == "new\n" + + +def test_explicit_mode_beats_an_existing_files_permissions(tmp_path): + """Preserving what is on disk is only what ``mode=None`` asks for. A file + that has started holding a credential has to be tightened despite being + world-readable a moment ago, and an explicit loosening has to stick too.""" + tightened = tmp_path / "config.local.yaml" + tightened.write_text("timezone: UTC\n", encoding="utf-8") + os.chmod(tightened, 0o644) + atomic_write_text(tightened, "openai_api_key: sk-x\n") + assert _mode(tightened) == 0o600 + + loosened = tmp_path / "shared.yaml" + loosened.write_text("old\n", encoding="utf-8") + os.chmod(loosened, 0o600) + atomic_write_text(loosened, "new\n", mode=0o644) + assert _mode(loosened) == 0o644 + + +def test_symlinked_destination_is_written_through(tmp_path): + """A config file linked into a dotfiles repo must keep being that link — + replacing it with a regular file would leave the repo copy stale and put + the new secret outside the repo.""" + real = tmp_path / "repo" / "config.local.yaml" + real.parent.mkdir() + real.write_text("old\n", encoding="utf-8") + link = tmp_path / "config.local.yaml" + link.symlink_to(real) + + atomic_write_text(link, "new\n") + + assert link.is_symlink() + assert real.read_text(encoding="utf-8") == "new\n" + assert _mode(real) == 0o600 + + +@pytest.mark.skipif( + not hasattr(os, "fchmod"), + reason="no os.fchmod here; the chmod fallback it forces is covered below", +) +def test_mode_is_set_before_the_content_is_written(tmp_path, permissive_umask, monkeypatch): + """Chmod'ing after the write is a race: the secret is already on disk, and + readable, for however far apart the two syscalls land.""" + at_chmod = [] + real_fchmod = os.fchmod + + def spy_fchmod(fd, mode): + at_chmod.append((os.fstat(fd).st_size, mode)) + return real_fchmod(fd, mode) + + monkeypatch.setattr(os, "fchmod", spy_fchmod) + atomic_write_text(tmp_path / "secrets.yaml", "token: hunter2\n") + + # Permissions settled while the file was still empty. + assert at_chmod == [(0, 0o600)] + + +def test_mode_is_set_before_the_content_is_written_without_fchmod( + tmp_path, permissive_umask, monkeypatch +): + """Windows has no ``os.fchmod``, so the writer falls back to ``os.chmod`` on + the temp file's name. Take the attribute away here rather than only on the + platform that lacks it, or the one path that needs checking is the one path + never exercised. + + The mode asked for is 0o644, not 0o600: ``mkstemp`` already creates at 0o600, + so a 0o600 expectation would pass just as happily if the fallback never ran. + """ + monkeypatch.delattr(os, "fchmod", raising=False) + at_chmod = [] + real_chmod = os.chmod + + def spy_chmod(path, mode, **kwargs): + at_chmod.append((os.stat(path).st_size, mode)) + return real_chmod(path, mode, **kwargs) + + monkeypatch.setattr(os, "chmod", spy_chmod) + target = tmp_path / "shared.yaml" + atomic_write_text(target, "timezone: UTC\n", mode=0o644) + + assert at_chmod == [(0, 0o644)] + assert _mode(target) == 0o644 + assert target.read_text(encoding="utf-8") == "timezone: UTC\n" + + +def test_existing_file_survives_a_failed_write(tmp_path, monkeypatch): + target = tmp_path / "config.local.yaml" + target.write_text("openai_api_key: sk-only-copy\n", encoding="utf-8") + + def boom(fd): + raise OSError("No space left on device") + + monkeypatch.setattr(os, "fsync", boom) + with pytest.raises(OSError): + atomic_write_text(target, "clobbered\n") + + assert target.read_text(encoding="utf-8") == "openai_api_key: sk-only-copy\n" + # ...and no debris left behind for the next reader to trip over. + assert [p.name for p in tmp_path.iterdir()] == ["config.local.yaml"] + + +def test_replaces_an_existing_file(tmp_path): + target = tmp_path / "f.txt" + target.write_text("old", encoding="utf-8") + atomic_write_text(target, "new") + assert target.read_text(encoding="utf-8") == "new" + assert list(tmp_path.iterdir()) == [target]