From 814927fc9227bf965e6fd2e5ad5e43431fc21ab9 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 28 Jul 2026 11:12:45 +0200 Subject: [PATCH 1/5] Lockdown: remote-only, read-only mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under lockdown the two machine-local layers are dropped and only the synced, tracked config applies, and runtime writes that would mutate tracked config are refused with a clear error. The flag is anchored in the environment as well as in the file, so an instance cannot be unlocked by editing the very config it is meant to be constrained by. Enabling lockdown no longer disables authentication: dropping the machine-local layers dropped the auth secret along with them. Paths are contained — cron files and the gate plugin directory must resolve inside /config/, an escaping override is ignored in favour of the in-workspace default, and if the default itself escapes through a symlink the instance refuses to start rather than running config nobody reviewed. Agent write paths that reach config/ are refused instead of reporting a success they did not earn, and the task read, write and move paths are guarded consistently — including the one in the task manager that nothing imports yet, so wiring it up later cannot reintroduce a deletion of tracked config. Backup separates what restore accepts from what backup collects, so a restore cannot smuggle files past the guard. Co-Authored-By: Claude --- docs/config.md | 195 +++- docs/cron.md | 6 +- nerve/agent/backends/claude.py | 65 ++ nerve/agent/backends/codex/backend.py | 42 +- nerve/agent/tools/handlers/tasks.py | 15 + nerve/backup.py | 56 + nerve/channels/telegram.py | 19 + nerve/cli.py | 25 +- nerve/config.py | 589 +++++++++- nerve/config_validate.py | 98 +- nerve/cron/jobs.py | 11 +- nerve/cron/service.py | 11 + nerve/gateway/auth.py | 10 + nerve/gateway/routes/config.py | 15 +- nerve/gateway/routes/external_agents.py | 4 +- nerve/gateway/routes/memory.py | 6 +- nerve/gateway/routes/tasks.py | 7 +- nerve/gateway/server.py | 17 + nerve/memory/manager.py | 10 +- nerve/skills/manager.py | 56 +- nerve/sync_service.py | 117 +- nerve/tasks/manager.py | 12 +- nerve/templates/config/settings.yaml | 19 + tests/test_backup.py | 86 ++ tests/test_lockdown.py | 1301 +++++++++++++++++++++++ tests/test_sync_service.py | 177 ++- 26 files changed, 2891 insertions(+), 78 deletions(-) create mode 100644 tests/test_lockdown.py diff --git a/docs/config.md b/docs/config.md index e9705f6b..64afbe05 100644 --- a/docs/config.md +++ b/docs/config.md @@ -95,6 +95,8 @@ type, including `int | None`, `list[int]` and `list[str]`. So `port: ${PORT}` an - An unrecognized value is logged with the field that owns it, and that field keeps its documented default. Integers are parsed with `int()`, so `"1.5"` and `"1e3"` are rejected rather than truncated. +- [`lockdown`](#lockdown-remote-only-read-only) is the exception: its default is + "unprotected", so an unreadable or empty value there is a hard error instead. - Defaults are not re-scanned: `${A:-${B}}` yields the literal `${B}` when `A` is unset. Use a single reference instead. @@ -104,11 +106,12 @@ type, including `int | None`, `list[int]` and `list[str]`. So `port: ${PORT}` an error, so it can gate a config repo in CI: ```bash -nerve config validate # the active install's config -nerve config validate --workspace . # a checked-out config repo -nerve config validate --portable-only # ignore this machine's config.yaml layers -nerve config validate --strict-keys # unknown keys become errors -nerve config validate --strict-env # every ${ENV_VAR} must be set +nerve config validate # the active install's config +nerve config validate --workspace . # a checked-out config repo +nerve config validate --portable-only # ignore this machine's config.yaml layers +nerve config validate --strict-keys # unknown keys become errors +nerve config validate --strict-env # every ${ENV_VAR} must be set +nerve config validate --assume-lockdown # check the view a locked box loads ``` It runs even when the config cannot otherwise load, so a missing secret does not @@ -180,6 +183,9 @@ jobs: # nerve is not published to PyPI: the bare name is an unrelated project. - run: uv pip install --system "nerve @ git+https://github.com/ClickHouse/nerve" - run: nerve config validate --workspace . --portable-only --strict-keys + # Add --assume-lockdown if any instance served by this repo is locked and + # the flag comes from the environment. Without it CI checks the unlocked + # view and the lockdown checks never run. See "Lockdown" below. ``` Installing from the default branch keeps the validator at or ahead of your @@ -249,6 +255,184 @@ config object every cycle, so it adds no staleness of its own, but nothing refre that object while the process runs. Turning `enabled` on needs a restart in any case, because the sync task is only created at startup. +## Lockdown (remote-only, read-only) + +Lockdown guarantees that the configuration an instance runs came from the tracked +workspace repo, and that the runtime cannot change it. The machine-local layers are +dropped, the write paths refuse, and config changes arrive only as a reviewed, +merged commit that sync pulls in. + +It is a config-integrity control, not a sandbox. It raises the bar against the +agent rewriting its own configuration; it does not confine the agent. Read +[What lockdown does not cover](#what-lockdown-does-not-cover) before relying on +it. + +### Before you turn it on + +**Move any required secrets into the environment first.** `config.local.yaml` is +ignored when locked, so a secret that lives only there stops being read, and the +feature depending on it breaks on the next restart. Supply each one as `${ENV_VAR}` +referenced from `settings.yaml` before you lock the box. The usual ones: +`auth.jwt_secret`, `auth.password_hash`, `telegram.bot_token`, +`anthropic_api_key`/`openai_api_key`, `xmemory.api_key`. + +`auth.jwt_secret` is the one to get right. A locked instance that ends up without +it does not fall back to the unauthenticated dev mode an unlocked box would — the +gateway refuses every request with a 503 and websockets are declined — so the box +comes up unusable rather than open. Note also that a `${VAR}` left unresolved +survives as its literal text, which is a perfectly usable signing key and one +published in the config repo, so check that the variable is actually set on the box. + +### Turning it on + +Set `lockdown: true` in the tracked `workspace/config/settings.yaml`, so the +remote is the authority: + +```yaml +# workspace/config/settings.yaml +lockdown: true +``` + +The value may be an environment reference, `lockdown: ${NERVE_LOCKDOWN}`, so one +shared repo can serve a fleet where only some boxes are locked. Unlike every other +boolean, this one is not lenient: a value that is neither true nor false is a hard +error and the instance refuses to start, because the only default it could fall +back to is "unlocked". An empty value counts as unreadable, so +`${NERVE_LOCKDOWN}` with the variable unset or blank is refused rather than read +as off. Write `${NERVE_LOCKDOWN:-false}` when you want an explicit unlocked +default. `nerve config validate` reports the same error, so a bad value fails the +PR instead of the box. + +### Better: set it in the environment + +Setting the flag in `settings.yaml` alone leaves a way around it. Which +`settings.yaml` gets read is decided by `workspace:` in the machine-local +`config.yaml`, so editing that one line repoints the instance at a tree that never +mentions lockdown — the flag reads false and the machine-local layers come back, +`auth.jwt_secret` included. + +Closing that means putting both values in the environment, set where the service is +defined. That is the one place neither a config edit nor the agent can reach: + +```ini +# /etc/systemd/system/nerve.service +[Service] +Environment=NERVE_LOCKDOWN=1 +Environment=NERVE_WORKSPACE=/srv/nerve-workspace +``` + +```bash +docker run -e NERVE_LOCKDOWN=1 -e NERVE_WORKSPACE=/root/nerve-workspace ... +``` + +Set both, not just one. `NERVE_LOCKDOWN` without `NERVE_WORKSPACE` is refused at +startup: it would lock the instance onto whatever tree `config.yaml` happens to +name, which is worse than not locking it at all. Once `NERVE_WORKSPACE` is set, +`workspace:` in `config.yaml` is ignored. + +Two things to know about `NERVE_LOCKDOWN`: + +- **It can only lock, never unlock.** The instance is locked if the variable says so + *or* the tracked `settings.yaml` does. `NERVE_LOCKDOWN=false`, empty, and unset + all mean the environment expresses no opinion; none of them force an unlock. So no + file arriving later can undo it, and equally you cannot use the environment to + escape a locked tracked config — unlocking takes a merged change. A value that is + neither true nor false is refused at startup rather than read as no opinion. +- **It is the same switch as `lockdown: ${NERVE_LOCKDOWN}`.** With the variable set + you need not mention `lockdown` in the tracked file at all, and a fleet repo that + already writes `${NERVE_LOCKDOWN:-false}` gets the same protection, along with the + `NERVE_WORKSPACE` requirement on the boxes where it resolves true. + +### When locked + +- **Config is remote-only.** Only `workspace/config/` and `${ENV_VAR}` are used. + The machine-local `config.yaml` and `config.local.yaml` and the legacy + `~/.nerve/cron` are ignored, and secrets come from the environment. +- **The lever is remote-owned.** Lockdown is read only from the tracked + `settings.yaml`, so a local edit to `config.yaml` or `config.local.yaml` cannot + unlock the instance or fake-lock it. With `NERVE_WORKSPACE` set, neither can + repointing `workspace:`. +- **Runtime edits to tracked config are blocked.** Creating, updating, deleting or + toggling a skill, Telegram pairing, writing a workspace file that lands inside + `config/`, and other config mutations fail with a "locked" error (HTTP 403). + Change config by opening a PR against the workspace repo and letting sync apply + the merge. Files elsewhere in the workspace are unaffected: the workspace is also + the agent's working directory, and lockdown covers tracked config rather than the + whole box. +- **The agent's own `Write`/`Edit` are refused inside `config/`.** Every + non-interactive tool is otherwise auto-approved, so without this the ordinary way + an agent edits a file would never meet the guards above. The refusal names the PR + flow, so a capable agent routes to it instead of retrying. Writes elsewhere + (memory, skills content, task files) are unaffected. `Bash` is not covered; see + below. +- **The tracked config subtree must really be in the workspace.** + `/config` has to resolve inside ``. If it is a symlink out, + nothing under it is part of the reviewed repo, `settings.yaml` included, and the + instance refuses to start. Where the workspace itself lives stays a machine-local + decision, symlink included. +- **Cron cannot be pointed out of the workspace.** `cron.jobs_file`, + `cron.system_file` and `cron.gate_plugins_dir` must resolve inside + `/config/`. One that does not is ignored, with a warning, in favour of + the in-workspace default. `..`, absolute paths and symlinks out of the tree are + all caught, because the resolved path is what is checked. This matters most for + `gate_plugins_dir`, whose `.py` files the daemon imports: without the check, a + pure-YAML change to a reviewed file would be enough to get on-disk code executed. + When the in-workspace default is itself what escapes (`config/cron`, or + `config/cron/gates` committed as a symlink, which needs no config key at all) + there is nothing contained left to fall back to and the instance refuses to + start. +- **Sync is stricter about local files.** [Workspace + sync](#git-backed-workspace-sync) already refuses to merge into a config subtree + with local changes; on a locked instance a `.gitignore`d file in there is a + refusal too rather than a warning. An ignored `config/cron/gates/*.py` is local + code the daemon runs, invisible to both the reviewer and the validator. + +Run `nerve config validate --workspace .` before merging; it validates the locked +view when `lockdown: true`, which is where the checks above are decided. It also +names the machine-local keys the bundle strands, so a key that quietly falls back +to its default on every locked box is visible in the PR rather than at boot. + +**If the flag comes from the environment, add `--assume-lockdown` in CI.** With +`lockdown: ${NERVE_LOCKDOWN:-false}` the validator resolves false wherever the +variable is unset, which is every CI runner, so it checks the unlocked view and +none of the checks above have anything to fire on — the only machine to find out is +the locked one, at boot. `--assume-lockdown` forces the locked view. A plain run +warns when it sees an env-controlled flag resolve to false, so the gap is at least +visible. + +**Flipping the flag needs the config reloaded.** A sync does that, and so does +`POST /api/config/reload`. The write guards, gateway authentication and the sync +loop pick the change up immediately; cron jobs and gates already built follow on the +next cron reload, and anything captured at startup needs a restart. A hand edit on +the box refreshes nothing, and under lockdown it is ignored anyway. If a sync merges +a config the daemon then cannot load, it reports `ok: false` with `apply_error` +rather than claiming the change took. + +### What lockdown does not cover + +Lockdown is a config-integrity control, not a sandbox and not a permission system. + +**The agent can still reach the files through the shell.** `Bash` is auto-approved +like every other non-interactive tool, so refusing `Write` and `Edit` closes the +path an agent takes by default and by accident, not the one it takes deliberately. +Nerve does not filter command strings — `sh -c`, redirection, `python -c` and every +editor defeat such a filter, and a filter that looks like a boundary without being +one is worse than a documented gap. If your threat model includes a deliberately +hostile agent, run the daemon as a user that cannot write the workspace repo, or +mount the config subtree read-only. + +**The codex backend is weaker still.** Codex's sandbox is a mode, not a path list, +so "everything except `config/`" cannot be expressed to it. Nerve declines a codex +file change landing in the tracked subtree, but only through codex's approval +request, and the shipped defaults (`sandbox: danger-full-access`, +`approval_policy: never`) never ask. Treat the `Write`/`Edit` protection above as +absent there unless you have configured approvals. + +**Operator CLI commands are out of scope.** `nerve init`, `nerve migrate` and +`nerve restore` write config without consulting the flag: they are how an operator +sets a box up or repairs one, and anyone who can run them can already edit the +files directly. + ## Migrating an Existing Install Installs from before the workspace-config layout are migrated automatically and @@ -336,6 +520,7 @@ from any working directory: |-----|------|---------|-------------| | `workspace` | path | `~/nerve-workspace` | Path to workspace directory | | `timezone` | string | `America/New_York` | Local timezone for scheduling | +| `lockdown` | bool | `false` | [Remote-only, read-only mode](#lockdown-remote-only-read-only). Honored only in `workspace/config/settings.yaml`; an unreadable value is an error, not a default. | | `deployment` | string | `server` | `server` (bare metal) or `docker`. Set during `nerve init`; determines whether CLI commands run directly or proxy to `docker compose`. | > **Note:** The _mode_ (personal vs worker) is not a config field — it's determined at `nerve init` time and expressed through which workspace templates, cron jobs, and memory categories are active. There's no `mode` key in config. diff --git a/docs/cron.md b/docs/cron.md index eaa3b8cc..8ec49a15 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -18,7 +18,11 @@ git-syncable workspace subtree): > `/config/cron/`. Installs that still have the legacy > `~/.nerve/cron/` keep working: if the workspace location doesn't exist yet, > Nerve reads from `~/.nerve/cron/` automatically. You can also pin the paths -> explicitly with `cron.jobs_file` / `cron.system_file` / `cron.gate_plugins_dir`. +> explicitly with `cron.jobs_file` / `cron.system_file` / `cron.gate_plugins_dir` +> — except under [lockdown](config.md#lockdown-remote-only-read-only), where all +> three must resolve inside `/config/`. An escaping path is ignored in +> favour of the in-workspace default; if the default is what escapes (a symlinked +> `config/cron` or `config/cron/gates`) the instance refuses to start. Both files use the same format. On startup, CronService loads and merges both: - If a job ID appears in both files, the **user version wins** (with a warning in the log). diff --git a/nerve/agent/backends/claude.py b/nerve/agent/backends/claude.py index a451e8ae..61aa05ee 100644 --- a/nerve/agent/backends/claude.py +++ b/nerve/agent/backends/claude.py @@ -216,6 +216,42 @@ def _translate_tool_result( ) +def lockdown_denial(tool_name: str, tool_input: dict[str, Any]) -> str | None: + """Why a locked instance refuses this file-writing tool call, if it does. + + A locked instance promises that the config it runs came from a reviewed, + merged change. The write guards on the REST and MCP surfaces enforce that for + callers coming in over the network; the agent's own ``Write``/``Edit`` did not + go through any of them, and every non-interactive tool here is auto-approved. + This is where that gap closes. + + **Scope, precisely.** It covers the tools that name a path — ``Write``, + ``Edit``, ``NotebookEdit`` — and nothing else. It is emphatically not a + sandbox: ``Bash`` is auto-approved on the same code path, so an agent that + means to write ``/config/`` can still do it through a shell. That + is a known and documented limitation. Filtering command strings is not the + answer — quoting, ``sh -c``, redirection, ``python -c``, ``tee`` and every + editor defeat it — and a filter that looks like a boundary without being one + is worse than a gap someone can read about. Real sandboxing is separate work. + + What this does buy is that the ordinary way an agent edits a file no longer + silently rewrites the config the box is promising to run, and that the refusal + says what to do instead. + """ + if tool_name not in FILE_MODIFY_TOOLS: + return None + path = tool_input.get("file_path") or tool_input.get("notebook_path") + if not path: + return None + from nerve.config import tracked_config_write_refusal + + try: + return tracked_config_write_refusal(path) + except Exception as e: # noqa: BLE001 — a broken guard must not kill the turn + logger.warning("lockdown write check failed for %r: %s", path, e) + return None + + # ------------------------------------------------------------------ # # can_use_tool adapter # # ------------------------------------------------------------------ # @@ -241,6 +277,18 @@ async def can_use_tool( context: ToolPermissionContext, ) -> PermissionResult: hub = self._hub + # Checked before the snapshot: a refused write has nothing to snapshot, + # and marking it snapshotted would suppress the snapshot of a later, + # allowed write to the same path. + denial = lockdown_denial(tool_name, tool_input) + if denial: + logger.info( + "Session %s: denying %s under lockdown (%s)", + hub.session_id, tool_name, + tool_input.get("file_path") or tool_input.get("notebook_path"), + ) + return PermissionResultDeny(message=denial) + # Capture pre-execution file snapshot for diff tracking # (also done via PreToolUse hook in the backend as primary path). if hub.snapshot_fn and tool_name in FILE_MODIFY_TOOLS: @@ -712,10 +760,27 @@ async def _grant_permission_hook(hook_input, tool_use_id, context): approved via ``can_use_tool``, which never fires for them, so the exclusion protected nothing. The image validator still runs for Read and its deny wins over this allow. + + The lockdown check is repeated here rather than left to + ``can_use_tool``: for a background sub-agent this hook is the only + thing that runs, so the allow issued here is the entire decision. + It only ever refuses the path-naming write tools, so the Read + parity above is unaffected by it. """ tool_name = hook_input.get("tool_name", "") if tool_name in INTERACTIVE_TOOLS: return {"hookSpecificOutput": {"hookEventName": "PreToolUse"}} + denial = lockdown_denial( + tool_name, hook_input.get("tool_input", {}) or {}, + ) + if denial: + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": denial, + } + } return { "hookSpecificOutput": { "hookEventName": "PreToolUse", diff --git a/nerve/agent/backends/codex/backend.py b/nerve/agent/backends/codex/backend.py index 87543766..3d502c0e 100644 --- a/nerve/agent/backends/codex/backend.py +++ b/nerve/agent/backends/codex/backend.py @@ -1479,9 +1479,12 @@ async def _handle_server_request(self, method: str, params: dict) -> dict: "command_approval", self._approval_payload(params), ) if method == "item/fileChange/requestApproval": - return await self._approval( - "file_approval", self._approval_payload(params), - ) + payload = self._approval_payload(params) + refusal = self._lockdown_refusal(payload) + if refusal: + logger.warning("codex: declining file change — %s", refusal) + return {"decision": "decline"} + return await self._approval("file_approval", payload) if method == "item/permissions/requestApproval": # The response type requires a constructed GrantedPermissionProfile # — there is no decline variant to express. Unsupported in v1 @@ -1507,6 +1510,39 @@ async def _handle_server_request(self, method: str, params: dict) -> dict: logger.warning("codex: unknown server request %s — empty reply", method) return {} + def _lockdown_refusal(self, payload: dict) -> str | None: + """Why a locked instance declines this file change, if it does. + + **This is a partial control and the shape of the gap matters.** Codex's + sandbox is a mode — ``read-only`` / ``workspace-write`` / + ``danger-full-access`` — not a path list, so there is no way to express + "everything except ``/config/``" to the app-server. The one + in-band hook is this approval request, and it only fires when + ``approval_policy`` asks for approvals; nerve ships ``never`` with + ``danger-full-access``, so under the default configuration codex never + asks and this never runs. Wired up anyway, because an operator who does + run codex with approvals should get the same answer the Claude backend + gives, and because a control that exists is easier to reach for than one + that has to be invented later. Documented as a gap rather than presented + as a boundary. + """ + from nerve.config import tracked_config_write_refusal + + item = payload.get("item") + changes = self._changes(item) if isinstance(item, dict) else [] + for change in changes: + path = str(change.get("path") or "") + if not path: + continue + try: + refusal = tracked_config_write_refusal(path) + except Exception as e: # noqa: BLE001 — never break the turn + logger.warning("lockdown write check failed for %r: %s", path, e) + continue + if refusal: + return refusal + return None + def _approval_payload(self, params: dict) -> dict: """Attach the originating item's context (the raw request carries only ids/reason — the UI wants the command / changed files).""" diff --git a/nerve/agent/tools/handlers/tasks.py b/nerve/agent/tools/handlers/tasks.py index cc7eea23..5990640a 100644 --- a/nerve/agent/tools/handlers/tasks.py +++ b/nerve/agent/tools/handlers/tasks.py @@ -30,6 +30,7 @@ TASK_UPDATE_SCHEMA, TASK_WRITE_SCHEMA, ) +from nerve.config import ensure_path_not_tracked_config from nerve.db.task_statuses import ( DEFAULT_STATUS, STATUS_NAME_RE, @@ -315,6 +316,7 @@ async def task_update_handler(ctx: ToolContext, args: dict) -> ToolResult: if ctx.workspace and (note or deadline or raw_tags or new_title): file_path = ctx.workspace / task["file_path"] + ensure_path_not_tracked_config(file_path, "write") if file_path.exists(): content = await asyncio.to_thread( file_path.read_text, encoding="utf-8", @@ -371,6 +373,11 @@ async def task_read_handler(ctx: ToolContext, args: dict) -> ToolResult: if ctx.workspace: file_path = ctx.workspace / task["file_path"] + # Deliberately unguarded. Lockdown makes the tracked config subtree + # unwritable, not unreadable — it arrives from a repo the agent can + # already read, and the HTTP GET for a task doesn't guard either. + # Guarding here would refuse a read with a "Cannot write" message + # the caller has no way to act on. if file_path.exists(): content = await asyncio.to_thread( file_path.read_text, encoding="utf-8", @@ -404,6 +411,7 @@ async def task_write_handler(ctx: ToolContext, args: dict) -> ToolResult: return ToolResult.text("Workspace not configured.") file_path = ctx.workspace / task["file_path"] + ensure_path_not_tracked_config(file_path, "write") await asyncio.to_thread(file_path.write_text, new_content, encoding="utf-8") from nerve.tasks.models import ( @@ -441,6 +449,13 @@ async def task_done_handler(ctx: ToolContext, args: dict) -> ToolResult: if not task: return ToolResult.text(f"Task not found: {task_id}") + # Done is a write like any other — it copies the file into done/ and + # unlinks the source, so a stored ``file_path`` inside the tracked config + # subtree would delete config. Refuse before the status flip, or a + # refusal leaves a task marked done whose file never moved. + if ctx.workspace: + ensure_path_not_tracked_config(ctx.workspace / task["file_path"], "move") + await ctx.db.update_task_status(task_id, "done") # Mark any implementing plans for this task as done diff --git a/nerve/backup.py b/nerve/backup.py index 9311adce..60fc6829 100644 --- a/nerve/backup.py +++ b/nerve/backup.py @@ -107,6 +107,18 @@ WORKSPACE_INCLUDE_DIRS: tuple[str, ...] = ("config", "memory", "scripts", "skills") WORKSPACE_INCLUDE_FILE_GLOBS: tuple[str, ...] = ("*.md",) +# What restore may write back, which is deliberately *not* what backup collects. +# The asymmetry is the point, and it is one directory wide: `config` is captured +# so a lost machine loses nothing, and never written back, because the authority +# for tracked config is the git remote rather than a tarball. Writing it back +# would replace reviewed config on a locked instance, and on any instance leave +# the checkout dirty enough that the next sync refuses to merge. +# +# These must stay separate constants. Defining the restore side as "whatever +# backup collects" is what silently opened that hole the moment `config` was +# added above — one edit, two opposite meanings. +WORKSPACE_RESTORE_DIRS: tuple[str, ...] = ("memory", "scripts", "skills") + # Directory names always pruned while walking the included workspace dirs. _PRUNE_DIR_NAMES: frozenset[str] = frozenset({ ".git", "node_modules", ".venv", "venv", "__pycache__", @@ -117,6 +129,30 @@ # missed something (e.g. a build artifact landed under memory/). WORKSPACE_WARN_BYTES = 2 * 1024 * 1024 * 1024 # 2 GB + +def _restorable_workspace_path(rel: Path) -> bool: + """Whether a workspace path from a bundle may be written back on restore. + + Restore applies :data:`WORKSPACE_RESTORE_DIRS`, not the collect-side + allowlist. Bundles this module writes *do* carry ``config/`` — deliberately, + so a lost machine loses no settings or schedule — and a bundle it is handed + can carry anything at all. Either way, writing ``config/settings.yaml`` or + ``config/cron/gates/x.py`` back would let a tarball replace the git-tracked + config subtree: on a locked instance that displaces the reviewed remote + config the whole mode exists to guarantee, and on any instance it leaves the + checkout dirty enough that the next sync refuses to merge. + + So the config subtree travels in the bundle and is never written back. It is + still there to be read out and applied through review if a restore really + needs it, which the skip warning says. + """ + import fnmatch + + parts = rel.parts + if len(parts) == 1: + return any(fnmatch.fnmatch(parts[0], g) for g in WORKSPACE_INCLUDE_FILE_GLOBS) + return parts[0] in WORKSPACE_RESTORE_DIRS + # Retention prune only ever touches files matching this exact pattern, so it # can never delete an unrelated file that happens to share the directory. # An optional ``-`` disambiguates bundles created within the same second. @@ -876,13 +912,33 @@ def restore_bundle( staged_ws = staging / "workspace" if staged_ws.is_dir(): workspace.mkdir(parents=True, exist_ok=True) + refused: list[str] = [] for src in sorted(staged_ws.rglob("*")): if not src.is_file(): continue rel = src.relative_to(staged_ws) + if not _restorable_workspace_path(rel): + refused.append(str(rel)) + continue dst = workspace / rel dst.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(src, dst) + if refused: + logger.warning( + "Restore: skipped %d workspace file(s) not restorable (%s): " + "%s. The config subtree is intentionally among these — it " + "travels in the bundle but is never written back, because " + "tracked config comes from the git remote. Read it out of " + "the bundle and apply it through review if you need it.", + len(refused), ", ".join(WORKSPACE_RESTORE_DIRS), + ", ".join(refused[:10]), + ) + report.warnings.append( + f"skipped {len(refused)} workspace file(s) this bundle " + f"carries that restore does not write back (the config " + f"subtree is deliberately one of them) — including " + f"{refused[0]!r}" + ) logger.info("Restore complete: %s → %s", path.name, nerve_dir) return report diff --git a/nerve/channels/telegram.py b/nerve/channels/telegram.py index 81d66fae..e58473cd 100644 --- a/nerve/channels/telegram.py +++ b/nerve/channels/telegram.py @@ -1023,10 +1023,29 @@ async def _handle_pair(self, update: Update, context: Any) -> None: ) return + # Refuse to pair under lockdown — allowed users must come from the + # tracked remote config, not a local runtime edit. Check BEFORE the + # in-memory add so lockdown isn't bypassed for the current run. + from nerve.config import LockdownError, is_locked + + if is_locked(): + await update.message.reply_text( + "This instance is locked (remote-only). Add your Telegram user " + "to telegram.allowed_users in the workspace config repo instead." + ) + return + # Success: authorize in memory and persist to config.local.yaml self._allowed_users.add(user_id) try: append_telegram_allowed_user(self.config.config_dir, user_id) + except LockdownError: + # Belt-and-suspenders: config was locked between the check and here. + self._allowed_users.discard(user_id) + await update.message.reply_text( + "This instance is locked (remote-only) — pairing is disabled." + ) + return except Exception: logger.exception("Paired user %d but failed to persist to config", user_id) await update.message.reply_text( diff --git a/nerve/cli.py b/nerve/cli.py index 336b1d04..1e43451a 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -955,6 +955,19 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s lines.append("[--] Anthropic API key not set (using proxy)") elif config.anthropic_api_key: lines.append(f"[OK] Anthropic API key: ...{config.anthropic_api_key[-4:]}") + elif config.lockdown: + # The generic message names config.local.yaml, which a locked instance + # never reads. The cause is also often upstream of the key: if the + # provider block was only in config.yaml, this instance has already + # reverted to the Anthropic default and is failing for a key it would not + # otherwise need. + errors.append( + "[ERR] No Anthropic API key. Lockdown does not read " + "config.local.yaml, so supply it as ${ANTHROPIC_API_KEY} in " + "workspace/config/settings.yaml. If this box should be using " + "Bedrock, provider.type is absent from the tracked settings and this " + "instance has fallen back to 'anthropic'" + ) else: errors.append("[ERR] Anthropic API key not set and proxy not enabled (config.local.yaml)") @@ -1239,6 +1252,7 @@ def config_sync( config.workspace, ctx.obj["config_dir"], branch=branch, validate=not no_validate, strict_env=config.workspace_sync.strict_env and not no_strict_env, + locked=config.lockdown, ) for warning in result.validation_warnings: click.secho(f" [WARN] {warning}", fg="yellow") @@ -1273,10 +1287,17 @@ def config_sync( help="Ignore this machine's config.yaml / config.local.yaml and validate " "only the portable workspace config — what a shared repo carries.", ) +@click.option( + "--assume-lockdown", "assume_locked", is_flag=True, + help="Validate the locked view whatever this bundle's lockdown flag resolves " + "to here. A fleet repo writes `lockdown: ${NERVE_LOCKDOWN:-false}`, so " + "CI resolves it to false and checks a config no locked box will run. Use " + "this in CI on any repo one of whose instances is locked.", +) @click.pass_context def config_validate( ctx: click.Context, workspace: str | None, strict_env: bool, strict_keys: bool, - portable_only: bool, + portable_only: bool, assume_locked: bool, ) -> None: """Validate the configuration bundle. Non-zero exit on any error (CI-ready).""" from nerve.config_validate import validate_config_bundle @@ -1285,7 +1306,7 @@ def config_validate( result = validate_config_bundle( config_dir, workspace_override=workspace, strict_env=strict_env, strict_keys=strict_keys, - portable_only=portable_only, + portable_only=portable_only, assume_locked=assume_locked, ) for msg in result.info: click.echo(f"[info] {msg}") diff --git a/nerve/config.py b/nerve/config.py index 99661e98..c5edd663 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -15,6 +15,8 @@ from typing import Any from nerve import paths +from nerve.coerce import FALSY, TRUTHY +from nerve.coerce import as_bool as _as_bool from nerve.coerce import coerced as _coerced from nerve.coerce import lenient_int as _lenient_int @@ -240,7 +242,11 @@ def _read_yaml_mapping(path: Path, *, strict: bool = False) -> dict[str, Any]: except yaml.YAMLError as e: raise ConfigError(f"Failed to parse {path}: {e}") from e except (OSError, UnicodeDecodeError) as e: - raise ConfigError(f"Failed to read {path}: {e}") from e + # A file that exists but cannot be read — mode 000, a directory in its + # place, a dead mount — or one whose bytes are not the encoding we pinned. + # Same promise as a parse failure: the callers that route around a broken + # config report it, they don't show a traceback. + raise ConfigError(f"Cannot read {path}: {e}") from e if data is None: return {} if not isinstance(data, dict): @@ -285,6 +291,196 @@ def _load_workspace_settings(workspace: Path) -> dict[str, Any]: return data +# The literal an unresolved ``${VAR}`` reference leaves behind. Interpolation is +# a string substitution, so a reference nothing resolved is still sitting in the +# value — which is how a "present" secret can turn out to be nothing of the kind. +_UNRESOLVED_REF = "${" + +# The two variables that anchor lockdown outside every file on the box. Set in +# the service definition (systemd ``Environment=``, docker ``-e``), which is the +# one place neither the agent nor a config edit reaches. +LOCKDOWN_ANCHOR_ENV = "NERVE_LOCKDOWN" +WORKSPACE_ANCHOR_ENV = "NERVE_WORKSPACE" + + +def lockdown_anchor() -> bool: + """Whether the environment forces this instance into lockdown. + + Hardening the flag's *value* does nothing about which file supplies it, and + that file is chosen by ``workspace:`` in the machine-local ``config.yaml``. + One local edit repoints the workspace at a tree whose ``settings.yaml`` says + nothing, lockdown evaluates to false, the machine layers come back, and with + them ``auth.jwt_secret``. Every guarantee in this module rests on a file the + thing being guarded against can rewrite. The anchor is the way out: an + environment variable set by whatever starts the daemon is outside the config + tree, outside the workspace, and outside anything the agent can edit. + + **The anchor can only lock.** The effective state is this OR the tracked + flag, so ``NERVE_LOCKDOWN=false`` and an unset variable both mean "the + environment has no opinion", never "force unlocked". Monotonicity is the + property worth having: once an operator has anchored a box, nothing that + arrives in a file can take it back — including a file the agent wrote. It + also means the anchor cannot be used to *escape* a locked tracked config; + unlocking still takes a reviewed, merged change. + + An empty value is "no opinion" rather than an error, unlike an empty + ``lockdown:`` in the tracked file. ``Environment=NERVE_LOCKDOWN=`` and a + bare ``docker -e NERVE_LOCKDOWN`` both produce it, and under the OR above it + can only ever fail to *add* a restriction — the tracked flag still decides. + An unreadable value is refused, because the only other reading is "no + opinion", which would silently discard an operator's intent to lock. + """ + raw = os.environ.get(LOCKDOWN_ANCHOR_ENV) + if raw is None: + return False + text = raw.strip().lower() + if not text or text in FALSY: + return False + if text in TRUTHY: + return True + raise ConfigError( + f"{LOCKDOWN_ANCHOR_ENV}={raw!r} is neither true nor false. Accepted " + f"spellings are true/false, 1/0, yes/no, on/off (case-insensitive); " + f"unset or empty means the environment has no opinion and the tracked " + f"settings decide. Refused rather than assumed, because assuming would " + f"mean discarding an instruction to lock this instance." + ) + + +def _anchored_workspace() -> Path: + """The workspace an env-anchored instance must use. + + Anchoring the flag alone buys nothing. ``workspace:`` still comes from the + machine-local ``config.yaml``, so an attacker who can write that file + repoints the workspace and gets an instance that is locked *onto config they + supplied* — strictly worse than an unlocked one, because it now treats that + tree as its sole authority and stops reading anything else. The anchor has + to cover both, so the two variables are a single deployment unit: whatever + sets ``NERVE_LOCKDOWN`` sets ``NERVE_WORKSPACE`` beside it. + + Refused rather than defaulted. Falling back to ``~/nerve-workspace`` would + quietly re-admit a path that is a machine-local decision, and defaulting is + how an operator ends up believing an anchor holds when it does not. + """ + raw = os.environ.get(WORKSPACE_ANCHOR_ENV, "").strip() + if not raw: + raise ConfigError( + f"{LOCKDOWN_ANCHOR_ENV} anchors this instance in lockdown, but " + f"{WORKSPACE_ANCHOR_ENV} is not set. Anchoring the flag alone is not " + f"an anchor: the workspace — which selects the settings.yaml that is " + f"then the only source of truth — would still come from the " + f"machine-local config.yaml. Set both in the service definition " + f"(systemd Environment=, docker -e)." + ) + workspace = _expand_path(raw) + if workspace is None or not workspace.is_absolute(): + raise ConfigError( + f"{WORKSPACE_ANCHOR_ENV}={raw!r} must be an absolute path — a " + f"relative one means a different directory in every process that " + f"reads it." + ) + return workspace + + +def _as_lockdown(value: Any) -> bool: + """Parse the ``lockdown`` flag, refusing anything that isn't a plain yes/no. + + Every other boolean in this module is coerced *leniently*: a value nobody can + parse falls back to the field's declared default, so one typo in a section + that isn't even switched on cannot stop the daemon from booting. That trade + is the wrong way round here, because this field's default is ``False``, and + ``False`` is the position in which nothing else is protected — the + machine-local layers come back, the legacy cron directory comes back, and + runtime writes to tracked config are allowed again. Guessing "off" from a + value we could not read would quietly undo every guarantee the flag exists to + make, and it would do it on a box nobody is watching. So an unreadable value + is a hard :class:`ConfigError` instead: the instance does not start, and + ``nerve config validate`` reports it before the change is ever merged. + + ``None`` (a bare ``lockdown:`` in YAML, or the key absent) is off — that is + the file declining to set the flag, which is what the default is for. An + *empty string* is not: it arrives from an environment variable that exists + and carries no value, and while blanking a variable is a perfectly good way + to switch an ordinary feature off, the one switch that governs whether + anything else is enforced must not be flipped by a value that says nothing. + Spell a default-unlocked box ``${VAR:-false}``. + """ + if isinstance(value, bool): + return value + if value is None: + return False + shown = repr(value) + if isinstance(value, str): + # The flag decides which layers get merged, so it is read before the + # single post-merge interpolation pass and has to resolve its own + # reference — otherwise `lockdown: ${NERVE_LOCKDOWN}` is judged as the + # literal string it still is, which is truthy whatever the variable says. + resolved = _interpolate_str(value, []).strip() + if resolved.lower() in TRUTHY: + return True + if resolved.lower() in FALSY: + return False + if resolved != value.strip(): + shown = f"{value!r} (which resolved to {resolved!r})" + if _UNRESOLVED_REF in resolved: + raise ConfigError( + f"lockdown: {shown} names an environment variable that is not " + f"set, so whether this instance is locked cannot be determined. " + f"Set it, or write ${{VAR:-false}} to declare the unlocked " + f"default explicitly." + ) + raise ConfigError( + f"lockdown must be true or false, got {shown}. Accepted spellings are " + f"true/false, 1/0, yes/no, on/off (case-insensitive); an empty value is " + f"not one of them. Refused rather than assumed, because assuming would " + f"mean assuming 'unlocked' — the setting that drops every restriction " + f"lockdown exists to impose." + ) + + +def _resolved(path: Path) -> Path: + """``path`` with symlinks followed; the path itself if it cannot be resolved.""" + try: + return path.resolve() + except (OSError, RuntimeError, ValueError): + return path + + +def _is_within(path: Path, root: Path) -> bool: + """True if ``path`` resolves inside ``root``, symlinks followed. + + Both sides are resolved, so ``..`` segments, an absolute path elsewhere, a + symlink pointing out of the tree, and a path inside the workspace but + outside its ``config/`` subtree all answer False. An unreadable path (a + symlink loop, a permission refusal partway up) answers False too: not being + able to establish that something is contained is not containment. + + Note what resolving ``root`` too means: containment is judged *relative to + wherever root actually is*. If ``root`` is itself a symlink out of the tree, + everything under it is contained and this answers True — correctly, for the + question it is asked. Whether ``root`` is the tree it claims to be is a + separate question, and :func:`lockdown_workspace_problems` is where it is + asked. + """ + try: + return path.resolve().is_relative_to(root.resolve()) + except (OSError, RuntimeError, ValueError): + return False + + +def read_machine_layers(config_dir: Path) -> dict[str, Any]: + """The machine-local overlay on its own, for diagnostics. + + Lockdown discards these layers, so reporting what was discarded needs a way + to read them afterwards. Two small YAML reads, not memoized: a caller asking + for this wants the current contents of the files. + """ + return _deep_merge( + _read_yaml_mapping(config_dir / "config.yaml"), + _read_yaml_mapping(config_dir / "config.local.yaml"), + ) + + def _read_config_sources(config_dir: Path) -> dict[str, Any]: """Merge all config layers for ``config_dir`` and resolve ${ENV_VAR} refs. @@ -294,22 +490,57 @@ def _read_config_sources(config_dir: Path) -> dict[str, Any]: base = _read_yaml_mapping(config_dir / "config.yaml") local = _read_yaml_mapping(config_dir / "config.local.yaml") - # The workspace path comes from the machine-local config (or the default), - # never from the tracked settings file — resolve it first. Best-effort - # ${VAR}/${VAR:-default} interpolation is applied so an env-based workspace - # path is honored when locating settings.yaml; an unresolved required ref is - # left intact here and surfaces later in the full _resolve_env_refs pass. machine = _deep_merge(base, local) - ws_raw = machine.get("workspace") - if isinstance(ws_raw, str) and "${" in ws_raw: - ws_raw = _interpolate_str(ws_raw, []) - workspace = _expand_path(ws_raw) or paths.default_workspace() + # An env-anchored instance takes its workspace from the environment too, so + # that the file which decides everything else cannot decide *which* file that + # is. Without the anchor nothing changes: the workspace comes from the + # machine-local config, which is where it has always come from and which is + # the right place for a per-machine path. + anchored = lockdown_anchor() + if anchored: + workspace = _anchored_workspace() + else: + # The workspace path comes from the machine-local config (or the + # default), never from the tracked settings file — resolve it first. + # Best-effort ${VAR}/${VAR:-default} interpolation is applied so an + # env-based workspace path is honored when locating settings.yaml; an + # unresolved required ref is left intact here and surfaces later in the + # full _resolve_env_refs pass. + ws_raw = machine.get("workspace") + if isinstance(ws_raw, str) and "${" in ws_raw: + ws_raw = _interpolate_str(ws_raw, []) + workspace = _expand_path(ws_raw) or paths.default_workspace() ws_settings = _load_workspace_settings(workspace) - # Precedence, lowest first: workspace settings < config.yaml < config.local.yaml - merged = _deep_merge(ws_settings, base) - merged = _deep_merge(merged, local) + # Lockdown is owned by the *tracked* settings file only, so a local edit to + # config.yaml/config.local.yaml can't unlock (or fake-lock) an instance — the + # remote is the authority. When locked, the machine layers are dropped + # entirely: config comes only from workspace/config + ${ENV_VAR} (secrets from + # the environment), never from local drop-in files. + # OR, not override: the environment can add lockdown but never remove it. + # The tracked flag is still evaluated (and still refuses an unreadable value) + # so an anchored box doesn't quietly run a bundle nobody can parse. + locked = _as_lockdown(ws_settings.get("lockdown")) or anchored + if "lockdown" in machine: + # Mirrors the warning for a `workspace` key in settings.yaml. Ignoring it + # is the point of the feature, but ignoring it *in silence* leaves someone + # who wrote it in the wrong file believing the box is locked when nothing + # about it is, and no other signal would ever tell them. + logger.warning( + "config: ignoring 'lockdown' in config.yaml/config.local.yaml — the " + "flag is honored only in workspace/config/settings.yaml, so that a " + "local edit cannot unlock or fake-lock an instance. This instance is " + "%s.", "LOCKED" if locked else "not locked", + ) + if locked: + merged = dict(ws_settings) + merged["workspace"] = str(workspace) # keep the machine-local workspace path + else: + # Precedence, lowest first: settings < config.yaml < config.local.yaml + merged = _deep_merge(_deep_merge(ws_settings, base), local) + # Authoritative: the effective lockdown flag matches the resolution decision. + merged["lockdown"] = locked return _resolve_env_refs(merged) @@ -562,7 +793,7 @@ class TelegramConfig: @classmethod @_coerced - def from_dict(cls, d: dict) -> TelegramConfig: + def from_dict(cls, d: dict, locked: bool = False) -> TelegramConfig: dm_policy = d.get("dm_policy", "pairing") if dm_policy not in ("pairing", "open"): logger.warning( @@ -571,8 +802,27 @@ def from_dict(cls, d: dict) -> TelegramConfig: dm_policy, ) dm_policy = "pairing" + # Whether a given box answers Telegram DMs is a per-machine decision, so + # `nerve init` writes this key to the machine-local config.yaml. Lockdown + # drops that layer, which would leave the declared default — on — + # deciding for it: a box where Telegram was switched off would start + # serving DMs, with full agent access, the moment the shared settings + # carried a token this machine's environment can resolve. + # + # So the fallback direction is chosen by lockdown, and parsed here rather + # than left to @_coerced. The decorator falls back to the *declared* + # default, which is True, so `enabled: ${TG_ON:-nope}` — an env-reference + # spelling this file's own docs recommend for per-box fleet config — would + # turn the bot on everywhere one bad value reached. Deliberately still + # lenient rather than fatal: an unreadable value in one section must not + # stop the daemon booting, and off is the safe end of the guess. + enabled = ( + _as_bool(d["enabled"], not locked, label="TelegramConfig.enabled") + if "enabled" in d + else not locked + ) return cls( - enabled=d.get("enabled", True), + enabled=enabled, bot_token=d.get("bot_token", ""), # Deliberately uncast. The declared list[int] is converted after # construction, which logs an unconvertible entry and keeps it @@ -917,7 +1167,7 @@ def _cron_dir_has_jobs(d: Path) -> bool: return (d / "jobs.yaml").exists() or (d / "system.yaml").exists() -def _resolve_cron_dir(workspace: Path | None) -> Path: +def _resolve_cron_dir(workspace: Path | None, locked: bool = False) -> Path: """Effective directory holding cron config (jobs/system/gates). Prefers the git-syncable ``workspace/config/cron`` so cron definitions live @@ -929,11 +1179,16 @@ def _resolve_cron_dir(workspace: Path | None) -> Path: * If the workspace location has job files → use it. * Else, if the legacy location has job files → use it (un-migrated install). * Else → the workspace location (new install; may be about to be populated). + + Under ``locked`` (lockdown), the legacy ``~/.nerve/cron`` fallback is disabled + entirely — cron config comes only from the workspace. """ legacy = paths.cron_dir() if workspace is None: return legacy ws_cron = workspace_config_dir(workspace) / "cron" + if locked: + return ws_cron if _cron_dir_has_jobs(ws_cron): return ws_cron if _cron_dir_has_jobs(legacy): @@ -941,6 +1196,49 @@ def _resolve_cron_dir(workspace: Path | None) -> Path: return ws_cron +def _locked_cron_path(configured: Path, default: Path, root: Path, key: str) -> Path: + """Keep one locked cron path inside the tracked config subtree. + + A locked instance runs only what the reviewed workspace repo carries, and + these three keys decide which files it reads — including, for + ``gate_plugins_dir``, which ``.py`` files it imports and *executes* at + start-up and on every cron reload. A tracked ``settings.yaml`` that pointed + any of them elsewhere would hand the box job definitions and code that never + went through review, using nothing but a YAML edit. So a path that escapes is + dropped in favour of the in-workspace default and reported. + + Called only when locked. An unmigrated install is entitled to point cron at + the legacy ``~/.nerve/cron``, and that is the whole reason the machine-local + layers exist. + + The default is verified too, and the instance refuses to load when it also + escapes. That is not a defensive afterthought — it is the case with no YAML + key in it at all. ``/config/cron/gates`` is a name a config repo + can carry as a *symlink*, and git tracks symlinks, so a reviewed and merged + bundle can point the default itself anywhere. Substituting the default there + would hand back the exact path just rejected, log a warning that contradicts + itself, and import whatever was on the other end. + """ + if _is_within(configured, root): + return configured + if _is_within(default, root): + logger.warning( + "cron.%s (%s) resolves outside %s — ignoring it and using %s " + "instead, because a locked instance takes its cron config, and its " + "gate plugins, only from the tracked workspace", + key, configured, root, default, + ) + return default + also_default = "" if configured == default else f" — as does the default {default}" + raise ConfigError( + f"lockdown: cron.{key} resolves outside the tracked config subtree " + f"{root}: {configured} points at {_resolved(configured)}{also_default}. " + f"A locked instance reads its cron config, and imports its gate plugins, " + f"only from the reviewed workspace, and there is no contained path left " + f"to fall back to." + ) + + @dataclass class CronConfig: # Bare defaults (no workspace context) point at the legacy machine-local @@ -953,12 +1251,33 @@ class CronConfig: @classmethod @_coerced - def from_dict(cls, d: dict, workspace: Path | None = None) -> CronConfig: - base = _resolve_cron_dir(workspace) + def from_dict(cls, d: dict, workspace: Path | None = None, locked: bool = False) -> CronConfig: + base = _resolve_cron_dir(workspace, locked=locked) + jobs_file = _expand_path(d.get("jobs_file")) or base / "jobs.yaml" + system_file = _expand_path(d.get("system_file")) or base / "system.yaml" + gate_plugins_dir = _expand_path(d.get("gate_plugins_dir")) or base / "gates" + if locked and workspace is not None: + root = workspace_config_dir(workspace) + if not _is_within(base, root): + # The subtree itself leaves the workspace — a symlinked + # config/cron, say. No substitution fixes that, since every + # default is derived from it, so refuse to run rather than read + # cron config from a directory outside the reviewed tree. + raise ConfigError( + f"lockdown: the cron directory {base} resolves outside the " + f"tracked config subtree {root}. A locked instance must read " + f"its cron config, and import its gate plugins, only from the " + f"reviewed workspace." + ) + jobs_file = _locked_cron_path(jobs_file, base / "jobs.yaml", root, "jobs_file") + system_file = _locked_cron_path(system_file, base / "system.yaml", root, "system_file") + gate_plugins_dir = _locked_cron_path( + gate_plugins_dir, base / "gates", root, "gate_plugins_dir", + ) return cls( - jobs_file=_expand_path(d.get("jobs_file")) or base / "jobs.yaml", - system_file=_expand_path(d.get("system_file")) or base / "system.yaml", - gate_plugins_dir=_expand_path(d.get("gate_plugins_dir")) or base / "gates", + jobs_file=jobs_file, + system_file=system_file, + gate_plugins_dir=gate_plugins_dir, ) @@ -1913,6 +2232,11 @@ def from_dict(cls, d: dict) -> "XmemoryConfig": class NerveConfig: workspace: Path = field(default_factory=paths.default_workspace) timezone: str = "America/New_York" + # Remote-only, read-only mode. When set (in workspace/config/settings.yaml — + # the tracked file the remote controls), config comes ONLY from the workspace + # + ${ENV_VAR}; machine config.yaml/config.local.yaml overrides and legacy + # ~/.nerve/cron are ignored, and runtime edits to tracked config are blocked. + lockdown: bool = False deployment: str = "server" # "server" or "docker" quiet_start: str = "02:00" # HH:MM — start of quiet period (local timezone) quiet_end: str = "08:00" # HH:MM — end of quiet period (local timezone) @@ -2116,19 +2440,25 @@ def _build_from_dict(cls, d: dict) -> NerveConfig: # Resolve the workspace once so workspace-aware sub-configs (e.g. cron, # which now lives in workspace/config/cron) can be located relative to it. workspace = _expand_path(d.get("workspace")) or paths.default_workspace() + # Parsed here rather than left to @_coerced, which would fall back to the + # field's default — i.e. to "unlocked" — for a value it cannot read. It is + # also needed *before* construction, because it changes how the cron paths + # and the Telegram switch resolve. See :func:`_as_lockdown`. + locked = _as_lockdown(d.get("lockdown")) return cls( workspace=workspace, timezone=d.get("timezone", "America/New_York"), + lockdown=locked, deployment=d.get("deployment", "server"), quiet_start=d.get("quiet_start", "02:00"), quiet_end=d.get("quiet_end", "08:00"), provider=ProviderConfig.from_dict(d.get("provider", {})), gateway=GatewayConfig.from_dict(d.get("gateway", {})), agent=AgentConfig.from_dict(d.get("agent", {})), - telegram=TelegramConfig.from_dict(d.get("telegram", {})), + telegram=TelegramConfig.from_dict(d.get("telegram", {}), locked=locked), sync=SyncConfig.from_dict(d.get("sync", {})), memory=MemoryConfig.from_dict(d.get("memory", {})), - cron=CronConfig.from_dict(d.get("cron", {}), workspace=workspace), + cron=CronConfig.from_dict(d.get("cron", {}), workspace=workspace, locked=locked), workspace_sync=WorkspaceSyncConfig.from_dict(d.get("workspace_sync", {})), backup=BackupConfig.from_dict(d.get("backup", {})), sessions=SessionsConfig.from_dict(d.get("sessions", {})), @@ -2271,9 +2601,130 @@ def load_config(config_dir: Path | None = None) -> NerveConfig: config = NerveConfig.from_dict(merged) config.config_dir = Path(config_dir) + problems = lockdown_workspace_problems(config.workspace) if config.lockdown else [] + for problem in problems: + # Fail closed: don't boot a locked instance that would take its "tracked" + # config from somewhere the config repo never saw. + raise ConfigError(problem) + for note in lockdown_machine_local_notes( + merged, read_machine_layers(config_dir) if config.lockdown else None + ): + logger.warning("config: %s", note) return config +def lockdown_workspace_problems(workspace: Path) -> list[str]: + """Ways a locked instance's tracked config tree isn't the tree it claims. + + Everything else here judges a path against ``/config``. That + settles whether a path stays inside the subtree, and says nothing about + whether the subtree is the one the config repo was reviewed in. If ``config`` + is a symlink out of the workspace, every path under it is contained relative + to itself, every containment check passes, and the whole tracked layer — + settings.yaml, the cron files, the gate plugins the daemon imports — comes + from an untracked directory somewhere else on the box, with lockdown itself + read from there and reporting that all is well. + + So the subtree is judged against the workspace instead, once, and a locked + instance that fails it does not start. The workspace *path* may still be + anything, including a symlink: where a machine keeps its workspace is a + machine-local decision and the one thing lockdown never took away. + """ + config_root = workspace_config_dir(workspace) + if _is_within(config_root, workspace): + return [] + return [ + f"lockdown is enabled but the tracked config subtree {config_root} " + f"resolves to {_resolved(config_root)}, outside the workspace " + f"{_resolved(workspace)} — so nothing it holds is part of the reviewed " + f"repo. A locked instance takes its config only from the workspace's own " + f"config/ directory." + ] + + +def _has_dotted(data: dict[str, Any], dotted: str) -> bool: + """Whether ``dotted`` names a key present in ``data``. + + Presence, not truthiness: ``enabled: false`` and ``accounts: []`` are stated + values, and reporting them as unstated would be wrong. + """ + node: Any = data + for part in dotted.split("."): + if not isinstance(node, dict) or part not in node: + return False + node = node[part] + return True + + +def lockdown_machine_local_notes( + merged: dict[str, Any], machine: dict[str, Any] | None = None +) -> list[str]: + """Machine-local settings a locked bundle leaves for the defaults to decide. + + Lockdown reads only the tracked settings, so a key whose natural home is + ``config.yaml`` has nowhere left to be said — and the declared default + silently answers for it on every box in the fleet. That is invisible in the + diff of the change that turned lockdown on, which is exactly when someone + could still do something about it, so name it while the config is being + loaded and while it is being validated. + + The per-box answer belongs in the tracked settings as an ``${ENV_VAR}`` + reference; that is the only channel a locked instance still has. + + ``machine`` is the dropped ``config.yaml``/``config.local.yaml`` overlay, + which callers still hold at the point lockdown discards it. Without it this + function can only report that a key is absent from the tracked settings, and + most machine-local keys are legitimately absent with an acceptable default — + so warning on all of them is noise. With it, the note fires only when a local + file states something that lockdown is about to ignore. + """ + if not _as_lockdown(merged.get("lockdown")): + return [] + notes: list[str] = [] + + if machine: + # The list of machine-local paths lives next to the wizard split it + # mirrors, and is guarded by tests in both directions. Imported inside the + # function because nerve.migrate imports this module. + from nerve.migrate import _is_machine_local, _leaf_paths + + # `workspace` is the one key lockdown keeps, so it is never dropped. + dropped = sorted( + path + for path in _leaf_paths(machine) + if path != "workspace" and not _has_dotted(merged, path) + ) + shareable = [p for p in dropped if not _is_machine_local(p)] + local_only = [p for p in dropped if _is_machine_local(p)] + if shareable: + notes.append( + f"{', '.join(shareable)} are set in config.yaml/config.local.yaml" + " and not in the tracked settings. Lockdown does not read those" + " files, so this instance is using the declared defaults." + " These values are shareable: move them to" + " workspace/config/settings.yaml." + ) + if local_only: + notes.append( + f"{', '.join(local_only)} are machine-local, so lockdown leaves" + " them at their declared defaults. If a default is not the right" + " answer for this box, state the key in the tracked settings as" + " a ${VAR} reference." + ) + + telegram = merged.get("telegram") + telegram = telegram if isinstance(telegram, dict) else {} + if telegram.get("bot_token") and "enabled" not in telegram: + notes.append( + "workspace/config/settings.yaml sets telegram.bot_token but not " + "telegram.enabled, and lockdown does not read config.yaml (where " + "`nerve init` puts it) — so the Telegram bot stays OFF on this " + "instance. Set telegram.enabled in the tracked settings, using " + "${VAR} if the answer differs per machine." + ) + return notes + + # --- Unknown-key validation --- # YAML keys that are intentionally not dataclass fields — keyed by dotted @@ -2348,7 +2799,11 @@ def append_telegram_allowed_user(config_dir: Path, user_id: int) -> bool: Used by the pairing flow. Reads, merges, and rewrites the local config (config.local.yaml is generated — comment loss is acceptable there). Returns True if the file was updated (False if the ID was already present). + + Blocked under lockdown: config.local.yaml isn't even loaded when locked, and + tracked config is remote-only — pair by updating the workspace repo instead. """ + ensure_not_locked("add a Telegram user") local_path = Path(config_dir) / "config.local.yaml" data: dict[str, Any] = {} if local_path.exists(): @@ -2392,3 +2847,91 @@ def set_config(config: NerveConfig) -> None: """Override the global config (for testing or CLI-driven loading).""" global _config _config = config + + +class LockdownError(RuntimeError): + """Raised when a runtime edit to tracked config is attempted under lockdown.""" + + +def is_locked() -> bool: + """True if the *loaded* config puts this instance in lockdown. + + A statement about the process, not about the box: with no config loaded there + is no lockdown state and this answers False. That is a fail-open, and it is + reachable — the CLI hands ``config=None`` to the commands that exist to repair + a config which failed to load, and never calls :func:`set_config` on that + path. Left as it is deliberately, because every caller of the guards below is + a server-side write path that runs with a loaded config, and making this read + files would put I/O behind a predicate called on every skill toggle to protect + a case none of them are in. + + What that does mean is that a guard cannot be *added* by calling this from a + command on the ``config=None`` path and expecting it to hold. Such a caller + has to be given the flag explicitly. + """ + return _config is not None and bool(_config.lockdown) + + +def ensure_not_locked(action: str = "modify configuration") -> None: + """Raise :class:`LockdownError` if locked. Call before any runtime edit to + tracked config (skills, cron, pairing, ...).""" + if is_locked(): + raise LockdownError( + f"Cannot {action}: this instance is in lockdown (remote-only, " + "read-only). Change config via a PR to the workspace repo instead." + ) + + +def ensure_path_not_tracked_config(path: Path, action: str = "write") -> None: + """Refuse, under lockdown, to touch a path inside the tracked config subtree. + + The other guards sit in front of a specific operation — creating a skill, + persisting a pairing — and know what they are about to change. This one is + for the endpoints that take a caller-supplied path anywhere under the + workspace: whether they are editing tracked config is a property of the + argument, not of the endpoint. Left unguarded, ``/config/`` is + reachable through them, which means both ``settings.yaml`` (including the + ``lockdown`` flag itself) and ``cron/gates/*.py``, which the daemon imports + and runs. Either one turns lockdown off from inside the box it is meant to be + protecting. + + A path outside the subtree is none of lockdown's business — the workspace is + also the agent's working directory, and normal files there are the point. + """ + reason = tracked_config_write_refusal(path) + if reason: + raise LockdownError(f"Cannot {action} {path}: {reason}") + + +def tracked_config_write_refusal(path: Path | str) -> str | None: + """Why a locked instance refuses to write ``path``, or ``None`` to allow it. + + The same rule as :func:`ensure_path_not_tracked_config`, phrased as a value + rather than an exception, because the agent backends express a refusal by + returning one — a raise there aborts the turn instead of telling the model + what to do differently. The wording is aimed at the model for that reason: + it names the alternative, so a capable agent routes to the review flow rather + than retrying the same write with a different tool. + + A relative path is taken as relative to the workspace, which is the agent's + working directory. That can in principle over-refuse for an agent that has + changed directory into a subtree of its own with a ``config/`` in it; erring + that way costs a clear message and a retry, and erring the other way is the + hole this exists to close. + """ + if not is_locked(): + return None + workspace = get_config().workspace + target = Path(path) + if not target.is_absolute(): + target = workspace / target + config_root = workspace_config_dir(workspace) + if not _is_within(target, config_root): + return None + return ( + f"this instance is in lockdown (remote-only, read-only) and {target} is " + f"inside the tracked config subtree {config_root}, which only a reviewed, " + f"merged change to the workspace repo may alter. Open a pull request " + f"against that repo instead; the instance will pick the change up when it " + f"syncs. Everything outside {config_root} is still writable." + ) diff --git a/nerve/config_validate.py b/nerve/config_validate.py index 6b6c98ec..1eee8d1b 100644 --- a/nerve/config_validate.py +++ b/nerve/config_validate.py @@ -43,6 +43,11 @@ class ValidationResult: errors: list[str] = field(default_factory=list) warnings: list[str] = field(default_factory=list) info: list[str] = field(default_factory=list) + #: Names of ``${VAR}`` references the bundle needs and the environment does + #: not have. Kept as data rather than only as prose, because which bucket the + #: prose lands in depends on ``strict_env`` — and a caller that tolerates + #: unset variables still needs to know the daemon will refuse this config. + unresolved_env: list[str] = field(default_factory=list) @property def ok(self) -> bool: @@ -55,6 +60,7 @@ def validate_config_bundle( strict_env: bool = False, strict_keys: bool = False, portable_only: bool = False, + assume_locked: bool = False, ) -> ValidationResult: """Validate the config bundle rooted at ``config_dir``. @@ -71,6 +77,15 @@ def validate_config_bundle( running the check. Pair it with ``workspace_override``: with no machine config left to read the workspace location from, it falls back to the default one. + + ``assume_locked`` judges the locked view whatever the bundle's own + ``lockdown`` flag resolves to here. A fleet repo states the flag as + ``${NERVE_LOCKDOWN:-false}`` so that one bundle can serve locked and unlocked + boxes — which means CI, where the variable is unset, resolves it to false and + validates the view *no locked box will ever run*. The lockdown checks then + have nothing to fire on and the only machine that finds out is the locked one, + at boot. This is how a config repo whose members include a locked instance + checks the configuration that instance will actually load. """ result = ValidationResult() config_dir = Path(config_dir) @@ -91,8 +106,28 @@ def validate_config_bundle( base, local = layers[0] or {}, layers[1] or {} machine = cfg._deep_merge(base, local) + # An environment anchor forces lockdown regardless of the files, so a run in + # the daemon's environment has to judge the same thing the daemon will. + # Errors from a malformed anchor are reported rather than raised — the + # validator's job is to say what is wrong, not to become unusable. + try: + anchored = cfg.lockdown_anchor() + except cfg.ConfigError as e: + result.errors.append(str(e)) + anchored = False + if workspace_override is not None: workspace = Path(workspace_override).expanduser() + elif anchored: + # Same rule as the loader: the anchor selects the workspace too, so an + # unpinned run in an anchored environment looks at the tree the daemon + # will. An explicit --workspace still wins — pointing the validator at a + # checkout is the whole point of the flag, and CI is not the anchored box. + try: + workspace = cfg._anchored_workspace() + except cfg.ConfigError as e: + result.errors.append(str(e)) + workspace = cfg.paths.default_workspace() else: # Resolve the workspace the same way load_config does, incl. best-effort # ${VAR}/${VAR:-default} interpolation of the path itself. Under @@ -105,9 +140,50 @@ def validate_config_bundle( workspace = cfg._expand_path(ws_raw) or cfg.paths.default_workspace() ws_settings = _read_workspace_settings(workspace, result) - merged = cfg._deep_merge(cfg._deep_merge(ws_settings, base), local) - # Before the pin below overwrites the bundle's own ``workspace`` value with - # the resolved one — that value is part of what is under review. + # Mirror _read_config_sources: when the tracked settings lock the instance, + # validate the LOCKED view (workspace-only), since that's what production runs. + # An unreadable flag is refused rather than assumed, exactly as at load time — + # reported here, before the change is merged, which is the whole point. That + # error is already fatal, so nothing is fail-open in carrying on with the + # unlocked view; judging the locked one instead would bury the real problem + # under a pile of errors derived from a lockdown the bundle never asked for. + raw_lockdown = ws_settings.get("lockdown") + try: + locked = cfg._as_lockdown(raw_lockdown) + except cfg.ConfigError as e: + result.errors.append(str(e)) + locked = False + if anchored and not locked: + result.info.append( + f"validating the LOCKED view: {cfg.LOCKDOWN_ANCHOR_ENV} is set in " + f"this environment, which locks the instance whatever the tracked " + f"settings say" + ) + locked = True + if assume_locked and not locked: + result.info.append( + "validating the LOCKED view on request (--assume-lockdown); this " + "bundle's own lockdown flag resolves to false in this environment" + ) + locked = True + elif not locked and isinstance(raw_lockdown, str) and "${" in raw_lockdown: + # The flag is env-controlled and this environment says off, so everything + # below judges a view no locked member of the fleet will run. Say so: + # nobody would otherwise think to ask for the other one. + result.warnings.append( + f"lockdown is set from the environment ({raw_lockdown}) and resolves " + f"to false here, so the locked view was NOT validated — a locked " + f"instance loads a different config than this. Re-run with " + f"--assume-lockdown to check it." + ) + if locked: + merged = dict(ws_settings) + else: + merged = cfg._deep_merge(cfg._deep_merge(ws_settings, base), local) + merged["lockdown"] = locked + # Judge the same view the instance will run, so a locked bundle is checked on + # its tracked values alone. Before the pin below overwrites the bundle's own + # ``workspace`` value with the resolved one — that value is under review too. _validate_working_dir_paths(merged, result) # Pin the workspace so cron paths resolve against the validated workspace. merged["workspace"] = str(workspace) @@ -116,6 +192,7 @@ def validate_config_bundle( missing: list[str] = [] merged = cfg._interpolate_env(merged, missing) env_names = ", ".join(sorted(set(missing))) + result.unresolved_env = sorted(set(missing)) if missing: msg = f"references unset environment variable(s): {env_names}" # Say which layer asked for them. The refs are collected from the merged @@ -196,6 +273,19 @@ def validate_config_bundle( # be env-caused, with a real one behind it. result.errors.append(f"config error: {residual}") + if locked: + # Judged on the workspace the run was pointed at, so a candidate bundle + # is checked against its own checkout rather than this machine's tree. + result.errors.extend(cfg.lockdown_workspace_problems(workspace)) + if config is not None: + # The machine overlay read at the top of this function is what a locked + # instance discards, so pass it through: with it the note can name the + # keys this bundle strands rather than describing the hazard in general. + # Empty under portable_only, which the callee guards against. + result.warnings.extend( + cfg.lockdown_machine_local_notes(merged, machine if locked else None) + ) + # Resolve the cron locations even if full typed construction failed above. if config is not None: cron_files = ( @@ -203,7 +293,7 @@ def validate_config_bundle( ("jobs", config.cron.jobs_file), ) else: - base_cron = cfg._resolve_cron_dir(workspace) + base_cron = cfg._resolve_cron_dir(workspace, locked=locked) cron_files = ( ("system", base_cron / "system.yaml"), ("jobs", base_cron / "jobs.yaml"), diff --git a/nerve/cron/jobs.py b/nerve/cron/jobs.py index d28e3670..c43592d2 100644 --- a/nerve/cron/jobs.py +++ b/nerve/cron/jobs.py @@ -345,7 +345,16 @@ def load_jobs( def save_jobs(jobs: list[CronJob], jobs_file: Path) -> None: - """Save cron jobs to a YAML file.""" + """Save cron jobs to a YAML file. + + Guarded even though nothing calls it today: the file it writes is + ``/config/cron/jobs.yaml`` on a migrated install, which is tracked + config, and a guard added when the first caller appears is a guard that was + missing for however long the caller took to notice. + """ + from nerve.config import ensure_path_not_tracked_config + + ensure_path_not_tracked_config(jobs_file, "save cron jobs to") jobs_file.parent.mkdir(parents=True, exist_ok=True) data = {"jobs": []} for job in jobs: diff --git a/nerve/cron/service.py b/nerve/cron/service.py index 9e67f41f..aa7ac4cc 100644 --- a/nerve/cron/service.py +++ b/nerve/cron/service.py @@ -16,6 +16,7 @@ from apscheduler.triggers.cron import CronTrigger from apscheduler.triggers.interval import IntervalTrigger +from nerve import paths from nerve.agent.engine import AgentEngine from nerve.config import ConfigError, NerveConfig from nerve.cron.jobs import ( @@ -268,6 +269,16 @@ async def start(self) -> None: # Load job definitions from both files self._jobs = self._load_merged_jobs() + # Under lockdown the legacy machine-local cron fallback is disabled, so + # an unpopulated workspace cron dir means zero user crons — surface it. + if self.config.lockdown and not any( + j.metadata.get("_source") == "user" for j in self._jobs + ): + logger.warning( + "Lockdown: no user crons found in %s (legacy %s is ignored when " + "locked)", self.config.cron.jobs_file.parent, paths.path_label("cron"), + ) + # Register cron jobs with persistent timer alignment for job in self._jobs: if not job.enabled: diff --git a/nerve/gateway/auth.py b/nerve/gateway/auth.py index 3ef61956..e6a90149 100644 --- a/nerve/gateway/auth.py +++ b/nerve/gateway/auth.py @@ -141,6 +141,14 @@ async def require_auth(request: Request) -> dict: """FastAPI dependency: require valid authentication.""" config = get_config() if not config.auth.jwt_secret: + if config.lockdown: + # Fail closed: a locked instance must never fall into the open + # dev-mode bypass just because its jwt_secret wasn't supplied via env. + raise HTTPException( + status_code=503, + detail="Locked instance has no auth.jwt_secret (set it via " + "${ENV_VAR} in settings.yaml or the environment).", + ) # Auth not configured — allow access (development mode) return {"sub": "user"} @@ -156,6 +164,8 @@ async def authenticate_websocket(websocket: WebSocket) -> bool: """ config = get_config() if not config.auth.jwt_secret: + if config.lockdown: + return False # Fail closed under lockdown — never open the socket return True # Dev mode # Check query parameter diff --git a/nerve/gateway/routes/config.py b/nerve/gateway/routes/config.py index 36bb9657..733f0394 100644 --- a/nerve/gateway/routes/config.py +++ b/nerve/gateway/routes/config.py @@ -2,6 +2,8 @@ from __future__ import annotations +from pathlib import Path + from fastapi import APIRouter, Depends, HTTPException from nerve.gateway.auth import require_auth @@ -33,6 +35,7 @@ async def sync_workspace_route(user: dict = Depends(require_auth)): branch=config.workspace_sync.branch, validate=config.workspace_sync.validate, strict_env=config.workspace_sync.strict_env, + locked=config.lockdown, ) if not result.ok: raise HTTPException( @@ -43,12 +46,20 @@ async def sync_workspace_route(user: dict = Depends(require_auth)): "warnings": result.validation_warnings, }, ) + apply_error = None if result.changed: deps = get_deps() - await _apply_sync(deps.engine, _cron_service) + config_dir = Path(config.config_dir) if config.config_dir else Path(config.workspace) + apply_error = await _apply_sync(deps.engine, _cron_service, config_dir) + # The merge is a real state change, so this is not a 4xx — but a merge whose + # config the daemon then refused to load has applied nothing, and reporting + # ok for it would tell an operator who just enabled lockdown that the box is + # locked while its write guards are still open. return { - "ok": True, + "ok": apply_error is None, "changed": result.changed, + "applied": result.changed and apply_error is None, + "apply_error": apply_error, "message": result.message, "old_rev": result.old_rev, "new_rev": result.new_rev, diff --git a/nerve/gateway/routes/external_agents.py b/nerve/gateway/routes/external_agents.py index 0834d8bb..0bd4c401 100644 --- a/nerve/gateway/routes/external_agents.py +++ b/nerve/gateway/routes/external_agents.py @@ -22,7 +22,7 @@ from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel -from nerve.config import get_config, set_config +from nerve.config import ensure_not_locked, get_config, set_config from nerve.external_agents.registry import AGENT_REGISTRY from nerve.gateway.auth import require_auth from nerve.gateway.routes._deps import get_deps @@ -125,6 +125,7 @@ async def remove_agent(name: str, user: dict = Depends(require_auth)) -> dict[st user can re-add later without re-bootstrapping. To wipe the files too, they should delete them manually. """ + ensure_not_locked("remove an external agent") config = get_config() before = len(config.external_agents.targets) config.external_agents.targets = [ @@ -139,6 +140,7 @@ async def remove_agent(name: str, user: dict = Depends(require_auth)) -> dict[st def _toggle_agent(name: str, *, enabled: bool) -> ToggleResponse: + ensure_not_locked("change external agents") config = get_config() target = next( (t for t in config.external_agents.targets if t.name == name), diff --git a/nerve/gateway/routes/memory.py b/nerve/gateway/routes/memory.py index 59220482..358fe668 100644 --- a/nerve/gateway/routes/memory.py +++ b/nerve/gateway/routes/memory.py @@ -12,7 +12,7 @@ from fastapi import APIRouter, Depends, HTTPException, Response from pydantic import BaseModel -from nerve.config import get_config +from nerve.config import ensure_path_not_tracked_config, get_config from nerve.gateway.auth import require_auth from nerve.gateway.routes._deps import get_deps @@ -86,6 +86,10 @@ async def write_memory_file(file_path: str, req: FileWriteRequest, user: dict = full_path.resolve().relative_to(config.workspace.resolve()) except ValueError: raise HTTPException(status_code=403, detail="Access denied") + # This endpoint writes anywhere under the workspace, which includes the + # tracked config subtree — settings.yaml and the gate plugins the daemon + # executes. A locked instance must not be editable through it. + ensure_path_not_tracked_config(full_path, "write") def _write() -> None: full_path.parent.mkdir(parents=True, exist_ok=True) diff --git a/nerve/gateway/routes/tasks.py b/nerve/gateway/routes/tasks.py index 396faa21..128febdc 100644 --- a/nerve/gateway/routes/tasks.py +++ b/nerve/gateway/routes/tasks.py @@ -7,7 +7,7 @@ from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel -from nerve.config import get_config +from nerve.config import ensure_path_not_tracked_config, get_config from nerve.db.task_statuses import STATUS_NAME_RE, normalize_color from nerve.gateway.auth import require_auth from nerve.gateway.routes._deps import ( @@ -129,6 +129,11 @@ async def update_task(task_id: str, req: TaskUpdateRequest, user: dict = Depends if req.content: config = get_config() file_path = config.workspace / task["file_path"] + # file_path comes from the DB row, which the task indexer only ever fills + # from a glob of tasks/ — but it is still a stored path being joined to + # the workspace and written to, so it gets the same guard as every other + # caller-influenced write. + ensure_path_not_tracked_config(file_path, "write") if file_path.exists(): await asyncio.to_thread( file_path.write_text, req.content, encoding="utf-8", diff --git a/nerve/gateway/server.py b/nerve/gateway/server.py index 717ded36..9564d374 100644 --- a/nerve/gateway/server.py +++ b/nerve/gateway/server.py @@ -690,6 +690,23 @@ def create_app() -> FastAPI: lifespan=lifespan, ) + # Render a lockdown violation as a clean 403 instead of an opaque 500. + from fastapi.responses import JSONResponse + + from nerve.config import LockdownError + + @app.exception_handler(LockdownError) + async def _lockdown_handler(request, exc: LockdownError): # noqa: ANN001 + return JSONResponse(status_code=403, content={"detail": str(exc)}) + + # A malformed skill id is the caller's mistake, not a server fault, and the + # id arrives as a path segment so it is trivially reachable. + from nerve.skills.manager import SkillIdError + + @app.exception_handler(SkillIdError) + async def _skill_id_handler(request, exc: SkillIdError): # noqa: ANN001 + return JSONResponse(status_code=400, content={"detail": str(exc)}) + # CORS for development app.add_middleware( CORSMiddleware, diff --git a/nerve/memory/manager.py b/nerve/memory/manager.py index b94c5f37..d24d4c9a 100644 --- a/nerve/memory/manager.py +++ b/nerve/memory/manager.py @@ -116,13 +116,21 @@ def read_file(self, relative_path: str) -> str | None: return None def write_file(self, relative_path: str, content: str) -> bool: - """Write a file in the workspace by relative path.""" + """Write a file in the workspace by relative path. + + Raises :class:`~nerve.config.LockdownError` for a path inside the tracked + config subtree on a locked instance — the caller supplies the path, so + whether this is a config edit is a property of the argument. + """ + from nerve.config import ensure_path_not_tracked_config + full_path = self.workspace / relative_path try: full_path.resolve().relative_to(self.workspace.resolve()) except ValueError: logger.warning("Attempted path traversal: %s", relative_path) return False + ensure_path_not_tracked_config(full_path, "write") full_path.parent.mkdir(parents=True, exist_ok=True) full_path.write_text(content, encoding="utf-8") diff --git a/nerve/skills/manager.py b/nerve/skills/manager.py index e73e257c..9225abe1 100644 --- a/nerve/skills/manager.py +++ b/nerve/skills/manager.py @@ -26,6 +26,7 @@ import yaml +from nerve.config import ensure_not_locked from nerve.db import Database logger = logging.getLogger(__name__) @@ -65,6 +66,41 @@ def _slugify(name: str) -> str: return slug[:60] or "unnamed-skill" +# One path component and nothing else: no separator, on either platform, and no +# drive or UNC prefix. Deliberately wider than what _slugify emits, because the +# filesystem is the source of truth here and discover() adopts whatever directory +# names it finds — a pre-existing `My_Skill` must stay editable. +_SKILL_ID_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._+-]{0,127}\Z") + + +class SkillIdError(ValueError): + """Raised for a skill id that could not name a directory under ``skills/``.""" + + +def _skill_id(skill_id: str) -> str: + """Validate a caller-supplied skill id before it is joined to a path. + + ``_slugify`` runs on *create* only; every other entry point takes the id + straight from an HTTP path segment or a tool argument and joins it to + ``skills_dir``. That made ``..`` a working path component — and since delete + removes the whole directory tree it names, ``DELETE /api/skills/../config`` + took out the workspace's tracked config subtree. + + Checked as a *shape* rather than by containing the result afterwards: a skill + id names one directory directly under ``skills/``, so anything that is not a + single path component is not a skill id, whatever it would have resolved to. + A leading dot is refused too, which keeps ``.`` and ``..`` out without + special-casing them. + """ + if not isinstance(skill_id, str) or not _SKILL_ID_RE.match(skill_id): + raise SkillIdError( + f"invalid skill id {skill_id!r}: a skill id names a single directory " + f"under skills/ — letters, digits, '.', '_', '+' and '-', not " + f"starting with a dot" + ) + return skill_id + + def _parse_skill_md(raw: str) -> tuple[dict, str]: """Parse SKILL.md into (frontmatter_dict, body_content). @@ -219,7 +255,7 @@ def _scan_skill_dirs() -> list[tuple[Path, str]]: async def get_skill(self, skill_id: str) -> SkillContent | None: """Load full SKILL.md content + metadata for a skill.""" - skill_dir = self.skills_dir / skill_id + skill_dir = self.skills_dir / _skill_id(skill_id) skill_md = skill_dir / "SKILL.md" if not skill_md.exists(): return None @@ -266,8 +302,9 @@ async def create_skill( version: str = "1.0.0", ) -> SkillMeta: """Create a new skill directory + SKILL.md and index in DB.""" + ensure_not_locked("create a skill") skill_id = _slugify(name) - skill_dir = self.skills_dir / skill_id + skill_dir = self.skills_dir / _skill_id(skill_id) # Build SKILL.md raw = _build_skill_md(name, description, content, version) @@ -293,7 +330,8 @@ def _write_skill() -> None: async def update_skill(self, skill_id: str, content: str) -> SkillMeta | None: """Update SKILL.md content (full raw file), re-parse frontmatter, sync DB.""" - skill_dir = self.skills_dir / skill_id + ensure_not_locked("update a skill") + skill_dir = self.skills_dir / _skill_id(skill_id) skill_md = skill_dir / "SKILL.md" if not skill_md.exists(): return None @@ -325,7 +363,8 @@ async def update_skill(self, skill_id: str, content: str) -> SkillMeta | None: async def delete_skill(self, skill_id: str) -> bool: """Remove skill directory and DB record.""" - skill_dir = self.skills_dir / skill_id + ensure_not_locked("delete a skill") + skill_dir = self.skills_dir / _skill_id(skill_id) if skill_dir.exists(): await asyncio.to_thread(shutil.rmtree, skill_dir) await self.db.delete_skill_row(skill_id) @@ -334,6 +373,7 @@ async def delete_skill(self, skill_id: str) -> bool: async def toggle_skill(self, skill_id: str, enabled: bool) -> bool: """Enable or disable a skill.""" + ensure_not_locked("toggle a skill") existing = await self.db.get_skill_row(skill_id) if not existing: return False @@ -344,7 +384,7 @@ async def toggle_skill(self, skill_id: str, enabled: bool) -> bool: async def list_references(self, skill_id: str) -> list[str]: """List reference files in a skill's references/ directory.""" - refs_dir = self.skills_dir / skill_id / "references" + refs_dir = self.skills_dir / _skill_id(skill_id) / "references" if not refs_dir.is_dir(): return [] @@ -359,7 +399,7 @@ def _walk() -> list[str]: async def read_reference(self, skill_id: str, rel_path: str) -> str | None: """Read a reference file from a skill.""" - ref_file = self.skills_dir / skill_id / "references" / rel_path + ref_file = self.skills_dir / _skill_id(skill_id) / "references" / rel_path # Prevent path traversal try: ref_file.resolve().relative_to((self.skills_dir / skill_id).resolve()) @@ -371,7 +411,7 @@ async def read_reference(self, skill_id: str, rel_path: str) -> str | None: async def run_script(self, skill_id: str, rel_path: str, args: str = "") -> str: """Execute a script from a skill's scripts/ directory.""" - script_file = self.skills_dir / skill_id / "scripts" / rel_path + script_file = self.skills_dir / _skill_id(skill_id) / "scripts" / rel_path # Prevent path traversal try: script_file.resolve().relative_to((self.skills_dir / skill_id).resolve()) @@ -399,7 +439,7 @@ async def run_script(self, skill_id: str, rel_path: str, args: str = "") -> str: capture_output=True, text=True, timeout=30, - cwd=str(self.skills_dir / skill_id), + cwd=str(self.skills_dir / _skill_id(skill_id)), ) output = result.stdout if result.returncode != 0: diff --git a/nerve/sync_service.py b/nerve/sync_service.py index de4c54ec..b2175719 100644 --- a/nerve/sync_service.py +++ b/nerve/sync_service.py @@ -119,7 +119,7 @@ def _config_pathspec(workspace: Path) -> str: def _local_config_divergence( - workspace: Path, rev: str, + workspace: Path, rev: str, locked: bool = False, ) -> tuple[list[str], list[str]]: """Local state in the config subtree that the validated rev does not contain. @@ -145,6 +145,16 @@ def _local_config_divergence( in it are normal and none of sync's business — and where they *would* break the fast-forward, git says so itself. + ``locked`` promotes ``.gitignore``d files inside the subtree from a warning to + a refusal. On an ordinary box those files are the operator's own, kept + deliberately out of the shared repo, and refusing a merge over them would be + sync passing judgement on what a machine may hold locally. A locked instance + has already made that judgement: its stated contract is that only reviewed, + merged remote config runs there, and an ignored ``cron/gates/*.py`` is local + unreviewed code the daemon executes, invisible to both the reviewer and the + validator. Merging on top of it would report success over a bundle that is + not the one that passed. + Returns ``(blocking, warnings)``. """ blocking: list[str] = [] @@ -170,11 +180,17 @@ def _local_config_divergence( if code == "!!": # An ignored file inside a *tracked config subtree* is a layout # mistake — it is config the shared repo can never carry and no - # reviewer will ever see. Worth saying; not worth refusing a merge - # over, since refusing would be a policy decision about what the - # box is allowed to have locally rather than a statement about - # whether the merge is sound. - warnings.append(f"ignored file inside the tracked config subtree: {path}") + # reviewer will ever see. Worth saying; on an unlocked box not worth + # refusing a merge over, since refusing would be a policy decision + # about what the machine is allowed to have locally rather than a + # statement about whether the merge is sound. Under lockdown that + # policy decision has already been made. + if locked: + blocking.append(f"!! {path} (gitignored)") + else: + warnings.append( + f"ignored file inside the tracked config subtree: {path}" + ) else: blocking.append(f"{code.strip() or '??'} {path}") @@ -292,6 +308,7 @@ def sync_workspace( branch: str = "", validate: bool = True, strict_env: bool = True, + locked: bool = False, ) -> SyncResult: """Fetch the workspace remote, validate the fetched bundle, then ff-merge it. @@ -311,6 +328,9 @@ def sync_workspace( refuses to load on its next restart. Turn it off only when running somewhere that legitimately lacks the daemon's environment, e.g. an operator's shell. + ``locked`` tightens the local-changes check for an instance that has promised + to run only reviewed remote config — see :func:`_local_config_divergence`. + "Never raises" is the whole contract, not an aspiration: the HTTP route turns anything that escapes into a 500 and the daemon's loop would report a stack trace instead of a config problem. The guard here is deliberately @@ -325,6 +345,7 @@ def sync_workspace( with _sync_lock: return _sync_workspace( Path(workspace), Path(config_dir), branch, validate, strict_env, + locked, ) except Exception as e: # noqa: BLE001 — see the contract above logger.warning("Workspace sync failed unexpectedly: %s", e, exc_info=True) @@ -337,6 +358,7 @@ def _sync_workspace( branch: str, validate: bool, strict_env: bool, + locked: bool = False, ) -> SyncResult: """The fetch → validate → merge sequence. See :func:`sync_workspace`.""" if not is_git_repo(workspace): @@ -362,7 +384,7 @@ def _sync_workspace( if new == old: return SyncResult(ok=True, changed=False, old_rev=old, new_rev=new, message="up to date") - blocking, warnings = _local_config_divergence(workspace, new) + blocking, warnings = _local_config_divergence(workspace, new, locked) if blocking: return SyncResult( ok=False, changed=False, old_rev=old, new_rev=new, @@ -384,6 +406,19 @@ def _sync_workspace( # gate type nothing recognizes is how a cron job quietly starts running # unconditionally, and the only place that is visible is here. warnings += report.warnings + if report.unresolved_env and not strict_env: + # With strict_env on this is already an error and the merge is + # refused. With it off the validator files it as *info*, which + # nothing here propagates — so the gate would see the one thing that + # predicts a failed post-merge reload and drop it on the floor. The + # merge still goes ahead: switching strict_env off is exactly a + # request to tolerate this. Saying so is not. + warnings.append( + f"the fetched bundle references unset environment variable(s): " + f"{', '.join(report.unresolved_env)} — workspace_sync.strict_env " + f"is off so the merge proceeds, but this daemon will refuse to " + f"load the merged config" + ) if report.errors: return SyncResult( ok=False, changed=False, old_rev=old, new_rev=new, @@ -430,14 +465,13 @@ async def run_periodic_sync(config, engine, cron_service, stop_event: asyncio.Ev Every cycle re-reads the process-wide config object rather than working from a reference captured before the loop, so the loop can never be the reason a - setting is stuck. **On its own that changes nothing today:** nothing refreshes - that object after start-up — it is loaded once and only replaced by the CLI - at process start — so editing ``workspace_sync`` still requires a restart to - take effect. Once something does refresh it, ``branch``, ``validate``, + setting is stuck. What refreshes that object is :func:`_apply_sync`, i.e. a + sync that actually merged something — after which ``branch``, ``validate``, ``strict_env``, ``interval_minutes``, the workspace location and turning sync - off will apply from the following cycle with no further work here. Turning - sync *on* will still need a restart regardless: this task is created at - start-up only when sync is already enabled, and nothing creates it later. + off all apply from the following cycle. Nothing *else* refreshes it, so a + hand-edited ``config.yaml`` on the box still needs a restart. Turning sync + *on* needs one regardless: this task is created at start-up only when sync is + already enabled, and nothing creates it later. """ from nerve.config import get_config @@ -471,6 +505,7 @@ async def run_periodic_sync(config, engine, cron_service, stop_event: asyncio.Ev sync_workspace, config.workspace, config.config_dir or config.workspace, branch=cfg.branch, validate=cfg.validate, strict_env=cfg.strict_env, + locked=config.lockdown, ) for warning in result.validation_warnings: logger.warning("Workspace sync: config warning: %s", warning) @@ -481,21 +516,65 @@ async def run_periodic_sync(config, engine, cron_service, stop_event: asyncio.Ev continue if result.changed: logger.info("Workspace sync: %s — applying", result.message) - await _apply_sync(engine, cron_service) + apply_error = await _apply_sync( + engine, cron_service, config.config_dir or config.workspace, + ) + if apply_error: + # The merge landed but the daemon is still running the old + # config. Logged at WARNING beside the INFO line above, which + # otherwise reads as "applied". + logger.warning( + "Workspace sync: merged %s but the new config could not " + "be loaded, so NOTHING was applied and this daemon is " + "still running the previous configuration: %s", + result.new_rev[:8], apply_error, + ) except Exception as e: # noqa: BLE001 — never let the loop die logger.warning("Workspace sync cycle failed: %s", e) continue -async def _apply_sync(engine, cron_service) -> None: - """Hot-reload the subsystems affected by a workspace pull.""" +async def _apply_sync(engine, cron_service, config_dir) -> str | None: + """Hot-reload the subsystems affected by a workspace pull. + + Re-reads the typed config and re-points the singleton, so a synced settings + change engages here without a restart for everything that reads the singleton + per use: the lockdown write guards (``is_locked``), gateway authentication, + and — because it re-reads the singleton every cycle — the sync loop itself. + ``cron_service.config`` and ``engine.config`` are re-pointed too. What is + *not*: cron jobs and gates already built, which follow on the next + ``cron_service.reload()``, and anything captured in a closure or a dataclass + at start-up, which follows on a restart. This is also the only path that + refreshes any of it — a hand edit to a config file on the box is not picked + up, by design for a locked instance and by omission otherwise. + + Returns the config-reload error, if any. A merge that lands config the daemon + cannot load has applied nothing, however well the merge itself went, and the + caller must not report success for it: the case that makes this concrete is a + pull that turns ``lockdown`` on, where reporting success tells the operator a + box is locked while its write guards are still open. + """ + from nerve.config import load_config, set_config + + config_error: str | None = None + try: + new_config = load_config(config_dir) + set_config(new_config) + if cron_service is not None: + cron_service.config = new_config + if engine is not None and hasattr(engine, "config"): + engine.config = new_config + except Exception as e: # noqa: BLE001 — a bad reload must not kill the loop + config_error = f"{type(e).__name__}: {e}" + logger.warning("config reload after sync failed: %s", e, exc_info=True) if cron_service is not None: try: await cron_service.reload() except Exception as e: # noqa: BLE001 - logger.warning("cron reload after sync failed: %s", e) + logger.warning("cron reload after sync failed: %s", e, exc_info=True) if engine is not None: try: await engine.reload_mcp_config() except Exception as e: # noqa: BLE001 - logger.warning("MCP reload after sync failed: %s", e) + logger.warning("MCP reload after sync failed: %s", e, exc_info=True) + return config_error diff --git a/nerve/tasks/manager.py b/nerve/tasks/manager.py index 7c00e721..a9a41a10 100644 --- a/nerve/tasks/manager.py +++ b/nerve/tasks/manager.py @@ -7,6 +7,7 @@ from datetime import datetime, timezone from pathlib import Path +from nerve.config import ensure_path_not_tracked_config from nerve.db import Database from nerve.tasks.models import Task, TaskStatus, parse_task_frontmatter, parse_task_title @@ -76,12 +77,21 @@ async def get_task(self, task_id: str) -> Task | None: return task async def mark_done(self, task_id: str) -> bool: - """Mark a task as done and move its file.""" + """Mark a task as done and move its file. + + Raises :class:`~nerve.config.LockdownError` on a locked instance when the + stored ``file_path`` lands inside the tracked config subtree. + """ row = await self.db.get_task(task_id) if not row: return False src = self.workspace / row["file_path"] + # Done is a write like any other — it copies the file into done/ and + # unlinks the source, so a stored ``file_path`` inside the tracked config + # subtree would delete config. Refuse before any of it runs, or the + # refusal still leaves that config file mirrored into done/. + ensure_path_not_tracked_config(src, "move") if src.exists(): dst = self.done_dir / src.name content = await asyncio.to_thread(src.read_text, encoding="utf-8") diff --git a/nerve/templates/config/settings.yaml b/nerve/templates/config/settings.yaml index 6aa498ef..98190bbf 100644 --- a/nerve/templates/config/settings.yaml +++ b/nerve/templates/config/settings.yaml @@ -21,6 +21,25 @@ # # Everything below is commented out; uncomment and edit what you want to share. +# Remote-only, read-only mode. When true, config comes ONLY from this workspace +# + ${ENV_VAR}; machine config.yaml/config.local.yaml overrides are ignored and +# runtime edits are blocked. Secrets (incl. auth.jwt_secret) have to be supplied +# via ${ENV_VAR} here or the environment — config.local.yaml is not read when +# locked, so a secret that lives only there stops being read. +# May be an env reference (lockdown: ${NERVE_LOCKDOWN}) so one repo can serve a +# fleet where only some boxes are locked; a value that is neither true nor false — +# an unset or blank variable included — is refused rather than read as unlocked, +# so spell a default-unlocked box ${NERVE_LOCKDOWN:-false}. +# NERVE_LOCKDOWN can also be set in the environment directly: put it (with +# NERVE_WORKSPACE beside it) in the service definition and the box is locked +# whatever any file says, which is what stops a machine-local edit from repointing +# `workspace:` at a tree that isn't locked. Setting it here alone does not survive +# that. +# Anything `nerve init` writes to config.yaml has to be restated here to survive +# locking: notably telegram.enabled, which a locked instance treats as OFF unless +# this file says otherwise. +# lockdown: true + # timezone: America/New_York # Where the gateway listens. A box needing a different port overrides in its own diff --git a/tests/test_backup.py b/tests/test_backup.py index 74a779f2..87c77bf1 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -267,6 +267,44 @@ def test_backup_excludes_junk(nerve_dir, workspace, config_dir, tmp_path): assert any(m.endswith("workspace/scripts/helper.py") for m in members) +def test_config_travels_in_the_bundle_but_is_not_written_back( + nerve_dir, workspace, config_dir, tmp_path, +): + """The two halves of the config asymmetry, on an ordinary bundle. + + The other restore test smuggles entries in to prove a hostile bundle is + refused. This one uses a bundle this module wrote itself, because the + surprising half is that our *own* config/ is not written back either — it is + captured so nothing is lost, and withheld because tracked config comes from + the git remote, not a tarball. + """ + cfg = workspace / "config" + (cfg / "cron").mkdir(parents=True) + (cfg / "settings.yaml").write_text("timezone: UTC\n") + (cfg / "cron" / "jobs.yaml").write_text("jobs:\n - id: nightly\n") + + out = tmp_path / "out" + result = backup_mod.create_backup(nerve_dir, workspace, out, config_dir=config_dir) + + # Captured. + members = _bundle_members(result.path) + assert any(m.endswith("workspace/config/settings.yaml") for m in members) + assert any(m.endswith("workspace/config/cron/jobs.yaml") for m in members) + + # Not written back — and what is already on disk survives untouched. + ws_out = tmp_path / "ws_restored" + (ws_out / "config").mkdir(parents=True) + (ws_out / "config" / "settings.yaml").write_text("timezone: Europe/Berlin\n") + report = backup_mod.restore_bundle(result.path, tmp_path / "target", ws_out) + + assert (ws_out / "config" / "settings.yaml").read_text() == "timezone: Europe/Berlin\n" + assert not (ws_out / "config" / "cron").exists() + # The rest of the brain did come back. + assert (ws_out / "SOUL.md").read_text() == "soul" + assert (ws_out / "skills" / "s1" / "SKILL.md").exists() + assert any("skipped" in w for w in report.warnings), report.warnings + + def test_backup_captures_tracked_workspace_config( nerve_dir, workspace, config_dir, tmp_path, ): @@ -421,6 +459,54 @@ def test_restore_force_relocates_old_dir(nerve_dir, workspace, config_dir, tmp_p assert "old_marker.txt" not in [p.name for p in target.iterdir()] +def test_restore_refuses_workspace_paths_the_backup_would_never_take( + nerve_dir, workspace, config_dir, tmp_path, +): + """The workspace allowlist is applied when a bundle is *collected*; restore + has to apply it again when a bundle is *read*. + + "config/ is never in a bundle" is true of bundles this module writes and says + nothing about bundles it is handed. One carrying config/settings.yaml or a gate + plugin would overwrite the git-tracked config subtree from a tarball — which on + a locked instance replaces the reviewed remote config outright, and on any + instance leaves a checkout dirty enough that the next sync refuses to merge. + """ + out = tmp_path / "out" + result = backup_mod.create_backup(nerve_dir, workspace, out, config_dir=config_dir) + compression = backup_mod._compression_for(result.path) + + # Re-pack the bundle with two extra workspace entries the collector would + # never have produced, using the module's own reader/writer so it stays a + # bundle this code accepts. + staging = tmp_path / "repack" + staging.mkdir() + with backup_mod._tar_reader(result.path, compression) as tf: + tf.extractall(staging) + smuggled = staging / "workspace" / "config" / "cron" / "gates" + smuggled.mkdir(parents=True) + (smuggled / "evil.py").write_text("MARKER = 1\n") + (staging / "workspace" / "config" / "settings.yaml").write_text("lockdown: false\n") + result.path.unlink() + with backup_mod._tar_writer(result.path, compression) as tf: + for entry in sorted(staging.iterdir()): + tf.add(entry, arcname=entry.name) + + target = tmp_path / "restored" + ws_out = tmp_path / "ws_restored" + ws_out.mkdir() + (ws_out / "config").mkdir() + (ws_out / "config" / "settings.yaml").write_text("lockdown: true\n") + + report = backup_mod.restore_bundle(result.path, target, ws_out) + + # The tracked subtree is untouched; the allowlisted brain still came back. + assert (ws_out / "config" / "settings.yaml").read_text() == "lockdown: true\n" + assert not (ws_out / "config" / "cron").exists() + assert (ws_out / "SOUL.md").read_text() == "soul" + assert (ws_out / "memory" / "people.md").exists() + assert any("skipped" in w for w in report.warnings), report.warnings + + # --------------------------------------------------------------------------- # # 5. Schema-version guard # # --------------------------------------------------------------------------- # diff --git a/tests/test_lockdown.py b/tests/test_lockdown.py new file mode 100644 index 00000000..0f5ec2d2 --- /dev/null +++ b/tests/test_lockdown.py @@ -0,0 +1,1301 @@ +"""Tests for lockdown / remote-only read-only mode.""" + +from pathlib import Path + +import pytest + +import nerve.config as cfg +from nerve.config import ( + ConfigError, + LockdownError, + NerveConfig, + _resolve_cron_dir, + ensure_not_locked, + is_locked, + load_config, + workspace_settings_file, +) + + +def _install(tmp_path, *, settings="", base="", local=""): + config_dir = tmp_path / "cfg" + workspace = tmp_path / "ws" + config_dir.mkdir(parents=True) + (workspace / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {workspace}\n" + base, encoding="utf-8") + if local: + (config_dir / "config.local.yaml").write_text(local, encoding="utf-8") + if settings: + workspace_settings_file(workspace).write_text(settings, encoding="utf-8") + return config_dir, workspace + + +_JWT = "auth:\n jwt_secret: test-secret\n" + + +class TestLockdownResolution: + _JWT = "auth:\n jwt_secret: test-secret\n" + + def test_locked_drops_machine_overrides(self, tmp_path): + # settings says UTC + locked; config.yaml tries to override timezone. + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\ntimezone: UTC\n" + self._JWT, + base="timezone: America/New_York\n", + local="timezone: Europe/Berlin\n", + ) + c = load_config(config_dir) + assert c.lockdown is True + assert c.timezone == "UTC" # machine layers ignored; only settings applies + + def test_local_cannot_override_lockdown(self, tmp_path): + # Tamper attempt: config.yaml/local set lockdown:false, settings sets true. + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + self._JWT, + base="lockdown: false\n", + local="lockdown: false\n", + ) + c = load_config(config_dir) + assert c.lockdown is True # the remote (settings.yaml) is authoritative + + def test_lockdown_not_settable_from_config_yaml(self, tmp_path): + # Only the tracked settings file controls lockdown. + config_dir, ws = _install(tmp_path, base="lockdown: true\n") + c = load_config(config_dir) + assert c.lockdown is False + + def test_not_locked_merges_normally(self, tmp_path): + config_dir, ws = _install( + tmp_path, settings="timezone: UTC\n", base="timezone: America/New_York\n" + ) + c = load_config(config_dir) + assert c.lockdown is False + assert c.timezone == "America/New_York" # config.yaml wins when unlocked + + def test_locked_still_resolves_env(self, tmp_path, monkeypatch): + monkeypatch.setenv("SECRET_TZ", "Asia/Tokyo") + config_dir, ws = _install( + tmp_path, settings="lockdown: true\ntimezone: ${SECRET_TZ}\n" + self._JWT, + ) + c = load_config(config_dir) + assert c.timezone == "Asia/Tokyo" # secrets still come from env + + +class TestLockdownAuthFailClosed: + @pytest.mark.asyncio + async def test_require_auth_denies_when_locked_no_secret(self, monkeypatch): + from fastapi import HTTPException + + from nerve.gateway.auth import require_auth + + c = NerveConfig(lockdown=True) # jwt_secret empty + monkeypatch.setattr(cfg, "_config", c) + with pytest.raises(HTTPException) as ei: + await require_auth(request=None) + assert ei.value.status_code == 503 + + @pytest.mark.asyncio + async def test_websocket_denies_when_locked_no_secret(self, monkeypatch): + from nerve.gateway.auth import authenticate_websocket + + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True)) + assert await authenticate_websocket(websocket=None) is False + + +class TestValidateRespectsLockdown: + def test_validate_uses_locked_view(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + # settings locks + provides jwt; config.yaml has an unknown key that must + # be ignored under lockdown (machine layers dropped). + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir(parents=True) + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text( + f"workspace: {ws}\ntiimezone: UTC\n", encoding="utf-8" + ) + workspace_settings_file(ws).write_text( + "lockdown: true\nauth:\n jwt_secret: x\n", encoding="utf-8" + ) + result = validate_config_bundle(config_dir, workspace_override=ws, strict_keys=True) + # config.yaml's typo is dropped under lockdown → not reported. + assert not any("tiimezone" in e for e in result.errors) + + +class TestLockdownCron: + def test_locked_forces_workspace_cron_no_legacy(self, tmp_path): + from nerve import paths + + workspace = tmp_path / "ws" + legacy = paths.cron_dir() + legacy.mkdir(parents=True, exist_ok=True) + (legacy / "jobs.yaml").write_text("jobs: []\n", encoding="utf-8") + # Unlocked would fall back to legacy (workspace has no jobs); locked won't. + assert _resolve_cron_dir(workspace, locked=False) == legacy + assert _resolve_cron_dir(workspace, locked=True) == workspace / "config" / "cron" + + +class TestLockdownGuards: + def test_is_locked_reflects_config(self, monkeypatch): + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True)) + assert is_locked() is True + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False)) + assert is_locked() is False + + def test_ensure_not_locked_raises_when_locked(self, monkeypatch): + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True)) + with pytest.raises(LockdownError): + ensure_not_locked("do a thing") + + def test_ensure_not_locked_noop_when_unlocked(self, monkeypatch): + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False)) + ensure_not_locked("do a thing") # no raise + + def test_telegram_write_blocked_when_locked(self, tmp_path, monkeypatch): + from nerve.config import append_telegram_allowed_user + + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True)) + with pytest.raises(LockdownError): + append_telegram_allowed_user(tmp_path, 42) + + +class TestLockdownSkillWrites: + @pytest.mark.asyncio + async def test_skill_writes_blocked_when_locked(self, tmp_path, db, monkeypatch): + from nerve.skills.manager import SkillManager + + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True)) + mgr = SkillManager(tmp_path / "ws", db) + with pytest.raises(LockdownError): + await mgr.create_skill("Test", "desc") + with pytest.raises(LockdownError): + await mgr.update_skill("x", "content") + with pytest.raises(LockdownError): + await mgr.delete_skill("x") + with pytest.raises(LockdownError): + await mgr.toggle_skill("x", True) + + @pytest.mark.asyncio + async def test_skill_create_works_when_unlocked(self, tmp_path, db, monkeypatch): + from nerve.skills.manager import SkillManager + + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False)) + mgr = SkillManager(tmp_path / "ws", db) + meta = await mgr.create_skill("Test Skill", "a description") + assert meta.id == "test-skill" + + +class TestLockdownFlagFromEnvironment: + """The flag is read before the post-merge interpolation pass, so it has to + resolve its own ``${VAR}`` — otherwise it is judged as the literal reference + text, which is truthy no matter what the variable says. + + Both directions of getting that wrong are here. Reading ``True`` when the + config says ``false`` locks a fleet by accident. Reading ``False`` when the + config says ``true`` is an authentication and integrity bypass, so a value + that cannot be read is refused rather than resolved to the safer-looking + default. + """ + + def test_env_ref_false_leaves_the_instance_unlocked(self, tmp_path, monkeypatch): + monkeypatch.setenv("NERVE_LOCKDOWN", "false") + config_dir, ws = _install( + tmp_path, + settings="lockdown: ${NERVE_LOCKDOWN}\ntimezone: UTC\n", + base="timezone: America/New_York\n", + ) + c = load_config(config_dir) + assert c.lockdown is False + # Not just the flag: the machine-local layer is merged again, which is + # the behavior that was actually lost. + assert c.timezone == "America/New_York" + + def test_env_ref_true_locks(self, tmp_path, monkeypatch): + # NERVE_LOCKDOWN is also the environment anchor, so setting it truthy + # requires NERVE_WORKSPACE alongside — see TestLockdownEnvironmentAnchor. + monkeypatch.setenv("NERVE_LOCKDOWN", "true") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "ws")) + config_dir, ws = _install( + tmp_path, + settings="lockdown: ${NERVE_LOCKDOWN}\ntimezone: UTC\n" + _JWT, + base="timezone: America/New_York\n", + ) + c = load_config(config_dir) + assert c.lockdown is True + assert c.timezone == "UTC" + + def test_the_tracked_reference_and_the_anchor_are_one_switch(self, tmp_path, monkeypatch): + """``lockdown: ${NERVE_LOCKDOWN}`` and the anchor read the same variable + on purpose: "this box is locked" should have one spelling. With the anchor + the tracked file need not mention the flag at all, and a fleet repo that + does mention it gets the anchor's protection for free.""" + monkeypatch.setenv("NERVE_LOCKDOWN", "1") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "ws")) + config_dir, ws = _install(tmp_path, settings=_JWT) # no lockdown key + assert load_config(config_dir).lockdown is True + + def test_optional_ref_default_false_leaves_it_unlocked(self, tmp_path, monkeypatch): + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, ws = _install( + tmp_path, settings="lockdown: ${NERVE_LOCKDOWN:-false}\n", + ) + assert load_config(config_dir).lockdown is False + + def test_unparseable_value_is_refused_not_read_as_unlocked(self, tmp_path): + # jwt_secret is supplied so the only thing left to complain about is the + # flag itself — an unreadable value must not resolve to either position. + config_dir, ws = _install(tmp_path, settings="lockdown: yess\n" + _JWT) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "lockdown must be true or false" in str(ei.value) + + def test_unset_required_ref_is_refused(self, tmp_path, monkeypatch): + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, ws = _install( + tmp_path, settings="lockdown: ${NERVE_LOCKDOWN}\n" + _JWT, + ) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "NERVE_LOCKDOWN" in str(ei.value) + + def test_empty_env_var_is_refused(self, tmp_path, monkeypatch): + """``FLAG=`` switches an ordinary feature off; it must not switch this + one off, because a variable that failed to be populated would silently + drop every restriction the flag imposes.""" + monkeypatch.setenv("NERVE_LOCKDOWN", "") + config_dir, ws = _install( + tmp_path, settings="lockdown: ${NERVE_LOCKDOWN}\n" + _JWT, + ) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "lockdown must be true or false" in str(ei.value) + + def test_bare_key_is_off(self, tmp_path): + config_dir, ws = _install(tmp_path, settings="lockdown:\ntimezone: UTC\n") + assert load_config(config_dir).lockdown is False + + def test_machine_layer_cannot_unlock_with_an_env_ref(self, tmp_path, monkeypatch): + """The tracked file stays the only authority once the flag is a reference: + a local layer that resolves to false must not reach it.""" + monkeypatch.setenv("NERVE_LOCKDOWN", "false") + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT, + base="lockdown: ${NERVE_LOCKDOWN}\n", + local="lockdown: false\n", + ) + assert load_config(config_dir).lockdown is True + + def test_from_dict_parses_a_string_flag(self): + """``NerveConfig.from_dict`` is called with an already-merged dict by the + validator and by tests, so it must agree with the loader.""" + assert NerveConfig.from_dict({"lockdown": "false"}).lockdown is False + assert NerveConfig.from_dict({"lockdown": "1"}).lockdown is True + with pytest.raises(ConfigError): + NerveConfig.from_dict({"lockdown": "sure"}) + + def test_validator_reports_an_unreadable_flag(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install(tmp_path, settings="lockdown: perhaps\n" + _JWT) + result = validate_config_bundle(config_dir, workspace_override=ws) + assert any("lockdown must be true or false" in e for e in result.errors) + + def test_machine_layer_lockdown_is_warned_about(self, tmp_path, caplog): + """Ignoring it is the feature; ignoring it in silence leaves whoever put + it in the wrong file believing the box is locked.""" + import logging + + config_dir, ws = _install(tmp_path, base="lockdown: true\n") + with caplog.at_level(logging.WARNING, logger="nerve.config"): + assert load_config(config_dir).lockdown is False + assert any( + "lockdown" in r.message and "settings.yaml" in r.message + for r in caplog.records + ), [r.message for r in caplog.records] + + def test_unreadable_settings_file_is_a_config_error(self, tmp_path): + """``_read_yaml_mapping`` promises ConfigError rather than a traceback for + a file it cannot use; that covered a parse failure but not a read one.""" + config_dir, ws = _install(tmp_path, settings="lockdown: false\n") + settings = workspace_settings_file(ws) + settings.unlink() + settings.mkdir() # a directory where the file should be + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "Cannot read" in str(ei.value) + + +class TestValidatingTheLockedViewInCi: + """A fleet repo writes ``lockdown: ${NERVE_LOCKDOWN:-false}`` so one bundle can + serve locked and unlocked boxes. CI has no such variable, so it resolved false + and validated the view no locked box will ever run — leaving the lockdown + checks with nothing to fire on and the locked instance to find out at boot. + """ + + _FLEET = "lockdown: ${NERVE_LOCKDOWN:-false}\ntimezone: UTC\n" + + def test_env_controlled_flag_passes_by_default(self, tmp_path, monkeypatch): + from nerve.config_validate import validate_config_bundle + + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, ws = _install(tmp_path, settings=self._FLEET) + # No auth.jwt_secret anywhere: broken for a locked box, fine for this one. + result = validate_config_bundle( + config_dir, workspace_override=ws, strict_env=True, + ) + assert result.ok + # ...but the gap is at least named, which is how anyone learns to ask. + assert any("locked view was NOT validated" in w for w in result.warnings) + + def test_assume_lockdown_catches_it(self, tmp_path, monkeypatch): + """A locked-only error — here a config subtree symlinked out of the + workspace — is invisible to the default run and fatal under the flag.""" + from nerve.config_validate import validate_config_bundle + + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, ws = _install(tmp_path, settings=self._FLEET) + outside = tmp_path / "elsewhere" + outside.mkdir() + (outside / "settings.yaml").write_text(self._FLEET, encoding="utf-8") + (ws / "config").rename(tmp_path / "discarded") + (ws / "config").symlink_to(outside) + + assert validate_config_bundle(config_dir, workspace_override=ws).ok + result = validate_config_bundle( + config_dir, workspace_override=ws, assume_locked=True, + ) + assert not result.ok + assert any("outside the workspace" in e for e in result.errors), result.errors + + def test_assume_lockdown_says_it_is_assuming(self, tmp_path, monkeypatch): + from nerve.config_validate import validate_config_bundle + + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, ws = _install(tmp_path, settings=self._FLEET + _JWT) + result = validate_config_bundle( + config_dir, workspace_override=ws, assume_locked=True, + ) + assert result.ok + assert any("LOCKED view on request" in i for i in result.info) + + def test_a_literal_false_is_not_warned_about(self, tmp_path): + """Only an env-controlled flag leaves a locked view unchecked. A bundle + that says `lockdown: false` outright has no locked view to check.""" + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install(tmp_path, settings="lockdown: false\n") + result = validate_config_bundle(config_dir, workspace_override=ws) + assert not any("locked view" in w for w in result.warnings) + + +class TestLockdownCronContainment: + """Cron's three path keys decide which files a locked instance reads, and + ``gate_plugins_dir`` decides which ``.py`` files it *executes*. A tracked + ``settings.yaml`` is pure YAML that a reviewer may wave through, so pointing + those keys out of the reviewed tree would turn a config edit into arbitrary + on-disk code execution. + """ + + def _locked(self, tmp_path, cron_yaml, *, workspace=None): + config_dir, ws = _install( + tmp_path, settings="lockdown: true\n" + _JWT + cron_yaml, + ) + (ws / "config" / "cron").mkdir(parents=True, exist_ok=True) + return load_config(config_dir), ws + + def test_gate_plugins_dir_outside_the_workspace_is_dropped(self, tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + c, ws = self._locked(tmp_path, f"cron:\n gate_plugins_dir: {outside}\n") + assert c.cron.gate_plugins_dir == ws / "config" / "cron" / "gates" + + def test_the_redirect_no_longer_gets_code_executed(self, tmp_path): + """The end that matters: with the path contained, the plugin loader never + sees the outside directory, so nothing in it runs.""" + from nerve.cron.gate_plugins import load_gate_plugins + + outside = tmp_path / "outside" + outside.mkdir() + marker = tmp_path / "executed" + (outside / "evil.py").write_text( + f"open({str(marker)!r}, 'w').write('ran')\n", encoding="utf-8", + ) + c, ws = self._locked(tmp_path, f"cron:\n gate_plugins_dir: {outside}\n") + assert load_gate_plugins(c.cron.gate_plugins_dir) == 0 + assert not marker.exists() + + def test_jobs_and_system_files_outside_are_dropped(self, tmp_path): + c, ws = self._locked( + tmp_path, + f"cron:\n" + f" jobs_file: {tmp_path}/elsewhere/jobs.yaml\n" + f" system_file: {ws_escape(tmp_path)}\n", + ) + assert c.cron.jobs_file == ws / "config" / "cron" / "jobs.yaml" + assert c.cron.system_file == ws / "config" / "cron" / "system.yaml" + + def test_inside_the_workspace_but_outside_config_is_dropped(self, tmp_path): + """Containment is to ``/config``, not to the workspace: the + workspace is also the agent's working directory, and a path it can write + to freely is not a reviewed one.""" + c, ws = self._locked( + tmp_path, f"cron:\n gate_plugins_dir: {tmp_path}/ws/scratch/gates\n", + ) + assert c.cron.gate_plugins_dir == ws / "config" / "cron" / "gates" + + def test_a_symlink_out_of_the_tree_is_dropped(self, tmp_path): + """A path that *is* inside the subtree by name but resolves out of it — + containment is judged on the resolved path for exactly this reason.""" + outside = tmp_path / "outside" + outside.mkdir() + config_dir, ws = _install(tmp_path, settings="lockdown: true\n" + _JWT) + (ws / "config" / "cron").mkdir(parents=True) + link = ws / "config" / "cron" / "borrowed-gates" + link.symlink_to(outside, target_is_directory=True) + workspace_settings_file(ws).write_text( + "lockdown: true\n" + _JWT + + f"cron:\n gate_plugins_dir: {link}\n", encoding="utf-8", + ) + c = load_config(config_dir) + assert c.cron.gate_plugins_dir == ws / "config" / "cron" / "gates" + + def test_a_symlinked_cron_directory_refuses_to_load(self, tmp_path): + """No substitution can fix this one — every default is derived from the + escaping directory — so a locked instance declines to start.""" + outside = tmp_path / "outside" + outside.mkdir() + config_dir, ws = _install(tmp_path, settings="lockdown: true\n" + _JWT) + (ws / "config" / "cron").symlink_to(outside, target_is_directory=True) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the tracked config subtree" in str(ei.value) + + @pytest.mark.parametrize("name", ["gates", "jobs.yaml", "system.yaml"]) + def test_the_default_name_itself_as_a_symlink_refuses_to_load(self, tmp_path, name): + """The case with no config key in it at all. + + The fallback for an escaping path is the in-workspace default, so when the + *default's own name* is the symlink there is nothing contained to fall back + to: substituting it hands back the path just rejected, with a warning that + contradicts itself. Git tracks symlinks, so this needs no local write — + a reviewed, merged config repo is enough. + """ + outside = tmp_path / "outside" + outside.mkdir() + (outside / "x.py").write_text("MARKER = 1\n", encoding="utf-8") + config_dir, ws = _install(tmp_path, settings="lockdown: true\n" + _JWT) + cron = ws / "config" / "cron" + cron.mkdir(parents=True) + target = outside if name == "gates" else outside / "x.py" + (cron / name).symlink_to(target, target_is_directory=(name == "gates")) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the tracked config subtree" in str(ei.value) + + def test_a_symlinked_gates_default_never_gets_code_executed(self, tmp_path): + """The same case, ended at the consequence rather than the exception.""" + from nerve.cron.gate_plugins import load_gate_plugins + + outside = tmp_path / "unreviewed_gates" + outside.mkdir() + marker = tmp_path / "executed" + (outside / "evil.py").write_text( + f"open({str(marker)!r}, 'w').write('ran')\n", encoding="utf-8", + ) + config_dir, ws = _install(tmp_path, settings="lockdown: true\n" + _JWT) + (ws / "config" / "cron").mkdir(parents=True) + (ws / "config" / "cron" / "gates").symlink_to(outside, target_is_directory=True) + with pytest.raises(ConfigError): + config = load_config(config_dir) + load_gate_plugins(config.cron.gate_plugins_dir) + assert not marker.exists() + + +class TestLockdownTrackedSubtreeIsInTheWorkspace: + """One level above cron: ``/config`` itself. + + Every other containment check judges a path against that directory, so if it + is a symlink out of the workspace they all pass while nothing underneath is in + the reviewed repo — settings.yaml and the gate plugins included, with lockdown + read from there and reporting that all is well. + """ + + def _elsewhere(self, tmp_path, settings): + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + elsewhere = tmp_path / "local-config" + config_dir.mkdir() + ws.mkdir() + (elsewhere / "cron").mkdir(parents=True) + (config_dir / "config.yaml").write_text( + f"workspace: {ws}\n", encoding="utf-8", + ) + (ws / "config").symlink_to(elsewhere, target_is_directory=True) + (elsewhere / "settings.yaml").write_text(settings, encoding="utf-8") + return config_dir, ws + + def test_config_symlinked_out_of_the_workspace_refuses_to_load(self, tmp_path): + config_dir, ws = self._elsewhere(tmp_path, "lockdown: true\n" + _JWT) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the workspace" in str(ei.value) + + def test_unlocked_is_unaffected(self, tmp_path): + """A symlinked config/ is a legitimate machine-local arrangement; only a + locked instance, which claims that tree *is* the reviewed repo, cannot.""" + config_dir, ws = self._elsewhere(tmp_path, "timezone: UTC\n") + assert load_config(config_dir).timezone == "UTC" + + def test_the_validator_reports_it(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + config_dir, ws = self._elsewhere(tmp_path, "lockdown: true\n" + _JWT) + result = validate_config_bundle(config_dir, workspace_override=ws) + assert any("outside the workspace" in e for e in result.errors) + + def test_a_symlinked_workspace_is_still_fine(self, tmp_path): + """Where a machine keeps its workspace stays a machine-local decision.""" + real = tmp_path / "real-ws" + (real / "config").mkdir(parents=True) + link = tmp_path / "ws" + link.symlink_to(real, target_is_directory=True) + config_dir = tmp_path / "cfg" + config_dir.mkdir() + (config_dir / "config.yaml").write_text( + f"workspace: {link}\n", encoding="utf-8", + ) + (real / "config" / "settings.yaml").write_text( + "lockdown: true\n" + _JWT, encoding="utf-8", + ) + assert load_config(config_dir).lockdown is True + + def test_unlocked_overrides_are_still_honored(self, tmp_path): + """An unmigrated install is entitled to point cron anywhere — that is what + the machine-local layers are for.""" + outside = tmp_path / "outside" + outside.mkdir() + config_dir, ws = _install( + tmp_path, base=f"cron:\n gate_plugins_dir: {outside}\n", + ) + assert load_config(config_dir).cron.gate_plugins_dir == outside + + def test_validation_contains_paths_against_the_candidate_workspace(self, tmp_path): + """The validator pins the workspace to the tree under review, so the same + containment has to be judged there — a candidate bundle must not be able + to make validation read from outside its own checkout.""" + from nerve.config_validate import validate_config_bundle + + config_dir = tmp_path / "cfg" + candidate = tmp_path / "candidate" + outside = tmp_path / "outside" + config_dir.mkdir() + (candidate / "config" / "cron").mkdir(parents=True) + outside.mkdir() + (outside / "jobs.yaml").write_text("jobs: not-a-list\n", encoding="utf-8") + (candidate / "config" / "cron" / "jobs.yaml").write_text( + "jobs: []\n", encoding="utf-8", + ) + workspace_settings_file(candidate).write_text( + "lockdown: true\n" + _JWT + + f"cron:\n jobs_file: {outside}/jobs.yaml\n", encoding="utf-8", + ) + result = validate_config_bundle(config_dir, workspace_override=candidate) + assert result.ok, result.errors + assert any(str(candidate) in i and "cron jobs" in i for i in result.info) + + +def ws_escape(tmp_path: Path) -> str: + """A ``..``-traversal path that climbs out of the workspace config subtree.""" + return f"{tmp_path}/ws/config/cron/../../../system.yaml" + + +class TestLockdownTelegramDefault: + """``telegram.enabled`` is a per-machine decision, so ``nerve init`` writes it + to ``config.yaml`` — the layer lockdown drops. Left to its declared default + that would read as "on", so a box where Telegram was switched off would start + answering DMs, with full agent access, as soon as the shared settings carried + a token its environment can resolve. + """ + + _TOKEN = "telegram:\n bot_token: 12345:abc\n" + + def test_locked_leaves_telegram_off_when_the_tracked_file_is_silent(self, tmp_path): + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + self._TOKEN, + base="telegram:\n enabled: false\n", + ) + c = load_config(config_dir) + assert c.telegram.enabled is False + assert c.telegram.bot_token # the token is there; the switch is not + + def test_locked_honors_an_explicit_enable(self, tmp_path): + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + + "telegram:\n enabled: true\n bot_token: 12345:abc\n", + ) + assert load_config(config_dir).telegram.enabled is True + + def test_locked_honors_an_env_ref_per_machine(self, tmp_path, monkeypatch): + monkeypatch.setenv("TG_ON", "true") + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + + "telegram:\n enabled: ${TG_ON}\n bot_token: 12345:abc\n", + ) + assert load_config(config_dir).telegram.enabled is True + + def test_unlocked_default_is_unchanged(self, tmp_path): + config_dir, ws = _install(tmp_path, base=self._TOKEN) + assert load_config(config_dir).telegram.enabled is True + + @pytest.mark.parametrize( + "spelling", ["yess", "disabled", "${TG_ON:-nope}", "[]"], + ) + def test_locked_reads_an_unparseable_value_as_off(self, tmp_path, spelling): + """Being unstated was not the only way to reach the wrong answer. + + A value the parser cannot read falls back to a default, and coercion's + default is the field's *declared* one — ``True``. So the whole guard was + one typo wide, and `${TG_ON:-nope}` is the very spelling the docs + recommend for a per-box fleet setting: one bad env value would have turned + the bot on across the fleet. + """ + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + + f"telegram:\n enabled: {spelling}\n bot_token: 12345:abc\n", + base="telegram:\n enabled: false\n", + ) + assert load_config(config_dir).telegram.enabled is False + + def test_unlocked_unparseable_value_keeps_the_declared_default(self, tmp_path): + """Only the fallback *direction* is lockdown's business. Unlocked, the + machine layer is where this is normally stated and reverting to the + documented default is what an operator expects.""" + config_dir, ws = _install( + tmp_path, base="telegram:\n enabled: yess\n bot_token: 12345:abc\n", + ) + assert load_config(config_dir).telegram.enabled is True + + def test_validator_names_the_setting_that_has_nowhere_to_live(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install( + tmp_path, settings="lockdown: true\n" + _JWT + self._TOKEN, + ) + result = validate_config_bundle(config_dir, workspace_override=ws) + assert result.ok + assert any("telegram.enabled" in w for w in result.warnings) + + def test_every_dropped_key_is_named_not_just_telegram(self, tmp_path): + """The note previously covered one key of the seventeen on the list. + + The uncovered case that mattered was a locked box reverting from Bedrock + to the Anthropic default: nothing reported it, and the only symptom was a + missing API key the box would not otherwise have needed. + """ + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT, + base=( + "gateway:\n port: 9100\n ssl:\n cert: /etc/tls/c.pem\n" + "provider:\n type: bedrock\n aws_region: eu-west-1\n" + " aws_profile: prod\n" + "sync:\n gmail:\n accounts: [me@example.com]\n" + ), + ) + notes = " ".join( + validate_config_bundle(config_dir, workspace_override=ws).warnings + ) + # Shareable values that should not have been stranded in config.yaml. + for path in ("gateway.port", "provider.type", "provider.aws_region"): + assert path in notes, path + # ...reported separately from the ones that are local by design, because + # the fix differs: move the first group, decide about the second. + for path in ("gateway.ssl.cert", "provider.aws_profile", + "sync.gmail.accounts"): + assert path in notes, path + assert "move them to workspace/config/settings.yaml" in notes + + def test_a_key_the_tracked_settings_restate_is_not_reported(self, tmp_path): + """Only silence is worth a warning. A tracked value that config.yaml also + sets is a normal local override, and on a locked box it simply wins.""" + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + "gateway:\n port: 9100\n", + base="gateway:\n port: 9100\n", + ) + notes = " ".join( + validate_config_bundle(config_dir, workspace_override=ws).warnings + ) + assert "gateway.port" not in notes + + def test_a_falsey_tracked_value_counts_as_stated(self, tmp_path): + """`enabled: false` is an answer, not an absence — presence is the test.""" + from nerve.config_validate import validate_config_bundle + + config_dir, ws = _install( + tmp_path, + settings="lockdown: true\n" + _JWT + "proxy:\n enabled: false\n", + base="proxy:\n enabled: true\n", + ) + notes = " ".join( + validate_config_bundle(config_dir, workspace_override=ws).warnings + ) + assert "proxy.enabled" not in notes + + +class TestLockdownTrackedConfigWrites: + """Guards that sit in front of a named operation only cover the operations + they were put on. An endpoint that takes a caller-supplied path is a + different shape: whether it edits tracked config depends on the argument. + """ + + @pytest.mark.asyncio + async def test_memory_file_route_cannot_edit_tracked_config(self, tmp_path, monkeypatch): + from nerve.gateway.routes.memory import FileWriteRequest, write_memory_file + + ws = tmp_path / "ws" + (ws / "config" / "cron" / "gates").mkdir(parents=True) + monkeypatch.setattr( + cfg, "_config", NerveConfig(lockdown=True, workspace=ws), + ) + for path in ("config/settings.yaml", "config/cron/gates/evil.py"): + with pytest.raises(LockdownError): + await write_memory_file( + path, FileWriteRequest(content="lockdown: false\n"), user={}, + ) + assert not (ws / "config" / "settings.yaml").exists() + assert not (ws / "config" / "cron" / "gates" / "evil.py").exists() + + @pytest.mark.asyncio + async def test_memory_file_route_still_writes_elsewhere(self, tmp_path, monkeypatch): + """The workspace is also the agent's working directory — lockdown is + about tracked config, not about making the box read-only.""" + from nerve.gateway.routes.memory import FileWriteRequest, write_memory_file + + ws = tmp_path / "ws" + ws.mkdir() + monkeypatch.setattr( + cfg, "_config", NerveConfig(lockdown=True, workspace=ws), + ) + await write_memory_file( + "memory/notes.md", FileWriteRequest(content="hello\n"), user={}, + ) + assert (ws / "memory" / "notes.md").read_text() == "hello\n" + + @pytest.mark.asyncio + async def test_unlocked_route_writes_config_freely(self, tmp_path, monkeypatch): + from nerve.gateway.routes.memory import FileWriteRequest, write_memory_file + + ws = tmp_path / "ws" + ws.mkdir() + monkeypatch.setattr( + cfg, "_config", NerveConfig(lockdown=False, workspace=ws), + ) + await write_memory_file( + "config/settings.yaml", FileWriteRequest(content="timezone: UTC\n"), user={}, + ) + assert (ws / "config" / "settings.yaml").exists() + + def test_memory_manager_write_file_is_guarded_too(self, tmp_path, monkeypatch): + """The route's twin. Guarding one and not the other is not a guard.""" + from nerve.memory.manager import MemoryManager + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + mgr = MemoryManager(ws) + with pytest.raises(LockdownError): + mgr.write_file("config/settings.yaml", "lockdown: false\n") + assert mgr.write_file("memory/notes.md", "hello\n") is True + + def test_save_jobs_is_guarded(self, tmp_path, monkeypatch): + """No caller today, but the file it writes is tracked cron config.""" + from nerve.cron.jobs import save_jobs + + ws = tmp_path / "ws" + (ws / "config" / "cron").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + with pytest.raises(LockdownError): + save_jobs([], ws / "config" / "cron" / "jobs.yaml") + + @pytest.mark.asyncio + async def test_task_route_cannot_be_pointed_at_tracked_config(self, tmp_path, monkeypatch): + """``task["file_path"]`` is a stored path joined to the workspace. Today + the indexer only ever fills it from a glob of ``tasks/``, so this is depth + rather than a live hole — but it is the same shape as the memory PUT.""" + from nerve.gateway.routes import tasks as tasks_route + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + (ws / "config" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + + class _Db: + async def get_task(self, task_id): + return {"file_path": "config/settings.yaml", "status": "pending"} + + monkeypatch.setattr( + tasks_route, "get_deps", lambda: type("D", (), {"db": _Db()})(), + ) + with pytest.raises(LockdownError): + await tasks_route.update_task( + "t1", tasks_route.TaskUpdateRequest(content="lockdown: false\n"), + user={}, + ) + assert "lockdown: true" in (ws / "config" / "settings.yaml").read_text() + + +class TestLockdownTaskHandlersGuardWritesOnly: + """The task tools join a stored ``file_path`` to the workspace, so each one + that *changes* the file gets the guard. ``task_read`` must not: lockdown + makes tracked config unwritable, not unreadable. + """ + + async def _locked_task(self, tmp_path, db, monkeypatch): + """A locked instance whose task row points at tracked config.""" + from nerve.agent.tools.handlers import tasks as task_handlers + from nerve.agent.tools.registry import ToolContext + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + (ws / "config" / "settings.yaml").write_text( + "lockdown: true\n", encoding="utf-8", + ) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + # Process-wide read-before-write set: give each test its own so the + # write refusal below can only come from lockdown. + monkeypatch.setattr(task_handlers, "_tasks_read", set()) + await db.upsert_task( + task_id="t1", file_path="config/settings.yaml", + title="T1", status="pending", + ) + return ws, ToolContext(session_id="test", db=db, workspace=ws) + + @pytest.mark.asyncio + async def test_read_is_not_refused_by_the_write_guard(self, tmp_path, db, monkeypatch): + from nerve.agent.tools.handlers.tasks import task_read_handler + + _ws, ctx = await self._locked_task(tmp_path, db, monkeypatch) + result = await task_read_handler(ctx, {"task_id": "t1"}) + assert "lockdown: true" in result.content[0]["text"] + + @pytest.mark.asyncio + async def test_write_on_the_same_path_is_still_refused(self, tmp_path, db, monkeypatch): + from nerve.agent.tools.handlers.tasks import ( + task_read_handler, + task_write_handler, + ) + + ws, ctx = await self._locked_task(tmp_path, db, monkeypatch) + await task_read_handler(ctx, {"task_id": "t1"}) # satisfies read-before-write + with pytest.raises(LockdownError): + await task_write_handler( + ctx, {"task_id": "t1", "content": "lockdown: false\n"}, + ) + assert "lockdown: true" in (ws / "config" / "settings.yaml").read_text() + + @pytest.mark.asyncio + async def test_update_is_still_refused(self, tmp_path, db, monkeypatch): + from nerve.agent.tools.handlers.tasks import task_update_handler + + ws, ctx = await self._locked_task(tmp_path, db, monkeypatch) + with pytest.raises(LockdownError): + await task_update_handler(ctx, {"task_id": "t1", "note": "appended"}) + assert "lockdown: true" in (ws / "config" / "settings.yaml").read_text() + + @pytest.mark.asyncio + async def test_done_is_refused_before_it_unlinks(self, tmp_path, db, monkeypatch): + """``task_done`` copies the file into ``done/`` and unlinks the source — + a delete of tracked config, and the most destructive of the three.""" + from nerve.agent.tools.handlers.tasks import task_done_handler + + ws, ctx = await self._locked_task(tmp_path, db, monkeypatch) + with pytest.raises(LockdownError): + await task_done_handler(ctx, {"task_id": "t1"}) + assert (ws / "config" / "settings.yaml").exists() + # Refused before the DB was touched: no task left claiming to be done + # with its file still sitting in the config subtree. + assert (await db.get_task("t1"))["status"] == "pending" + + @pytest.mark.asyncio + async def test_an_ordinary_task_is_untouched_by_any_of_them(self, tmp_path, db, monkeypatch): + """The workspace is also the agent's working directory: a task file in + its normal home is not config, locked or not.""" + from nerve.agent.tools.handlers import tasks as task_handlers + from nerve.agent.tools.registry import ToolContext + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + active = ws / "memory" / "tasks" / "active" + active.mkdir(parents=True) + (active / "t2.md").write_text("# T2\n", encoding="utf-8") + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + monkeypatch.setattr(task_handlers, "_tasks_read", set()) + await db.upsert_task( + task_id="t2", file_path="memory/tasks/active/t2.md", + title="T2", status="pending", + ) + ctx = ToolContext(session_id="test", db=db, workspace=ws) + + assert "# T2" in ( + await task_handlers.task_read_handler(ctx, {"task_id": "t2"}) + ).content[0]["text"] + await task_handlers.task_write_handler( + ctx, {"task_id": "t2", "content": "# T2 edited\n"}, + ) + assert (active / "t2.md").read_text() == "# T2 edited\n" + await task_handlers.task_done_handler(ctx, {"task_id": "t2"}) + assert not (active / "t2.md").exists() + assert (ws / "memory" / "tasks" / "done" / "t2.md").exists() + + +class TestLockdownTaskManagerGuardsTheMove: + """``TaskManager.mark_done`` has the same read-copy-unlink shape as the + ``task_done`` tool — a stored ``file_path`` joined to the workspace, copied + into ``done/`` and then unlinked — so it needs the same guard, or it is a + second route to deleting tracked config. + + Both cases run locked, so the second one is what stops the guard from being + written as "refuse every move". + """ + + def _locked_manager(self, tmp_path, db, monkeypatch): + from nerve.tasks.manager import TaskManager + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + return ws, TaskManager(ws, db) + + @pytest.mark.asyncio + async def test_a_row_pointing_at_tracked_config_is_refused(self, tmp_path, db, monkeypatch): + ws, manager = self._locked_manager(tmp_path, db, monkeypatch) + (ws / "config" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + await db.upsert_task( + task_id="t1", file_path="config/settings.yaml", + title="T1", status="pending", + ) + + with pytest.raises(LockdownError): + await manager.mark_done("t1") + + assert (ws / "config" / "settings.yaml").read_text() == "lockdown: true\n" + # Nothing half-done: no copy left in done/, no row claiming otherwise. + assert not (ws / "memory" / "tasks" / "done" / "settings.yaml").exists() + assert (await db.get_task("t1"))["status"] == "pending" + + @pytest.mark.asyncio + async def test_an_ordinary_task_file_still_moves(self, tmp_path, db, monkeypatch): + ws, manager = self._locked_manager(tmp_path, db, monkeypatch) + active = ws / "memory" / "tasks" / "active" + (active / "t2.md").write_text("# T2\n", encoding="utf-8") + await db.upsert_task( + task_id="t2", file_path="memory/tasks/active/t2.md", + title="T2", status="pending", + ) + + assert await manager.mark_done("t2") is True + assert not (active / "t2.md").exists() + assert "DONE" in (ws / "memory" / "tasks" / "done" / "t2.md").read_text() + assert (await db.get_task("t2"))["status"] == "done" + + +class TestSkillIdIsOnePathComponent: + """``skill_id`` reaches ``skills_dir / skill_id`` straight from an HTTP path + segment; ``_slugify`` only runs on create. Delete removes the whole tree it + names, so ``../config`` took out the tracked config subtree.""" + + @pytest.mark.asyncio + async def test_delete_cannot_escape_the_skills_directory(self, tmp_path, db): + from nerve.skills.manager import SkillIdError, SkillManager + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + (ws / "config" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + mgr = SkillManager(ws, db) + for bad in ("../config", "../../etc", "..", ".", "a/b", ".hidden"): + with pytest.raises(SkillIdError): + await mgr.delete_skill(bad) + assert (ws / "config" / "settings.yaml").exists() + + @pytest.mark.asyncio + async def test_existing_directory_names_stay_usable(self, tmp_path, db): + """The filesystem is the source of truth and discover() adopts whatever + directory names it finds, so the check has to be wider than _slugify.""" + from nerve.skills.manager import SkillManager + + ws = tmp_path / "ws" + mgr = SkillManager(ws, db) + skill_dir = ws / "skills" / "My_Skill.v2" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: My Skill\ndescription: d\n---\nbody\n", encoding="utf-8", + ) + await mgr.discover() + assert await mgr.update_skill( + "My_Skill.v2", "---\nname: My Skill\ndescription: e\n---\nbody\n", + ) is not None + + +class TestLockdownEnvironmentAnchor: + """Hardening the flag's value says nothing about which file supplies it. + + ``workspace:`` lives in the machine-local ``config.yaml`` and selects the + ``settings.yaml`` that is then the only authority, so one local edit repoints + the whole chain and lockdown evaluates to false. The anchor moves the decision + out of every file on the box and into the service definition. + """ + + def _two_trees(self, tmp_path, *, machine_workspace): + """A locked real workspace and an unlocked one an attacker points at.""" + config_dir = tmp_path / "cfg" + real = tmp_path / "real-ws" + attacker = tmp_path / "attacker-ws" + config_dir.mkdir() + (real / "config").mkdir(parents=True) + (attacker / "config").mkdir(parents=True) + workspace_settings_file(real).write_text( + "lockdown: true\ntimezone: UTC\n" + _JWT, encoding="utf-8", + ) + workspace_settings_file(attacker).write_text( + "timezone: Europe/Berlin\n", encoding="utf-8", + ) + (config_dir / "config.yaml").write_text( + f"workspace: {machine_workspace}\nauth:\n jwt_secret: attacker\n", + encoding="utf-8", + ) + return config_dir, real, attacker + + def test_repointing_the_workspace_unlocks_an_unanchored_box(self, tmp_path, monkeypatch): + """The bypass, demonstrated. Nothing here is a bug on its own — this is + what the anchor exists for, and it is why the anchor has to cover the + workspace as well as the flag.""" + monkeypatch.delenv("NERVE_LOCKDOWN", raising=False) + config_dir, real, attacker = self._two_trees( + tmp_path, machine_workspace=tmp_path / "attacker-ws", + ) + c = load_config(config_dir) + assert c.lockdown is False + assert c.workspace == attacker + # ...and with the machine layers back, so is the machine's jwt_secret. + assert c.auth.jwt_secret == "attacker" + + def test_the_anchor_closes_it(self, tmp_path, monkeypatch): + monkeypatch.setenv("NERVE_LOCKDOWN", "1") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "real-ws")) + config_dir, real, attacker = self._two_trees( + tmp_path, machine_workspace=tmp_path / "attacker-ws", + ) + c = load_config(config_dir) + assert c.lockdown is True + assert c.workspace == real # the repoint is ignored + assert c.timezone == "UTC" # from the real tracked settings + assert c.auth.jwt_secret == "test-secret" # not the machine layer's + + def test_the_anchor_requires_a_workspace(self, tmp_path, monkeypatch): + """Anchoring the flag alone would produce a box locked *onto* whatever + tree config.yaml names — worse than unlocked, since it now trusts that + tree exclusively.""" + monkeypatch.setenv("NERVE_LOCKDOWN", "true") + monkeypatch.delenv("NERVE_WORKSPACE", raising=False) + config_dir, real, attacker = self._two_trees( + tmp_path, machine_workspace=tmp_path / "attacker-ws", + ) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "NERVE_WORKSPACE" in str(ei.value) + + def test_a_relative_anchor_workspace_is_refused(self, tmp_path, monkeypatch): + monkeypatch.setenv("NERVE_LOCKDOWN", "true") + monkeypatch.setenv("NERVE_WORKSPACE", "relative/ws") + config_dir, real, attacker = self._two_trees( + tmp_path, machine_workspace=tmp_path / "real-ws", + ) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "absolute" in str(ei.value) + + @pytest.mark.parametrize("value", ["false", "0", "off", "no", ""]) + def test_the_anchor_never_unlocks(self, tmp_path, monkeypatch, value): + """Monotonic by design: the env can add lockdown, never remove it. A + box whose reviewed config says locked cannot be unlocked from the + environment — that still takes a merged change.""" + monkeypatch.setenv("NERVE_LOCKDOWN", value) + monkeypatch.delenv("NERVE_WORKSPACE", raising=False) + config_dir, ws = _install(tmp_path, settings="lockdown: true\n" + _JWT) + assert load_config(config_dir).lockdown is True + + @pytest.mark.parametrize("value", ["false", "0", "off", ""]) + def test_no_opinion_leaves_an_unlocked_box_alone(self, tmp_path, monkeypatch, value): + monkeypatch.setenv("NERVE_LOCKDOWN", value) + monkeypatch.delenv("NERVE_WORKSPACE", raising=False) + config_dir, ws = _install(tmp_path, base="timezone: UTC\n") + c = load_config(config_dir) + assert c.lockdown is False + assert c.timezone == "UTC" # machine layer still applies + + @pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", "on"]) + def test_accepted_spellings(self, tmp_path, monkeypatch, value): + monkeypatch.setenv("NERVE_LOCKDOWN", value) + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "ws")) + config_dir, ws = _install(tmp_path, settings=_JWT) + assert load_config(config_dir).lockdown is True + + def test_an_unreadable_anchor_is_refused(self, tmp_path, monkeypatch): + """The only other reading is "no opinion", which would silently discard + an instruction to lock.""" + monkeypatch.setenv("NERVE_LOCKDOWN", "maybe") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "ws")) + config_dir, ws = _install(tmp_path, settings=_JWT) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "NERVE_LOCKDOWN" in str(ei.value) + + def test_the_validator_judges_the_anchored_view(self, tmp_path, monkeypatch): + from nerve.config_validate import validate_config_bundle + + monkeypatch.setenv("NERVE_LOCKDOWN", "1") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "ws")) + # The bundle never says lockdown, so an unlocked run would judge the + # unlocked view; the environment is what makes this the locked one. + config_dir, ws = _install(tmp_path, base="timezone: America/New_York\n") + result = validate_config_bundle(config_dir) + assert any("LOCKED view" in i for i in result.info) + # config.yaml is what a locked box discards, so its keys are named. + assert any("timezone" in w for w in result.warnings), result.warnings + + def test_an_explicit_workspace_still_wins_for_the_validator(self, tmp_path, monkeypatch): + """CI is not the anchored box; --workspace points at a checkout.""" + from nerve.config_validate import validate_config_bundle + + monkeypatch.setenv("NERVE_LOCKDOWN", "1") + monkeypatch.setenv("NERVE_WORKSPACE", str(tmp_path / "nonexistent")) + config_dir, ws = _install(tmp_path, settings="timezone: UTC\n" + _JWT) + result = validate_config_bundle(config_dir, workspace_override=ws) + assert result.ok, result.errors + + +class TestAgentCannotWriteTrackedConfig: + """``Write``/``Edit`` are auto-approved for every non-interactive tool, and + never passed through the REST guards, so the agent's ordinary way of editing + a file reached the config the box promises to run. + + Not a sandbox: ``Bash`` is auto-approved on the same path and is deliberately + not filtered. See ``lockdown_denial``. + """ + + def _hub(self, session_id="s1"): + class _Hub: + snapshot_fn = None + interactive_capable = False + + def __init__(self, sid): + self.session_id = sid + + def mark_snapshotted(self, _p): + return False + + return _Hub(session_id) + + def _locked(self, monkeypatch, tmp_path): + ws = tmp_path / "ws" + (ws / "config" / "cron" / "gates").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + return ws + + @pytest.mark.asyncio + @pytest.mark.parametrize("tool", ["Write", "Edit", "NotebookEdit"]) + async def test_can_use_tool_denies_writes_into_config(self, tmp_path, monkeypatch, tool): + from nerve.agent.backends.claude import ClaudeToolPermissions + + ws = self._locked(monkeypatch, tmp_path) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + tool, {"file_path": str(ws / "config" / "settings.yaml")}, context=None, + ) + assert type(result).__name__ == "PermissionResultDeny" + assert "lockdown" in result.message + # The refusal has to route the agent somewhere, not just say no. + assert "pull request" in result.message + + @pytest.mark.asyncio + async def test_a_gate_plugin_is_refused_too(self, tmp_path, monkeypatch): + from nerve.agent.backends.claude import ClaudeToolPermissions + + ws = self._locked(monkeypatch, tmp_path) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + "Write", + {"file_path": str(ws / "config" / "cron" / "gates" / "evil.py")}, + context=None, + ) + assert type(result).__name__ == "PermissionResultDeny" + + @pytest.mark.asyncio + async def test_a_relative_path_is_resolved_against_the_workspace(self, tmp_path, monkeypatch): + from nerve.agent.backends.claude import ClaudeToolPermissions + + self._locked(monkeypatch, tmp_path) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + "Write", {"file_path": "config/settings.yaml"}, context=None, + ) + assert type(result).__name__ == "PermissionResultDeny" + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "path", ["memory/notes.md", "skills/x/SKILL.md", "tasks/active/t1.md"], + ) + async def test_ordinary_agent_writes_are_untouched(self, tmp_path, monkeypatch, path): + from nerve.agent.backends.claude import ClaudeToolPermissions + + ws = self._locked(monkeypatch, tmp_path) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + "Write", {"file_path": str(ws / path)}, context=None, + ) + assert type(result).__name__ == "PermissionResultAllow" + + @pytest.mark.asyncio + async def test_unlocked_writes_config_freely(self, tmp_path, monkeypatch): + from nerve.agent.backends.claude import ClaudeToolPermissions + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False, workspace=ws)) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + "Write", {"file_path": str(ws / "config" / "settings.yaml")}, context=None, + ) + assert type(result).__name__ == "PermissionResultAllow" + + def test_bash_is_deliberately_not_filtered(self, tmp_path, monkeypatch): + """The documented gap, asserted so it cannot be quietly "fixed" with a + command-string filter that looks like a boundary and is not one.""" + from nerve.agent.backends.claude import lockdown_denial + + ws = self._locked(monkeypatch, tmp_path) + assert lockdown_denial( + "Bash", {"command": f"echo x > {ws}/config/settings.yaml"}, + ) is None + + def test_the_background_subagent_hook_denies_too(self, tmp_path, monkeypatch): + """For a background sub-agent the PreToolUse hook is the only thing that + runs, so an allow issued there is the whole decision.""" + from nerve.agent.backends.claude import lockdown_denial + + ws = self._locked(monkeypatch, tmp_path) + assert lockdown_denial( + "Write", {"file_path": str(ws / "config" / "settings.yaml")}, + ) diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 1c4be711..e09b321d 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -426,6 +426,59 @@ def test_ignored_file_warns_but_does_not_block(self, tmp_path): assert result.ok and result.changed, result.message assert any("secrets.local.yaml" in w for w in result.validation_warnings) + def test_ignored_gate_plugin_blocks_a_locked_instance(self, tmp_path): + """A locked box promises that only reviewed remote config runs on it. + + An ignored file in the config subtree is invisible to the reviewer, to + the validation checkout, and to the blocking check above — but a + ``cron/gates/*.py`` there is imported and executed by the daemon all the + same. On an ordinary box refusing over it would be sync policing what the + machine may hold locally; a locked box has already settled that question, + so the same file is a refusal there and a warning here. + """ + origin, ws = self._pair(tmp_path) + (origin / ".gitignore").write_text("config/cron/gates/local_*.py\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "ignore", cwd=origin) + self._git("pull", "-q", "--ff-only", "origin", "main", cwd=ws) + self._upstream_commit(origin) + gates = ws / "config" / "cron" / "gates" + gates.mkdir(parents=True) + (gates / "local_unreviewed.py").write_text("MARKER = 1\n") + + before = self._git("rev-parse", "HEAD", cwd=ws).stdout.strip() + unlocked = sync_workspace(ws, tmp_path / "cfg", branch="main", validate=True) + assert unlocked.ok, unlocked.message + assert any("local_unreviewed.py" in w for w in unlocked.validation_warnings) + + # Same tree, same commit — only lockdown differs. + self._git("reset", "-q", "--hard", before, cwd=ws) + locked = sync_workspace( + ws, tmp_path / "cfg", branch="main", validate=True, locked=True, + ) + assert not locked.ok and not locked.changed + assert "local_unreviewed.py" in locked.message + assert "Europe/Berlin" not in (ws / "config" / "settings.yaml").read_text() + + def test_unset_env_var_is_reported_even_when_tolerated(self, tmp_path): + """With strict_env off the validator files this as *info*, which nothing + propagated — so the gate saw the one signal that predicts a failed + post-merge reload and dropped it. Tolerating it is what the setting asks + for; saying nothing about it is not.""" + origin, ws = self._pair(tmp_path) + (origin / "config" / "settings.yaml").write_text( + "timezone: ${NO_SUCH_VAR_FOR_SYNC}\n", + ) + self._git("commit", "-am", "env ref", cwd=origin) + + result = sync_workspace( + ws, tmp_path / "cfg", branch="main", validate=True, strict_env=False, + ) + assert result.ok and result.changed, result.message + assert any( + "NO_SUCH_VAR_FOR_SYNC" in w for w in result.validation_warnings + ), result.validation_warnings + def test_submodule_in_the_subtree_is_flagged_as_unvalidated(self, tmp_path): origin, ws = self._pair(tmp_path) sha = self._git("rev-parse", "HEAD", cwd=origin).stdout.strip() @@ -636,20 +689,96 @@ def test_validation_actually_skipped_when_env_says_off(self, tmp_path, monkeypat class TestApplySync: @pytest.mark.asyncio - async def test_apply_triggers_both_reloads(self): + async def test_apply_triggers_both_reloads(self, tmp_path): cron, engine = AsyncMock(), AsyncMock() - await sync._apply_sync(engine, cron) + await sync._apply_sync(engine, cron, tmp_path) cron.reload.assert_awaited_once() engine.reload_mcp_config.assert_awaited_once() @pytest.mark.asyncio - async def test_apply_survives_reload_error(self): + async def test_apply_survives_reload_error(self, tmp_path): cron = AsyncMock() cron.reload.side_effect = RuntimeError("boom") engine = AsyncMock() - await sync._apply_sync(engine, cron) # must not raise + await sync._apply_sync(engine, cron, tmp_path) # must not raise engine.reload_mcp_config.assert_awaited_once() + @pytest.mark.asyncio + async def test_apply_reloads_config_singleton_engages_lockdown(self, tmp_path, monkeypatch): + """A synced lockdown flip must engage without a restart.""" + import nerve.config as cfgmod + from nerve.config import is_locked, workspace_settings_file + + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir(parents=True) + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + workspace_settings_file(ws).write_text( + "lockdown: true\nauth:\n jwt_secret: x\n", encoding="utf-8" + ) + monkeypatch.setattr(cfgmod, "_config", cfgmod.NerveConfig(lockdown=False)) + assert not is_locked() + err = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) + assert err is None + assert is_locked() # the reloaded config engaged lockdown + + @pytest.mark.asyncio + async def test_apply_reports_when_lockdown_fails_to_engage(self, tmp_path, monkeypatch): + """The merge landed `lockdown: true` and the daemon could not load it, so + the box is *not* locked. That has to come back to the caller: reporting a + clean success here tells an operator the write guards are closed while + they are wide open.""" + import nerve.config as cfgmod + from nerve.config import is_locked, workspace_settings_file + + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir(parents=True) + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + # Locks the box and names a variable this environment does not have, so + # load_config raises on the merged result. + workspace_settings_file(ws).write_text( + "lockdown: true\nauth:\n jwt_secret: ${NO_SUCH_SECRET_HERE}\n", + encoding="utf-8", + ) + monkeypatch.setattr(cfgmod, "_config", cfgmod.NerveConfig(lockdown=False)) + err = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) + assert err and "NO_SUCH_SECRET_HERE" in err + assert not is_locked() # still running the old config — which the caller must say + + @pytest.mark.asyncio + async def test_route_does_not_claim_success_when_apply_failed(self, tmp_path, monkeypatch): + import nerve.gateway.server as srv + + import nerve.gateway.routes.config as route_mod + from nerve.sync_service import SyncResult + + fake_cfg = type("C", (), {})() + fake_cfg.workspace = str(tmp_path / "ws") + fake_cfg.config_dir = str(tmp_path / "cfg") + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True) + fake_cfg.lockdown = False + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr( + route_mod, "get_deps", lambda: type("D", (), {"engine": None})(), + ) + monkeypatch.setattr( + sync, "sync_workspace", + lambda *a, **k: SyncResult(ok=True, changed=True, message="updated"), + ) + + async def _failing_apply(engine, cron_service, config_dir): + return "ConfigError: nope" + + monkeypatch.setattr(sync, "_apply_sync", _failing_apply) + body = await route_mod.sync_workspace_route(user={}) + assert body["ok"] is False + assert body["changed"] is True and body["applied"] is False + assert body["apply_error"] == "ConfigError: nope" + class TestNeverRaises: """``sync_workspace`` promises a SyncResult for every outcome. @@ -750,6 +879,7 @@ async def test_route_reports_400_not_500(self, tmp_path, monkeypatch): fake_cfg.workspace = str(ws) fake_cfg.config_dir = str(tmp_path / "cfg") fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True, validate=True) + fake_cfg.lockdown = False monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) monkeypatch.setattr(srv, "_cron_service", None, raising=False) monkeypatch.setattr(sync, "_git", _FakeGit(head="aaaaaaaa", upstream="bbbbbbbb")) @@ -761,7 +891,6 @@ async def test_route_reports_400_not_500(self, tmp_path, monkeypatch): await route_mod.sync_workspace_route(user={}) assert ei.value.status_code == 400 - class _FakeClock: """Replaces the loop's interval wait so cycles run without real time passing. @@ -816,6 +945,7 @@ def _loop_config(**overrides): cfg = type("C", (), {})() cfg.workspace = overrides.pop("workspace", "/tmp/ws") cfg.config_dir = overrides.pop("config_dir", None) + cfg.lockdown = overrides.pop("lockdown", False) overrides.setdefault("enabled", True) overrides.setdefault("interval_minutes", 60) cfg.workspace_sync = WorkspaceSyncConfig(**overrides) @@ -968,6 +1098,7 @@ async def test_route_400_on_invalid(self, monkeypatch): fake_cfg.workspace = "/tmp/ws" fake_cfg.config_dir = "/tmp/cfg" fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True) + fake_cfg.lockdown = False monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) monkeypatch.setattr(srv, "_cron_service", None, raising=False) # The route imports sync_workspace from nerve.sync_service at call time. @@ -978,3 +1109,39 @@ async def test_route_400_on_invalid(self, monkeypatch): with pytest.raises(HTTPException) as ei: await route_mod.sync_workspace_route(user={}) assert ei.value.status_code == 400 + + @pytest.mark.asyncio + async def test_route_applies_a_changed_sync(self, monkeypatch, tmp_path): + """The success path, which the 400 tests never reach: it resolves the + config dir before applying, so a name that only exists on that branch + would otherwise surface as a 500 on the first sync that changed anything. + """ + import nerve.gateway.server as srv + + import nerve.gateway.routes.config as route_mod + from nerve.sync_service import SyncResult + + fake_cfg = type("C", (), {})() + fake_cfg.workspace = str(tmp_path / "ws") + fake_cfg.config_dir = str(tmp_path / "cfg") + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True) + fake_cfg.lockdown = False + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr( + route_mod, "get_deps", lambda: type("D", (), {"engine": None})(), + ) + monkeypatch.setattr( + sync, "sync_workspace", + lambda *a, **k: SyncResult(ok=True, changed=True, message="updated"), + ) + applied: list = [] + + async def _fake_apply(engine, cron_service, config_dir): + applied.append(Path(config_dir)) + + monkeypatch.setattr(sync, "_apply_sync", _fake_apply) + body = await route_mod.sync_workspace_route(user={}) + assert body["ok"] and body["changed"] + assert applied == [tmp_path / "cfg"] + From 78025e5ef49de7ebf4a26a23edbac03db7ad78a1 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 30 Jul 2026 16:59:32 +0200 Subject: [PATCH 2/5] Review: check containment per reviewed file, not just per subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lockdown checks verified that `config/` resolves inside the workspace, then trusted everything under it. `settings.yaml` carries the lockdown flag itself, the auth secret and every cron path, and a symlink there was invisible: `lockdown_workspace_problems` returned `[]`, the instance came up reporting `lockdown = True`, and the tracked layer arrived from a file the reviewer never saw. With a relative link the target is an ordinary workspace file the agent may write directly, so "still inside the workspace" was not good enough. This is an omission against a rule the branch already set for itself — cron's `jobs_file`, `system_file` and `gate_plugins_dir` each get a per-file containment check, including their default names as symlinks, and a test comment already says the intent covers "settings.yaml and the gate plugins". It is not a sandbox escape: lockdown is a config-integrity control, the agent has an unfiltered shell, and installing the link needs access that already suffices on its own. What it defeats is the audit trail — a `120000` mode change is not a `lockdown` flip, and after merge nothing reported the layout. The write guard resolved its target, so a symlink at a reviewed *name* made it conclude the write was outside the subtree and allow it. Both views are now checked. The resolved one stays, and is what already refuses the opposite direction — a link in the workspace pointing into `config/`, a directory link, a `..` climb — so the two are paired deliberately, and the inverse cases are pinned by tests so a later refactor cannot swap one for the other. Also refuses a symlinked `config/` directory under lockdown. git does not descend into one, so `git status -- /config` reports nothing whatever is underneath: sync calls the workspace clean while the daemon loads settings.yaml and imports `cron/gates/*.py` from a directory the config repo has never held a file of. Verified rather than assumed — `_local_config_divergence` returns `([], [])` for an untracked gate plugin there. Problems are now collected instead of returning the first, so an operator fixing a layout sees the whole list. `config_validate` already calls this, so `nerve config validate` and sync's pre-merge validation inherit all of it. Co-Authored-By: Claude --- docs/config.md | 18 +++- nerve/config.py | 100 ++++++++++++++++++-- tests/test_lockdown.py | 203 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 307 insertions(+), 14 deletions(-) diff --git a/docs/config.md b/docs/config.md index 64afbe05..1adeb4b2 100644 --- a/docs/config.md +++ b/docs/config.md @@ -366,10 +366,20 @@ Two things to know about `NERVE_LOCKDOWN`: (memory, skills content, task files) are unaffected. `Bash` is not covered; see below. - **The tracked config subtree must really be in the workspace.** - `/config` has to resolve inside ``. If it is a symlink out, - nothing under it is part of the reviewed repo, `settings.yaml` included, and the - instance refuses to start. Where the workspace itself lives stays a machine-local - decision, symlink included. + `/config` has to resolve inside ``, and has to be a real + directory rather than a symlink. If it is a symlink out, nothing under it is part + of the reviewed repo, `settings.yaml` included. If it is a symlink to a sibling + inside the workspace, git does not descend into it, so nothing under it is tracked + and sync reports the workspace clean whatever it holds. Either way the instance + refuses to start. Where the workspace itself lives stays a machine-local decision, + symlink included. +- **`settings.yaml` must be inside that subtree.** `/config/settings.yaml` + has to resolve inside `/config/`, so a symlink there cannot source the + lockdown flag, the auth secret and the cron paths from a file no reviewer saw — an + ordinary workspace file the agent can write included. A link to another file + *within* the subtree is fine; both ends are tracked. The write guard refuses the + path `config/settings.yaml` by name as well as by where it resolves, so a symlink + cannot turn a reviewed name into an allowed write. - **Cron cannot be pointed out of the workspace.** `cron.jobs_file`, `cron.system_file` and `cron.gate_plugins_dir` must resolve inside `/config/`. One that does not is ignored, with a warning, in favour of diff --git a/nerve/config.py b/nerve/config.py index c5edd663..7a7dcfd5 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -468,6 +468,31 @@ def _is_within(path: Path, root: Path) -> bool: return False +def _is_lexically_within(path: Path, root: Path) -> bool: + """True if ``path`` *names* a location under ``root``, symlinks not followed. + + The counterpart to :func:`_is_within`, and needed because the two disagree + exactly where a symlink is. ``_is_within`` answers "where does this path end + up", which is the right question for a path that reaches into the subtree + from outside. It is the wrong question for a reviewed *name*: put a symlink + at ``config/settings.yaml`` and the resolved path leaves the subtree, so a + guard that only resolves concludes the write is none of its business and + permits it — writing, through the link, the file the daemon then loads as + tracked config. + + ``normpath`` collapses ``.`` and ``..`` textually, without touching the + filesystem, which is the point: a lexical comparison a symlink can influence + would answer the resolved question again. Collapsing ``..`` across a + symlinked directory can name a location the OS would not, so this can + over-refuse; callers pair it with :func:`_is_within` and only ever add + refusals that way. + """ + try: + return Path(os.path.normpath(path)).is_relative_to(os.path.normpath(root)) + except (OSError, RuntimeError, ValueError): + return False + + def read_machine_layers(config_dir: Path) -> dict[str, Any]: """The machine-local overlay on its own, for diagnostics. @@ -2629,17 +2654,61 @@ def lockdown_workspace_problems(workspace: Path) -> list[str]: instance that fails it does not start. The workspace *path* may still be anything, including a symlink: where a machine keeps its workspace is a machine-local decision and the one thing lockdown never took away. + + Two more ways the same claim fails, checked here for the same reason. The + subtree can stay inside the workspace and still not be the reviewed tree — + ``config`` as a symlink to a sibling directory — and ``settings.yaml`` alone + can leave it while everything around it stays put. + + Every problem found is reported rather than the first: an operator fixing a + layout wants the whole list, and the caller that refuses to boot only needs + one of them. """ + problems: list[str] = [] config_root = workspace_config_dir(workspace) - if _is_within(config_root, workspace): - return [] - return [ - f"lockdown is enabled but the tracked config subtree {config_root} " - f"resolves to {_resolved(config_root)}, outside the workspace " - f"{_resolved(workspace)} — so nothing it holds is part of the reviewed " - f"repo. A locked instance takes its config only from the workspace's own " - f"config/ directory." - ] + if not _is_within(config_root, workspace): + problems.append( + f"lockdown is enabled but the tracked config subtree {config_root} " + f"resolves to {_resolved(config_root)}, outside the workspace " + f"{_resolved(workspace)} — so nothing it holds is part of the reviewed " + f"repo. A locked instance takes its config only from the workspace's own " + f"config/ directory." + ) + elif config_root.is_symlink(): + # Contained, and still not the reviewed tree. git does not descend into a + # symlinked directory, so `git status -- /config` reports nothing no + # matter what is under there: sync calls the workspace clean while the + # daemon reads settings.yaml, and imports cron/gates/*.py, from a + # directory the config repo has never seen a single file of. The + # arrangement is also writable by the agent through its real path, which + # the write guard judges against config/ and lets past. + problems.append( + f"lockdown is enabled but the tracked config subtree {config_root} is a " + f"symlink to {_resolved(config_root)}. git does not track anything " + f"through a symlinked directory, so no file under it is part of the " + f"reviewed repo and a sync would report the workspace clean regardless " + f"of what it holds. A locked instance's config/ must be a real " + f"directory." + ) + + # The subtree can be the reviewed tree and this one file still not be part of + # it. settings.yaml carries the lockdown flag itself, the auth secret and + # every cron path, so a symlink here is the whole tracked layer arriving from + # somewhere the reviewer never looked — including from an ordinary workspace + # file the agent may write, which is why "still inside the workspace" is not + # good enough. The write guard refuses the name; this refuses the instance, + # because a file that is already in place was never written through the guard. + settings = workspace_settings_file(workspace) + if not _is_within(settings, config_root): + problems.append( + f"lockdown is enabled but the tracked settings file {settings} resolves " + f"to {_resolved(settings)}, outside the tracked config subtree " + f"{_resolved(config_root)} — so the file that states lockdown, and " + f"everything else this instance runs on, is not part of the reviewed " + f"repo. A locked instance reads its settings only from the workspace's " + f"own config/ directory." + ) + return problems def _has_dotted(data: dict[str, Any], dotted: str) -> bool: @@ -2918,6 +2987,15 @@ def tracked_config_write_refusal(path: Path | str) -> str | None: changed directory into a subtree of its own with a ``config/`` in it; erring that way costs a clear message and a retry, and erring the other way is the hole this exists to close. + + Both views of the path are checked, because a symlink is the case where they + disagree and each direction of the disagreement is a way through. Resolving + catches a path that arrives from outside and lands in the subtree — a link in + the workspace pointing at ``config/``, a directory link, a ``..`` climb. The + lexical name catches the reverse: a symlink sitting *at* a reviewed path + resolves elsewhere, and a guard that only resolved would decide the write was + outside the subtree and allow it, while the daemon goes on loading that name + as tracked config. """ if not is_locked(): return None @@ -2926,7 +3004,9 @@ def tracked_config_write_refusal(path: Path | str) -> str | None: if not target.is_absolute(): target = workspace / target config_root = workspace_config_dir(workspace) - if not _is_within(target, config_root): + if not ( + _is_within(target, config_root) or _is_lexically_within(target, config_root) + ): return None return ( f"this instance is in lockdown (remote-only, read-only) and {target} is " diff --git a/tests/test_lockdown.py b/tests/test_lockdown.py index 0f5ec2d2..36335a22 100644 --- a/tests/test_lockdown.py +++ b/tests/test_lockdown.py @@ -1,5 +1,8 @@ """Tests for lockdown / remote-only read-only mode.""" +import os +import shutil +import subprocess from pathlib import Path import pytest @@ -13,6 +16,8 @@ ensure_not_locked, is_locked, load_config, + lockdown_workspace_problems, + tracked_config_write_refusal, workspace_settings_file, ) @@ -606,6 +611,204 @@ def test_validation_contains_paths_against_the_candidate_workspace(self, tmp_pat assert result.ok, result.errors assert any(str(candidate) in i and "cron jobs" in i for i in result.info) + def _sibling(self, tmp_path, settings): + """``config`` as a symlink to a sibling directory *inside* the workspace.""" + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir() + (ws / "notes" / "cfg" / "cron" / "gates").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + (ws / "config").symlink_to(Path("notes") / "cfg", target_is_directory=True) + (ws / "notes" / "cfg" / "settings.yaml").write_text(settings, encoding="utf-8") + return config_dir, ws + + def test_config_symlinked_to_a_sibling_directory_refuses_to_load(self, tmp_path): + """Containment alone does not make the subtree the reviewed one. The link + target is inside the workspace, so every check that resolves passes.""" + config_dir, ws = self._sibling(tmp_path, "lockdown: true\n" + _JWT) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "must be a real directory" in str(ei.value) + + def test_a_symlinked_config_stays_fine_unlocked(self, tmp_path): + config_dir, ws = self._sibling(tmp_path, "timezone: UTC\n") + assert load_config(config_dir).timezone == "UTC" + + @pytest.mark.skipif(not shutil.which("git"), reason="git not available") + def test_git_reports_nothing_under_a_symlinked_config(self, tmp_path): + """Why the symlink is refused rather than tolerated: git does not descend + into it, so sync's divergence check — the thing that keeps unreviewed + ``cron/gates/*.py`` off a locked box — sees an empty subtree and calls the + workspace clean while the daemon imports what is there. + """ + from nerve.sync_service import _local_config_divergence + + config_dir, ws = self._sibling(tmp_path, "lockdown: true\n" + _JWT) + env = {"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", + "HOME": str(tmp_path), "PATH": os.environ["PATH"]} + for args in (["init", "-b", "main"], ["add", "-A"], ["commit", "-m", "init"]): + subprocess.run(["git", *args], cwd=str(ws), check=True, + capture_output=True, text=True, env=env) + (ws / "notes" / "cfg" / "cron" / "gates" / "evil.py").write_text( + "MARKER = 1\n", encoding="utf-8", + ) + # Untracked code the daemon would import, and git says the subtree is clean. + assert _local_config_divergence(ws, "HEAD", True) == ([], []) + # So the layout has to be refused at the only place that can see it. + with pytest.raises(ConfigError): + load_config(config_dir) + + +class TestLockdownSettingsFileIsInTheSubtree: + """The subtree can be the reviewed tree and ``settings.yaml`` still not be + part of it. It carries the ``lockdown`` flag, the auth secret and the cron + paths, so a symlink there is the whole tracked layer arriving from a file no + reviewer saw — and, when it points at an ordinary workspace file, from one the + agent may write with the tools lockdown leaves it. + """ + + def _linked(self, tmp_path, target, settings="lockdown: true\n" + _JWT): + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir() + (ws / "config").mkdir(parents=True) + (ws / "notes").mkdir() + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + real = ws / "notes" / "settings.yaml" if target == "inside" else tmp_path / "outside.yaml" + real.write_text(settings, encoding="utf-8") + link = Path("..") / "notes" / "settings.yaml" if target == "inside" else real + workspace_settings_file(ws).symlink_to(link) + return config_dir, ws + + def test_settings_symlinked_out_of_the_workspace_refuses_to_load(self, tmp_path): + config_dir, ws = self._linked(tmp_path, "outside") + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the tracked config subtree" in str(ei.value) + + def test_settings_symlinked_to_an_ordinary_workspace_file_refuses_to_load(self, tmp_path): + """A relative link that never leaves the workspace, which is the harder + case: nothing about it escapes, and the target is a file the agent writes + the same way it writes its notes.""" + config_dir, ws = self._linked( + tmp_path, "inside", "lockdown: true\nauth:\n jwt_secret: attacker\n", + ) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the tracked config subtree" in str(ei.value) + + def test_the_validator_reports_it(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + config_dir, ws = self._linked(tmp_path, "inside") + result = validate_config_bundle(config_dir, workspace_override=ws) + assert any("outside the tracked config subtree" in e for e in result.errors) + + def test_unlocked_is_unaffected(self, tmp_path): + config_dir, ws = self._linked(tmp_path, "outside", "timezone: UTC\n") + assert load_config(config_dir).timezone == "UTC" + + def test_a_real_settings_file_loads(self, tmp_path): + config_dir, ws = _install(tmp_path, settings="lockdown: true\ntimezone: UTC\n" + _JWT) + assert lockdown_workspace_problems(ws) == [] + assert load_config(config_dir).timezone == "UTC" + + def test_an_absent_settings_file_is_not_a_problem(self, tmp_path): + """Nothing to point anywhere. A locked instance can be anchored by the + environment with no tracked settings file at all.""" + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + assert lockdown_workspace_problems(ws) == [] + + def test_a_link_within_the_subtree_is_fine(self, tmp_path): + """Both ends are tracked and reviewed, so the name is a layout choice the + config repo is entitled to make. The check is containment, not a ban on + symlinks.""" + config_dir, ws = _install(tmp_path) + (ws / "config" / "shared.yaml").write_text( + "lockdown: true\ntimezone: UTC\n" + _JWT, encoding="utf-8", + ) + workspace_settings_file(ws).symlink_to(Path("shared.yaml")) + assert load_config(config_dir).timezone == "UTC" + + def test_every_problem_is_reported_not_just_the_first(self, tmp_path): + """An operator fixing a layout wants the list; the loader only needs one.""" + ws = tmp_path / "ws" + (ws / "notes" / "cfg").mkdir(parents=True) + (ws / "notes" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + (ws / "config").symlink_to(Path("notes") / "cfg", target_is_directory=True) + (ws / "notes" / "cfg" / "settings.yaml").symlink_to( + Path("..") / "settings.yaml", + ) + problems = lockdown_workspace_problems(ws) + assert len(problems) == 2 + assert any("must be a real directory" in p for p in problems) + assert any("outside the tracked config subtree" in p for p in problems) + + +class TestLockdownWriteGuardJudgesBothViewsOfAPath: + """A symlink is where "where does this path end up" and "what does this path + name" stop agreeing, and each direction of the disagreement is a way through. + Both are checked, and both directions are pinned here so a later refactor + cannot drop one for the other. + """ + + def _locked(self, tmp_path, monkeypatch): + ws = tmp_path / "ws" + (ws / "config" / "cron" / "gates").mkdir(parents=True) + (ws / "notes").mkdir() + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + return ws + + def test_a_symlink_at_the_reviewed_name_is_refused(self, tmp_path, monkeypatch): + """The lexical direction. Resolving alone puts the target outside the + subtree, which reads as "not lockdown's business" — while the write goes + through the link to the file the daemon loads as tracked config.""" + ws = self._locked(tmp_path, monkeypatch) + outside = tmp_path / "outside.yaml" + outside.write_text("lockdown: true\n", encoding="utf-8") + (ws / "config" / "settings.yaml").symlink_to(outside) + assert tracked_config_write_refusal("config/settings.yaml") + assert tracked_config_write_refusal(ws / "config" / "settings.yaml") + + def test_a_relative_link_that_stays_in_the_workspace_is_refused_too(self, tmp_path, monkeypatch): + ws = self._locked(tmp_path, monkeypatch) + (ws / "notes" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + (ws / "config" / "settings.yaml").symlink_to( + Path("..") / "notes" / "settings.yaml", + ) + assert tracked_config_write_refusal("config/settings.yaml") + + @pytest.mark.parametrize("path", [ + "notes/settings-link.yaml", # a file symlink into the subtree + "notes/cfgdir/settings.yaml", # through a directory symlink + "notes/cfgdir/cron/gates/new.py", # a path that does not exist yet + "notes/../config/settings.yaml", # .. traversal back in + ]) + def test_a_path_that_reaches_into_the_subtree_is_refused(self, tmp_path, monkeypatch, path): + """The resolving direction, which the lexical check cannot see. Dropping + it in favour of comparing names would reopen every one of these.""" + ws = self._locked(tmp_path, monkeypatch) + (ws / "config" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") + (ws / "notes" / "settings-link.yaml").symlink_to(ws / "config" / "settings.yaml") + (ws / "notes" / "cfgdir").symlink_to(ws / "config", target_is_directory=True) + assert tracked_config_write_refusal(path) + + def test_ordinary_workspace_files_stay_writable(self, tmp_path, monkeypatch): + """Including the target of the link above. The workspace is the agent's + working directory and lockdown does not make it read-only; what keeps a + link at ``config/settings.yaml`` from mattering is that a locked instance + with one refuses to start — see + :class:`TestLockdownSettingsFileIsInTheSubtree`. + """ + ws = self._locked(tmp_path, monkeypatch) + (ws / "config" / "settings.yaml").symlink_to( + Path("..") / "notes" / "settings.yaml", + ) + assert tracked_config_write_refusal("notes/settings.yaml") is None + assert tracked_config_write_refusal("memory/notes.md") is None + def ws_escape(tmp_path: Path) -> str: """A ``..``-traversal path that climbs out of the workspace config subtree.""" From 2c24b5909eca1fa9477e073cf5c2e30e05a22365 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 31 Jul 2026 08:08:00 +0200 Subject: [PATCH 3/5] Review: judge the whole reviewed surface, not just config/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This branch made `skills/` tracked config in one place and not in the other. `skills/manager.py` refuses create, update, delete and toggle under lockdown, while `tracked_config_write_refusal` scoped itself to `config/`, so `Write` to `skills/backdoor/SKILL.md` was allowed — as were the five root instruction files. `SkillManager.discover()` is not lockdown-gated and runs at startup and on every reload, so a planted file is indexed as a live model-invocable skill, with its own `allowed-tools` frontmatter, and the 403 on `create_skill` is never involved. No `Bash` required. That is the path the project's own standard says to close: `docs/config.md` argues refusing `Write`/`Edit` "closes the path an agent takes by default and by accident, not the one it takes deliberately". The "Bash exists" argument cannot excuse the asymmetry, because it would delete the `config/` guard too. This is not a sandbox and does not become one; the point is that a change which takes effect is visible in the PR carrying it. `REVIEWED_DIRS` and `REVIEWED_ROOT_FILES` name the surface once. Two constants and a helper, not a policy object. `PROMPT_FILES` is deliberately not reused: it includes `TASK.md`, runtime state the agent has to go on writing. Sync's dirty-state check now covers the same list, so the guard and the check cannot disagree about what a reviewed file is — a path one of them missed was a file sync merged over without reporting that the live copy differed from the one that passed validation. A submodule inside the surface is promoted from warning to blocking when locked, matching the rule already applied to gitignored files. `docs/config.md` said both things, in the same section: that toggling a skill fails with a 403, and that "writes elsewhere (memory, skills content, task files) are unaffected". The second was right about memory and tasks and wrong about skills. Widening the guard would have made `PUT /api/memory/file/SOUL.md` return 403 for a file the web UI offers to edit, so both read endpoints now report `read_only`, computed from the refusal itself rather than from a second copy of the rule, and the editor renders it read-only. One table of representative paths pins the write guard, the sync pathspec and the startup check against the same expectations, each asked behaviourally — the sync leg against a real git repo, so a mis-spelled pathspec fails it. Known and documented: the startup check covers subtrees, so a symlinked root instruction file still starts. The write guard resolves, so both the link and its target are refused meanwhile. Co-Authored-By: Claude --- docs/config.md | 95 ++++--- nerve/config.py | 157 ++++++---- nerve/gateway/routes/memory.py | 25 +- nerve/sync_service.py | 60 ++-- tests/test_lockdown.py | 362 +++++++++++++++++++++++- tests/test_sync_service.py | 48 ++++ web/src/api/client.ts | 2 +- web/src/components/Files/FileEditor.tsx | 20 +- web/src/components/Files/FileTree.tsx | 8 +- web/src/pages/FilesPage.tsx | 1 + web/src/stores/filesStore.ts | 8 +- web/src/utils/fileTree.ts | 6 +- 12 files changed, 659 insertions(+), 133 deletions(-) diff --git a/docs/config.md b/docs/config.md index 1adeb4b2..51a5e077 100644 --- a/docs/config.md +++ b/docs/config.md @@ -228,17 +228,21 @@ config, so the merged change takes effect immediately. CI on the PR is still the first line of defense. The remote and credentials come from git itself, so configure `git remote` and auth in the workspace as usual. -**Keep the config subtree clean.** Sync refuses to merge while -`/config/` has local changes: an edited or deleted tracked file, a -staged change, or an untracked file. Validation judges a clean checkout of the -fetched commit, but the merge lands in your working tree, and `--ff-only` only -refuses when the incoming commit touches the same path. Anything else would survive -the merge unchecked, leaving a bundle on disk that is not the one that passed. The -case that matters most is an untracked `config/cron/gates/*.py`: the daemon imports -and runs gate plugins and validation never loads them, so a box meant to run only -reviewed config would be running local code. Commit, discard or push local edits; -the failure message names the paths. Files matched by `.gitignore` inside `config/` -are warnings rather than refusals, since the shared repo can never carry them. +**Keep the reviewed files clean.** Sync refuses to merge while the workspace's +reviewed files have local changes: an edited or deleted tracked file, a staged +change, or an untracked file. That is `/config/`, `/skills/` +and the root instruction files (`AGENTS.md`, `SOUL.md`, `IDENTITY.md`, `USER.md`, +`TOOLS.md`) — the same set lockdown's write guard refuses. Validation judges a clean +checkout of the fetched commit, but the merge lands in your working tree, and +`--ff-only` only refuses when the incoming commit touches the same path. Anything +else would survive the merge unchecked, leaving a bundle on disk that is not the one +that passed. The case that matters most is an untracked `config/cron/gates/*.py`: +the daemon imports and runs gate plugins and validation never loads them, so a box +meant to run only reviewed config would be running local code. Commit, discard or +push local edits; the failure message names the paths. So a skill the agent created +at runtime, or a locally edited `SOUL.md`, blocks the merge until it is committed or +dropped. Files matched by `.gitignore` in there are warnings rather than refusals, +since the shared repo can never carry them. Sync validates more strictly than CI: an unset required `${VAR}` blocks the merge. CI has no secrets, so it reports those as info, but the daemon does have them, and a @@ -352,27 +356,38 @@ Two things to know about `NERVE_LOCKDOWN`: `settings.yaml`, so a local edit to `config.yaml` or `config.local.yaml` cannot unlock the instance or fake-lock it. With `NERVE_WORKSPACE` set, neither can repointing `workspace:`. -- **Runtime edits to tracked config are blocked.** Creating, updating, deleting or - toggling a skill, Telegram pairing, writing a workspace file that lands inside - `config/`, and other config mutations fail with a "locked" error (HTTP 403). - Change config by opening a PR against the workspace repo and letting sync apply - the merge. Files elsewhere in the workspace are unaffected: the workspace is also - the agent's working directory, and lockdown covers tracked config rather than the - whole box. -- **The agent's own `Write`/`Edit` are refused inside `config/`.** Every +- **What lockdown protects is the workspace's *reviewed surface*.** That is + `config/`, `skills/`, and the root instruction files `AGENTS.md`, `SOUL.md`, + `IDENTITY.md`, `USER.md` and `TOOLS.md`. `config/` is the declarative half; a + `SKILL.md` is model-invocable text with its own `allowed-tools` frontmatter that + the skill index picks up on the next reload, and the root files are the system + prompt the agent starts every turn from. `memory/`, `tasks/`, `TASK.md` and + ordinary workspace files are outside it and stay writable. +- **Runtime edits to the reviewed surface are blocked.** Creating, updating, + deleting or toggling a skill, Telegram pairing, writing a workspace file that + lands in that surface, and other config mutations fail with a "locked" error + (HTTP 403). Change config by opening a PR against the workspace repo and letting + sync apply the merge. The rest of the workspace is unaffected: it is also the + agent's working directory, and lockdown covers what the box was reviewed to run + rather than the whole box. +- **The agent's own `Write`/`Edit` are refused across that surface.** Every non-interactive tool is otherwise auto-approved, so without this the ordinary way - an agent edits a file would never meet the guards above. The refusal names the PR - flow, so a capable agent routes to it instead of retrying. Writes elsewhere - (memory, skills content, task files) are unaffected. `Bash` is not covered; see - below. -- **The tracked config subtree must really be in the workspace.** - `/config` has to resolve inside ``, and has to be a real - directory rather than a symlink. If it is a symlink out, nothing under it is part - of the reviewed repo, `settings.yaml` included. If it is a symlink to a sibling - inside the workspace, git does not descend into it, so nothing under it is tracked - and sync reports the workspace clean whatever it holds. Either way the instance - refuses to start. Where the workspace itself lives stays a machine-local decision, - symlink included. + an agent edits a file would never meet the guards above — and the skill + endpoints' 403 would be beside the point, since the index picks up whatever is in + `skills/` on the next reload however it got there. The refusal names the PR flow, + so a capable agent routes to it instead of retrying. Writes elsewhere (memory, + task files, scratch files) are unaffected. `Bash` is not covered; see below. +- **The reviewed subtrees must really be in the workspace.** `/config` + and `/skills` have to resolve inside ``, and each has to be + a real directory rather than a symlink. If one is a symlink out, nothing under it + is part of the reviewed repo, `settings.yaml` included. If it is a symlink to a + sibling inside the workspace, git does not descend into it, so nothing under it is + tracked and sync reports the workspace clean whatever it holds. Either way the + instance refuses to start. Where the workspace itself lives stays a machine-local + decision, symlink included. The root instruction files get no such startup check: + a locked instance whose `SOUL.md` is a symlink starts. What covers that case is + the write guard, which resolves the link and refuses its target too, so the file + the prompt is built from is not writable under another name. - **`settings.yaml` must be inside that subtree.** `/config/settings.yaml` has to resolve inside `/config/`, so a symlink there cannot source the lockdown flag, the auth secret and the cron paths from a file no reviewer saw — an @@ -392,10 +407,12 @@ Two things to know about `NERVE_LOCKDOWN`: there is nothing contained left to fall back to and the instance refuses to start. - **Sync is stricter about local files.** [Workspace - sync](#git-backed-workspace-sync) already refuses to merge into a config subtree - with local changes; on a locked instance a `.gitignore`d file in there is a - refusal too rather than a warning. An ignored `config/cron/gates/*.py` is local - code the daemon runs, invisible to both the reviewer and the validator. + sync](#git-backed-workspace-sync) already refuses to merge when the reviewed + surface has local changes; on a locked instance a `.gitignore`d file in there is + a refusal too rather than a warning, and so is a submodule, which a validation + checkout never initializes and a fast-forward never updates. An ignored + `config/cron/gates/*.py` is local code the daemon runs, invisible to both the + reviewer and the validator. Run `nerve config validate --workspace .` before merging; it validates the locked view when `lockdown: true`, which is where the checks above are decided. It also @@ -429,12 +446,12 @@ Nerve does not filter command strings — `sh -c`, redirection, `python -c` and editor defeat such a filter, and a filter that looks like a boundary without being one is worse than a documented gap. If your threat model includes a deliberately hostile agent, run the daemon as a user that cannot write the workspace repo, or -mount the config subtree read-only. +mount the reviewed subtrees read-only. **The codex backend is weaker still.** Codex's sandbox is a mode, not a path list, -so "everything except `config/`" cannot be expressed to it. Nerve declines a codex -file change landing in the tracked subtree, but only through codex's approval -request, and the shipped defaults (`sandbox: danger-full-access`, +so "everything except the reviewed surface" cannot be expressed to it. Nerve +declines a codex file change landing in that surface, but only through codex's +approval request, and the shipped defaults (`sandbox: danger-full-access`, `approval_policy: never`) never ask. Treat the `Write`/`Edit` protection above as absent there unless you have configured approvals. diff --git a/nerve/config.py b/nerve/config.py index 7a7dcfd5..c32d652c 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -265,6 +265,43 @@ def workspace_config_dir(workspace: Path) -> Path: return Path(workspace) / "config" +# The reviewed surface of a workspace: what a locked instance takes as +# instruction rather than as data, and therefore what may only arrive as a +# reviewed, merged commit. +# +# It is not one directory because the layout is not one directory. ``config/`` +# is the declarative half. ``skills/`` is executable policy in all but name: a +# SKILL.md is model-invocable text carrying its own ``allowed-tools`` +# frontmatter, and ``SkillManager.discover`` indexes whatever is on disk at +# startup and on every reload without consulting lockdown, so a file dropped +# there is live on the next sync. The skill endpoints already refuse under +# lockdown; refusing the endpoint and not the file it would have written is not +# a refusal. The root instruction files are the system prompt itself — the +# standing orders every turn starts from. +# +# Not a discovery list, and deliberately not derived from one: +# ``nerve.agent.prompts.PROMPT_FILES`` also names TASK.md, which is runtime state +# the agent maintains and has to go on being able to write. +REVIEWED_DIRS = ("config", "skills") +REVIEWED_ROOT_FILES = frozenset({ + "AGENTS.md", "SOUL.md", "IDENTITY.md", "USER.md", "TOOLS.md", +}) + + +def workspace_reviewed_paths(workspace: Path) -> list[Path]: + """Every member of the reviewed surface of ``workspace``, subtrees first. + + One list so the write guard and sync's dirty-state check ask about the same + files. They answer different questions — may this be written, has this been + edited locally — and when the two disagreed about what the surface *is*, a + skill the reviewer never saw was both writable and invisible to sync. + """ + ws = Path(workspace) + return [ws / name for name in REVIEWED_DIRS] + [ + ws / name for name in sorted(REVIEWED_ROOT_FILES) + ] + + def workspace_settings_file(workspace: Path) -> Path: """Shareable settings file inside a workspace (``/config/settings.yaml``).""" return workspace_config_dir(workspace) / "settings.yaml" @@ -2639,23 +2676,25 @@ def load_config(config_dir: Path | None = None) -> NerveConfig: def lockdown_workspace_problems(workspace: Path) -> list[str]: - """Ways a locked instance's tracked config tree isn't the tree it claims. - - Everything else here judges a path against ``/config``. That - settles whether a path stays inside the subtree, and says nothing about - whether the subtree is the one the config repo was reviewed in. If ``config`` - is a symlink out of the workspace, every path under it is contained relative - to itself, every containment check passes, and the whole tracked layer — - settings.yaml, the cron files, the gate plugins the daemon imports — comes - from an untracked directory somewhere else on the box, with lockdown itself - read from there and reporting that all is well. - - So the subtree is judged against the workspace instead, once, and a locked - instance that fails it does not start. The workspace *path* may still be + """Ways a locked instance's reviewed subtrees aren't the trees they claim. + + Everything else here judges a path against ``/config`` or + ``/skills``. That settles whether a path stays inside a subtree, + and says nothing about whether the subtree is the one the config repo was + reviewed in. If ``config`` is a symlink out of the workspace, every path + under it is contained relative to itself, every containment check passes, and + the whole tracked layer — settings.yaml, the cron files, the gate plugins the + daemon imports — comes from an untracked directory somewhere else on the box, + with lockdown itself read from there and reporting that all is well. A + redirected ``skills`` is the same arrangement one directory over: whatever is + there gets indexed as model-invocable skills on the next reload. + + So each subtree is judged against the workspace instead, once, and a locked + instance that fails does not start. The workspace *path* may still be anything, including a symlink: where a machine keeps its workspace is a machine-local decision and the one thing lockdown never took away. - Two more ways the same claim fails, checked here for the same reason. The + Two more ways the same claim fails, checked here for the same reason. A subtree can stay inside the workspace and still not be the reviewed tree — ``config`` as a symlink to a sibling directory — and ``settings.yaml`` alone can leave it while everything around it stays put. @@ -2666,30 +2705,31 @@ def lockdown_workspace_problems(workspace: Path) -> list[str]: """ problems: list[str] = [] config_root = workspace_config_dir(workspace) - if not _is_within(config_root, workspace): - problems.append( - f"lockdown is enabled but the tracked config subtree {config_root} " - f"resolves to {_resolved(config_root)}, outside the workspace " - f"{_resolved(workspace)} — so nothing it holds is part of the reviewed " - f"repo. A locked instance takes its config only from the workspace's own " - f"config/ directory." - ) - elif config_root.is_symlink(): - # Contained, and still not the reviewed tree. git does not descend into a - # symlinked directory, so `git status -- /config` reports nothing no - # matter what is under there: sync calls the workspace clean while the - # daemon reads settings.yaml, and imports cron/gates/*.py, from a - # directory the config repo has never seen a single file of. The - # arrangement is also writable by the agent through its real path, which - # the write guard judges against config/ and lets past. - problems.append( - f"lockdown is enabled but the tracked config subtree {config_root} is a " - f"symlink to {_resolved(config_root)}. git does not track anything " - f"through a symlinked directory, so no file under it is part of the " - f"reviewed repo and a sync would report the workspace clean regardless " - f"of what it holds. A locked instance's config/ must be a real " - f"directory." - ) + for reviewed in (Path(workspace) / name for name in REVIEWED_DIRS): + if not _is_within(reviewed, workspace): + problems.append( + f"lockdown is enabled but the reviewed subtree {reviewed} resolves " + f"to {_resolved(reviewed)}, outside the workspace " + f"{_resolved(workspace)} — so nothing it holds is part of the " + f"reviewed repo. A locked instance reads {reviewed.name}/ only " + f"from the workspace itself." + ) + elif reviewed.is_symlink(): + # Contained, and still not the reviewed tree. git does not descend + # into a symlinked directory, so `git status -- /config` reports + # nothing whatever is under there: sync calls the workspace clean + # while the daemon reads settings.yaml, and imports cron/gates/*.py, + # from a directory the config repo has never seen a single file of. + # A redirected skills/ is the same arrangement, with the skill index + # reading whatever the link points at. + problems.append( + f"lockdown is enabled but the reviewed subtree {reviewed} is a " + f"symlink to {_resolved(reviewed)}. git does not track anything " + f"through a symlinked directory, so no file under it is part of " + f"the reviewed repo and a sync would report the workspace clean " + f"regardless of what it holds. A locked instance's " + f"{reviewed.name}/ must be a real directory." + ) # The subtree can be the reviewed tree and this one file still not be part of # it. settings.yaml carries the lockdown flag itself, the auth secret and @@ -2952,7 +2992,7 @@ def ensure_not_locked(action: str = "modify configuration") -> None: def ensure_path_not_tracked_config(path: Path, action: str = "write") -> None: - """Refuse, under lockdown, to touch a path inside the tracked config subtree. + """Refuse, under lockdown, to touch a path in the workspace's reviewed surface. The other guards sit in front of a specific operation — creating a skill, persisting a pairing — and know what they are about to change. This one is @@ -2962,9 +3002,10 @@ def ensure_path_not_tracked_config(path: Path, action: str = "write") -> None: reachable through them, which means both ``settings.yaml`` (including the ``lockdown`` flag itself) and ``cron/gates/*.py``, which the daemon imports and runs. Either one turns lockdown off from inside the box it is meant to be - protecting. + protecting. ``skills/`` and the root instruction files are reachable the same + way and decide what the agent does next, so they are judged the same. - A path outside the subtree is none of lockdown's business — the workspace is + A path outside that surface is none of lockdown's business — the workspace is also the agent's working directory, and normal files there are the point. """ reason = tracked_config_write_refusal(path) @@ -2988,13 +3029,20 @@ def tracked_config_write_refusal(path: Path | str) -> str | None: that way costs a clear message and a retry, and erring the other way is the hole this exists to close. + Every member of :func:`workspace_reviewed_paths` is judged, not ``config/`` + alone. ``skills/`` and the root instruction files are refused by the named + operations already — ``create_skill`` and friends — and leaving them writable + here left the whole point of those refusals reachable by the plainest tool + the agent has: write ``skills/x/SKILL.md``, wait for the reload that indexes + it, and no 403 was ever involved. + Both views of the path are checked, because a symlink is the case where they disagree and each direction of the disagreement is a way through. Resolving - catches a path that arrives from outside and lands in the subtree — a link in + catches a path that arrives from outside and lands in the surface — a link in the workspace pointing at ``config/``, a directory link, a ``..`` climb. The lexical name catches the reverse: a symlink sitting *at* a reviewed path resolves elsewhere, and a guard that only resolved would decide the write was - outside the subtree and allow it, while the daemon goes on loading that name + outside the surface and allow it, while the daemon goes on loading that name as tracked config. """ if not is_locked(): @@ -3003,15 +3051,14 @@ def tracked_config_write_refusal(path: Path | str) -> str | None: target = Path(path) if not target.is_absolute(): target = workspace / target - config_root = workspace_config_dir(workspace) - if not ( - _is_within(target, config_root) or _is_lexically_within(target, config_root) - ): - return None - return ( - f"this instance is in lockdown (remote-only, read-only) and {target} is " - f"inside the tracked config subtree {config_root}, which only a reviewed, " - f"merged change to the workspace repo may alter. Open a pull request " - f"against that repo instead; the instance will pick the change up when it " - f"syncs. Everything outside {config_root} is still writable." - ) + for reviewed in workspace_reviewed_paths(workspace): + if _is_within(target, reviewed) or _is_lexically_within(target, reviewed): + return ( + f"this instance is in lockdown (remote-only, read-only) and " + f"{target} is part of {reviewed}, which only a reviewed, merged " + f"change to the workspace repo may alter. Open a pull request " + f"against that repo instead; the instance will pick the change up " + f"when it syncs. Memory, task files and the rest of the workspace " + f"are still writable." + ) + return None diff --git a/nerve/gateway/routes/memory.py b/nerve/gateway/routes/memory.py index 358fe668..d38a3665 100644 --- a/nerve/gateway/routes/memory.py +++ b/nerve/gateway/routes/memory.py @@ -12,7 +12,11 @@ from fastapi import APIRouter, Depends, HTTPException, Response from pydantic import BaseModel -from nerve.config import ensure_path_not_tracked_config, get_config +from nerve.config import ( + ensure_path_not_tracked_config, + get_config, + tracked_config_write_refusal, +) from nerve.gateway.auth import require_auth from nerve.gateway.routes._deps import get_deps @@ -24,7 +28,14 @@ # --- Memory files --- def _list_memory_files_sync(workspace: Path) -> list[dict]: - """Walk + stat memory markdown files. Sync — call via asyncio.to_thread.""" + """Walk + stat memory markdown files. Sync — call via asyncio.to_thread. + + Each entry carries ``read_only``, answered by the same guard the write route + enforces. The listing includes the workspace's root markdown, which is where + SOUL.md and AGENTS.md live, and those are reviewed files a locked instance + will not write — so without this the UI offers an edit whose save comes back + a 403. + """ files = [] memory_dir = workspace / "memory" if memory_dir.exists(): @@ -36,6 +47,7 @@ def _list_memory_files_sync(workspace: Path) -> list[dict]: "name": f.name, "size": st.st_size, "modified": datetime.fromtimestamp(st.st_mtime, timezone.utc).isoformat(), + "read_only": tracked_config_write_refusal(f) is not None, }) # Also include root-level md files @@ -46,6 +58,7 @@ def _list_memory_files_sync(workspace: Path) -> list[dict]: "name": f.name, "size": st.st_size, "modified": datetime.fromtimestamp(st.st_mtime, timezone.utc).isoformat(), + "read_only": tracked_config_write_refusal(f) is not None, }) return files @@ -71,7 +84,13 @@ async def read_memory_file(file_path: str, user: dict = Depends(require_auth)): if not full_path.exists() or not full_path.is_file(): raise HTTPException(status_code=404, detail="File not found") content = await asyncio.to_thread(full_path.read_text, encoding="utf-8") - return {"path": file_path, "content": content} + # An editor that opened this file needs the same answer the listing gave, or + # it re-offers the edit the PUT below refuses. + return { + "path": file_path, + "content": content, + "read_only": tracked_config_write_refusal(full_path) is not None, + } class FileWriteRequest(BaseModel): diff --git a/nerve/sync_service.py b/nerve/sync_service.py index b2175719..9880aae4 100644 --- a/nerve/sync_service.py +++ b/nerve/sync_service.py @@ -111,17 +111,22 @@ def _rev(ref: str, cwd: Path) -> str: return r.stdout.strip() if r.returncode == 0 else "" -def _config_pathspec(workspace: Path) -> str: - """The git pathspec for the portable config subtree of ``workspace``.""" - from nerve.config import workspace_config_dir +def _reviewed_pathspec(workspace: Path) -> list[str]: + """The git pathspecs for the reviewed surface of ``workspace``. - return str(workspace_config_dir(workspace)) + The same list the write guard refuses, so the two cannot disagree about what + a reviewed file is. A path this misses is one sync merges over without ever + reporting that the live copy differs from the one that passed validation. + """ + from nerve.config import workspace_reviewed_paths + + return [str(p) for p in workspace_reviewed_paths(workspace)] def _local_config_divergence( workspace: Path, rev: str, locked: bool = False, ) -> tuple[list[str], list[str]]: - """Local state in the config subtree that the validated rev does not contain. + """Local state in the reviewed surface that the validated rev does not contain. Validation runs against a clean detached checkout of ``rev``; the merge lands in the live working tree. ``git merge --ff-only`` only refuses when the @@ -140,12 +145,12 @@ def _local_config_divergence( about what ends up on disk, and it cannot make that promise about a tree somebody else is also editing. - Scoped to the config subtree on purpose. A nerve workspace is also the + Scoped to the reviewed surface on purpose. A nerve workspace is also the agent's working directory, so uncommitted notes and scratch files elsewhere in it are normal and none of sync's business — and where they *would* break the fast-forward, git says so itself. - ``locked`` promotes ``.gitignore``d files inside the subtree from a warning to + ``locked`` promotes ``.gitignore``d files inside the surface from a warning to a refusal. On an ordinary box those files are the operator's own, kept deliberately out of the shared repo, and refusing a merge over them would be sync passing judgement on what a machine may hold locally. A locked instance @@ -159,11 +164,11 @@ def _local_config_divergence( """ blocking: list[str] = [] warnings: list[str] = [] - pathspec = _config_pathspec(workspace) + pathspec = _reviewed_pathspec(workspace) status = _git([ *_QUOTEPATH_OFF, "status", "--porcelain", - "--untracked-files=all", "--ignored=matching", "--", pathspec, + "--untracked-files=all", "--ignored=matching", "--", *pathspec, ], workspace) if status.returncode != 0: # Fail closed: unable to establish that the tree is clean is not the @@ -178,36 +183,43 @@ def _local_config_divergence( continue code, path = line[:2], line[3:] if code == "!!": - # An ignored file inside a *tracked config subtree* is a layout - # mistake — it is config the shared repo can never carry and no - # reviewer will ever see. Worth saying; on an unlocked box not worth - # refusing a merge over, since refusing would be a policy decision - # about what the machine is allowed to have locally rather than a - # statement about whether the merge is sound. Under lockdown that - # policy decision has already been made. + # An ignored file inside the *reviewed surface* is a layout mistake — + # it is config the shared repo can never carry and no reviewer will + # ever see. Worth saying; on an unlocked box not worth refusing a + # merge over, since refusing would be a policy decision about what + # the machine is allowed to have locally rather than a statement + # about whether the merge is sound. Under lockdown that policy + # decision has already been made. if locked: blocking.append(f"!! {path} (gitignored)") else: warnings.append( - f"ignored file inside the tracked config subtree: {path}" + f"ignored file inside the reviewed surface: {path}" ) else: blocking.append(f"{code.strip() or '??'} {path}") - # A submodule under the config subtree is a third case: `git worktree add` + # A submodule inside the reviewed surface is a third case: `git worktree add` # does not initialize submodules, so validation saw an empty directory, and # the fast-forward leaves the live checkout on its old commit. Neither the # old contents nor the new ones were checked. - tree = _git([*_QUOTEPATH_OFF, "ls-tree", "-r", rev, "--", pathspec], workspace) + # + # ``locked`` promotes it for the same reason as an ignored file: unvalidated + # content the daemon reads anyway is exactly what a locked box says cannot be + # there, and the submodule case is the worse of the two, since the working + # copy is whatever the box happens to have checked out and no remote change + # ever moves it. + tree = _git([*_QUOTEPATH_OFF, "ls-tree", "-r", rev, "--", *pathspec], workspace) if tree.returncode == 0: for line in tree.stdout.splitlines(): if line.startswith("160000 "): sub = line.split("\t", 1)[-1] - warnings.append( - f"submodule {sub!r} in the config subtree was not validated " + note = ( + f"submodule {sub!r} in the reviewed surface was not validated " f"(a validation checkout does not initialize submodules) and " f"a fast-forward does not update it" ) + (blocking if locked else warnings).append(note) return blocking, warnings @@ -318,8 +330,8 @@ def sync_workspace( :class:`SyncResult`. That guarantee is about the tree the daemon will read, not merely about the - commit that was checked, so a merge is also refused when the live config - subtree has local changes of its own — see + commit that was checked, so a merge is also refused when the live reviewed + files have local changes of their own — see :func:`_local_config_divergence`. ``changed`` reports whether the live tree actually moved; a refusal leaves it exactly where it was. @@ -390,7 +402,7 @@ def _sync_workspace( ok=False, changed=False, old_rev=old, new_rev=new, validation_warnings=warnings, message=( - f"fetched {new[:8]} but the workspace config subtree has local " + f"fetched {new[:8]} but the workspace's reviewed files have local " f"changes — not applying, because they would survive the " f"fast-forward without ever having been validated: " f"{_describe_paths(blocking)}. Commit, discard or push them." diff --git a/tests/test_lockdown.py b/tests/test_lockdown.py index 36335a22..f0bd996e 100644 --- a/tests/test_lockdown.py +++ b/tests/test_lockdown.py @@ -810,6 +810,291 @@ def test_ordinary_workspace_files_stay_writable(self, tmp_path, monkeypatch): assert tracked_config_write_refusal("memory/notes.md") is None +class TestLockdownReviewedSurface: + """``config/`` is not the whole of what a locked instance was reviewed for. + + ``skills/`` reaches the model as instructions with their own + ``allowed-tools``, indexed by ``SkillManager.discover`` at startup and on + every reload, and the root instruction files are the system prompt. The skill + endpoints refuse under lockdown already, which only meant the 403 could be + walked around with the plainest tool the agent has. + """ + + def _locked(self, tmp_path, monkeypatch): + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + return ws + + @pytest.mark.parametrize("path", [ + "skills/backdoor/SKILL.md", + "skills/backdoor/scripts/run.sh", + "skills", + "AGENTS.md", + "SOUL.md", + "IDENTITY.md", + "USER.md", + "TOOLS.md", + ]) + def test_the_reviewed_surface_is_refused(self, tmp_path, monkeypatch, path): + self._locked(tmp_path, monkeypatch) + refusal = tracked_config_write_refusal(path) + assert refusal, path + assert "pull request" in refusal + + @pytest.mark.parametrize("path", [ + "memory/notes.md", + "tasks/active/t1.md", + "README.md", + # In PROMPT_FILES beside SOUL.md and read into the same prompt, and + # deliberately not reviewed: it is the running task list the agent keeps. + "TASK.md", + # Neighbouring names, to pin that the match is on path components. + "skills.md", + "SOUL.md.bak", + "memory/SOUL.md", + ]) + def test_the_rest_of_the_workspace_stays_writable(self, tmp_path, monkeypatch, path): + self._locked(tmp_path, monkeypatch) + assert tracked_config_write_refusal(path) is None, path + + def test_a_symlink_at_a_reviewed_root_file_is_refused(self, tmp_path, monkeypatch): + """The lexical half, one directory up from ``config/``: writing through + the link changes the file the prompt is built from. + + The link's target goes with it, unlike the ``config/settings.yaml`` case + where the reviewed root is the directory and a link at a file inside it + leaves the target an ordinary workspace file. Here the reviewed root *is* + the file, so resolving it names the target — and that is the answer this + one needs: nothing refuses a locked instance whose ``SOUL.md`` is a + symlink, so the write guard is the only thing standing between the agent + and its own instructions. + """ + ws = self._locked(tmp_path, monkeypatch) + (ws / "notes").mkdir() + (ws / "notes" / "soul.md").write_text("you are helpful\n", encoding="utf-8") + (ws / "SOUL.md").symlink_to(Path("notes") / "soul.md") + assert tracked_config_write_refusal("SOUL.md") + assert tracked_config_write_refusal("notes/soul.md") + + def test_a_path_that_reaches_into_skills_is_refused(self, tmp_path, monkeypatch): + ws = self._locked(tmp_path, monkeypatch) + (ws / "skills" / "x").mkdir(parents=True) + (ws / "notes").mkdir() + (ws / "notes" / "skilldir").symlink_to(ws / "skills", target_is_directory=True) + assert tracked_config_write_refusal("notes/skilldir/x/SKILL.md") + assert tracked_config_write_refusal("notes/../skills/x/SKILL.md") + + def test_unlocked_writes_skills_freely(self, tmp_path, monkeypatch): + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False, workspace=ws)) + assert tracked_config_write_refusal("skills/x/SKILL.md") is None + assert tracked_config_write_refusal("SOUL.md") is None + + def test_a_symlinked_skills_directory_refuses_to_load(self, tmp_path): + """Same layout, same reason as a symlinked ``config/``: git does not + descend into it, so nothing under it is tracked while ``discover`` indexes + whatever is there.""" + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir() + (ws / "config").mkdir(parents=True) + (ws / "notes" / "skills").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + workspace_settings_file(ws).write_text("lockdown: true\n" + _JWT, encoding="utf-8") + (ws / "skills").symlink_to(Path("notes") / "skills", target_is_directory=True) + + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "must be a real directory" in str(ei.value) + assert "skills" in str(ei.value) + + def test_skills_symlinked_out_of_the_workspace_refuses_to_load(self, tmp_path): + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + elsewhere = tmp_path / "local-skills" + config_dir.mkdir() + elsewhere.mkdir() + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + workspace_settings_file(ws).write_text("lockdown: true\n" + _JWT, encoding="utf-8") + (ws / "skills").symlink_to(elsewhere, target_is_directory=True) + + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "outside the workspace" in str(ei.value) + + def test_an_absent_skills_directory_is_not_a_problem(self, tmp_path): + """Most workspaces have no skills at all, and a missing directory is not + a redirected one.""" + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + assert lockdown_workspace_problems(ws) == [] + + def test_a_symlinked_skills_directory_stays_fine_unlocked(self, tmp_path): + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + elsewhere = tmp_path / "local-skills" + config_dir.mkdir() + elsewhere.mkdir() + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + workspace_settings_file(ws).write_text("timezone: UTC\n", encoding="utf-8") + (ws / "skills").symlink_to(elsewhere, target_is_directory=True) + assert load_config(config_dir).timezone == "UTC" + + +# Representative workspace-relative paths, and whether each is part of the +# reviewed surface. Hand-written on purpose: it is the statement the three +# implementations below are measured against, and deriving it from any of them +# would only make each agree with itself. +_SURFACE_TABLE: dict[str, bool] = { + "config/settings.yaml": True, + "config/cron/jobs.yaml": True, + "config/cron/gates/gate.py": True, + "skills/x/SKILL.md": True, + "AGENTS.md": True, + "SOUL.md": True, + "IDENTITY.md": True, + "USER.md": True, + "TOOLS.md": True, + "memory/notes.md": False, + "tasks/active/t1.md": False, + "TASK.md": False, + "README.md": False, + "../outside.md": False, +} + + +class TestReviewedSurfaceAgreement: + """Three implementations, one surface, and nothing that made them agree. + + The write guard decides what a locked instance may not write, sync's + divergence check decides what a local edit blocks a merge over, and the + startup check decides which subtrees have to really be in the workspace. + They were written at different times against ``config/`` alone, and the + branch that started treating ``skills/`` as reviewed changed one of them: + ``create_skill`` refused while ``Write`` did not, so the same file was + forbidden through the endpoint, allowed through the tool, and invisible to + sync. + + Each row of ``_SURFACE_TABLE`` is put to all three. What is asserted is + agreement with the table, not agreement with each other — three + implementations can be uniformly wrong. + """ + + def test_the_table_covers_the_declared_surface(self): + """The table is the artifact the rest of this class trusts, so it is + checked against the constants rather than against any of the guards.""" + from nerve.config import REVIEWED_DIRS, REVIEWED_ROOT_FILES + + reviewed = [p for p, want in _SURFACE_TABLE.items() if want] + dirs_covered = {p.split("/")[0] for p in reviewed if "/" in p} + files_covered = {p for p in reviewed if "/" not in p} + assert set(REVIEWED_DIRS) <= dirs_covered, ( + "a reviewed directory no row of the table names — every guard below " + "would pass without ever looking at it" + ) + assert REVIEWED_ROOT_FILES <= files_covered, ( + "a reviewed root file no row of the table names" + ) + stray = [ + p for p in reviewed + if p.split("/")[0] not in REVIEWED_DIRS and p not in REVIEWED_ROOT_FILES + ] + assert not stray, f"rows claiming to be reviewed that nothing declares: {stray}" + + @pytest.mark.parametrize("path,reviewed", sorted(_SURFACE_TABLE.items())) + def test_the_write_guard_agrees(self, tmp_path, monkeypatch, path, reviewed): + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + assert (tracked_config_write_refusal(path) is not None) is reviewed, path + + @pytest.mark.skipif(not shutil.which("git"), reason="git not available") + def test_the_sync_pathspec_agrees(self, tmp_path): + """Asked of git, not of the pathspec: an entry that is spelled wrong, or + that git reads as something other than a path, matches nothing and would + pass a comparison against the list itself. + """ + from nerve.sync_service import _local_config_divergence + + ws = tmp_path / "ws" + ws.mkdir() + env = {"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", + "HOME": str(tmp_path), "PATH": os.environ["PATH"]} + (ws / ".keep").write_text("", encoding="utf-8") + for args in (["init", "-b", "main"], ["add", "-A"], ["commit", "-m", "init"]): + subprocess.run(["git", *args], cwd=str(ws), check=True, + capture_output=True, text=True, env=env) + + # Outside the repository, so git has nothing to say about it either way. + skipped = [p for p in _SURFACE_TABLE if p.startswith("../")] + assert all(not _SURFACE_TABLE[p] for p in skipped), ( + "a reviewed path outside the workspace makes no sense; the sync check " + "could never see it" + ) + for path in _SURFACE_TABLE: + if path in skipped: + continue + target = ws / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("local\n", encoding="utf-8") + + blocking, _warnings = _local_config_divergence(ws, "HEAD") + named = {entry.split(" ", 1)[1] for entry in blocking} + for path, reviewed in _SURFACE_TABLE.items(): + if path in skipped: + continue + assert (path in named) is reviewed, (path, sorted(named)) + + def test_the_startup_check_agrees(self, tmp_path): + """Redirect the subtree each path lives in and ask whether a locked + instance still calls the layout sound. + + The construction needs a subtree to redirect, so rows naming a file at + the workspace root, or a path outside it, have nothing to build — see the + skip assertion. What that leaves uncovered is real and named in + ``docs/config.md``: a locked instance whose ``SOUL.md`` is a symlink + starts, and it is the write guard, refusing the link's target as well, + that keeps the file the prompt is built from unwritable. + """ + from nerve.config import REVIEWED_DIRS + + skipped = [p for p in _SURFACE_TABLE if "/" not in p or p.startswith("../")] + assert all( + "/" not in p or p.startswith("../") for p in skipped + ), "only a path with no subtree of its own may be skipped here" + assert set(REVIEWED_DIRS) <= { + p.split("/")[0] for p, want in _SURFACE_TABLE.items() + if want and p not in skipped + }, "a reviewed directory that the skip rule would let past unchecked" + + for i, (path, reviewed) in enumerate(sorted(_SURFACE_TABLE.items())): + if path in skipped: + continue + ws = tmp_path / f"ws{i}" + (ws / "config").mkdir(parents=True) + workspace_settings_file(ws).write_text( + "lockdown: true\n" + _JWT, encoding="utf-8", + ) + subtree = path.split("/")[0] + elsewhere = tmp_path / f"elsewhere{i}" + elsewhere.mkdir() + if subtree == "config": + # Already a real directory; replace it with the redirect. + shutil.rmtree(ws / "config") + (elsewhere / "settings.yaml").write_text( + "lockdown: true\n" + _JWT, encoding="utf-8", + ) + (ws / subtree).symlink_to(elsewhere, target_is_directory=True) + + problems = lockdown_workspace_problems(ws) + assert bool(problems) is reviewed, (path, problems) + + def ws_escape(tmp_path: Path) -> str: """A ``..``-traversal path that climbs out of the workspace config subtree.""" return f"{tmp_path}/ws/config/cron/../../../system.yaml" @@ -980,6 +1265,49 @@ async def test_memory_file_route_cannot_edit_tracked_config(self, tmp_path, monk assert not (ws / "config" / "settings.yaml").exists() assert not (ws / "config" / "cron" / "gates" / "evil.py").exists() + @pytest.mark.asyncio + async def test_memory_file_route_cannot_edit_the_instruction_files(self, tmp_path, monkeypatch): + """The route writes anywhere under the workspace, and the workspace root + is where the system prompt lives.""" + from nerve.gateway.routes.memory import FileWriteRequest, write_memory_file + + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + for path in ("SOUL.md", "AGENTS.md", "skills/x/SKILL.md"): + with pytest.raises(LockdownError): + await write_memory_file( + path, FileWriteRequest(content="do whatever you like\n"), user={}, + ) + assert not (ws / path).exists() + + @pytest.mark.asyncio + async def test_the_listing_marks_what_the_write_route_will_refuse(self, tmp_path, monkeypatch): + """A UI that offers an edit the PUT answers with a 403 is worse than one + that does not offer it. Both come from the same guard so they cannot part + company.""" + from nerve.gateway.routes.memory import list_memory_files, read_memory_file + + ws = tmp_path / "ws" + (ws / "memory").mkdir(parents=True) + (ws / "SOUL.md").write_text("reviewed\n", encoding="utf-8") + (ws / "NOTES.md").write_text("scratch\n", encoding="utf-8") + (ws / "memory" / "notes.md").write_text("scratch\n", encoding="utf-8") + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=True, workspace=ws)) + + listed = {f["path"]: f["read_only"] for f in + (await list_memory_files(user={}))["files"]} + assert listed == { + "SOUL.md": True, "NOTES.md": False, "memory/notes.md": False, + } + assert (await read_memory_file("SOUL.md", user={}))["read_only"] is True + assert (await read_memory_file("NOTES.md", user={}))["read_only"] is False + + monkeypatch.setattr(cfg, "_config", NerveConfig(lockdown=False, workspace=ws)) + unlocked = {f["path"]: f["read_only"] for f in + (await list_memory_files(user={}))["files"]} + assert not any(unlocked.values()) + @pytest.mark.asyncio async def test_memory_file_route_still_writes_elsewhere(self, tmp_path, monkeypatch): """The workspace is also the agent's working directory — lockdown is @@ -1456,9 +1784,41 @@ async def test_a_relative_path_is_resolved_against_the_workspace(self, tmp_path, ) assert type(result).__name__ == "PermissionResultDeny" + @pytest.mark.asyncio + @pytest.mark.parametrize("path", [ + "skills/backdoor/SKILL.md", + "skills/existing/scripts/run.sh", + "AGENTS.md", + "SOUL.md", + "IDENTITY.md", + "USER.md", + "TOOLS.md", + ]) + async def test_can_use_tool_denies_writes_across_the_reviewed_surface( + self, tmp_path, monkeypatch, path, + ): + """The plant-a-skill path, closed where the agent actually takes it. + + ``create_skill`` raises under lockdown, but nothing consults that on the + way to a ``Write``: the file lands, ``SkillManager.discover`` indexes it + on the next reload, and the model can invoke it with whatever + ``allowed-tools`` its frontmatter claims. The root instruction files are + the same shape one directory up — they are the system prompt. + """ + from nerve.agent.backends.claude import ClaudeToolPermissions + + ws = self._locked(monkeypatch, tmp_path) + perms = ClaudeToolPermissions(self._hub()) + result = await perms.can_use_tool( + "Write", {"file_path": str(ws / path)}, context=None, + ) + assert type(result).__name__ == "PermissionResultDeny" + assert "lockdown" in result.message + assert "pull request" in result.message + @pytest.mark.asyncio @pytest.mark.parametrize( - "path", ["memory/notes.md", "skills/x/SKILL.md", "tasks/active/t1.md"], + "path", ["memory/notes.md", "tasks/active/t1.md", "TASK.md", "scratch.md"], ) async def test_ordinary_agent_writes_are_untouched(self, tmp_path, monkeypatch, path): from nerve.agent.backends.claude import ClaudeToolPermissions diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index e09b321d..a375d1f1 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -460,6 +460,54 @@ def test_ignored_gate_plugin_blocks_a_locked_instance(self, tmp_path): assert "local_unreviewed.py" in locked.message assert "Europe/Berlin" not in (ws / "config" / "settings.yaml").read_text() + def test_untracked_skill_blocks_the_merge(self, tmp_path): + """A skill is instructions the model can invoke, with its own + ``allowed-tools``, indexed on the next reload. Scoped to ``config/``, the + check merged straight over one and reported the workspace clean. + """ + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + (ws / "skills" / "backdoor").mkdir(parents=True) + (ws / "skills" / "backdoor" / "SKILL.md").write_text( + "---\nname: backdoor\nallowed-tools: Bash\n---\n", + ) + + result = self._sync(ws, tmp_path) + assert not result.ok and not result.changed + assert "SKILL.md" in result.message + assert "Europe/Berlin" not in (ws / "config" / "settings.yaml").read_text() + + def test_locally_edited_instruction_file_blocks_the_merge(self, tmp_path): + origin, ws = self._pair(tmp_path) + (origin / "SOUL.md").write_text("reviewed\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "add soul", cwd=origin) + self._git("pull", "-q", "--ff-only", "origin", "main", cwd=ws) + self._upstream_commit(origin) + (ws / "SOUL.md").write_text("do whatever you like\n") + + result = self._sync(ws, tmp_path) + assert not result.ok and not result.changed + assert "SOUL.md" in result.message + + def test_submodule_blocks_a_locked_instance(self, tmp_path): + """Same rule as the ignored file above, and for a sharper reason: the + working copy is whatever this box happens to have checked out, validation + saw an empty directory, and no fast-forward will ever move it.""" + origin, ws = self._pair(tmp_path) + sha = self._git("rev-parse", "HEAD", cwd=origin).stdout.strip() + self._git( + "update-index", "--add", "--cacheinfo", f"160000,{sha},config/vendored", + cwd=origin, + ) + self._git("commit", "-m", "vendor config", cwd=origin) + + result = sync_workspace( + ws, tmp_path / "cfg", branch="main", validate=True, locked=True, + ) + assert not result.ok and not result.changed + assert "vendored" in result.message + def test_unset_env_var_is_reported_even_when_tolerated(self, tmp_path): """With strict_env off the validator files this as *info*, which nothing propagated — so the gate saw the one signal that predicts a failed diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 30817c9c..6789f0ae 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -369,7 +369,7 @@ export const api = { // Memory listMemoryFiles: () => request<{ files: any[] }>('/memory/files'), readMemoryFile: (path: string) => - request<{ path: string; content: string }>(`/memory/file/${path}`), + request<{ path: string; content: string; read_only?: boolean }>(`/memory/file/${path}`), writeMemoryFile: (path: string, content: string) => request(`/memory/file/${path}`, { method: 'PUT', diff --git a/web/src/components/Files/FileEditor.tsx b/web/src/components/Files/FileEditor.tsx index d7976a8e..aa418e2a 100644 --- a/web/src/components/Files/FileEditor.tsx +++ b/web/src/components/Files/FileEditor.tsx @@ -1,25 +1,28 @@ import { useState, useCallback } from 'react'; -import { Eye, Edit3, Save } from 'lucide-react'; +import { Eye, Edit3, Lock, Save } from 'lucide-react'; import { MarkdownContent } from '../Chat/MarkdownContent'; interface FileEditorProps { path: string; content: string; modified: boolean; + // Set for a file the server will refuse to write — a reviewed file on a + // locked instance. The save is offered nowhere it would come back a 403. + readOnly?: boolean; saving: boolean; onContentChange: (content: string) => void; onSave: () => void; } -export function FileEditor({ path, content, modified, saving, onContentChange, onSave }: FileEditorProps) { +export function FileEditor({ path, content, modified, readOnly, saving, onContentChange, onSave }: FileEditorProps) { const [mode, setMode] = useState<'edit' | 'preview'>('edit'); const handleKeyDown = useCallback((e: React.KeyboardEvent) => { if ((e.metaKey || e.ctrlKey) && e.key === 's') { e.preventDefault(); - onSave(); + if (!readOnly) onSave(); } - }, [onSave]); + }, [onSave, readOnly]); return (
@@ -43,7 +46,13 @@ export function FileEditor({ path, content, modified, saving, onContentChange, o
- {modified && ( + {readOnly && ( + + + Read-only (lockdown) + + )} + {modified && !readOnly && ( ); diff --git a/web/src/pages/FilesPage.tsx b/web/src/pages/FilesPage.tsx index 6684fc6c..b6c0da80 100644 --- a/web/src/pages/FilesPage.tsx +++ b/web/src/pages/FilesPage.tsx @@ -42,6 +42,7 @@ export function FilesPage() { path={currentFile.path} content={currentFile.content} modified={currentFile.modified} + readOnly={currentFile.readOnly} saving={saving} onContentChange={(c) => updateContent(currentFile.path, c)} onSave={() => saveFile(currentFile.path)} diff --git a/web/src/stores/filesStore.ts b/web/src/stores/filesStore.ts index f65b2cc4..d38bf74b 100644 --- a/web/src/stores/filesStore.ts +++ b/web/src/stores/filesStore.ts @@ -8,6 +8,7 @@ interface OpenFile { content: string; originalContent: string; modified: boolean; + readOnly: boolean; } interface FilesState { @@ -51,10 +52,13 @@ export const useFilesStore = create((set, get) => ({ set({ loading: true }); try { - const { content } = await api.readMemoryFile(path); + const { content, read_only } = await api.readMemoryFile(path); const name = path.split('/').pop() || path; set(s => ({ - openFiles: [...s.openFiles, { path, name, content, originalContent: content, modified: false }], + openFiles: [...s.openFiles, { + path, name, content, originalContent: content, modified: false, + readOnly: !!read_only, + }], activeFile: path, loading: false, })); diff --git a/web/src/utils/fileTree.ts b/web/src/utils/fileTree.ts index 905eefe4..723e041a 100644 --- a/web/src/utils/fileTree.ts +++ b/web/src/utils/fileTree.ts @@ -4,10 +4,13 @@ export interface FileNode { type: 'file' | 'directory'; size?: number; modified?: string; + // The server's answer, not the client's guess: on a locked instance the + // reviewed files (SOUL.md, AGENTS.md, ...) are readable and not writable. + readOnly?: boolean; children?: FileNode[]; } -export function buildFileTree(files: { path: string; name: string; size: number; modified?: string }[]): FileNode[] { +export function buildFileTree(files: { path: string; name: string; size: number; modified?: string; read_only?: boolean }[]): FileNode[] { const root: FileNode = { name: '', path: '', type: 'directory', children: [] }; for (const file of files) { @@ -31,6 +34,7 @@ export function buildFileTree(files: { path: string; name: string; size: number; type: 'file', size: file.size, modified: file.modified, + readOnly: file.read_only, }); } From 85a50930053abfbe26284675d95a2e3f7c06cf33 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 31 Jul 2026 08:08:19 +0200 Subject: [PATCH 4/5] Drop a trailing blank line at EOF Co-Authored-By: Claude --- tests/test_sync_service.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index a375d1f1..255eabfe 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -1192,4 +1192,3 @@ async def _fake_apply(engine, cron_service, config_dir): body = await route_mod.sync_workspace_route(user={}) assert body["ok"] and body["changed"] assert applied == [tmp_path / "cfg"] - From cba2ba60edceb97e7a45cdf921de8a82a6bd3dab Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 31 Jul 2026 08:48:39 +0200 Subject: [PATCH 5/5] Refuse lockdown on a workspace with nowhere to sync from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lockdown makes an instance remote-only: every local change to the reviewed surface is refused, and changes arrive only as a merged commit that sync pulls in. Nothing checked that the second half was possible. `lockdown: true` on a workspace with no git remote — or no repository at all — refused every local edit while having no route to receive one, so the box kept whatever config it held at the moment it was locked, for good, with nothing reporting it. Refused at startup rather than by ignoring the flag. Treating a missing remote as "not locked after all" would make removing a remote an unlock, and the design already has two rules whose point is that there is no such path: the `NERVE_LOCKDOWN` anchor can lock but never unlock, and `lockdown` in `config.yaml` is ignored so a machine-local edit cannot turn it off. A remote of any name counts, not `origin` specifically: with `workspace_sync.branch` unset, sync resolves the current branch's own upstream, which may be on any remote. `workspace_sync.enabled` is not consulted — `nerve config sync` is a manual route that works on a locked box with the periodic loop off, so a remote is a route either way. Git exiting non-zero counts as no route, and its own error text is passed through, so a workspace the daemon cannot read for ownership reasons now says so instead of failing to fetch in silence. Existing locked test workspaces were bricked by this, which is the check working: the fixtures now `git init` and add a remote, rather than the check being narrowed to keep them passing. Co-Authored-By: Claude --- docs/config.md | 20 ++++++- nerve/config.py | 70 ++++++++++++++++++++++ tests/test_lockdown.py | 117 +++++++++++++++++++++++++++++++++++++ tests/test_sync_service.py | 4 ++ 4 files changed, 210 insertions(+), 1 deletion(-) diff --git a/docs/config.md b/docs/config.md index 51a5e077..a5d7d738 100644 --- a/docs/config.md +++ b/docs/config.md @@ -273,6 +273,11 @@ it. ### Before you turn it on +**Clone the config repo as the workspace first.** A locked instance takes config +changes only as a merged commit that sync pulls in, so a workspace with no git +remote has nowhere to receive one from and refuses to start. `git remote -v` in the +workspace is the check. + **Move any required secrets into the environment first.** `config.local.yaml` is ignored when locked, so a secret that lives only there stops being read, and the feature depending on it breaks on the next restart. Supply each one as `${ENV_VAR}` @@ -377,6 +382,16 @@ Two things to know about `NERVE_LOCKDOWN`: `skills/` on the next reload however it got there. The refusal names the PR flow, so a capable agent routes to it instead of retrying. Writes elsewhere (memory, task files, scratch files) are unaffected. `Bash` is not covered; see below. +- **The workspace must be a git repository with a remote.** Every local change to + the reviewed surface is refused, so a merged commit that sync pulls in is the only + way a config change can arrive. A workspace that is not a repository, or is one + with no remote, has no route at all: the instance would keep the configuration it + happens to hold, and nothing would report it. A locked instance without one + refuses to start rather than running unlocked — removing a remote is a + machine-local change, and a machine-local change does not unlock a box. Any remote + counts, and `workspace_sync.enabled: false` is not part of this: sync follows the + branch's own upstream when `workspace_sync.branch` is unset, and `nerve config + sync` is a manual route that works with the periodic loop off. - **The reviewed subtrees must really be in the workspace.** `/config` and `/skills` have to resolve inside ``, and each has to be a real directory rather than a symlink. If one is a symlink out, nothing under it @@ -417,7 +432,10 @@ Two things to know about `NERVE_LOCKDOWN`: Run `nerve config validate --workspace .` before merging; it validates the locked view when `lockdown: true`, which is where the checks above are decided. It also names the machine-local keys the bundle strands, so a key that quietly falls back -to its default on every locked box is visible in the PR rather than at boot. +to its default on every locked box is visible in the PR rather than at boot. The +remote check is the one that judges the checkout the command runs in rather than +the bundle: `actions/checkout` sets `origin`, but a run against an unpacked +tarball reports it missing. **If the flag comes from the environment, add `--assume-lockdown` in CI.** With `lockdown: ${NERVE_LOCKDOWN:-false}` the validator resolves false wherever the diff --git a/nerve/config.py b/nerve/config.py index c32d652c..83917e89 100644 --- a/nerve/config.py +++ b/nerve/config.py @@ -2699,6 +2699,11 @@ def lockdown_workspace_problems(workspace: Path) -> list[str]: ``config`` as a symlink to a sibling directory — and ``settings.yaml`` alone can leave it while everything around it stays put. + One problem here is not about a subtree at all: whether the workspace has a + git remote. It belongs with these because it is the same claim from the other + side — the reviewed surface is only reviewed if reviewed changes can reach it. + See :func:`_workspace_remote_problem`. + Every problem found is reported rather than the first: an operator fixing a layout wants the whole list, and the caller that refuses to boot only needs one of them. @@ -2748,9 +2753,74 @@ def lockdown_workspace_problems(workspace: Path) -> list[str]: f"repo. A locked instance reads its settings only from the workspace's " f"own config/ directory." ) + + remote = _workspace_remote_problem(Path(workspace)) + if remote: + problems.append(remote) return problems +def _workspace_remote_problem(workspace: Path) -> str | None: + """Why this locked workspace has nowhere to receive reviewed config from. + + Lockdown refuses every local change to the reviewed surface and its refusals + say so, naming the PR flow as the way to change config. That flow ends in a + git pull: sync is the only route left in. A locked workspace that is not a + repository, or is one with no remote, has no route at all — every local + change is refused and no remote change can arrive, so the instance keeps the + configuration it happens to hold and nothing on the box says why. Nothing + else reports it, because every other check here judges a tree that is present + and this one is about a tree that can never change. + + Refused rather than ignored, for the reason the other two rules exist: + ``lockdown`` in config.yaml/config.local.yaml is not read, and + ``NERVE_LOCKDOWN`` can lock but never unlock, both so that a machine-local + change cannot turn lockdown off. ``git remote remove origin`` is a + machine-local change. Honoring the flag only when a remote happens to be + configured would make it one more way to unlock the box. + + Any remote counts. Which one to fetch is sync's decision, not this one's: + with ``workspace_sync.branch`` unset it follows the current branch's own + upstream, which need not be named ``origin``, so demanding that name would + refuse a workspace that syncs today. Whether the periodic loop is enabled is + not asked either — ``nerve config sync`` and ``POST /api/config/sync`` are + manual routes that work on a locked box with the loop off, so a remote is a + route regardless. + + Git being unusable is treated as no remote. The answer decides whether a + locked instance starts, and "could not tell" is not "yes"; a box that cannot + run git cannot sync either. + """ + # Imported inside the function because sync_service imports this module. Its + # git runner is the one the sync route itself uses: captured, timed out and + # incapable of raising, which is what a call on the config load path needs. + from nerve.sync_service import _git, is_git_repo + + if not is_git_repo(workspace): + found = f"the workspace {workspace} is not a git repository" + remedy = "Clone the config repo as the workspace" + else: + listed = _git(["remote"], workspace) + if listed.returncode == 0 and listed.stdout.strip(): + return None + if listed.returncode == 0: + found = f"the workspace repository {workspace} has no git remote" + remedy = "Add the config repo as a remote (git remote add origin )" + else: + found = ( + f"git could not list the remotes of the workspace repository " + f"{workspace}: {listed.stderr.strip() or listed.stdout.strip()}" + ) + remedy = "Fix the error git reports" + return ( + f"lockdown is enabled but {found}, so reviewed config has no way to reach " + f"this instance. A locked instance refuses every local change to its " + f"reviewed surface and takes changes only as a merged commit that sync " + f"pulls in, so it would keep the config it currently holds for good. " + f"{remedy}, or drop lockdown from the tracked settings." + ) + + def _has_dotted(data: dict[str, Any], dotted: str) -> bool: """Whether ``dotted`` names a key present in ``data``. diff --git a/tests/test_lockdown.py b/tests/test_lockdown.py index f0bd996e..fed09ec8 100644 --- a/tests/test_lockdown.py +++ b/tests/test_lockdown.py @@ -22,11 +22,32 @@ ) +def _repo(ws: Path, *, remote: str | None = "origin") -> Path: + """Make ``ws`` what a locked workspace is on a real box: a git repository + with a remote to receive reviewed config from. + + A locked instance with nowhere to receive it from refuses to start — see + :class:`TestLockdownNeedsSomewhereToReceiveConfigFrom` — so every locked + workspace here that is expected to load has one. The remote is never + contacted; the check asks whether one is configured. ``remote=None`` leaves + the repository without one. + """ + if not shutil.which("git"): + pytest.skip("git not available") + runs = [["init", "-q"]] + if remote: + runs.append(["remote", "add", remote, "https://example.invalid/config.git"]) + for args in runs: + subprocess.run(["git", *args], cwd=str(ws), check=True, capture_output=True) + return ws + + def _install(tmp_path, *, settings="", base="", local=""): config_dir = tmp_path / "cfg" workspace = tmp_path / "ws" config_dir.mkdir(parents=True) (workspace / "config").mkdir(parents=True) + _repo(workspace) (config_dir / "config.yaml").write_text(f"workspace: {workspace}\n" + base, encoding="utf-8") if local: (config_dir / "config.local.yaml").write_text(local, encoding="utf-8") @@ -565,6 +586,7 @@ def test_a_symlinked_workspace_is_still_fine(self, tmp_path): """Where a machine keeps its workspace stays a machine-local decision.""" real = tmp_path / "real-ws" (real / "config").mkdir(parents=True) + _repo(real) link = tmp_path / "ws" link.symlink_to(real, target_is_directory=True) config_dir = tmp_path / "cfg" @@ -598,6 +620,7 @@ def test_validation_contains_paths_against_the_candidate_workspace(self, tmp_pat outside = tmp_path / "outside" config_dir.mkdir() (candidate / "config" / "cron").mkdir(parents=True) + _repo(candidate) outside.mkdir() (outside / "jobs.yaml").write_text("jobs: not-a-list\n", encoding="utf-8") (candidate / "config" / "cron" / "jobs.yaml").write_text( @@ -719,6 +742,7 @@ def test_an_absent_settings_file_is_not_a_problem(self, tmp_path): environment with no tracked settings file at all.""" ws = tmp_path / "ws" (ws / "config").mkdir(parents=True) + _repo(ws) assert lockdown_workspace_problems(ws) == [] def test_a_link_within_the_subtree_is_fine(self, tmp_path): @@ -736,6 +760,7 @@ def test_every_problem_is_reported_not_just_the_first(self, tmp_path): """An operator fixing a layout wants the list; the loader only needs one.""" ws = tmp_path / "ws" (ws / "notes" / "cfg").mkdir(parents=True) + _repo(ws) (ws / "notes" / "settings.yaml").write_text("lockdown: true\n", encoding="utf-8") (ws / "config").symlink_to(Path("notes") / "cfg", target_is_directory=True) (ws / "notes" / "cfg" / "settings.yaml").symlink_to( @@ -747,6 +772,95 @@ def test_every_problem_is_reported_not_just_the_first(self, tmp_path): assert any("outside the tracked config subtree" in p for p in problems) +@pytest.mark.skipif(not shutil.which("git"), reason="git not available") +class TestLockdownNeedsSomewhereToReceiveConfigFrom: + """A locked instance refuses every local change to the reviewed surface and + tells the operator to open a PR instead. That flow ends in a git pull, so + sync is the only route left in, and a workspace with no remote has no route + at all: nothing local may change the config and nothing remote can arrive. + + Refused rather than ignored. Honoring the flag only where a remote happens to + be configured would make ``git remote remove origin`` an unlock — a + machine-local one, which is what ``lockdown`` being unreadable from + config.yaml and ``NERVE_LOCKDOWN`` never unlocking already exist to prevent. + """ + + def _workspace(self, tmp_path, settings="lockdown: true\n" + _JWT): + config_dir = tmp_path / "cfg" + ws = tmp_path / "ws" + config_dir.mkdir() + (ws / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {ws}\n", encoding="utf-8") + workspace_settings_file(ws).write_text(settings, encoding="utf-8") + return config_dir, ws + + def test_a_workspace_that_is_not_a_repository_refuses_to_load(self, tmp_path): + config_dir, ws = self._workspace(tmp_path) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "is not a git repository" in str(ei.value) + + def test_a_repository_with_no_remote_refuses_to_load(self, tmp_path): + config_dir, ws = self._workspace(tmp_path) + _repo(ws, remote=None) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "has no git remote" in str(ei.value) + + def test_a_remote_is_all_it_takes(self, tmp_path): + config_dir, ws = self._workspace(tmp_path) + _repo(ws) + assert load_config(config_dir).lockdown is True + + def test_the_remote_need_not_be_named_origin(self, tmp_path): + """Which remote to fetch is sync's decision, not this check's: with + ``workspace_sync.branch`` unset it follows the current branch's own + upstream, which need not be ``origin``. Demanding the name would refuse a + workspace that syncs today.""" + config_dir, ws = self._workspace(tmp_path) + _repo(ws, remote="upstream") + assert load_config(config_dir).lockdown is True + + def test_dropping_the_remote_does_not_unlock_the_box(self, tmp_path): + """The fail-open version of this check, stated as what it would do. A box + that came up locked must not come up unlocked after one local command.""" + config_dir, ws = self._workspace(tmp_path) + _repo(ws) + assert load_config(config_dir).lockdown is True + subprocess.run(["git", "remote", "remove", "origin"], cwd=str(ws), + check=True, capture_output=True) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "has no git remote" in str(ei.value) + + def test_unlocked_is_unaffected(self, tmp_path): + """Most workspaces are not config repos at all. Where the config came from + is only a question for an instance that claims it came from review.""" + config_dir, ws = self._workspace(tmp_path, "timezone: UTC\n") + assert load_config(config_dir).timezone == "UTC" + + def test_the_validator_reports_it(self, tmp_path): + from nerve.config_validate import validate_config_bundle + + config_dir, ws = self._workspace(tmp_path) + result = validate_config_bundle(config_dir, workspace_override=ws) + assert any("no way to reach this instance" in e for e in result.errors) + + def test_git_being_unusable_is_not_a_pass(self, tmp_path, monkeypatch): + """Not being able to tell is not a yes, and a box that cannot run git + cannot sync either.""" + import nerve.sync_service as sync + + config_dir, ws = self._workspace(tmp_path) + _repo(ws) + monkeypatch.setattr(sync, "_git", lambda args, cwd: subprocess.CompletedProcess( + args, 1, "", "could not run git: [Errno 2] No such file or directory: 'git'", + )) + with pytest.raises(ConfigError) as ei: + load_config(config_dir) + assert "could not list the remotes" in str(ei.value) + + class TestLockdownWriteGuardJudgesBothViewsOfAPath: """A symlink is where "where does this path end up" and "what does this path name" stop agreeing, and each direction of the disagreement is a way through. @@ -930,6 +1044,7 @@ def test_an_absent_skills_directory_is_not_a_problem(self, tmp_path): a redirected one.""" ws = tmp_path / "ws" (ws / "config").mkdir(parents=True) + _repo(ws) assert lockdown_workspace_problems(ws) == [] def test_a_symlinked_skills_directory_stays_fine_unlocked(self, tmp_path): @@ -1077,6 +1192,7 @@ def test_the_startup_check_agrees(self, tmp_path): continue ws = tmp_path / f"ws{i}" (ws / "config").mkdir(parents=True) + _repo(ws) workspace_settings_file(ws).write_text( "lockdown: true\n" + _JWT, encoding="utf-8", ) @@ -1596,6 +1712,7 @@ def _two_trees(self, tmp_path, *, machine_workspace): config_dir.mkdir() (real / "config").mkdir(parents=True) (attacker / "config").mkdir(parents=True) + _repo(real) workspace_settings_file(real).write_text( "lockdown: true\ntimezone: UTC\n" + _JWT, encoding="utf-8", ) diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 255eabfe..73564618 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -765,6 +765,10 @@ async def test_apply_reloads_config_singleton_engages_lockdown(self, tmp_path, m workspace_settings_file(ws).write_text( "lockdown: true\nauth:\n jwt_secret: x\n", encoding="utf-8" ) + # A locked workspace has to be a repo with a remote or the load refuses; + # this one is synced, so it is one. + (ws / ".git").mkdir() + monkeypatch.setattr(sync, "_git", lambda args, cwd: _cp(stdout="origin\n")) monkeypatch.setattr(cfgmod, "_config", cfgmod.NerveConfig(lockdown=False)) assert not is_locked() err = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir)