diff --git a/config.example.yaml b/config.example.yaml index d688d508..c2b1c6fc 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -133,7 +133,8 @@ memory: # value wins outright, so it disables both the workspace location and the # fallback, and `nerve init` will keep regenerating a system.yaml nothing reads. # -# Editing the cron files does not apply itself: POST /api/cron/reload does. +# Editing the cron files does not apply itself: run `nerve reload`, or let the +# next workspace sync do it. # Workflow runs — budget-capped multi-agent jobs (Claude Workflow tool or # Codex Ultracode) in dedicated tracked sessions. Nerve meters real dollar diff --git a/docs/config.md b/docs/config.md index a5d7d738..ffaddac9 100644 --- a/docs/config.md +++ b/docs/config.md @@ -24,7 +24,7 @@ and migration splits a legacy `config.yaml` on the same table: | Layer | Gets | |-------|------| | `config.yaml` | `workspace`, `deployment`, `provider.aws_profile`, `gateway.ssl.*`, `proxy`, `docker`, `telegram.enabled`, `sync.gmail.accounts`, `external_agents`, `mcp_endpoint`, `workflows.runs_dir` | -| `settings.yaml` | `timezone`, `gateway.host`/`port`, `provider.type`/`aws_region` (incl. the region-scoped Bedrock model IDs), `agent.*`, `memory.*`, `sessions.*`, `sync.*`, `houseofagents.*`, quiet hours, `telegram.dm_policy`/`stream_mode` | +| `settings.yaml` | `timezone`, `gateway.host`/`port`, `provider.type`/`aws_region` (incl. the region-scoped Bedrock model IDs), `agent.*`, `memory.*`, `sessions.*`, `sync.*`, the rest of `workflows.*` (the budget caps and cadence), `houseofagents.*`, quiet hours, `telegram.dm_policy`/`stream_mode` | The test is whether the value would be wrong on another machine: filesystem paths, credential handles, whose mailboxes this person syncs, which agent @@ -195,6 +195,141 @@ is too old to read shows up as a startup warning on the instance and in `nerve doctor`. If a key is ever retired upstream, `--strict-keys` flags it here first; drop the key, or install the ref you deploy (`nerve @ git+...@v1.2.3`) instead. +## Hot-Reload + +Many config changes apply without a restart. Not all do, and a config file +changing on disk is not by itself a reload. + +### What triggers a reload + +A reload is always explicit. Two things cause one: + +- **`nerve reload`**, or `POST /api/config/reload` directly. Re-reads all three + layers and reloads every reloadable subsystem at once. This is the one to use + after editing a file on the box. +- **A sync cycle that finds config this daemon is not running.** Applies exactly + the same set, so a reviewed config PR takes effect on the instance without + anyone logging in. `workspace_sync.interval_minutes` (default 1) bounds how long + that takes. The loop compares the revision on disk against the one it last + applied, not against what its own pull moved, so it also covers a HEAD that + moved out of band (`nerve config sync`, a bare `git pull` in the workspace) and + a previous cycle whose reload failed for one subsystem — that one is retried + every cycle until it takes. + +### What a reload applies + +| Change | Reloaded? | +|--------|-----------| +| Cron jobs (`config/cron/*.yaml`) | ✅ | +| Custom cron gate plugins (new, edited *and* deleted `.py` files) | ✅ the registry is rebuilt from the directory | +| Cron file locations (`cron.jobs_file`, `system_file`, `gate_plugins_dir`) | ✅ re-read from the new config | +| Cron sources: `sync.telegram`, `.gmail`, `.github`, `.github_events`, `.github_repos`, `.message_ttl_days`, and each source's `schedule` | ✅ runners are rebuilt and rescheduled, all or nothing: a `schedule` the scheduler will not take refuses the source reload with the running sources on their old triggers, and reports the error rather than `ok`. **`sync.codex` is not one of these**; see the restart table | +| MCP servers (`mcp_servers`) | ✅ new sessions get the new set | +| Skills (`skills/`) | ✅ re-scanned | +| `lockdown` | ✅ the write guards and the layer stack both follow | +| Web gateway auth (`auth.*`) | ✅ read per request. Only the gateway's own auth: the MCP endpoint checks `/mcp/v1` against the `auth.jwt_secret` it was mounted with, so rotating that secret is half-hot (see the restart table) | +| `notifications.*` | ✅ read per notification | +| `workspace_sync.*` | ✅ from the next sync cycle | +| `retention.*`, `backup.*`, and the `sessions.*` the background loops read | ✅ from the next cycle of that loop | +| `external_agents.targets` (including each target's `enabled`), `.sync_interval_minutes`, `.conflict_policy` | ✅ from the next sweep, provided at least one target existed at startup (see the restart table) | +| `sessions.sticky_period_minutes` | ✅ | +| `telegram.dm_policy`, `.stream_mode` | ✅ read per update. Tightening `open` to `pairing` takes effect on the next message; `allowed_users` does not follow it (see the restart table) | +| `workflows.*` and `workflows.review_loop.*` — budget caps, concurrency, the warning fraction, iteration and criteria caps, leg engines/models, the verifier sandbox | ✅ read per use, by loops and runs already in flight as well as new ones. The two `enabled` flags and the two loop cadences are the exceptions; see the restart table | +| `provider.*` and the API keys it selects (`aws_region`, `aws_profile`, `aws_access_key_id`, and the effective Anthropic key) | ✅ for sessions started **after** the reload. Each client's environment is built from the live reference when the session is created, by the same seam as `agent.*` below | +| **`agent.*` and `codex.*`**: backend choice and models (`agent.backend`, `agent.cron_model`, `agent.model`, `codex.model`, `codex.cron_model`), `max_turns`, `agent.effort`/`cron_effort` and `codex.effort_map`, `agent.thinking`, `agent.context_1m*`, `agent.background_agent_permissions`, idle timeouts, cache TTL, `codex.sandbox`, `.approval_policy`, `.web_search`, `.extra_config`, `.tool_timeout_sec`, `.bin_path`, `.auth`/`.api_key`/`.api_key_env`, `.pricing`, `.min_version`/`.max_version`, `.ultracode.*` | ✅ for sessions and turns **started after** the reload. The engine and both backends resolve these through one live reference, so a key cannot be hot in one and frozen in the other | + +All of that is reloaded together, and the response says what happened to each +piece: `POST /api/config/reload` returns `ok`, a per-subsystem `detail`, and an +`errors` map. A reload is best-effort by design, because a typo in `settings.yaml` +must not stop a valid cron edit from being applied, so `ok: false` with `detail` +showing four subsystems reloaded and one failed is a normal answer. Check `errors` +rather than reading the 200 as success. + +### What still needs a restart + +A reload compares the old and new config and reports any of these that changed, as +`restart_required` on `POST /api/config/reload` and as a log warning. Without that +report a reload returns success while the process keeps the old value, which +matters most for `gateway.host`/`port`: they live in `settings.yaml`, so the change +can arrive by workspace sync rather than by a local edit. + +The check covers the unconditional entries below. The conditional ones (turning X +on, adding the first target, a session already running) depend on runtime state the +reload cannot inspect, and are documented here only. + +| Change | Why | +|--------|-----| +| `gateway.host`, `.port`, `.ssl.*` | the socket is already bound | +| `timezone` | the cron scheduler and every trigger built from it carry the old zone | +| `agent.max_concurrent` | its semaphore cannot be resized under in-flight turns | +| `workspace` | the skill manager, the tool context, the memory bridges and each session's working directory all captured it at startup. Following it in one of them and not the others would be worse than not following it at all | +| `memory.*`, `xmemory.*` | the bridges hold the config they were constructed with | +| `codex.home_dir` | half-hot, which is why it is here: new sessions are handed the new `CODEX_HOME`, but the directory is only created when the backend is built, so nothing creates the new one. Change it and restart, rather than leaving sessions pointed somewhere that may not exist | +| `sync.codex.*` (`enabled`, every `origins[*]` field, `store_encrypted_reasoning`, `workspace_filter.*`) | Codex thread sync is a **different service** from the cron sources above, built once at startup with one polling worker per origin. Adding or editing an origin and reloading reports `ok` and ingests nothing | +| `langfuse.*` | set up before the engine, caching its host, redaction patterns and `LANGFUSE_*` environment exports in process globals | +| `telegram.enabled`, `.bot_token`, `.allowed_users` | the bot was built with that token, and the allow-list was copied into a set when it was built. Notification *delivery* does follow a reload, so after changing `allowed_users` the two can disagree until a restart. `dm_policy` and `stream_mode` are read per update and do follow a reload (see the table above) | +| `mcp_endpoint.*` | fixed when the app was created | +| `auth.jwt_secret` | half-hot: the web gateway reads it per request, so its own auth follows a reload, but the MCP endpoint captured it when the app was mounted and keeps checking `/mcp/v1` against the old secret. Rotating it moves one and not the other until a restart | +| `workflows.enabled`, `workflows.review_loop.enabled` | each service is created at startup and only when its flag is on. Turning one **off** does not stop the service already running, and turning it **on** creates nothing for a reload to reach | +| `workflows.poll_interval_seconds`, `workflows.review_loop.reconcile_interval_seconds` | both loops were handed their interval when they started. Everything else under `workflows.*` is read per use (see the table above) | +| `proxy.*` | the proxy process is started at startup, so turning it on, turning it off or moving its port needs one. The backend does read the proxy host and port per session, so those can point somewhere nothing is listening until you restart | +| Turning `ollama.enabled` **on** while the proxy is not already running | Ollama routes through the proxy as its translation layer, and that process only starts at startup. With the proxy already up, this is read per use and follows a reload | +| Turning `workspace_sync.enabled` or `retention.enabled` **on** | their loops are only created at startup, so there is nothing running to see the flag change. Turning either **off** is hot | +| `external_agents.enabled` (**both** directions) and adding the **first** target | the sweeper is only created when the flag is on *and* at least one target is configured; with none it is never created, so a first target added later reloads to `ok` and does nothing (`POST /api/external-agents/sync` answers 503). Once it exists, adding, removing and toggling targets is hot | +| A session that is **already running** | the agent process was spawned with the options in force at the time; the new ones apply to the next session | + +### From the command line + +```bash +nerve reload # apply config edits to the running daemon +``` + +It calls the endpoint above on this box's own gateway and prints the per-subsystem +result. It exits non-zero on a partial reload, so it can gate a deploy step rather +than only informing a human. Anything in the restart table that changed is printed +as a warning: nothing failed, but the new value is not live yet. + +With no daemon running there is nothing to reload and the command says so. Config +is read fresh at startup, so `nerve start` already picks the edit up. + +It authenticates the way the gateway asks to be authenticated, which depends on +`auth.jwt_secret` in the config it just read: + +- **Set** → it signs a token with it. If that is not the secret the running daemon + started with, the gateway rejects the request and only a restart resolves it. +- **Empty, unlocked** → the gateway is in dev mode and does not ask for a token + (`require_auth` runs open), so the call goes unauthenticated. +- **Empty, locked** → refused before anything is sent. A locked gateway never takes + the open path, so no request from that shell can be authenticated. If the secret + comes from `${ENV_VAR}`, export it in that shell too. + +`auth.password_hash` is not an alternative here. It gates the browser login, which +is what mints a token from it; `require_auth` reads `auth.jwt_secret` alone, so a +password neither makes the endpoint ask for a token nor gives the CLI one to sign. + +`POST /api/config/sync` runs the same reload but scores it differently, because it +answers a different question. Its `ok` is about the *merge*: true once the merged +config is loaded and in effect, false only when the daemon could not load it, in +which case the merge applied nothing at all. A subsystem that failed *after* the +config loaded leaves `ok: true`, because the merged settings really are live, and +shows up as `applied: false` with the reason in `reload_errors`. So the same skills +failure gives `ok: false` on `/api/config/reload` and `ok: true, applied: false` on +`/api/config/sync`. **On the sync endpoint, read `applied`, not `ok`**, unless what +you want to know is specifically whether the merge took. + +`applied: false` on its own does not say why, since a sync with nothing to merge +reports it too. `status` is the field to branch on: + +| `status` | Meaning | +|----------|---------| +| `applied` | merged, and every subsystem took it | +| `partial` | merged and the config loaded, but some subsystem did not take it (`reload_errors`) | +| `not-applied` | merged, and the daemon could not load the merged config — nothing is in effect | +| `up-to-date` | nothing to merge; this call applied nothing | + +It describes *this call*. Whether the daemon is running the revision currently on +disk is a different question, answered by `GET /api/config/sync` below. + ## Git-Backed Workspace Sync The workspace can be a git repository whose remote is a shared **config repo**. @@ -223,10 +358,13 @@ Each sync is fetch, then validate, then fast-forward merge. The fetched bundle i validated in a throwaway git worktree and the live working tree is fast-forwarded only if that passes, so an invalid bundle never lands on disk and there is nothing for a later reload or restart to pick up (`POST /api/config/sync` returns 400 and -leaves the workspace untouched). A pull that changed something reloads cron and MCP -config, so the merged change takes effect immediately. CI on the PR is still the -first line of defense. The remote and credentials come from git itself, so -configure `git remote` and auth in the workspace as usual. +leaves the workspace untouched). A pull that changed something runs the same reload +as `nerve reload`, so see [Hot-Reload](#hot-reload) for what that covers; the +response names which subsystems took the merged change (`applied`, `reload`, +`reload_errors`). A merge whose config the daemon then refuses to load has applied +nothing and reports `ok: false`. CI on the PR is still the first line of defense. +The remote and credentials come from git itself, so configure `git remote` and auth +in the workspace as usual. **Keep the reviewed files clean.** Sync refuses to merge while the workspace's reviewed files have local changes: an edited or deleted tracked file, a staged @@ -244,6 +382,32 @@ at runtime, or a locally edited `SOUL.md`, blocks the merge until it is committe dropped. Files matched by `.gitignore` in there are warnings rather than refusals, since the shared repo can never carry them. +**A blocked sync says so.** Left to a log line it is invisible: the instance keeps +answering normally while every later config change stops arriving, indefinitely. +So the first cycle that refuses sends a notification through the usual channels — +naming the paths and what clears them — and only that first one, not one per +cycle. Recovery sends a second, low-priority, when the merge goes through again. + +The state behind those is queryable rather than log-only. `nerve doctor` reports +it, and `GET /api/config/sync` returns the daemon's record of the last cycle: + +```json +{ + "enabled": true, "checked": true, "ok": false, + "applied_rev": "a1b2c3d4...", "fetched_rev": "e5f6a7b8...", + "blocked": true, "blocked_paths": ["?? skills/backdoor/SKILL.md"], + "reload_errors": {}, "message": "fetched e5f6a7b8 but ...", + "checked_at": "2026-07-30T12:00:00+00:00" +} +``` + +`applied_rev` is what this daemon has actually applied everywhere, which is not +the same as what is checked out: a subsystem that refused the merged config, or a +HEAD moved out of band, leaves the two different and the loop retries until they +agree. `checked: false` means this process has no answer (sync off, or no cycle +yet) rather than that all is well. `nerve doctor` runs the clean-tree check +itself, so it answers from a shell where the daemon's own record is not visible. + Sync validates more strictly than CI: an unset required `${VAR}` blocks the merge. CI has no secrets, so it reports those as info, but the daemon does have them, and a bundle with an unresolved required variable is one it will refuse to load on its @@ -254,10 +418,13 @@ use `workspace_sync.strict_env: false` rather than letting every sync fail. for a one-off. Warnings never block a merge: an unrecognized cron gate type, an unknown key, or a skipped validation. -`workspace_sync` changes need a daemon restart. The sync loop reads the current -config object every cycle, so it adds no staleness of its own, but nothing refreshes -that object while the process runs. Turning `enabled` on needs a restart in any -case, because the sync task is only created at startup. +`workspace_sync` changes apply from the next cycle, once a reload has run. The sync +loop reads the current config object every cycle rather than a copy taken at +startup, so an edit to `branch`, `interval_minutes`, `validate` or `strict_env` +reaches it as soon as something replaces that object: a sync that merged a change, +or `POST /api/config/reload` after you edited the file yourself. Turning `enabled` +**on** still needs a restart, because the sync task is only created at startup. +Turning it off is picked up like any other value. ## Lockdown (remote-only, read-only) @@ -446,12 +613,12 @@ warns when it sees an env-controlled flag resolve to false, so the gap is at lea visible. **Flipping the flag needs the config reloaded.** A sync does that, and so does -`POST /api/config/reload`. The write guards, gateway authentication and the sync -loop pick the change up immediately; cron jobs and gates already built follow on the -next cron reload, and anything captured at startup needs a restart. A hand edit on -the box refreshes nothing, and under lockdown it is ignored anyway. If a sync merges -a config the daemon then cannot load, it reports `ok: false` with `apply_error` -rather than claiming the change took. +`POST /api/config/reload` — both run the same reload. The write guards, gateway +authentication and the sync loop pick the change up immediately; cron jobs and gates +already built follow on the next cron reload, and anything captured at startup needs +a restart. A hand edit on the box refreshes nothing, and under lockdown it is +ignored anyway. If a sync merges a config the daemon then cannot load, it reports +`ok: false` with `apply_error` rather than claiming the change took. ### What lockdown does not cover diff --git a/docs/cron.md b/docs/cron.md index 8ec49a15..a485846d 100644 --- a/docs/cron.md +++ b/docs/cron.md @@ -58,7 +58,9 @@ Notes: registered gates as they were, so a deleted plugin file only takes its gate away once a reload succeeds. - A job holding a [reserved id](#job-fields) is **skipped**, at reload and at - startup alike. The reload itself still succeeds; only that job is dropped. + startup alike. The reload itself still succeeds; only that job is dropped, and + its id comes back in the reload's `rejected` list, so a job that never runs is + visible from the API and not only in the log. - An **invalid schedule** (a crontab whose fields the scheduler rejects, e.g. `99 * * * *`) is refused the same way at reload: `400`, nothing applied. Startup is the one case that differs — the daemon logs an error naming the job diff --git a/nerve/agent/backends/__init__.py b/nerve/agent/backends/__init__.py index f495ce78..2faf4130 100644 --- a/nerve/agent/backends/__init__.py +++ b/nerve/agent/backends/__init__.py @@ -45,9 +45,17 @@ class BackendDeps: Callables (rather than direct references) where the underlying value is hot-reloadable or wired up after engine construction. + + ``config`` is one of the hot ones. A config reload replaces the process + config object; a backend that captured the old one would keep sending the + options the operator has just changed, while the engine — which *is* + re-pointed — reported the change applied and rendered it in the UI. One key + live on one side of that seam and frozen on the other is the hardest state + to diagnose there is, so backends resolve config per read instead of holding + it. """ - config: "NerveConfig" + config: Callable[[], "NerveConfig"] db: "Database" registry: Any # ToolRegistry tool_ctx_factory: Callable[[str], Any] # session_id -> ToolContext diff --git a/nerve/agent/backends/claude.py b/nerve/agent/backends/claude.py index 61aa05ee..130f9847 100644 --- a/nerve/agent/backends/claude.py +++ b/nerve/agent/backends/claude.py @@ -367,7 +367,18 @@ class ClaudeBackend: def __init__(self, deps: Any): self._deps = deps - self.config = deps.config + + @property + def config(self): + """The live config, resolved per read rather than captured. + + Everything below builds SDK options straight off this — the model, the + thinking budget, the 1M-context header, the sub-agent permission mode. + Holding a reference would freeze all of them at start-up while the + engine's own copy moved on with a reload, so the context bar could show + a 200k budget for sessions still being opened with the 1M header. + """ + return self._deps.config() # -- policy -------------------------------------------------------- # diff --git a/nerve/agent/backends/codex/backend.py b/nerve/agent/backends/codex/backend.py index 3d502c0e..a0e50d08 100644 --- a/nerve/agent/backends/codex/backend.py +++ b/nerve/agent/backends/codex/backend.py @@ -127,14 +127,30 @@ class CodexBackend: def __init__(self, deps: Any): self._deps = deps - self.config = deps.config - self.codex = deps.config.codex Path(self._home_dir()).mkdir(parents=True, exist_ok=True) self._preflight_cache: tuple[float, dict[str, Any]] | None = None self._preflight_lock = asyncio.Lock() self._live_models: set[str] = set() self._rate_limits: dict[str, Any] | None = None + @property + def config(self): + """The live config, resolved per read rather than captured.""" + return self._deps.config() + + @property + def codex(self): + """The live ``codex`` section. + + A property and not an attribute for the same reason as :attr:`config`, + and more sharply: a sub-object caches one level deeper, so re-pointing + ``config`` alone would leave the sandbox mode, approval policy, effort + map and auth choice on their start-up values while everything read off + ``config`` had moved. Resolving through the deps callable means there is + no second place for the two to disagree. + """ + return self._deps.config().codex + # -- policy ---------------------------------------------------------- # def default_model(self, source: str) -> str: diff --git a/nerve/agent/engine.py b/nerve/agent/engine.py index 010079f0..a5376933 100644 --- a/nerve/agent/engine.py +++ b/nerve/agent/engine.py @@ -193,19 +193,15 @@ def __init__(self, config: NerveConfig, db: Database): # when Nerve is invoked from within a Claude Code session (e.g. CLI). os.environ.pop("CLAUDECODE", None) - self.config = config + self._config = config self.db = db - self.sessions = SessionManager( - db, - sticky_period_minutes=config.sessions.sticky_period_minutes, - default_backend=config.agent.backend, - cron_backend=config.agent.resolved_cron_backend, - backend_models={ - "claude": config.agent.model, - "codex": config.codex.model, - }, - default_cwd=str(config.workspace), - ) + # ``default_cwd`` is seeded once and never re-seeded: it comes from + # ``workspace``, which the skill manager, the tool context and the memory + # bridges also captured at construction, so following a reload here alone + # would put the session manager in a different workspace from everything + # else. Everything the setter below *does* re-seed follows a reload. + self.sessions = SessionManager(db, default_cwd=str(config.workspace)) + self.config = config self._semaphore = asyncio.Semaphore(config.agent.max_concurrent) self._memory_bridge = None self._xmemory_bridge = None @@ -281,9 +277,12 @@ def __init__(self, config: NerveConfig, db: Database): # session by the STICKY rule (stored sessions.backend first, then # config) — see _backend_for. ``_session_backends`` mirrors the # live client's backend for hot paths (finalize, idle watcher). + # Config is handed over as a callable reading this engine's attribute, + # so a reload that re-points the engine moves the backends with it and + # a single key can never be live here and frozen there. self._backends: dict[str, AgentBackend] = build_backends( BackendDeps( - config=self.config, + config=lambda: self.config, db=self.db, registry=self.registry, tool_ctx_factory=self._build_tool_context, @@ -295,6 +294,43 @@ def __init__(self, config: NerveConfig, db: Database): ) self._session_backends: dict[str, str] = {} + @property + def config(self) -> NerveConfig: + return self._config + + @config.setter + def config(self, config: NerveConfig) -> None: + """Adopt a freshly loaded config, moving the seeded collaborators with it. + + A setter rather than a method because assignment is how a config reload + hands the new object over, and because the values the session manager was + seeded with at construction have to move in the same step. Backend + resolution reads ``agent.backend`` live off this object but the session + manager holds its own copy for the rows it creates; leaving one behind + would route new sessions by the old default while every other read used + the new one — live on one side of the engine, frozen on the other, and + nothing to say which answer you were looking at. + + The backends need nothing here: they resolve config through a callable + onto this attribute, so they follow automatically. + + NOT re-seeded, and so still restart-only: ``agent.max_concurrent`` (its + semaphore cannot be resized under in-flight turns) and everything derived + from ``workspace``. + """ + self._config = config + sessions = getattr(self, "sessions", None) + if sessions is not None: + sessions.sticky_period_minutes = config.sessions.sticky_period_minutes + sessions.default_backend = config.agent.backend + sessions.cron_backend = ( + config.agent.resolved_cron_backend or config.agent.backend + ) + sessions.backend_models = { + "claude": config.agent.model, + "codex": config.codex.model, + } + def set_notification_service(self, service: Any) -> None: """Install the notification service used by per-session ``ToolContext``. diff --git a/nerve/channels/telegram.py b/nerve/channels/telegram.py index e58473cd..bb1d7b74 100644 --- a/nerve/channels/telegram.py +++ b/nerve/channels/telegram.py @@ -21,7 +21,7 @@ import zipfile from datetime import datetime, timezone from pathlib import Path -from typing import Any, TYPE_CHECKING +from typing import Any, Callable, TYPE_CHECKING from zoneinfo import ZoneInfo from telegram import InlineKeyboardButton, InlineKeyboardMarkup, Update @@ -439,11 +439,13 @@ class TelegramChannel(BaseChannel): Delegates session management and agent execution to the ChannelRouter. """ - def __init__(self, config: NerveConfig, router: ChannelRouter): - self.config = config + def __init__( + self, config: Callable[[], NerveConfig], router: ChannelRouter, + ): + self._config = config self.router = router self._app: Application | None = None - self._allowed_users: set[int] = set(config.telegram.allowed_users) + self._allowed_users: set[int] = set(self.config.telegram.allowed_users) self._notification_service = None # Set after service is created self._watchdog_task: asyncio.Task | None = None self._stopping = False @@ -463,6 +465,21 @@ def set_notification_service(self, service) -> None: """Wire the notification service for callback query handling.""" self._notification_service = service + @property + def config(self) -> NerveConfig: + """The live config, resolved per read rather than captured. + + The bot outlives every reload, and ``dm_policy`` decides on each update + whether a stranger may talk to the agent. Holding the start-up object + meant a reload that tightened ``open`` to ``pairing`` reported success + and authorized everyone until the daemon was restarted. + + Only the reads that happen per use follow from this. The bot token was + handed to the Application at build time and ``allowed_users`` was copied + into a set, so both still need a restart. + """ + return self._config() + @property def name(self) -> str: return "telegram" diff --git a/nerve/cli.py b/nerve/cli.py index 1e43451a..0a37d959 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -908,6 +908,57 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s else: errors.append(f"[ERR] Workspace not found: {config.workspace}") + # Workspace sync. Two questions, because the answers come from different + # places: what the daemon's loop last did (retained in-process, so only the + # in-daemon /doctor sees it), and whether the reviewed files are clean right + # now (a git check any process can run, which is the one that matters from a + # shell — a blocked sync leaves the box pinned to an old revision with + # nothing but a log line to say so). + sync_cfg = getattr(config, "workspace_sync", None) + # Also when locked with the loop off: `nerve config sync` is still the only + # way config reaches that box, so a refusal is just as terminal there. + if sync_cfg is not None and (sync_cfg.enabled or config.lockdown): + try: + from nerve.sync_service import ( + _describe_paths, + last_sync_state, + local_block_reasons, + ) + + state = last_sync_state() + if state is not None: + age = max(0, int(time.time() - state.checked_at)) + if state.ok: + lines.append( + f"[OK] Workspace sync: applied " + f"{state.applied_rev[:8] or '(unknown)'}, last checked " + f"{age}s ago" + ) + else: + # Covers the failures the local check below cannot see: a + # dead remote, a diverged branch, an invalid bundle. + warnings.append( + f"[WARN] Workspace sync: last cycle ({age}s ago) failed " + f"— {state.message}" + ) + for name, why in sorted(state.reload_errors.items()): + warnings.append( + f"[WARN] Workspace sync: {name} did not take the merged " + f"config ({why}) — retried every cycle" + ) + blocking = local_block_reasons(config.workspace, config.lockdown) + if blocking: + warnings.append( + f"[WARN] Workspace sync BLOCKED: the reviewed files have " + f"local changes, so no merged config reaches this instance " + f"— {_describe_paths(blocking)}. Commit them, discard them, " + f"or re-propose them as a PR with propose_config_change" + ) + else: + lines.append("[OK] Workspace sync: reviewed files clean") + except Exception as e: + warnings.append(f"[WARN] Workspace sync check failed: {e}") + # Check proxy if config.proxy.enabled: binary = config.proxy.binary_path.expanduser() @@ -1165,6 +1216,122 @@ def doctor(ctx: click.Context) -> None: ctx.exit(1) +# Bind addresses that name no connectable destination, so a client on the same +# box has to ask for loopback instead. +_WILDCARD_BINDS = ("", "0.0.0.0", "::", "*") + + +def _gateway_url(config, path: str) -> str: + """URL for a loopback call to this box's own gateway. + + ``gateway.host`` is a *bind* address, not necessarily somewhere a client can + connect to; see :data:`_WILDCARD_BINDS`. + """ + scheme = "https" if config.gateway.ssl.enabled else "http" + host = config.gateway.host + if host in _WILDCARD_BINDS: + host = "127.0.0.1" + return f"{scheme}://{host}:{config.gateway.port}{path}" + + +@main.command() +@click.pass_context +def reload(ctx: click.Context) -> None: + """Apply config edits to the running daemon without restarting it. + + Re-reads config.yaml, config.local.yaml and the workspace settings, then + reloads cron jobs, cron sources, MCP servers and skills. Anything that needs + a restart instead is listed rather than silently skipped; `docs/config.md` + has the full table. + """ + import httpx + + from nerve.gateway.auth import create_token + + config = ctx.obj["config"] + if config is None: + raise click.ClickException( + f"Config could not be loaded ({ctx.obj.get('config_error')}); " + "run 'nerve config validate' to see why." + ) + if not config.auth.jwt_secret and config.lockdown: + raise click.ClickException( + "No auth.jwt_secret in the config read here, and a locked gateway " + "never runs open, so nothing sent from this shell can be " + "authenticated. If the secret comes from ${ENV_VAR}, export it here " + "too; if the daemon has none either, it is refusing every request " + "and needs one before it can be reloaded." + ) + url = _gateway_url(config, "/api/config/reload") + # Certificate verification stands except in the one case where it cannot + # hold: a wildcard bind means the request goes to 127.0.0.1, and the + # daemon's certificate is issued for a hostname, so checking it there would + # fail on exactly the setups that bothered to configure TLS. When + # gateway.host names a real host the certificate should match it, and a + # failure there is worth hearing about rather than skipping past. + verify = config.gateway.host not in _WILDCARD_BINDS + # A token when there is a secret to sign one with, and otherwise none: an + # unlocked gateway with no auth.jwt_secret does not ask for one (require_auth + # runs open there), so an empty secret is not a reason to refuse to call. The + # operator hand-editing config on a dev box is the likeliest caller of all. + headers = {} + if config.auth.jwt_secret: + headers["Authorization"] = f"Bearer {create_token(config.auth.jwt_secret)}" + try: + resp = httpx.post( + url, + headers=headers, + verify=verify, + # A reload re-reads config and rebuilds cron, sources, MCP and + # skills; MCP servers in particular can take a while to come up. + timeout=120, + ) + except httpx.ConnectError: + raise click.ClickException( + f"No daemon answering at {url}, which is the address in the config " + "read here. If gateway.host, gateway.port or the SSL settings were " + "part of the edit, the running daemon is still bound to the old " + "address and only a restart moves it. Otherwise nothing is running, " + "and a stopped daemon reads config fresh when it starts " + "('nerve start')." + ) from None + except httpx.HTTPError as e: + raise click.ClickException(f"Could not reach {url}: {e}") from None + if resp.status_code in (401, 403): + raise click.ClickException( + "The gateway rejected the request. The running daemon's " + "auth.jwt_secret is not the one read here — it either started with a " + "different value, or with one this config no longer supplies. Restart " + "it to pick up the current config." + ) + if resp.status_code != 200: + raise click.ClickException(f"{url} returned {resp.status_code}: {resp.text[:400]}") + + body = resp.json() + errors = body.get("errors") or {} + for name, outcome in (body.get("detail") or {}).items(): + if name == "restart_required": + continue + if name in errors: + click.secho(f" [ERR] {name}: {errors[name]}", fg="red") + continue + if isinstance(outcome, dict): + outcome = ", ".join(f"{k}={v}" for k, v in outcome.items()) + click.echo(f" {name}: {outcome}") + if body.get("restart_required"): + click.secho( + f" [WARN] changed but needs a restart: {body['restart_required']}", + fg="yellow", + ) + if body.get("ok"): + click.secho("Config reloaded", fg="green") + else: + # Partial by design: a bad settings.yaml must not block a valid cron + # edit. Non-zero so a script doesn't read this as a clean apply. + click.secho("Config reload incomplete — see the errors above", fg="red") + ctx.exit(1) + + @main.command() @click.option("--dry-run", is_flag=True, help="Show what would change without writing.") @click.pass_context @@ -1238,8 +1405,10 @@ def config_sync( """Pull the workspace from its git remote (the shared config repo). Fast-forward only. This moves the files; it does not tell a running daemon - about them. The daemon applies them on its next sync cycle, or immediately - via POST /api/config/sync. + about them. The daemon applies them on its next sync cycle, because that loop + compares what is on disk against the revision it last applied. For that to + happen sooner, run `nerve reload`: POST /api/config/sync would find nothing + left to merge and reload nothing. """ from nerve.sync_service import sync_workspace @@ -1361,7 +1530,9 @@ def codex_doctor(ctx: click.Context, json_output: bool) -> None: ) config = ctx.obj["config"] - backend = CodexBackend(SimpleNamespace(config=config)) + # deps.config is a callable, not the object — see BackendDeps. Nothing + # reloads in a one-shot command, but the backend reads it as a callable. + backend = CodexBackend(SimpleNamespace(config=lambda: config)) status = asyncio.run(backend.preflight(force=True)) report = { "preflight": status, diff --git a/nerve/config_reload.py b/nerve/config_reload.py new file mode 100644 index 00000000..e30b2fd0 --- /dev/null +++ b/nerve/config_reload.py @@ -0,0 +1,346 @@ +"""Unified config hot-reload. + +A single entry point that re-reads config from disk and reloads every subsystem +that supports it without a restart: the process config object (so lockdown and +settings changes engage), the long-lived services that captured that object at +start-up, cron jobs, cron sources, MCP servers, and skills. Best-effort per +subsystem — one failure is reported but does not abort the rest, because +refusing a valid cron edit over an unrelated typo in ``settings.yaml`` is the +worse outcome. Which subsystems fell over is reported, never inferred: see +:func:`reload_failures`. + +Nothing reloads on its own. A reload happens when an operator asks for one +(``nerve reload`` / ``POST /api/config/reload``) or when a workspace sync merges a +change. Editing a config file on the box does not apply itself. + +Restart-only (NOT reloaded here): the gateway socket (host/port/SSL), the +Telegram bot's token and allow-list, the MCP endpoint (including the +``auth.jwt_secret`` it checks ``/mcp/v1`` against, which the web gateway reads +per request), Langfuse, the memory bridges, the Codex thread-sync service +(``sync.codex.*`` — a different service from the cron sources under ``sync.*``, +and the one place those two names diverge), anything a service derived from +config at construction, and a background loop that was never started because its +feature was off. The hot-reload table in ``docs/config.md`` is the +operator-facing list of exactly what a reload covers; keep the two in step. + +:func:`restart_required` diffs the old and new config over +:data:`_RESTART_ONLY_PATHS` and records what changed in the summary. Without it +the summary reports only what was applied, which a caller cannot distinguish from +"nothing needed applying". ``gateway.host``/``port`` are the case that matters: +they live in the tracked settings, so the change can arrive by workspace sync. + +It reports; it does not gate. A setting whose only protection is a line in that +report is a setting the daemon is not applying — which is tolerable for a bound +socket and not for a policy that was tightened. Where the holder can resolve +config per read instead, that is the fix, and the path leaves this list: see +:func:`_repoint`. +""" + +from __future__ import annotations + +import dataclasses +import logging +from pathlib import Path + +logger = logging.getLogger(__name__) + +# A subsystem that failed is reported in-band, as a marked string in the +# summary, so the caller sees the outcome of every subsystem rather than the +# first exception. Written and parsed through this one constant so the producer +# and the consumers cannot drift apart. +_ERROR_PREFIX = "error: " + +# Config paths a reload cannot apply. Mirrors the "What still needs a restart" +# table in ``docs/config.md``; keep the two in step. That table says the check +# covers every unconditional entry in it, so a row without an entry here is a +# promise the daemon does not keep — +# ``tests/test_config_reload.py::TestRestartTableCoverage`` reads the table and +# fails on the difference. +# +# Only the entries this function can decide by comparing two values. The +# conditional ones in that table ("turning X on", "adding the first target", "a +# session already running") depend on runtime state not available here, and +# reporting them unconditionally would mean false alarms on every reload. +# +# A path may name a whole section (``proxy``) rather than its fields: where +# everything under a section is restart-only and there is nothing useful to say +# about which field moved, the section compares in one entry and keeps covering +# it when a field is added later. Where only part of a section is restart-only, +# or the individual values are what an operator needs to read, the entry names +# the field. +_RESTART_ONLY_PATHS = ( + "agent.max_concurrent", + "auth.jwt_secret", + "codex.home_dir", + "external_agents.enabled", + "gateway.host", + "gateway.port", + "gateway.ssl.cert", + "gateway.ssl.key", + "langfuse.host", + "langfuse.public_key", + "langfuse.redact_patterns", + "langfuse.secret_key", + "mcp_endpoint.enabled", + "mcp_endpoint.include_hoa", + "mcp_endpoint.path", + "memory", + "proxy", + "sync.codex", + "telegram.allowed_users", + "telegram.bot_token", + "telegram.enabled", + "timezone", + "workflows.enabled", + "workflows.poll_interval_seconds", + "workflows.review_loop.enabled", + "workflows.review_loop.reconcile_interval_seconds", + "workspace", + "xmemory", +) + +# Paths whose value the summary must not carry. It is logged and returned over +# HTTP, so a secret printed here is a secret in the log file as well. +_SECRET_PATHS = frozenset({ + "auth.jwt_secret", + "langfuse.public_key", + "langfuse.secret_key", + "telegram.bot_token", +}) + +_UNSET = object() + + +def _dotted_attr(obj, dotted: str): + """Follow a dotted attribute path, returning ``_UNSET`` if any hop is missing. + + ``_UNSET`` rather than ``None`` so that an absent attribute is distinguishable + from one whose value is ``None``. + """ + node = obj + for part in dotted.split("."): + if not hasattr(node, part): + return _UNSET + node = getattr(node, part) + return node + + +def _describe(path: str, before, after) -> str: + """One summary line for a changed path. + + The values are what make the warning actionable — "8900 → 9100" says which + edit is waiting on the restart — so they are shown except where they cannot + be: a secret, and a whole section, whose repr is both unreadable and liable + to hold one (``proxy`` carries the local proxy's API key). + """ + if path in _SECRET_PATHS or dataclasses.is_dataclass(before): + return f"{path}: changed" + return f"{path}: {before!r} → {after!r}" + + +def restart_required(old_config, new_config) -> list[str]: + """Dotted paths that changed but cannot take effect until a restart. + + Returns an empty list if either argument is ``None`` — no live config yet, or + the new one failed to load — since there is nothing to compare. + """ + if old_config is None or new_config is None: + return [] + changed = [] + for path in _RESTART_ONLY_PATHS: + before = _dotted_attr(old_config, path) + after = _dotted_attr(new_config, path) + if before is _UNSET or after is _UNSET: + continue + if before != after: + changed.append(_describe(path, before, after)) + return changed + + +def reload_failures(summary: dict) -> dict[str, str]: + """The subsystems in a :func:`reload_all` summary that failed, mapped to why. + + ``reload_all`` never raises and keeps going after a failure, so this is the + only way to tell a reload that applied everything from one that applied part + of it. An empty dict means every subsystem took the new config. + """ + return { + name: outcome[len(_ERROR_PREFIX):] + for name, outcome in summary.items() + if isinstance(outcome, str) and outcome.startswith(_ERROR_PREFIX) + } + + +def _external_agents_service(): + """The running external-agents sweeper, or ``None`` outside a live gateway.""" + try: + from nerve.gateway.routes._deps import get_deps + + return get_deps().external_agents_sync + except Exception: # noqa: BLE001 — no gateway (CLI, tests): nothing to re-point + return None + + +def _workflow_run_service(): + """The running workflow-run service, or ``None`` outside a live gateway.""" + try: + from nerve.workflows import get_workflow_run_service + + return get_workflow_run_service() + except Exception: # noqa: BLE001 — no gateway (CLI, tests): nothing to re-point + return None + + +def _repoint(new_config, engine, cron_service) -> list[str]: + """Hand *new_config* to the long-lived objects still holding the old one. + + Replacing the process-wide config object covers everything that reads it per + use, but the services built during start-up each kept their own reference. A + service left pointing at the previous object is worse than a uniformly stale + daemon: half the process runs on the new config and half on the old, with + nothing to say which half is which. + + This list is deliberately short, and shrinking it is the better fix wherever + it can be done: the agent backends used to need re-pointing and now resolve + config through a callable onto the engine's attribute instead, which also + removed a second-level cache (``codex``) that re-pointing alone would have + missed. Prefer that shape — one authority, resolved per read — over adding a + holder here, because every holder here is a place the two halves can drift. + + Only objects whose config reads all happen per use are re-pointed. A value + some other object *derived* at construction — a semaphore's size, a + scheduler's timezone, a bot's cached allow-list, a bound socket — is not + rebuilt and still needs a restart. + + Returns a description of every holder that could not be re-pointed (normally + empty). Never raises: having loaded the config, a reload should report which + part of the hand-off failed rather than lose the whole step. It can only + report what it attempts, which is the other reason to keep the list short. + """ + problems: list[str] = [] + + def hand_over(label: str, target) -> None: + if target is None: + return + try: + target.config = new_config + except Exception as e: # noqa: BLE001 — report, don't abandon the rest + problems.append(f"{label}: {e}") + logger.warning( + "Could not re-point the %s at the reloaded config: %s", label, e, + ) + + hand_over("cron service", cron_service) + if engine is not None and hasattr(engine, "config"): + # Assignment, not a method call: the engine's setter takes the new object + # and moves the collaborators it seeded (the session manager's backend + # and model defaults) in the same step, and its backends read config + # through it, so the whole agent subtree lands together. + hand_over("agent engine", engine) + # Every notification setting is read at send time, so re-pointing the + # service is all it takes for channels, expiry and quiet hours to follow + # a reload. The Telegram *channel* is not here because it needs nothing: + # it resolves config through a callable. Its allow-list is still + # restart-only, cached as a set when the bot was built. + hand_over( + "notification service", getattr(engine, "notification_service", None), + ) + + # The workflow-run service holds its own reference and reads through it at + # use, so the budget ceiling, the concurrency limit, the warning fraction and + # the journal location all follow a reload. Its poll cadence does not: the + # monitor loop was handed an interval when it started, which is the + # start-up-derived case above and still wants a restart. + hand_over("workflow run service", _workflow_run_service()) + + # The external-agents sweeper keeps its own reference, and the routes that + # add, remove or toggle a target mutate the process config in place. Left on + # the old object it would keep rendering the old target list while every + # toggle reported success. + sweeper = _external_agents_service() + if sweeper is not None: + try: + sweeper.update_config(new_config) + except Exception as e: # noqa: BLE001 + problems.append(f"external-agents sync: {e}") + logger.warning( + "Could not re-point the external-agents sync at the reloaded " + "config: %s", e, + ) + + return problems + + +async def reload_all(engine, cron_service, config_dir: Path) -> dict: + """Re-read config and hot-reload all reloadable subsystems. + + Returns a per-subsystem summary dict; :func:`reload_failures` turns it into + the list of subsystems that did not take the new config. Never raises — each + step is guarded — so a caller that does not inspect the summary is claiming a + success it has not checked. + """ + from nerve.config import get_config, load_config, set_config + + summary: dict = {} + + # Captured before set_config replaces it, since the comparison below needs + # the values the running process is actually using. + try: + old_config = get_config() + except Exception: # noqa: BLE001 — no config set yet (early boot, tests) + old_config = None + + # 1. Config object — so lockdown and settings changes engage everywhere. + try: + new_config = load_config(config_dir) + set_config(new_config) + except Exception as e: # noqa: BLE001 — e.g. an invalid edit; report, keep going + summary["config"] = f"{_ERROR_PREFIX}{e}" + logger.warning("config reload failed: %s", e, exc_info=True) + else: + summary["config"] = "reloaded" + # Not an error — the reload applied everything it can. Reported so the + # caller can tell "applied" from "cannot apply without a restart". + pending = restart_required(old_config, new_config) + if pending: + summary["restart_required"] = "; ".join(pending) + logger.warning( + "config reload: these changed but need a restart to take " + "effect: %s", summary["restart_required"], + ) + stale = _repoint(new_config, engine, cron_service) + if stale: + # The config itself loaded, so this is its own line: the daemon is + # running the new config in some places and the old one in others, + # which is the state worth shouting about. + summary["services"] = f"{_ERROR_PREFIX}{'; '.join(stale)}" + + # 2. Cron jobs + sources. + if cron_service is not None: + try: + summary["cron"] = await cron_service.reload() + except Exception as e: # noqa: BLE001 + summary["cron"] = f"{_ERROR_PREFIX}{e}" + try: + summary["sources"] = await cron_service.reload_sources() + except Exception as e: # noqa: BLE001 + summary["sources"] = f"{_ERROR_PREFIX}{e}" + + # 3. MCP servers. + if engine is not None: + try: + servers = await engine.reload_mcp_config() + summary["mcp"] = f"{len(servers)} server(s)" + except Exception as e: # noqa: BLE001 + summary["mcp"] = f"{_ERROR_PREFIX}{e}" + + # 4. Skills (re-scan the workspace skills dir). + mgr = getattr(engine, "_skill_manager", None) if engine is not None else None + if mgr is not None: + try: + skills = await mgr.discover() + summary["skills"] = f"{len(skills)} discovered" + except Exception as e: # noqa: BLE001 + summary["skills"] = f"{_ERROR_PREFIX}{e}" + + return summary diff --git a/nerve/cron/service.py b/nerve/cron/service.py index aa7ac4cc..a1ccdd5a 100644 --- a/nerve/cron/service.py +++ b/nerve/cron/service.py @@ -52,16 +52,20 @@ def _resolve_wakeup_prompt(prompt: str) -> str: return _WAKEUP_SENTINEL_PROMPT if prompt.strip() in _WAKEUP_SENTINELS else prompt -def _drop_reserved(jobs: list[CronJob]) -> list[CronJob]: - """Drop jobs holding a reserved id, warning once per offender. +def _drop_reserved(jobs: list[CronJob]) -> tuple[list[CronJob], list[str]]: + """Split *jobs* into the ones that may be scheduled and the reserved ids. Nothing but a hand-edited YAML file can produce one of these, and a job that simply never runs is near-impossible to diagnose from the outside, so the - warning has to name the job and the way out of it. + warning has to name the job and the way out of it — and the ids come back to + the caller so a reload can report the refusal instead of leaving the operator + to wonder why their job vanished. """ kept: list[CronJob] = [] + rejected: list[str] = [] for job in jobs: if is_reserved_job_id(job.id): + rejected.append(job.id) logger.warning( "Cron job '%s' (from %s) will not be scheduled: its id is " "reserved by the daemon (%s). Rename the job to schedule it.", @@ -71,7 +75,7 @@ def _drop_reserved(jobs: list[CronJob]) -> list[CronJob]: ) else: kept.append(job) - return kept + return kept, rejected class NotCrontabError(ValueError): @@ -251,10 +255,17 @@ def __init__(self, config: NerveConfig, engine: AgentEngine, db: Database): self.timezone = ZoneInfo(config.timezone) self.scheduler = AsyncIOScheduler(timezone=self.timezone) self._jobs: list[CronJob] = [] + # Ids from the last load that were refused for being reserved. Reported + # by reload() so the operator hears about it through the same channel + # that told them the reload succeeded. + self._rejected_job_ids: list[str] = [] self._source_runners: list[SourceRunner] = [] self._job_locks: dict[str, asyncio.Lock] = {} - # Serialize reload() so the file watcher, the sync loop, and the HTTP - # route can't interleave scheduler mutations. + # Set by the gateway before start() so freshly-(re)built source runners + # get wired to health-alert notifications, including after a reload. + self.notification_service = None + # Serialize reload() so the sync loop and the HTTP routes can't + # interleave scheduler mutations. self._reload_lock = asyncio.Lock() async def start(self) -> None: @@ -316,52 +327,7 @@ async def start(self) -> None: logger.info("Scheduled job: %s (%s)", job.id, job.schedule) # Register source runners (pure ingestors — no engine needed) - try: - from nerve.sources.registry import build_source_runners - - self._source_runners = build_source_runners(self.config, self.db) - - for runner in self._source_runners: - source_name = runner.source.source_name - # Source names can be compound (e.g. "gmail:account@email.com"). - # The config key is the base type before the colon. - config_key = source_name.split(":")[0] - source_config = getattr(self.config.sync, config_key, None) - if source_config is None: - continue - schedule_str = getattr(source_config, "schedule", "*/15 * * * *") - - try: - trigger = _crontab_to_trigger( - schedule_str, timezone=self.timezone, - ) - except NotCrontabError: - seconds = _parse_interval(schedule_str) - trigger = IntervalTrigger( - seconds=seconds, timezone=self.timezone, - ) - except InvalidScheduleError as e: - # Same call as the cron loop above, same answer — skip this - # one, keep the rest. Letting it reach the enclosing - # `except Exception` would abandon every source after it in - # the list under a single "failed to register source - # runners" warning that names the wrong problem. - logger.error( - "Source '%s' will not be scheduled: %s", source_name, e, - ) - continue - - self.scheduler.add_job( - self._run_source_wrapper, - trigger, - args=[runner], - id=runner.job_id, - name=f"Source: {source_name}", - replace_existing=True, - ) - logger.info("Scheduled source: %s (%s)", source_name, schedule_str) - except Exception as e: - logger.warning("Failed to register source runners: %s", e, exc_info=True) + self._register_source_runners() # Daily cleanup of expired messages and consumer cursors self.scheduler.add_job( @@ -421,14 +387,18 @@ async def reload(self) -> dict: carry out raises with the running schedule exactly as it was. That covers the gate registry too; see :meth:`_reload_locked`. - Serialized via a lock so concurrent callers (file watcher, sync loop, - HTTP route) can't interleave scheduler mutations. The two properties are + Serialized via a lock so concurrent callers (sync loop, HTTP routes) + can't interleave scheduler mutations. The two properties are separate: the lock keeps two reloads from overlapping, and the planning pass keeps a single failing one from applying half of itself. - Returns a summary dict: ``{"added", "removed", "updated", "enabled"}``, - where "updated" is every enabled job that already had a trigger. It says - what the reload rescheduled, not which jobs the file changed. + Returns a summary dict: ``{"added", "removed", "updated", "enabled", + "rejected"}``. ``updated`` is every enabled job that already had a + trigger — it says what the reload rescheduled, not which jobs the file + changed. ``enabled`` counts jobs that are scheduled, not jobs whose YAML + says ``enabled: true``; ``rejected`` names the ids the loader refused + because the daemon reserves them, which would otherwise be the one class + of job that disappears from the reload without the reload mentioning it. """ async with self._reload_lock: return await self._reload_locked() @@ -555,18 +525,185 @@ async def _reload_from_disk( # Committed, so the gates that disappeared really are gone from the jobs # that are now live — say so (the loader was told to keep quiet above). warn_vanished_gates(gates_before) + # Every enabled job above either kept a trigger or was given one, so this + # count describes the scheduler and not just the file. Jobs the loader + # refused for holding a reserved id are not in new_by_id at all, and are + # reported separately rather than being silently absent. enabled = sum(1 for j in new_by_id.values() if j.enabled) + rejected = list(self._rejected_job_ids) logger.info( - "Cron reloaded: +%d added, ~%d updated, -%d removed (%d enabled)", - len(added), len(updated), len(removed), enabled, + "Cron reloaded: +%d added, ~%d updated, -%d removed (%d enabled, " + "%d rejected)", + len(added), len(updated), len(removed), enabled, len(rejected), ) return { "added": added, "removed": removed, "updated": updated, "enabled": enabled, + "rejected": rejected, } + def _source_schedule(self, runner) -> str | None: + """The configured schedule for *runner*, or ``None`` if it has no config. + + Source names can be compound (e.g. "gmail:account@email.com"). The + config key is the base type before the colon. + """ + config_key = runner.source.source_name.split(":")[0] + source_config = getattr(self.config.sync, config_key, None) + if source_config is None: + return None + return getattr(source_config, "schedule", "*/15 * * * *") + + def _plan_source_runners( + self, runners: list, + ) -> list[tuple["SourceRunner", CronTrigger | IntervalTrigger, str]]: + """Pair every runner with the trigger it is to be scheduled on. + + The planning half of a source reload: nothing here touches the + scheduler, and anything that cannot be scheduled raises before the + caller has removed the running jobs. + :meth:`_schedule_source_runners` is the start-up counterpart, which + skips what this refuses. + + Stricter than start-up on both counts. An interval string naming no + usable interval (``hourly``, ``@daily``) raises instead of taking + :func:`_parse_interval`'s 2h default: at boot a conservative cadence + beats a source that never runs, but on a reload it moves a source off + the cadence the operator wrote while the response reports success. A + runner whose source has no config section is dropped here rather than + at the scheduler, so the response can be built from what was scheduled. + + Raises :class:`InvalidScheduleError`, naming the source. + """ + planned: list[tuple[SourceRunner, CronTrigger | IntervalTrigger, str]] = [] + for runner in runners: + schedule_str = self._source_schedule(runner) + if schedule_str is None: + continue + source_name = runner.source.source_name + try: + trigger = _crontab_to_trigger(schedule_str, timezone=self.timezone) + except NotCrontabError: + seconds = _interval_seconds(schedule_str) + if seconds is None: + raise InvalidScheduleError( + f"Source '{source_name}': schedule {schedule_str!r} is " + "neither a crontab expression nor an interval", + ) from None + trigger = IntervalTrigger(seconds=seconds, timezone=self.timezone) + except InvalidScheduleError as e: + raise InvalidScheduleError(f"Source '{source_name}': {e}") from e + planned.append((runner, trigger, schedule_str)) + return planned + + def _apply_source_runners(self, planned: list) -> None: + """Put planned runner/trigger pairs on the scheduler (wiring + notifications). No parsing, so nothing here can fail half-way.""" + for runner, trigger, schedule_str in planned: + if self.notification_service is not None: + runner.set_notification_service(self.notification_service) + source_name = runner.source.source_name + self.scheduler.add_job( + self._run_source_wrapper, + trigger, + args=[runner], + id=runner.job_id, + name=f"Source: {source_name}", + replace_existing=True, + ) + logger.info("Scheduled source: %s (%s)", source_name, schedule_str) + + def _schedule_source_runners(self, runners: list) -> None: + """Schedule already-built source runners, skipping the unschedulable. + + The start-up path, deliberately lenient: one source with a typo in its + schedule must not cost the daemon the others, and an interval string the + parser cannot use falls back to 2h rather than leaving the source + unscheduled. A reload plans first and refuses the whole change instead + (:meth:`_plan_source_runners`). + """ + planned = [] + for runner in runners: + schedule_str = self._source_schedule(runner) + if schedule_str is None: + continue + try: + trigger = _crontab_to_trigger(schedule_str, timezone=self.timezone) + except NotCrontabError: + seconds = _parse_interval(schedule_str) + trigger = IntervalTrigger(seconds=seconds, timezone=self.timezone) + except InvalidScheduleError as e: + # Same call as the cron loop, same answer — skip this one, keep + # the rest. Letting it reach the caller's `except Exception` + # would abandon every source after it in the list under a single + # "failed to register source runners" warning naming the wrong + # problem. + logger.error( + "Source '%s' will not be scheduled: %s", + runner.source.source_name, e, + ) + continue + planned.append((runner, trigger, schedule_str)) + self._apply_source_runners(planned) + + def _register_source_runners(self) -> None: + """Build + schedule source runners at startup. Swallows errors so a bad + source config never crashes the daemon boot.""" + try: + from nerve.sources.registry import build_source_runners + + self._source_runners = build_source_runners(self.config, self.db) + self._schedule_source_runners(self._source_runners) + except Exception as e: # noqa: BLE001 — boot must not fail on a bad source + logger.warning("Failed to register source runners: %s", e, exc_info=True) + + async def reload_sources(self) -> dict: + """Rebuild source runners from config and reschedule them without a + restart (picks up added/removed sources and schedule changes). + + All-or-nothing, like :meth:`reload`. Building the runners and planning + every trigger both happen before a single job is removed, so a build + failure or a schedule the scheduler will not take raises with the + running sources exactly as they were. Removing first and parsing the new + schedules afterwards is how a working ``*/15`` came to be destroyed by + the typo meant to replace it, with the source still named under + ``sources`` and the reload answering ``ok``. + + The returned ``sources`` are the runners that are on the scheduler, not + the ones that were built: those are the same list only when nothing was + dropped, and the case where they differ is the one a caller needs told. + + Runners schedule under ``source:``, and the whole ``source:`` + namespace is refused to cron jobs when they are loaded, so the + ``replace_existing=True`` below cannot take a user's job away from them. + The reservation has to cover the namespace rather than the runners that + happen to be live: otherwise a job could hold ``source:telegram`` while + telegram is switched off and lose its trigger — permanently, and while + every reload kept counting it as enabled — the moment it was switched + back on. + """ + from nerve.sources.registry import build_source_runners + + async with self._reload_lock: + # -- Plan: everything that can fail, before anything is applied ---- + new_runners = build_source_runners(self.config, self.db) # may raise → caller reports + planned = self._plan_source_runners(new_runners) + + # -- Apply: scheduler bookkeeping only ----------------------------- + old_ids = {r.job_id for r in self._source_runners} + for jid in old_ids: + if self.scheduler.get_job(jid) is not None: + self.scheduler.remove_job(jid) + self._source_runners = new_runners + self._apply_source_runners(planned) + new_ids = {runner.job_id for runner, _, _ in planned} + return { + "sources": sorted(new_ids), + "removed": sorted(old_ids - new_ids), + } + async def stop(self) -> None: """Stop the scheduler.""" self.scheduler.shutdown(wait=False) @@ -897,7 +1034,8 @@ def _load_merged_jobs(self, strict: bool = False) -> list[CronJob]: # Tag all as user-sourced (no system file yet) for j in user_jobs: j.metadata["_source"] = "user" - return _drop_reserved(user_jobs) + kept, self._rejected_job_ids = _drop_reserved(user_jobs) + return kept # Tag sources for display in CLI for j in system_jobs: @@ -918,7 +1056,8 @@ def _load_merged_jobs(self, strict: bool = False) -> list[CronJob]: for j in user_jobs: jobs_by_id[j.id] = j - return _drop_reserved(list(jobs_by_id.values())) + kept, self._rejected_job_ids = _drop_reserved(list(jobs_by_id.values())) + return kept async def _run_job_wrapper(self, job: CronJob) -> None: """Wrapper to run a cron job with logging and optional lock.""" diff --git a/nerve/external_agents/sync_service.py b/nerve/external_agents/sync_service.py index ded81b71..08bda0b4 100644 --- a/nerve/external_agents/sync_service.py +++ b/nerve/external_agents/sync_service.py @@ -94,6 +94,24 @@ def __init__(self, config: NerveConfig) -> None: conflict_policy=config.external_agents.conflict_policy, ) + def update_config(self, config: NerveConfig) -> None: + """Adopt a freshly loaded config object after a config reload. + + Both the target list and the sweep interval are read from the config on + every sweep, so re-pointing is enough for a change to either to take + effect on the next one. The conflict policy is the one value cached + away from the config object, so the writer is rebuilt alongside. + + Necessary because the routes that add, remove or toggle a target edit + the *process* config object in place: after a reload replaced it, a + sweeper still holding the previous one would keep rendering the old + target list while every toggle reported success. + """ + self._config = config + self._writer = ConfigWriter( + conflict_policy=config.external_agents.conflict_policy, + ) + # ---- Lifecycle ------------------------------------------------- async def start(self) -> None: @@ -141,11 +159,17 @@ async def stop(self, timeout: float = GRACEFUL_STOP_SECONDS) -> None: self._task = None async def _loop(self) -> None: - interval = max( - _MIN_SWEEP_INTERVAL_SECONDS, - self._config.external_agents.sync_interval_minutes * 60, - ) + # Read per cycle, not once: a config reload replaces the object this + # service points at, and an interval frozen before the loop would + # outlive it. The top-level external_agents.enabled flag is a different + # matter — it is only consulted where this service is created, so + # switching it either way needs a restart. Per-target enabled flags are + # read on every sweep and do follow a reload. while not self._stop_event.is_set(): + interval = max( + _MIN_SWEEP_INTERVAL_SECONDS, + self._config.external_agents.sync_interval_minutes * 60, + ) try: await asyncio.wait_for( self._stop_event.wait(), timeout=interval, diff --git a/nerve/gateway/routes/config.py b/nerve/gateway/routes/config.py index 733f0394..30970b3f 100644 --- a/nerve/gateway/routes/config.py +++ b/nerve/gateway/routes/config.py @@ -12,15 +12,68 @@ router = APIRouter() +@router.post("/api/config/reload") +async def reload_config_route(user: dict = Depends(require_auth)): + """Re-read config and hot-reload cron, sources, MCP, and skills (no restart). + + Apply config edits made on this box without restarting. Some settings + (gateway host/port, provider/auth, agent backend) still require a restart — + ``docs/config.md`` has the full list. + + ``ok`` is derived from what the subsystems actually reported, and ``errors`` + names the ones that failed. A reload is best-effort by design: an invalid + ``settings.yaml`` must not stop a valid cron edit from being applied, so the + other subsystems still run and the response is still 200 — but it says so + rather than reporting a success it did not earn. + + ``restart_required`` lists config that changed but cannot take effect until + the process restarts (see ``docs/config.md``). It is a separate field rather + than a line in ``detail`` because a caller reading ``ok`` alone would treat it + as fully applied. It is not in ``errors`` and does not affect ``ok``: nothing + failed, and the reload applied everything it can. + """ + from nerve.config import get_config + from nerve.config_reload import reload_all, reload_failures + from nerve.gateway.server import _cron_service + + config = get_config() + deps = get_deps() + config_dir = Path(config.config_dir) if config.config_dir else Path(config.workspace) + summary = await reload_all(deps.engine, _cron_service, config_dir) + failures = reload_failures(summary) + return { + "ok": not failures, + "detail": summary, + "errors": failures, + "restart_required": summary.get("restart_required", ""), + } + + @router.post("/api/config/sync") async def sync_workspace_route(user: dict = Depends(require_auth)): - """Pull the workspace from its git remote and apply changes (cron + MCP). + """Pull the workspace from its git remote and apply what it merged. + + Fast-forward only; validates the pulled bundle before applying. Applying runs + the unified reload: the config object and the services holding their own + reference, then cron jobs, cron sources, MCP servers and skills. + + ``ok`` is scored differently here than on ``/api/config/reload``, which runs + the same reload: it answers "did the merge take", so it stays true when the + merged config loaded and only some later subsystem stumbled. That case is + ``applied: false`` with the reason in ``reload_errors``. Callers who want + "did everything apply" should read ``applied``. - Fast-forward only; validates the pulled bundle before applying. + ``applied`` alone cannot say *why* it is false: nothing needed merging and a + merge that no subsystem took both report false. ``status`` separates them — + ``applied``, ``partial``, ``not-applied``, ``up-to-date`` — so a script can + branch on the outcome rather than on the absence of one. It says what *this + call* did; whether the daemon is running the revision now on disk is + ``GET /api/config/sync``. """ import asyncio from nerve.config import get_config + from nerve.config_reload import reload_failures from nerve.gateway.server import _cron_service from nerve.sync_service import _apply_sync, sync_workspace @@ -46,22 +99,85 @@ async def sync_workspace_route(user: dict = Depends(require_auth)): "warnings": result.validation_warnings, }, ) - apply_error = None + summary: dict = {} if result.changed: deps = get_deps() config_dir = Path(config.config_dir) if config.config_dir else Path(config.workspace) - apply_error = await _apply_sync(deps.engine, _cron_service, config_dir) + summary = await _apply_sync(deps.engine, _cron_service, config_dir) + failures = reload_failures(summary) # The merge is a real state change, so this is not a 4xx — but a merge whose # config the daemon then refused to load has applied nothing, and reporting # ok for it would tell an operator who just enabled lockdown that the box is - # locked while its write guards are still open. + # locked while its write guards are still open. A subsystem that failed after + # the config loaded is less severe (the merged settings *are* in effect) but + # still leaves the daemon only partly on the new config, so it keeps `ok` and + # clears `applied` rather than being folded into either. + apply_error = failures.get("config") + if not result.changed: + status = "up-to-date" + elif apply_error is not None: + status = "not-applied" + elif failures: + status = "partial" + else: + status = "applied" return { "ok": apply_error is None, "changed": result.changed, - "applied": result.changed and apply_error is None, + "applied": result.changed and not failures, + "status": status, "apply_error": apply_error, + "reload": summary, + "reload_errors": failures, "message": result.message, "old_rev": result.old_rev, "new_rev": result.new_rev, "warnings": result.validation_warnings, } + + +@router.get("/api/config/sync") +async def sync_status_route(user: dict = Depends(require_auth)): + """What the periodic sync loop last did, and whether it is stuck. + + The loop is the only thing that applies merged config on its own, and until + this endpoint existed its outcome went to the log and nowhere else — so an + instance that had been refusing to merge for a week answered every other + question exactly like a healthy one. + + ``blocked_paths`` is the reviewed-surface local state that is making sync + refuse; while it is non-empty this instance is pinned to ``applied_rev`` and + no merged config reaches it. ``reload_errors`` names subsystems that did not + take the last applied config; those are retried every cycle. + + ``checked: false`` means this process has no answer — sync is off, or the + loop has not completed a cycle yet — not that everything is fine. + """ + from datetime import datetime, timezone + + from nerve.config import get_config + from nerve.sync_service import last_sync_state + + config = get_config() + state = last_sync_state() + body = { + "enabled": config.workspace_sync.enabled, + "interval_minutes": config.workspace_sync.interval_minutes, + "branch": config.workspace_sync.branch, + "checked": state is not None, + } + if state is None: + return body + return { + **body, + "ok": state.ok, + "applied_rev": state.applied_rev, + "fetched_rev": state.fetched_rev, + "blocked": bool(state.blocked_paths), + "blocked_paths": state.blocked_paths, + "reload_errors": state.reload_errors, + "message": state.message, + "checked_at": datetime.fromtimestamp( + state.checked_at, timezone.utc, + ).isoformat(), + } diff --git a/nerve/gateway/server.py b/nerve/gateway/server.py index 9564d374..5314dc2b 100644 --- a/nerve/gateway/server.py +++ b/nerve/gateway/server.py @@ -113,6 +113,117 @@ async def _send_session_status( await websocket.send_json(status_msg) +async def _periodic_db_retention(db) -> None: + """Opt-in DB retention loop: compact old memorized messages' blocks/thinking + JSON, prune append-only telemetry + file snapshots, checkpoint the WAL. + + The file shrink (VACUUM) is an explicit operator step (``nerve db vacuum``), + never on this loop. + + Every setting is read from the current config each cycle, so the interval and + the retention windows follow a config reload, and switching retention off + stops the work from the next tick. Switching it *on* needs a restart: with no + task running there is nothing left to notice the flag. + """ + if not get_config().retention.enabled: + return + while True: + retention = get_config().retention + await asyncio.sleep(retention.interval_hours * 3600) + retention = get_config().retention + if not retention.enabled: + continue + try: + report = await db.run_retention( + retention_days=retention.retention_days, + retention_full_days=retention.retention_full_days, + ) + if ( + report.get("messages_compacted") + or report.get("telemetry_deleted") + or report.get("snapshots_deleted") + ): + logger.info("DB retention: %s", report) + except Exception as e: + logger.error("DB retention failed: %s", e) + + +async def _periodic_backup(notification_service) -> None: + """Opt-in backup loop: hourly tick, runs a bundle when the newest one in the + target dir is older than ``backup.interval_hours`` (or none exists). + + The heavy work (consistent DB snapshots + tar) runs in a thread so it never + blocks the event loop. Failures notify high-priority — a backup that fails + silently is worse than no backup at all. + + The tick runs whether or not backups are on and reads the current config each + time, so every ``backup.*`` setting — enabling it included — takes effect + without a restart. + """ + from nerve import backup as backup_mod + + nerve_dir = paths.nerve_home() + while True: + await asyncio.sleep(3600) # hourly tick + config = get_config() + bcfg = config.backup + interval_s = max(1, bcfg.interval_hours) * 3600 + target = Path(bcfg.target_dir).expanduser() if bcfg.target_dir else None + if not bcfg.enabled or target is None: + continue + try: + age = await asyncio.to_thread( + backup_mod.latest_bundle_age_seconds, target, + ) + if age is not None and age < interval_s: + continue # not due yet + + result = await asyncio.to_thread( + backup_mod.create_backup, + nerve_dir, + config.workspace, + target, + config_dir=config.config_dir, + include_workspace=bcfg.include_workspace, + include_secrets=True, + workspace_excludes=bcfg.workspace_excludes, + ) + deleted = await asyncio.to_thread( + backup_mod.prune, target, bcfg.retention_count, + ) + size_str = ( + f"{result.size / (1024 ** 3):.1f} GB" + if result.size >= 1024 ** 3 + else f"{result.size / (1024 ** 2):.0f} MB" + ) + logger.info( + "Scheduled backup OK: %s (%s, pruned %d)", + result.path.name, size_str, len(deleted), + ) + if bcfg.notify_on_success: + await notification_service.send_notification( + session_id="system", + title="💾 Backup OK", + body=( + f"{size_str}, {len(backup_mod.list_bundles(target))} " + f"kept ({result.file_count} files)" + ), + priority="low", + ) + except Exception as e: + logger.error("Scheduled backup failed: %s", e, exc_info=True) + if bcfg.notify_on_failure: + try: + await notification_service.send_notification( + session_id="system", + title="⚠️ Nerve backup FAILED", + body=f"{e}\n\nTarget: {target}", + priority="high", + ) + except Exception as ne: + logger.error("Backup failure notify failed: %s", ne) + + @asynccontextmanager async def lifespan(app: FastAPI): """Application lifespan — initialize DB, engine, channels on startup.""" @@ -213,7 +324,9 @@ async def lifespan(app: FastAPI): telegram_channel = None if config.telegram.enabled and config.telegram.bot_token: from nerve.channels.telegram import TelegramChannel - telegram_channel = TelegramChannel(config, _engine.router) + # get_config, not the object read above: the channel resolves config per + # use so a reload reaches the reads that happen per update (dm_policy). + telegram_channel = TelegramChannel(get_config, _engine.router) telegram_channel.set_notification_service(notification_service) _engine.register_channel(telegram_channel) await telegram_channel.start() @@ -227,15 +340,14 @@ async def lifespan(app: FastAPI): try: from nerve.cron.service import CronService cron = CronService(config, _engine, db) + # Wire health-alert notifications before start() so source runners built + # during start (and any later reload) pick it up. + cron.notification_service = notification_service await cron.start() cron_task = cron _cron_service = cron logger.info("Cron service started") - # Wire notification service to source runners for health alerts - for runner in cron._source_runners: - runner.set_notification_service(notification_service) - # Register cron jobs that suppress the session label in notifications for job in cron._jobs: if not job.show_session_label: @@ -288,7 +400,7 @@ async def lifespan(app: FastAPI): from nerve.workflows import init_review_loop_service _review_loop_service = init_review_loop_service( - config, db, _engine, _workflow_run_service, + get_config, db, _engine, _workflow_run_service, ) if _review_loop_service is not None: await _review_loop_service.start() @@ -353,18 +465,26 @@ async def lifespan(app: FastAPI): # Periodic session cleanup. Default cadence is every 6 hours (unchanged); # it tightens to hourly only when the opt-in interactive idle auto-close # (sessions.interactive_archive_after_hours > 0) is enabled and needs finer resolution. + # + # This and the loops below re-read get_config() per cycle rather than closing + # over the start-up object: a config reload replaces that object, and a loop + # holding the old one would keep applying settings the operator has already + # changed, with nothing to show for it. async def _periodic_cleanup(): while True: interval = ( - 3600 if config.sessions.interactive_archive_after_hours > 0 else 6 * 3600 + 3600 + if get_config().sessions.interactive_archive_after_hours > 0 + else 6 * 3600 ) await asyncio.sleep(interval) try: if _engine: + sessions = get_config().sessions stats = await _engine.sessions.run_cleanup( - archive_after_days=config.sessions.archive_after_days, - max_sessions=config.sessions.max_sessions, - interactive_archive_after_hours=config.sessions.interactive_archive_after_hours, + archive_after_days=sessions.archive_after_days, + max_sessions=sessions.max_sessions, + interactive_archive_after_hours=sessions.interactive_archive_after_hours, ) if ( stats.get("archived_stale") @@ -391,7 +511,10 @@ async def _periodic_cleanup(): async def _periodic_memorize(): from datetime import datetime, timezone while True: - await asyncio.sleep(config.sessions.memorize_interval_minutes * 60) + interval_minutes = get_config().sessions.memorize_interval_minutes + # Keep the diagnostics figure honest about the cadence in force. + _memorize_stats["interval_minutes"] = interval_minutes + await asyncio.sleep(interval_minutes * 60) try: if _engine: result = await _engine.run_memorization_sweep() @@ -441,102 +564,10 @@ async def _periodic_notify_maintenance(): notify_maintenance_task = asyncio.create_task(_periodic_notify_maintenance()) - # Periodic DB retention (opt-in). Compacts old memorized messages' - # blocks/thinking JSON and prunes append-only telemetry + file snapshots, - # then checkpoints the WAL. No-ops unless retention.enabled is set. The - # file shrink (VACUUM) is an explicit operator step (`nerve db vacuum`), - # never on this loop. - async def _periodic_db_retention(): - if not config.retention.enabled: - return - interval = config.retention.interval_hours * 3600 - while True: - await asyncio.sleep(interval) - try: - report = await db.run_retention( - retention_days=config.retention.retention_days, - retention_full_days=config.retention.retention_full_days, - ) - if ( - report.get("messages_compacted") - or report.get("telemetry_deleted") - or report.get("snapshots_deleted") - ): - logger.info("DB retention: %s", report) - except Exception as e: - logger.error("DB retention failed: %s", e) - - db_retention_task = asyncio.create_task(_periodic_db_retention()) - - # Periodic backup (opt-in). Hourly tick; runs a bundle when the newest - # one in the target dir is older than interval_hours (or none exists). - # The heavy work (consistent DB snapshots + tar) runs in a thread so it - # never blocks the event loop. Failures notify high-priority — a backup - # that fails silently is worse than no backup at all. - async def _periodic_backup(): - from nerve import backup as backup_mod - - bcfg = config.backup - nerve_dir = paths.nerve_home() - interval_s = max(1, bcfg.interval_hours) * 3600 - target = Path(bcfg.target_dir).expanduser() if bcfg.target_dir else None - while True: - await asyncio.sleep(3600) # hourly tick - if not bcfg.enabled or target is None: - continue - try: - age = await asyncio.to_thread( - backup_mod.latest_bundle_age_seconds, target, - ) - if age is not None and age < interval_s: - continue # not due yet - - result = await asyncio.to_thread( - backup_mod.create_backup, - nerve_dir, - config.workspace, - target, - config_dir=config.config_dir, - include_workspace=bcfg.include_workspace, - include_secrets=True, - workspace_excludes=bcfg.workspace_excludes, - ) - deleted = await asyncio.to_thread( - backup_mod.prune, target, bcfg.retention_count, - ) - size_str = ( - f"{result.size / (1024 ** 3):.1f} GB" - if result.size >= 1024 ** 3 - else f"{result.size / (1024 ** 2):.0f} MB" - ) - logger.info( - "Scheduled backup OK: %s (%s, pruned %d)", - result.path.name, size_str, len(deleted), - ) - if bcfg.notify_on_success: - await notification_service.send_notification( - session_id="system", - title="💾 Backup OK", - body=( - f"{size_str}, {len(backup_mod.list_bundles(target))} " - f"kept ({result.file_count} files)" - ), - priority="low", - ) - except Exception as e: - logger.error("Scheduled backup failed: %s", e, exc_info=True) - if bcfg.notify_on_failure: - try: - await notification_service.send_notification( - session_id="system", - title="⚠️ Nerve backup FAILED", - body=f"{e}\n\nTarget: {target}", - priority="high", - ) - except Exception as ne: - logger.error("Backup failure notify failed: %s", ne) - - backup_task = asyncio.create_task(_periodic_backup()) + # Both opt-in, both no-ops unless their config says otherwise; see their + # docstrings for which of their settings survive a config reload. + db_retention_task = asyncio.create_task(_periodic_db_retention(db)) + backup_task = asyncio.create_task(_periodic_backup(notification_service)) # Start the external-agents sync service. It re-renders # ~/.codex/AGENTS.md, ~/.claude/CLAUDE.md, etc. from the workspace diff --git a/nerve/sync_service.py b/nerve/sync_service.py index 9880aae4..6d0d6728 100644 --- a/nerve/sync_service.py +++ b/nerve/sync_service.py @@ -6,9 +6,17 @@ hand-editing on the box. ``sync_workspace`` does a guarded ``git pull --ff-only`` and (optionally) -validates the pulled bundle. Applying the change (reloading cron / MCP) is the -caller's job: the in-daemon periodic loop reloads explicitly, while the CLI only -moves the files and leaves a running daemon to pick them up on its next cycle. +validates the pulled bundle. Applying the merged change is the caller's job: the +in-daemon periodic loop runs the unified reload itself — the config object and the +services holding their own reference, then cron jobs, cron sources, MCP servers +and skills — while the CLI only moves the files. + +A running daemon does pick those up on its next cycle, because the loop compares +what is on disk against the revision it last applied rather than against what its +own pull moved: a HEAD that moved out of band (``nerve config sync``, a bare +``git pull``) is a revision this process has never applied, and so is one whose +reload failed for some subsystem. With no daemon running nothing applies them +until one starts, which reads config from disk anyway. """ from __future__ import annotations @@ -69,6 +77,58 @@ class SyncResult: new_rev: str = "" validation_errors: list[str] = field(default_factory=list) validation_warnings: list[str] = field(default_factory=list) + # The reviewed-surface paths that made this sync refuse to merge. Also named + # in ``message``, but a caller deciding whether the instance has just entered + # the blocked state cannot get that out of prose. + blocked_paths: list[str] = field(default_factory=list) + + +@dataclass +class SyncState: + """What the periodic sync loop last did. + + The loop is the only thing that applies a merged change on its own, and its + outcome used to go to the log and nowhere else: nothing could ask whether + this instance was still taking config, so a sync that had been refusing for a + week looked exactly like one that had nothing to do. ``nerve doctor`` and + ``GET /api/config/sync`` read this. + + In memory, never persisted, for the same reason ``applied_rev`` is loop-local + (see :func:`run_periodic_sync`): a restart re-reads config from disk and + re-derives every field here. + """ + + # The revision this daemon has applied everywhere, as the loop tracks it. + applied_rev: str = "" + # The upstream revision the last cycle resolved. Different from + # ``applied_rev`` means config is on disk, or waiting on the remote, that + # this daemon is not running. + fetched_rev: str = "" + # Non-empty while sync is refusing to merge over local changes. + blocked_paths: list[str] = field(default_factory=list) + # Subsystems that did not take the last applied config; retried each cycle. + reload_errors: dict[str, str] = field(default_factory=dict) + ok: bool = True + message: str = "" + checked_at: float = 0.0 + + +_last_sync: SyncState | None = None + + +def last_sync_state() -> SyncState | None: + """The last sync cycle's outcome, or ``None`` if no loop has run here. + + ``None`` in every process that is not the daemon — the CLI, tests — and in + the daemon before the loop starts or when sync is off, so a reader has to + treat it as "not known here" rather than "nothing wrong". + """ + return _last_sync + + +def _record_sync_state(state: SyncState) -> None: + global _last_sync + _last_sync = state def _git(args: list[str], cwd: Path) -> subprocess.CompletedProcess: @@ -111,6 +171,20 @@ def _rev(ref: str, cwd: Path) -> str: return r.stdout.strip() if r.returncode == 0 else "" +def _head_rev(workspace) -> str: + """``HEAD`` of ``workspace``, or ``""`` when it cannot be read. + + Coerces the raw configured value inside the guard, for the reason + :func:`sync_workspace` spells out: this one runs before the periodic loop's + per-cycle ``try``, so a ``workspace`` that is ``None`` or a list would kill + the loop at start-up instead of failing a cycle. + """ + try: + return _rev("HEAD", Path(workspace)) + except Exception: # noqa: BLE001 — see above + return "" + + def _reviewed_pathspec(workspace: Path) -> list[str]: """The git pathspecs for the reviewed surface of ``workspace``. @@ -223,6 +297,25 @@ def _local_config_divergence( return blocking, warnings +def local_block_reasons(workspace, locked: bool = False) -> list[str]: + """What in ``workspace`` would make the next sync refuse to merge, now. + + The same check :func:`_sync_workspace` runs, minus the fetch — so it answers + from any process, which is what ``nerve doctor`` needs: the retained + :class:`SyncState` only exists inside the daemon, and the shell is where an + operator asks why config stopped arriving. It reports nothing when there is + no repository to check, since sync is not the answer to that question. + """ + try: + ws = Path(workspace) + if not is_git_repo(ws): + return [] + blocking, _warnings = _local_config_divergence(ws, "HEAD", locked) + return blocking + except Exception as e: # noqa: BLE001 — a diagnostic must not raise + return [f"could not check the workspace for local changes: {e}"] + + def _describe_paths(paths: list[str]) -> str: """Join paths for a message, summarizing once the list stops being readable.""" if len(paths) <= _MAX_LISTED_PATHS: @@ -400,7 +493,7 @@ def _sync_workspace( if blocking: return SyncResult( ok=False, changed=False, old_rev=old, new_rev=new, - validation_warnings=warnings, + validation_warnings=warnings, blocked_paths=blocking, message=( f"fetched {new[:8]} but the workspace's reviewed files have local " f"changes — not applying, because they would survive the " @@ -467,29 +560,54 @@ def _sync_workspace( async def run_periodic_sync(config, engine, cron_service, stop_event: asyncio.Event) -> None: """Daemon loop: pull the workspace every ``interval_minutes`` and apply. - On a successful, changed, valid pull it reloads MCP config and cron so the - merged changes take effect without a restart. Best-effort — a failed cycle is - logged and the loop continues. + On a successful, valid pull it runs the same unified reload as + ``POST /api/config/reload`` — the process-wide config object and the services + holding their own reference, then cron jobs, cron sources, MCP servers and + skills — so the merged changes take effect without a restart. Best-effort: a + failed cycle is logged and the loop continues. + + A cycle applies when the revision on disk is not the one this process has + applied, which is a wider condition than "this pull merged something" and + deliberately so — see ``applied_rev`` below. + + Every cycle's outcome is retained in a :class:`SyncState` and, when the merge + starts being refused, announced once to the operator — see + :func:`_record_cycle`. This loop is the only thing that applies a merged change on its own, so ``interval_minutes`` is the upper bound on how stale an instance can be. - ``POST /api/config/sync`` does the same pull-and-apply on demand. + ``POST /api/config/sync`` pulls and applies what it merged on demand; it does + not apply a revision that was already on disk when it ran, so the immediate + equivalent for that is ``POST /api/config/reload``. Every cycle re-reads the process-wide config object rather than working from a reference captured before the loop, so the loop can never be the reason a - setting is stuck. What refreshes that object is :func:`_apply_sync`, i.e. a - sync that actually merged something — after which ``branch``, ``validate``, - ``strict_env``, ``interval_minutes``, the workspace location and turning sync - off all apply from the following cycle. Nothing *else* refreshes it, so a - hand-edited ``config.yaml`` on the box still needs a restart. Turning sync - *on* needs one regardless: this task is created at start-up only when sync is - already enabled, and nothing creates it later. + setting is stuck. That object is replaced by any config reload — a sync that + merged something, or ``POST /api/config/reload`` — after which ``branch``, + ``validate``, ``strict_env``, ``interval_minutes``, the workspace location and + turning sync off all apply from the following cycle. A file edited on the box + does *not* reload itself, so a hand-edited ``config.yaml`` reaches this loop + only once a reload is asked for. Turning sync *on* needs a restart regardless: + this task is created at start-up only when sync is already enabled, and + nothing creates it later. """ from nerve.config import get_config + from nerve.config_reload import reload_failures cfg = config.workspace_sync interval = max(1, cfg.interval_minutes) * 60 enabled = True + # The revision every subsystem in this process has taken. Not the same + # question as what is checked out: HEAD moves without this loop (`nerve + # config sync`, a bare `git pull` in the workspace) and a reload can fail for + # one subsystem, and in both cases the config on disk is not the config the + # daemon is running. Comparing against HEAD alone reports "up to date" for + # exactly those two states and never applies them again. + # + # Loop-local and seeded from HEAD, because a restart reads config from disk + # anyway: the state this would have to carry across one is exactly the state + # a restart makes moot. + applied_rev = _head_rev(config.workspace) logger.info( "Workspace sync enabled: pulling %s every %d min", config.workspace, cfg.interval_minutes, @@ -521,72 +639,163 @@ async def run_periodic_sync(config, engine, cron_service, stop_event: asyncio.Ev ) for warning in result.validation_warnings: logger.warning("Workspace sync: config warning: %s", warning) + failures: dict[str, str] = {} if not result.ok: logger.warning("Workspace sync: %s", result.message) for err in result.validation_errors: logger.warning(" config error: %s", err) - continue - if result.changed: - logger.info("Workspace sync: %s — applying", result.message) - apply_error = await _apply_sync( + elif result.changed or (result.new_rev and result.new_rev != applied_rev): + if result.changed: + logger.info("Workspace sync: %s — applying", result.message) + else: + logger.info( + "Workspace sync: %s is on disk but this daemon last " + "applied %s — applying", + result.new_rev[:8], applied_rev[:8] or "nothing", + ) + summary = await _apply_sync( engine, cron_service, config.config_dir or config.workspace, ) - if apply_error: + failures = reload_failures(summary) + if not failures: + # Advanced only when every subsystem took it. A subsystem + # that refused the new config leaves this behind, so the next + # cycle retries rather than the daemon settling on the one + # warning below and never mentioning it again. + applied_rev = result.new_rev + # Both branches sit at WARNING beside the INFO line above, which + # on its own reads as "applied". + if "config" in failures: # The merge landed but the daemon is still running the old - # config. Logged at WARNING beside the INFO line above, which - # otherwise reads as "applied". + # config. logger.warning( "Workspace sync: merged %s but the new config could not " "be loaded, so NOTHING was applied and this daemon is " "still running the previous configuration: %s", - result.new_rev[:8], apply_error, + result.new_rev[:8], failures["config"], + ) + elif failures: + # Config loaded, so the merge is partly live — which is the + # state hardest to spot from the outside and so the one that + # has to be named rather than left to the summary line. + logger.warning( + "Workspace sync: merged %s and loaded the new config, but " + "%s did not take it — this daemon is running the merged " + "configuration only in part", + result.new_rev[:8], + "; ".join(f"{k} ({v})" for k, v in sorted(failures.items())), ) + await _record_cycle(engine, result, applied_rev, failures) except Exception as e: # noqa: BLE001 — never let the loop die logger.warning("Workspace sync cycle failed: %s", e) continue -async def _apply_sync(engine, cron_service, config_dir) -> str | None: - """Hot-reload the subsystems affected by a workspace pull. +async def _record_cycle( + engine, result: SyncResult, applied_rev: str, failures: dict[str, str], +) -> None: + """Retain what this cycle found, and notify when the box enters or leaves the + blocked state. + + A refused merge is the failure with no other trace: the instance stays pinned + to an old reviewed revision, every later config change stops arriving, and + the only evidence is a WARNING repeating every cycle. On a locked box that + defeats the deployment, and the check covers the whole reviewed surface + (``config/``, ``skills/``, the root instruction files), so the agent writing + a skill directly is enough to cause it. - Re-reads the typed config and re-points the singleton, so a synced settings - change engages here without a restart for everything that reads the singleton - per use: the lockdown write guards (``is_locked``), gateway authentication, - and — because it re-reads the singleton every cycle — the sync loop itself. - ``cron_service.config`` and ``engine.config`` are re-pointed too. What is - *not*: cron jobs and gates already built, which follow on the next - ``cron_service.reload()``, and anything captured in a closure or a dataclass - at start-up, which follows on a restart. This is also the only path that - refreshes any of it — a hand edit to a config file on the box is not picked - up, by design for a locked instance and by omission otherwise. - - Returns the config-reload error, if any. A merge that lands config the daemon - cannot load has applied nothing, however well the merge itself went, and the - caller must not report success for it: the case that makes this concrete is a - pull that turns ``lockdown`` on, where reporting success tells the operator a - box is locked while its write guards are still open. + Notified on the transition rather than on the state, which is what the + retained record buys: the state notified every cycle is 1,440 messages a day + at the default cadence, all of them the same one. """ - from nerve.config import load_config, set_config + previous = _last_sync + blocked = list(result.blocked_paths) + # The local check only runs on a cycle that resolved an upstream revision to + # merge. A cycle that failed before that — no repository, a failed fetch, no + # tracking branch — establishes nothing about the local tree, so the record + # keeps what it had: clearing it would report a recovery nobody made and then + # announce the same block again on the next cycle that gets through. + checked_the_tree = bool(result.new_rev) and result.new_rev != result.old_rev + if not blocked and not result.ok and not checked_the_tree: + blocked = list(previous.blocked_paths) if previous else [] + _record_sync_state(SyncState( + applied_rev=applied_rev, + fetched_rev=result.new_rev, + blocked_paths=blocked, + reload_errors=dict(failures), + ok=result.ok, + message=result.message, + checked_at=time.time(), + )) + was_blocked = bool(previous and previous.blocked_paths) + if blocked and not was_blocked: + await _notify( + engine, "Workspace sync blocked", + f"Config sync is refusing to merge: the reviewed files in the " + f"workspace have local changes, and a fast-forward would keep them " + f"without their ever having been reviewed or validated.\n\n" + f"{_describe_paths(blocked)}\n\n" + f"Until this clears, no merged config reaches this instance — it " + f"stays on {applied_rev[:8] or 'the revision it started with'}. " + f"Commit the changes, discard them, or re-propose them as a PR with " + f"propose_config_change.", + "high", + ) + elif was_blocked and not blocked and result.ok: + await _notify( + engine, "Workspace sync unblocked", + f"The reviewed files are clean again and config sync is merging. " + f"This instance is on {applied_rev[:8] or result.new_rev[:8]}.", + "low", + ) + - config_error: str | None = None +async def _notify(engine, title: str, body: str, priority: str) -> None: + """Send an operator notification through whatever the daemon has wired up. + + Same shape as every other in-daemon sender: the service hangs off the engine, + and outside a live gateway (the CLI, tests) there is none, so the caller + carries on silently. A delivery failure is logged and swallowed — sync's + outcome does not depend on the news getting out. + """ + svc = getattr(engine, "notification_service", None) + if svc is None: + return try: - new_config = load_config(config_dir) - set_config(new_config) - if cron_service is not None: - cron_service.config = new_config - if engine is not None and hasattr(engine, "config"): - engine.config = new_config - except Exception as e: # noqa: BLE001 — a bad reload must not kill the loop - config_error = f"{type(e).__name__}: {e}" - logger.warning("config reload after sync failed: %s", e, exc_info=True) - if cron_service is not None: - try: - await cron_service.reload() - except Exception as e: # noqa: BLE001 - logger.warning("cron reload after sync failed: %s", e, exc_info=True) - if engine is not None: - try: - await engine.reload_mcp_config() - except Exception as e: # noqa: BLE001 - logger.warning("MCP reload after sync failed: %s", e, exc_info=True) - return config_error + await svc.send_notification( + session_id="system", title=title, body=body, priority=priority, + ) + except Exception: # noqa: BLE001 — see above + logger.warning("Workspace sync notification failed", exc_info=True) + + +async def _apply_sync(engine, cron_service, config_dir) -> dict: + """Hot-reload the subsystems affected by a workspace pull. + + Delegates to the unified reload so a synced pull applies the SAME set of + subsystems as a manual ``POST /api/config/reload`` — the process config + object, cron jobs, sources, MCP, and skills. + + Replacing the config object is what makes a synced settings change engage + without a restart, for everything that reads it per use: the lockdown write + guards (``is_locked``), gateway authentication, and — because it re-reads it + every cycle — the sync loop itself. The services that captured the old object + are re-pointed with it. What is *not*: cron jobs and gates already built, + which follow on the next ``cron_service.reload()``, and anything a service + derived from config when it was constructed, which follows on a restart. + + Returns the per-subsystem summary. It has to be *read*, not assumed: a merge + that lands config the daemon cannot load has applied nothing however well the + merge itself went, and a merge whose config loaded but whose cron reload + failed is in effect only in part. The case that makes this concrete is a pull + that turns ``lockdown`` on, where reporting a clean success tells the operator + the box is locked while its write guards are still open. + """ + from nerve.config_reload import reload_all, reload_failures + + summary = await reload_all(engine, cron_service, config_dir) + if reload_failures(summary): + logger.warning("Applied synced config, with failures: %s", summary) + else: + logger.info("Applied synced config: %s", summary) + return summary diff --git a/nerve/workflows/__init__.py b/nerve/workflows/__init__.py index 892a9f2e..59d4769c 100644 --- a/nerve/workflows/__init__.py +++ b/nerve/workflows/__init__.py @@ -12,7 +12,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Callable if TYPE_CHECKING: from nerve.agent.engine import AgentEngine @@ -55,14 +55,21 @@ def reset_workflow_run_service() -> None: def init_review_loop_service( - config: NerveConfig, db: Database, engine: AgentEngine, runs, + config: Callable[[], NerveConfig], db: Database, engine: AgentEngine, runs, ): """Initialise the review-loop singleton. Requires a live WorkflowRunService (legs are workflow runs). Returns None when the - feature (or workflow runs) is disabled.""" + feature (or workflow runs) is disabled. + + ``config`` is a callable, not the object: the service reads it per use so + its budgets and caps follow a reload (see + :attr:`~nerve.workflows.review_loop.ReviewLoopService.rl`). The two flags + below are read once here, which is exactly why they stay restart-only — + with the feature off there is no service for a later reload to reach.""" global _review_loop_service - if runs is None or not config.workflows.enabled \ - or not config.workflows.review_loop.enabled: + settings = config() + if runs is None or not settings.workflows.enabled \ + or not settings.workflows.review_loop.enabled: _review_loop_service = None return None from nerve.workflows.review_loop import ReviewLoopService as _RL diff --git a/nerve/workflows/review_loop.py b/nerve/workflows/review_loop.py index 84a37fec..608fcbbe 100644 --- a/nerve/workflows/review_loop.py +++ b/nerve/workflows/review_loop.py @@ -38,7 +38,7 @@ import uuid from datetime import datetime, timedelta, timezone from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Callable from nerve.agent.streaming import broadcaster from nerve.db.review_loops import ( @@ -52,7 +52,7 @@ if TYPE_CHECKING: from nerve.agent.engine import AgentEngine - from nerve.config import NerveConfig + from nerve.config import NerveConfig, ReviewLoopConfig from nerve.db import Database from nerve.workflows.service import WorkflowRunService @@ -231,16 +231,15 @@ class ReviewLoopService: def __init__( self, - config: NerveConfig, + config: Callable[[], NerveConfig], db: Database, engine: AgentEngine, runs: WorkflowRunService, ): - self.config = config + self._config = config self.db = db self.engine = engine self.runs = runs - self.rl = config.workflows.review_loop self._queue: asyncio.Queue[str] = asyncio.Queue() self._worker_task: asyncio.Task | None = None self._tick_task: asyncio.Task | None = None @@ -248,6 +247,23 @@ def __init__( self._detached: set[asyncio.Task] = set() self._stopping = False + @property + def config(self) -> NerveConfig: + """The live config, resolved per read rather than captured.""" + return self._config() + + @property + def rl(self) -> ReviewLoopConfig: + """The live ``workflows.review_loop`` section. + + Resolved on every read, not bound once: this service is a lifespan + singleton, so a captured sub-object would still be answering with the + budget ceilings, iteration caps and sandbox mode the daemon booted with + long after a reload had reported them applied — and the dollar values + are read at the moment a leg is funded. + """ + return self._config().workflows.review_loop + # ------------------------------------------------------------------ # # Lifespan # # ------------------------------------------------------------------ # diff --git a/scripts/codex_smoke.py b/scripts/codex_smoke.py index 70fedc72..a8214b5a 100755 --- a/scripts/codex_smoke.py +++ b/scripts/codex_smoke.py @@ -81,7 +81,7 @@ async def main() -> int: }, }) deps = BackendDeps( - config=cfg, db=None, registry=None, + config=lambda: cfg, db=None, registry=None, tool_ctx_factory=lambda sid: None, external_mcp_servers=lambda: [], gateway_port=lambda: None, # no MCP bridge in the smoke diff --git a/tests/test_cache_policy.py b/tests/test_cache_policy.py index ca0a02fa..d3f1701e 100644 --- a/tests/test_cache_policy.py +++ b/tests/test_cache_policy.py @@ -177,7 +177,7 @@ def _make_env_backend( effective_api_key="", agent=SimpleNamespace(model_aliases=aliases or {}), ) - return ClaudeBackend(SimpleNamespace(config=config)) + return ClaudeBackend(SimpleNamespace(config=lambda: config)) def test_build_env_5m_has_no_cache_flag(): diff --git a/tests/test_cli_codex.py b/tests/test_cli_codex.py new file mode 100644 index 00000000..a18eb875 --- /dev/null +++ b/tests/test_cli_codex.py @@ -0,0 +1,87 @@ +"""`nerve codex doctor` — the one backend the CLI builds itself. + +Everywhere else a backend is constructed by the engine, which assembles +:class:`~nerve.agent.backends.BackendDeps`. The doctor assembles them by hand, +so it is the only caller that can get the shape wrong, and nothing exercised +it — which is how it came to hand over a config object where the backend +expects a callable returning one. + +The backend here is real. Only the step that needs the codex binary on PATH is +stubbed, so construction and config resolution are what these tests run. +""" + +from __future__ import annotations + +import json + +import pytest +from click.testing import CliRunner + +from nerve.agent.backends.codex.backend import CodexBackend +from nerve.cli import main + + +def _config(tmp_path, extra=""): + """A config dir whose codex home is under tmp_path, not the real one.""" + (tmp_path / "config.yaml").write_text( + f"codex:\n home_dir: {tmp_path / 'codex-home'}\n" + extra, + encoding="utf-8", + ) + return tmp_path + + +@pytest.fixture +def preflight(monkeypatch): + """Stub preflight; the returned dict is what the command reports on.""" + status = { + "available": True, "version": "0.9.9", "auth": "chatgpt", + "models": ["gpt-5-codex"], + } + + async def fake(self, *, force=False): + return status + + monkeypatch.setattr(CodexBackend, "preflight", fake) + return status + + +class TestDoctorCommand: + def test_it_runs(self, tmp_path, preflight): + """The regression guard: constructing the backend used to raise + TypeError before the command had done anything at all.""" + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "codex", "doctor"]) + assert result.exit_code == 0, result.output + assert "Traceback" not in result.output + assert "0.9.9" in result.output + + def test_the_backend_resolves_the_config_the_cli_loaded(self, tmp_path, preflight): + """Construction creates CODEX_HOME, so the directory appearing under + tmp_path says the deps callable returned this config and not a default.""" + cfg = _config(tmp_path) + result = CliRunner().invoke(main, ["-c", str(cfg), "codex", "doctor"]) + assert result.exit_code == 0, result.output + assert (tmp_path / "codex-home").is_dir() + + def test_an_unavailable_cli_exits_non_zero(self, tmp_path, preflight): + preflight.clear() + preflight.update({"available": False, "reason": "codex not found"}) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "codex", "doctor"]) + assert result.exit_code == 1 + assert "codex not found" in result.output + + def test_an_auth_mismatch_exits_non_zero(self, tmp_path, preflight): + """Available but authenticated as something other than the configured + mode: usable by hand, wrong for the daemon, so not a pass.""" + preflight.update({"auth_mismatch": True, "configured_auth": "api_key"}) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "codex", "doctor"]) + assert result.exit_code == 1 + assert "auth mismatch" in result.output + + def test_json_output_is_parseable(self, tmp_path, preflight): + result = CliRunner().invoke( + main, ["-c", str(_config(tmp_path)), "codex", "doctor", "--json-output"] + ) + assert result.exit_code == 0, result.output + report = json.loads(result.output) + assert report["preflight"]["version"] == "0.9.9" + assert report["recoverable_runs"] == [] diff --git a/tests/test_cli_reload.py b/tests/test_cli_reload.py new file mode 100644 index 00000000..e588121e --- /dev/null +++ b/tests/test_cli_reload.py @@ -0,0 +1,252 @@ +"""`nerve reload` — the CLI front end for POST /api/config/reload.""" + +from __future__ import annotations + +import httpx +import pytest +from click.testing import CliRunner + +from nerve.cli import _gateway_url, main + + +def _config(tmp_path, extra=""): + (tmp_path / "config.yaml").write_text( + "auth:\n jwt_secret: test-secret-value-long-enough-for-hs256\n" + extra, encoding="utf-8" + ) + return tmp_path + + +def _locked_config(tmp_path, extra=""): + """A config dir the daemon would read as locked. + + The flag has to come from the workspace's tracked settings.yaml: `lockdown` + in config.yaml is ignored by design, so setting it there would produce an + unlocked instance and a test that passes for the wrong reason. + + The workspace is a git repository with a remote for the same kind of reason. + A locked instance with nowhere to receive reviewed config from refuses to + start, so without one these would exercise that refusal instead of the + reload behaviour they are about. The remote is never contacted. + """ + import shutil + import subprocess + + from nerve.config import workspace_settings_file + + if not shutil.which("git"): + pytest.skip("git not available") + config_dir, workspace = tmp_path / "cfg", tmp_path / "ws" + config_dir.mkdir(parents=True, exist_ok=True) + (workspace / "config").mkdir(parents=True, exist_ok=True) + (config_dir / "config.yaml").write_text(f"workspace: {workspace}\n", encoding="utf-8") + workspace_settings_file(workspace).write_text("lockdown: true\n" + extra, encoding="utf-8") + for args in (["init", "-q"], + ["remote", "add", "origin", "https://example.invalid/config.git"]): + subprocess.run(["git", *args], cwd=str(workspace), check=True, capture_output=True) + return config_dir + + +class _Response: + def __init__(self, status_code=200, payload=None, text=""): + self.status_code = status_code + self._payload = payload or {} + self.text = text + + def json(self): + return self._payload + + +def _post(recorder=None, **kwargs): + """A stand-in for httpx.post that records how it was called.""" + def fake_post(url, **call_kwargs): + if recorder is not None: + recorder.append({"url": url, **call_kwargs}) + return _Response(**kwargs) + return fake_post + + +class TestGatewayUrl: + """A bind address is not a destination.""" + + @pytest.mark.parametrize("host", ["0.0.0.0", "::", "*", ""]) + def test_wildcard_binds_resolve_to_loopback(self, host): + from nerve.config import GatewayConfig, NerveConfig + + cfg = NerveConfig(gateway=GatewayConfig(host=host, port=8900)) + assert _gateway_url(cfg, "/x") == "http://127.0.0.1:8900/x" + + def test_a_real_host_is_kept(self): + from nerve.config import GatewayConfig, NerveConfig + + cfg = NerveConfig(gateway=GatewayConfig(host="10.0.0.4", port=9001)) + assert _gateway_url(cfg, "/x") == "http://10.0.0.4:9001/x" + + def test_tls_switches_the_scheme(self, tmp_path): + from nerve.config import GatewayConfig, NerveConfig, SSLConfig + + ssl = SSLConfig(cert=tmp_path / "c.pem", key=tmp_path / "k.pem") + cfg = NerveConfig(gateway=GatewayConfig(host="box", port=443, ssl=ssl)) + assert _gateway_url(cfg, "/x") == "https://box:443/x" + + +class TestReloadCommand: + def test_reports_each_subsystem_and_succeeds(self, tmp_path, monkeypatch): + monkeypatch.setattr(httpx, "post", _post(payload={ + "ok": True, + "detail": {"config": "reloaded", "cron": {"added": 1, "removed": 0}, + "mcp": "2 server(s)"}, + "errors": {}, + "restart_required": "", + })) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code == 0, result.output + assert "Config reloaded" in result.output + assert "config: reloaded" in result.output + assert "added=1" in result.output # dict summaries are flattened, not repr'd + assert "mcp: 2 server(s)" in result.output + + def test_a_partial_reload_exits_non_zero(self, tmp_path, monkeypatch): + """A reload is best-effort by design, so `ok: false` still comes back 200. + Exiting 0 there would let a script read a partial apply as a clean one.""" + monkeypatch.setattr(httpx, "post", _post(payload={ + "ok": False, + "detail": {"config": "reloaded", "cron": "error: bad gate type"}, + "errors": {"cron": "bad gate type"}, + "restart_required": "", + })) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code == 1 + assert "[ERR] cron: bad gate type" in result.output + assert "incomplete" in result.output + + def test_restart_required_is_surfaced_without_failing(self, tmp_path, monkeypatch): + """Nothing failed and everything reloadable was applied, so this is a + warning — but it must not be silent, or the value looks live when it + isn't.""" + monkeypatch.setattr(httpx, "post", _post(payload={ + "ok": True, + "detail": {"config": "reloaded", "restart_required": "gateway.port"}, + "errors": {}, + "restart_required": "gateway.port", + })) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code == 0, result.output + assert "needs a restart: gateway.port" in result.output + # Not also printed as an ordinary subsystem line. + assert "restart_required: gateway.port" not in result.output + + def test_no_daemon_is_a_clean_message(self, tmp_path, monkeypatch): + def refuse(url, **_kw): + raise httpx.ConnectError("connection refused") + + monkeypatch.setattr(httpx, "post", refuse) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code != 0 + assert "No daemon answering" in result.output + assert "Traceback" not in result.output + + def test_a_changed_port_is_offered_as_the_likelier_cause( + self, tmp_path, monkeypatch, + ): + """The URL is built from the config just read, so editing gateway.port is + itself a way to get here: the daemon is running fine on the old port and + this call goes to the new one. Telling the operator to start a daemon + that is already running sends them the wrong way. + """ + def refuse(url, **_kw): + raise httpx.ConnectError("connection refused") + + monkeypatch.setattr(httpx, "post", refuse) + config = _config(tmp_path, "gateway:\n port: 9100\n") + result = CliRunner().invoke(main, ["-c", str(config), "reload"]) + assert result.exit_code != 0 + assert "9100" in result.output + assert "gateway.port" in result.output + assert "restart" in result.output + + def test_rejected_token_explains_the_likely_cause(self, tmp_path, monkeypatch): + monkeypatch.setattr(httpx, "post", _post(status_code=401, text="nope")) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code != 0 + assert "jwt_secret" in result.output + assert "Traceback" not in result.output + + def test_no_jwt_secret_still_calls_the_gateway(self, tmp_path, monkeypatch): + """An unlocked gateway with no auth.jwt_secret does not ask for a token — + require_auth runs open there. Refusing to send one would refuse the reload + the endpoint would have accepted, on the dev box where the hand edits this + command exists for are most of the edits there are.""" + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls, payload={"ok": True})) + (tmp_path / "config.yaml").write_text("timezone: UTC\n", encoding="utf-8") + result = CliRunner().invoke(main, ["-c", str(tmp_path), "reload"]) + assert result.exit_code == 0, result.output + assert len(calls) == 1 + assert "Authorization" not in calls[0]["headers"] + + def test_a_password_hash_is_not_a_second_source_of_a_token(self, tmp_path, monkeypatch): + """auth.password_hash gates the browser login, which is what mints a token + from it. require_auth reads auth.jwt_secret alone, so a password neither + makes the endpoint ask for a token nor gives this command one to sign.""" + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls, payload={"ok": True})) + (tmp_path / "config.yaml").write_text( + "auth:\n password_hash: $2b$12$notarealhashatall\n", encoding="utf-8" + ) + result = CliRunner().invoke(main, ["-c", str(tmp_path), "reload"]) + assert result.exit_code == 0, result.output + assert "Authorization" not in calls[0]["headers"] + + def test_lockdown_without_a_secret_refuses_before_calling(self, tmp_path, monkeypatch): + """The one case where an empty secret is a dead end: a locked gateway never + takes the open path, so no request from this shell can be authenticated. + Sending it anyway would report the gateway's refusal as the problem.""" + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls)) + result = CliRunner().invoke(main, ["-c", str(_locked_config(tmp_path)), "reload"]) + assert result.exit_code != 0 + assert "locked gateway" in result.output + assert not calls + + def test_lockdown_with_a_secret_authenticates_normally(self, tmp_path, monkeypatch): + """The refusal above is about the missing secret, not about lockdown.""" + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls, payload={"ok": True})) + cfg = _locked_config( + tmp_path, extra="auth:\n jwt_secret: test-secret-value-long-enough-for-hs256\n" + ) + result = CliRunner().invoke(main, ["-c", str(cfg), "reload"]) + assert result.exit_code == 0, result.output + assert calls[0]["headers"]["Authorization"].startswith("Bearer ") + + def test_tls_verification_is_only_skipped_for_the_loopback_rewrite( + self, tmp_path, monkeypatch + ): + """A wildcard bind means we ask 127.0.0.1 for a certificate issued to a + hostname, which cannot verify. A real host is a different matter — the + certificate should match, so a failure there must not be skipped past.""" + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls, payload={"ok": True})) + certs = "gateway:\n ssl:\n cert: /c.pem\n key: /k.pem\n" + + cfg = _config(tmp_path, extra=certs + " host: 0.0.0.0\n") + CliRunner().invoke(main, ["-c", str(cfg), "reload"]) + assert calls[-1]["verify"] is False + + cfg = _config(tmp_path, extra=certs + " host: nerve.example.com\n") + CliRunner().invoke(main, ["-c", str(cfg), "reload"]) + assert calls[-1]["verify"] is True + + def test_sends_a_bearer_token_to_the_reload_endpoint(self, tmp_path, monkeypatch): + calls: list = [] + monkeypatch.setattr(httpx, "post", _post(calls, payload={"ok": True})) + result = CliRunner().invoke(main, ["-c", str(_config(tmp_path)), "reload"]) + assert result.exit_code == 0, result.output + assert len(calls) == 1 + assert calls[0]["url"].endswith("/api/config/reload") + auth = calls[0]["headers"]["Authorization"] + assert auth.startswith("Bearer ") + # A real token the gateway would accept, not a placeholder. + from nerve.gateway.auth import decode_token + + assert decode_token(auth.removeprefix("Bearer "), "test-secret-value-long-enough-for-hs256") diff --git a/tests/test_codex_appserver.py b/tests/test_codex_appserver.py index 8d1093db..1cba4bc4 100644 --- a/tests/test_codex_appserver.py +++ b/tests/test_codex_appserver.py @@ -46,7 +46,7 @@ def _config(tmp_path: Path, **codex_overrides) -> NerveConfig: def _deps(cfg: NerveConfig, *, gateway_port: int = 8900) -> BackendDeps: return BackendDeps( - config=cfg, + config=lambda: cfg, db=None, registry=None, tool_ctx_factory=lambda sid: None, diff --git a/tests/test_codex_protocol.py b/tests/test_codex_protocol.py index 38d51a47..51a64fc4 100644 --- a/tests/test_codex_protocol.py +++ b/tests/test_codex_protocol.py @@ -19,7 +19,7 @@ def _client(tmp_path, **codex_overrides) -> CodexClient: "codex": {"home_dir": str(tmp_path / "home"), **codex_overrides}, }) deps = SimpleNamespace( - config=cfg, + config=lambda: cfg, external_mcp_servers=lambda: [], gateway_port=lambda: None, mint_session_token=None, diff --git a/tests/test_config_reload.py b/tests/test_config_reload.py new file mode 100644 index 00000000..a093e705 --- /dev/null +++ b/tests/test_config_reload.py @@ -0,0 +1,812 @@ +"""Tests for unified config hot-reload.""" + +from __future__ import annotations + +import dataclasses +import re +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest +import pytest_asyncio + +import nerve.sources.registry as registry +from nerve.config_reload import ( + _RESTART_ONLY_PATHS, + _UNSET, + _dotted_attr, + reload_all, + reload_failures, +) +from nerve.cron.service import CronService + + +def _fake_runner(job_id, source_name): + return SimpleNamespace( + job_id=job_id, + source=SimpleNamespace(source_name=source_name), + set_notification_service=lambda *a, **k: None, + ) + + +@pytest_asyncio.fixture +async def svc(): + config = MagicMock() + config.timezone = "UTC" + config.sync.gmail.schedule = "5m" + config.sync.github.schedule = "10m" + db = AsyncMock() + db.get_last_successful_cron_run = AsyncMock(return_value=None) + service = CronService(config, AsyncMock(), db) + service.scheduler.start(paused=True) + try: + yield service + finally: + service.scheduler.shutdown(wait=False) + + +class TestReloadSources: + @pytest.mark.asyncio + async def test_reschedules_new_removes_old(self, svc, monkeypatch): + # Initially one source scheduled. + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_fake_runner("source:gmail", "gmail")], + ) + svc._register_source_runners() + assert svc.scheduler.get_job("source:gmail") is not None + + # Config now has a different source — reload swaps them. + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_fake_runner("source:github", "github")], + ) + result = await svc.reload_sources() + assert svc.scheduler.get_job("source:github") is not None + assert svc.scheduler.get_job("source:gmail") is None + assert result["removed"] == ["source:gmail"] + assert result["sources"] == ["source:github"] + + @pytest.mark.asyncio + async def test_build_failure_keeps_old_sources(self, svc, monkeypatch): + """A build failure during reload must NOT silently unschedule sources.""" + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_fake_runner("source:gmail", "gmail")], + ) + svc._register_source_runners() + assert svc.scheduler.get_job("source:gmail") is not None + + def _boom(config, db): + raise RuntimeError("bad source config") + + monkeypatch.setattr(registry, "build_source_runners", _boom) + with pytest.raises(RuntimeError): + await svc.reload_sources() + # Old source still scheduled — built new BEFORE removing old. + assert svc.scheduler.get_job("source:gmail") is not None + + @pytest.mark.asyncio + async def test_notification_service_rewired_on_reload(self, svc, monkeypatch): + wired = [] + runner = SimpleNamespace( + job_id="source:gmail", + source=SimpleNamespace(source_name="gmail"), + set_notification_service=lambda ns: wired.append(ns), + ) + monkeypatch.setattr(registry, "build_source_runners", lambda config, db: [runner]) + svc.notification_service = object() + await svc.reload_sources() + assert wired == [svc.notification_service] + + +class TestReloadAll: + @pytest.mark.asyncio + async def test_summary_covers_all_subsystems(self, tmp_path): + cron = MagicMock() + cron.reload = AsyncMock(return_value={"added": [], "removed": [], "updated": [], "enabled": 0}) + cron.reload_sources = AsyncMock(return_value={"sources": [], "removed": []}) + engine = MagicMock() + engine.reload_mcp_config = AsyncMock(return_value=[]) + engine._skill_manager = MagicMock() + engine._skill_manager.discover = AsyncMock(return_value=[1, 2]) + + summary = await reload_all(engine, cron, tmp_path) + assert summary["config"] == "reloaded" + assert summary["cron"]["enabled"] == 0 + assert summary["sources"] == {"sources": [], "removed": []} + assert summary["mcp"] == "0 server(s)" + assert summary["skills"] == "2 discovered" + cron.reload.assert_awaited_once() + cron.reload_sources.assert_awaited_once() + engine.reload_mcp_config.assert_awaited_once() + engine._skill_manager.discover.assert_awaited_once() + + @pytest.mark.asyncio + async def test_per_subsystem_error_isolated(self, tmp_path): + cron = MagicMock() + cron.reload = AsyncMock(side_effect=RuntimeError("boom")) + cron.reload_sources = AsyncMock(return_value={"sources": [], "removed": []}) + engine = MagicMock() + engine.reload_mcp_config = AsyncMock(return_value=[]) + engine._skill_manager = MagicMock() + engine._skill_manager.discover = AsyncMock(return_value=[]) + + summary = await reload_all(engine, cron, tmp_path) + assert summary["cron"].startswith("error:") + # Other subsystems still ran despite cron failing. + assert summary["mcp"] == "0 server(s)" + assert summary["skills"] == "0 discovered" + + @pytest.mark.asyncio + async def test_no_engine_no_cron(self, tmp_path): + summary = await reload_all(None, None, tmp_path) + assert summary["config"] == "reloaded" + assert "cron" not in summary and "mcp" not in summary + + @pytest.mark.asyncio + async def test_config_load_error_reported_not_applied(self, tmp_path, monkeypatch): + import nerve.config as cfgmod + + def _raise(config_dir=None): + raise cfgmod.ConfigError("locked but no jwt_secret") + + monkeypatch.setattr(cfgmod, "load_config", _raise) + summary = await reload_all(None, None, tmp_path) + assert summary["config"].startswith("error:") + assert "jwt_secret" in summary["config"] + + @pytest.mark.asyncio + async def test_reloads_cron_jobs_and_sources_together(self, tmp_path): + """Both, on every reload: a caller has no way to ask for one and not the + other, so a jobs edit and a sources edit can never be applied apart.""" + cron = MagicMock() + cron.reload = AsyncMock(return_value={"added": [], "removed": []}) + cron.reload_sources = AsyncMock(return_value={"sources": [], "removed": []}) + summary = await reload_all(None, cron, tmp_path) + cron.reload.assert_awaited_once() + cron.reload_sources.assert_awaited_once() + assert "cron" in summary and "sources" in summary + + +class TestSourceReloadScope: + def test_sync_codex_is_not_a_cron_source(self): + """``sync.codex`` sits on the same dataclass as the cron sources but + belongs to the Codex thread-sync service, which is built once at + start-up. A reload rebuilds cron source runners and touches none of it, + so the hot-reload table must not say "``sync.*``" — an operator who added + a Codex origin would read ``ok: true`` and get no ingestion. + """ + from nerve.config import NerveConfig + from nerve.sources.registry import build_source_runners + + off = NerveConfig() + off.sync.codex.enabled = False + on = NerveConfig() + on.sync.codex.enabled = True + + before = [r.job_id for r in build_source_runners(off, db=None)] + after = [r.job_id for r in build_source_runners(on, db=None)] + assert after == before # turning it on changes nothing here + assert "source:codex" not in after + + +_DOC = Path(__file__).resolve().parents[1] / "docs" / "config.md" + +# A backticked token that could be a config path: lowercase segments, and a +# trailing ``.*`` for a whole section. Rules out "/mcp/v1" and "origins[*]". +_PATH_TOKEN = re.compile(r"[a-z_][a-z0-9_]*(\.[a-z0-9_]+)*(\.\*)?$") + + +def _restart_table_paths() -> list[str]: + """The config paths in the "What still needs a restart" table of docs/config.md. + + Read out of the document, never off the tuple under test: a check sourced + from the code it checks agrees with itself whatever the operator was + promised, which is how six unconditional rows came to be documented as + reported and reported by nothing. + + A cell mixes paths with prose, so a backticked token counts as a path only + when its first segment is a field of ``NerveConfig``. That drops the + parentheticals naming a field of some sub-object ("`enabled`", + "`store_encrypted_reasoning`") along with the prose, and the leading-dot + shorthand ("`gateway.host`, `.port`") is resolved against the section of the + token before it. + """ + from nerve.config import NerveConfig + + body = _DOC.read_text(encoding="utf-8") + section = body.split("### What still needs a restart", 1)[1].split("\n### ", 1)[0] + roots = {f.name for f in dataclasses.fields(NerveConfig)} + paths: list[str] = [] + for line in section.splitlines(): + if not line.startswith("|"): + continue + cell, root = line.split("|")[1], "" + for token in re.findall(r"`([^`]+)`", cell): + if token.startswith("."): + token = root + token + if not _PATH_TOKEN.fullmatch(token): + continue + head = token.split(".")[0] + if head not in roots: + continue + root = head + paths.append(token) + return paths + + +def _leaf_paths(obj, prefix: str) -> list[str]: + """Every settable path under *obj*, descending into nested config sections.""" + leaves: list[str] = [] + for f in dataclasses.fields(obj): + value, child = getattr(obj, f.name), f"{prefix}.{f.name}" + if dataclasses.is_dataclass(value): + leaves.extend(_leaf_paths(value, child)) + else: + leaves.append(child) + return leaves + + +def _compared(path: str) -> bool: + """True when some entry in the tuple compares *path* — itself, or its section.""" + return any( + path == entry or path.startswith(f"{entry}.") + for entry in _RESTART_ONLY_PATHS + ) + + +class TestRestartTableCoverage: + """The table promises "the check covers the unconditional entries below". + + A row with no entry in ``_RESTART_ONLY_PATHS`` is that promise broken: the + reload reports nothing, the operator reads `ok`, and the daemon keeps the + old value. A dead entry is the same failure from the other side — one that + resolves to nothing can never fire, which is what `langfuse.enabled` did for + as long as it was listed. + """ + + # Rows the check cannot decide by comparing two values, with the reason. + EXEMPT = { + "ollama.enabled": "conditional: only while the proxy is not running", + "workspace_sync.enabled": "conditional: only turning it on", + "retention.enabled": "conditional: only turning it on", + } + + def test_every_documented_setting_is_compared(self): + from nerve.config import NerveConfig + + defaults = NerveConfig() + documented = _restart_table_paths() + assert len(documented) >= 20, ( + f"the table walk found only {len(documented)} paths — did it break?" + ) + + uncovered = [] + for path in documented: + if path in self.EXEMPT: + continue + if path.endswith(".*"): + section = path[:-2] + node = _dotted_attr(defaults, section) + assert node is not _UNSET, f"{path} names nothing in the config" + candidates = _leaf_paths(node, section) + else: + candidates = [path] + uncovered.extend(p for p in candidates if not _compared(p)) + + assert not uncovered, ( + "settings the restart table says a reload reports, that " + "_RESTART_ONLY_PATHS does not compare — add the path (or the " + "section holding it), or exempt the row here with the reason:\n" + + "\n".join(sorted(set(uncovered))) + ) + + def test_every_compared_setting_is_documented(self): + undocumented = [] + for entry in _RESTART_ONLY_PATHS: + if not any( + entry == doc + or entry.startswith(f"{doc}.") + or (doc.endswith(".*") and ( + entry == doc[:-2] or entry.startswith(doc[:-1]) + )) + for doc in _restart_table_paths() + ): + undocumented.append(entry) + assert not undocumented, ( + "_RESTART_ONLY_PATHS entries with no row in the restart table of " + "docs/config.md — the warning fires and the operator has nothing to " + "read about it:\n" + "\n".join(undocumented) + ) + + def test_every_compared_setting_exists(self): + """A path that resolves to nothing is skipped in silence, forever. + + ``restart_required`` treats a missing attribute as "not comparable" — + it has to, since the two configs can be of different vintages — so a + renamed or misspelled entry does not fail anything at runtime. It just + stops reporting. + """ + from nerve.config import NerveConfig + + defaults = NerveConfig() + missing = [ + path for path in _RESTART_ONLY_PATHS + if _dotted_attr(defaults, path) is _UNSET + ] + assert not missing, ( + "_RESTART_ONLY_PATHS entries that are not settings of the config " + "(renamed? misspelled? never existed?):\n" + "\n".join(missing) + ) + + def test_the_exemptions_still_name_rows_in_the_table(self): + """An exemption for a row that is gone hides the next one like it.""" + documented = set(_restart_table_paths()) + stale = sorted(set(self.EXEMPT) - documented) + assert not stale, ( + "exemptions above that no longer match a row of the restart " + "table:\n" + "\n".join(stale) + ) + + +class TestReloadFailures: + """The summary is a report, and ``reload_failures`` is how a caller reads it + instead of assuming it.""" + + def test_only_marked_strings_count_as_failures(self): + summary = { + "config": "reloaded", + "cron": {"added": [], "enabled": 2}, # a dict is not an error + "mcp": "0 server(s)", + "skills": "error: skills dir vanished", + } + assert reload_failures(summary) == {"skills": "skills dir vanished"} + + def test_empty_for_a_clean_reload(self): + assert reload_failures({"config": "reloaded", "mcp": "1 server(s)"}) == {} + + +def _write_config(config_dir, workspace, body=""): + config_dir.mkdir(parents=True, exist_ok=True) + (config_dir / "config.yaml").write_text( + f"workspace: {workspace}\n{body}", encoding="utf-8", + ) + + +class TestConfigObjectIsReplaced: + """Drives the real ``nerve.config`` singleton rather than a stand-in. + + The point of these is that ``get_config()`` only loads when its global is + ``None``, so for most of this daemon's life the loaded config is immortal. + A test that patches ``get_config`` would pass whether or not a reload + actually replaces it, which is exactly how the staleness went unnoticed. + """ + + @pytest.mark.asyncio + async def test_reload_picks_up_an_edited_file(self, tmp_path, monkeypatch): + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "timezone: UTC\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + started_with = cfgmod.get_config() + assert started_with.timezone == "UTC" + + _write_config(config_dir, ws, "timezone: Europe/Berlin\n") + # Reading it again without a reload still gives the start-up object. + assert cfgmod.get_config() is started_with + + summary = await reload_all(None, None, config_dir) + assert summary["config"] == "reloaded" + assert cfgmod.get_config() is not started_with + assert cfgmod.get_config().timezone == "Europe/Berlin" + + @pytest.mark.asyncio + async def test_a_changed_port_is_reported_as_needing_a_restart( + self, tmp_path, monkeypatch, + ): + """The socket is already bound, so the reload reports rather than applies. + + Previously the summary listed only what was applied, so a port change was + indistinguishable from no change. That was hard to hit while host/port + were machine-local and needed a local edit; they are in the tracked + settings now, so the change can arrive by workspace sync. + """ + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "gateway:\n port: 8900\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + + _write_config(config_dir, ws, "gateway:\n port: 9100\n") + summary = await reload_all(None, None, config_dir) + + assert summary["config"] == "reloaded" + assert "gateway.port" in summary["restart_required"] + assert "8900" in summary["restart_required"] + assert "9100" in summary["restart_required"] + # Nothing failed: the reload did everything it can do. + assert reload_failures(summary) == {} + + @pytest.mark.asyncio + async def test_a_hot_reloadable_change_claims_no_restart( + self, tmp_path, monkeypatch, + ): + """A hot-reloadable change must leave the field absent, so that its + presence carries information rather than firing on every reload.""" + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "agent:\n max_turns: 50\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + + _write_config(config_dir, ws, "agent:\n max_turns: 80\n") + summary = await reload_all(None, None, config_dir) + + assert summary["config"] == "reloaded" + assert "restart_required" not in summary + + @pytest.mark.asyncio + async def test_a_failed_load_leaves_the_previous_config_in_place( + self, tmp_path, monkeypatch, + ): + """Continuing on stale config is the deliberate choice; silently serving + a half-built one would not be.""" + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "timezone: UTC\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + started_with = cfgmod.get_config() + + (config_dir / "config.yaml").write_text("timezone: [oops\n", encoding="utf-8") + summary = await reload_all(None, None, config_dir) + assert "config" in reload_failures(summary) + assert cfgmod.get_config() is started_with + + @pytest.mark.asyncio + async def test_the_agent_backends_follow_the_engine(self, tmp_path, monkeypatch): + """The seam the whole exercise exists to close. + + The engine and its backends both answer questions about ``agent.*`` and + ``codex.*``. When the backends cached the start-up object, a reload moved + the engine and left them behind — so the context bar computed a 200k + budget off the engine while the Claude backend kept sending the 1M header + on every new session, and the reload reported ``ok`` with no errors. The + codex backend cached one level deeper still (``config.codex``), so + re-pointing ``.config`` would not have been enough either. + """ + import nerve.config as cfgmod + from nerve.agent.engine import AgentEngine + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, ( + "agent:\n context_1m: true\ncodex:\n sandbox: danger-full-access\n" + )) + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + engine = AgentEngine(cfgmod.get_config(), db=None) + claude, codex = engine._backends["claude"], engine._backends["codex"] + assert claude.config.agent.context_1m_enabled_for(None) is True + assert codex.codex.sandbox == "danger-full-access" + + _write_config(config_dir, ws, ( + "agent:\n context_1m: false\ncodex:\n sandbox: read-only\n" + )) + await reload_all(engine, None, config_dir) + + new = cfgmod.get_config() + assert engine.config is new + assert claude.config is new and codex.config is new + # The engine's answer and the backend's answer are the same answer. + assert engine.config.agent.context_1m_enabled_for(None) is False + assert claude.config.agent.context_1m_enabled_for(None) is False + assert codex.codex.sandbox == "read-only" + + @pytest.mark.asyncio + async def test_backend_choice_is_not_split_across_the_engine( + self, tmp_path, monkeypatch, + ): + """Backend resolution reads ``agent.backend`` off the live engine config, + but the session manager holds its own default for the rows it creates. + Both have to move, or new sessions route by the old default while every + other read reports the new one. + """ + import nerve.config as cfgmod + from nerve.agent.engine import AgentEngine + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, ( + "agent:\n backend: claude\n model: model-old\n" + "sessions:\n sticky_period_minutes: 120\n" + )) + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + engine = AgentEngine(cfgmod.get_config(), db=None) + assert engine.sessions.default_backend == "claude" + cwd_at_startup = engine.sessions.default_cwd + + _write_config(config_dir, ws, ( + "agent:\n backend: codex\n model: model-new\n" + "sessions:\n sticky_period_minutes: 30\n" + )) + await reload_all(engine, None, config_dir) + + assert engine.config.agent.backend == "codex" + assert engine.sessions.default_backend == "codex" + assert engine.sessions.cron_backend == "codex" + assert engine.sessions.backend_models["claude"] == "model-new" + assert engine.sessions.sticky_period_minutes == 30 + # workspace stays put: the skill manager, tool context and memory bridges + # all captured it, so following it here alone would split the daemon. + assert engine.sessions.default_cwd == cwd_at_startup + + @pytest.mark.asyncio + async def test_services_holding_the_old_object_are_re_pointed( + self, tmp_path, monkeypatch, + ): + """A daemon half on the new config and half on the old is worse than one + uniformly stale: nothing tells you which half you are looking at. So the + services that kept a reference get handed the new object too. + """ + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws) + old = cfgmod.load_config(config_dir) + monkeypatch.setattr(cfgmod, "_config", old) + + notifications = SimpleNamespace(config=old) + engine = SimpleNamespace(config=old, notification_service=notifications) + cron = SimpleNamespace(config=old) + + await reload_all(engine, cron, config_dir) + + new = cfgmod.get_config() + assert new is not old + assert engine.config is new + assert cron.config is new + assert notifications.config is new + + @pytest.mark.asyncio + async def test_external_agents_sweeper_is_re_pointed(self, tmp_path, monkeypatch): + """The add/remove/toggle routes edit the process config *in place*, so a + sweeper left on the previous object would keep rendering the old target + list while every toggle reported success. + """ + import nerve.config as cfgmod + from nerve.external_agents.sync_service import SyncService + from nerve.gateway.routes import _deps + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws) + old = cfgmod.load_config(config_dir) + monkeypatch.setattr(cfgmod, "_config", old) + + sweeper = SyncService(old) + monkeypatch.setattr( + _deps, "_deps", + _deps.RouteDeps(engine=None, db=None, external_agents_sync=sweeper), + ) + + await reload_all(None, None, config_dir) + assert sweeper._config is cfgmod.get_config() + assert sweeper._config is not old + + @pytest.mark.asyncio + async def test_workflow_run_service_is_re_pointed(self, tmp_path, monkeypatch): + """Budget enforcement is the point of this service, so it must not be + the half of the process still reading the old ceiling. It reads through + its own reference at use, so re-pointing is all it takes. + """ + import nerve.config as cfgmod + import nerve.config_reload as reload_mod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws) + old = cfgmod.load_config(config_dir) + monkeypatch.setattr(cfgmod, "_config", old) + + service = SimpleNamespace(config=old) + monkeypatch.setattr(reload_mod, "_workflow_run_service", lambda: service) + + await reload_all(None, None, config_dir) + assert service.config is cfgmod.get_config() + assert service.config is not old + + @pytest.mark.asyncio + async def test_no_workflow_run_service_is_not_a_failure(self, tmp_path, monkeypatch): + """The CLI and the tests have no gateway, so the lookup returning None + is the normal case, not something to report.""" + import nerve.config as cfgmod + import nerve.config_reload as reload_mod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws) + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + monkeypatch.setattr(reload_mod, "_workflow_run_service", lambda: None) + + summary = await reload_all(None, None, config_dir) + assert "workflow run service" not in summary.get("services", "") + + @pytest.mark.asyncio + async def test_a_holder_that_refuses_the_new_config_is_reported( + self, tmp_path, monkeypatch, + ): + """Loading the config and failing to hand it on is its own outcome — the + one where the daemon really is running two configurations at once.""" + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws) + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + + class Stubborn: + @property + def config(self): + return None # no setter: assignment raises + + summary = await reload_all(None, Stubborn(), config_dir) + assert summary["config"] == "reloaded" # the load itself was fine + assert "services" in reload_failures(summary) + assert "cron service" in summary["services"] + + +class TestTighteningActuallyTightens: + """Reporting a change is not applying it. + + Everything in ``_RESTART_ONLY_PATHS`` is a setting the daemon keeps running + without — acceptable for a bound socket, and not for a policy the operator + just tightened, where the reload answers `ok` and the old, looser value + keeps deciding. The two below hold their config through a callable for that + reason, so they are not in the tuple: there is nothing left to warn about. + """ + + @pytest.mark.asyncio + async def test_telegram_dm_policy_follows_a_reload(self, tmp_path, monkeypatch): + """`open` authorizes every Telegram user there is. Closing it used to + take a restart, with the reload reporting success in the meantime.""" + import nerve.config as cfgmod + from nerve.channels.telegram import TelegramChannel + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "telegram:\n dm_policy: open\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + channel = TelegramChannel(cfgmod.get_config, router=MagicMock()) + assert channel._is_authorized(99999) is True + + _write_config(config_dir, ws, "telegram:\n dm_policy: pairing\n") + summary = await reload_all(None, None, config_dir) + + assert channel._is_authorized(99999) is False + # Applied, so it is not something to warn about either. + assert "dm_policy" not in summary.get("restart_required", "") + + @pytest.mark.asyncio + async def test_the_telegram_allow_list_still_needs_a_restart( + self, tmp_path, monkeypatch, + ): + """The other half of the same channel, and the restart table says so: + the bot copied ``allowed_users`` into a set when it was built, so the + live config reaches ``dm_policy`` and stops there.""" + import nerve.config as cfgmod + from nerve.channels.telegram import TelegramChannel + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "telegram:\n allowed_users: [1]\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + channel = TelegramChannel(cfgmod.get_config, router=MagicMock()) + + _write_config(config_dir, ws, "telegram:\n allowed_users: [1, 2]\n") + summary = await reload_all(None, None, config_dir) + + assert channel._is_authorized(2) is False + assert "telegram.allowed_users" in summary["restart_required"] + + @pytest.mark.asyncio + async def test_review_loop_budgets_follow_a_reload(self, tmp_path, monkeypatch): + """The service is a lifespan singleton that captured + ``config.workflows.review_loop`` — a sub-object, so re-pointing its + ``config`` would not have been enough. It reads the ceiling at the + moment a leg is funded, which is where a lowered budget has to arrive. + """ + import nerve.config as cfgmod + from nerve.workflows.review_loop import ReviewLoopService + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, ( + "workflows:\n review_loop:\n default_budget_usd: 10.0\n" + " max_iterations: 5\n" + )) + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + service = ReviewLoopService(cfgmod.get_config, db=None, engine=None, runs=None) + assert service.rl.default_budget_usd == 10.0 + + _write_config(config_dir, ws, ( + "workflows:\n review_loop:\n default_budget_usd: 2.5\n" + " max_iterations: 2\n" + )) + await reload_all(None, None, config_dir) + + assert service.rl.default_budget_usd == 2.5 + assert service.rl.max_iterations == 2 + assert service.config is cfgmod.get_config() + + @pytest.mark.asyncio + async def test_a_rotated_secret_is_reported_without_its_value( + self, tmp_path, monkeypatch, + ): + """The summary goes into the log and back over HTTP. Reporting that the + secret needs a restart must not be a second copy of the secret.""" + import nerve.config as cfgmod + + config_dir, ws = tmp_path / "cfg", tmp_path / "ws" + ws.mkdir() + _write_config(config_dir, ws, "auth:\n jwt_secret: old-secret\n") + monkeypatch.setattr(cfgmod, "_config", cfgmod.load_config(config_dir)) + + _write_config(config_dir, ws, "auth:\n jwt_secret: new-secret\n") + summary = await reload_all(None, None, config_dir) + + assert "auth.jwt_secret" in summary["restart_required"] + assert "old-secret" not in summary["restart_required"] + assert "new-secret" not in summary["restart_required"] + + +class TestReloadRoute: + @pytest.mark.asyncio + async def test_route_returns_summary(self, tmp_path, monkeypatch): + import nerve.gateway.server as srv + import nerve.gateway.routes.config as route_mod + + fake_cfg = SimpleNamespace(config_dir=str(tmp_path), workspace=str(tmp_path)) + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr(route_mod, "get_deps", lambda: SimpleNamespace(engine=None)) + monkeypatch.setattr( + "nerve.config_reload.reload_all", + AsyncMock(return_value={"config": "reloaded"}), + ) + result = await route_mod.reload_config_route(user={}) + assert result["ok"] is True + assert result["detail"] == {"config": "reloaded"} + assert result["errors"] == {} + + @pytest.mark.asyncio + async def test_route_does_not_claim_a_reload_that_failed(self, tmp_path, monkeypatch): + """The route used to answer ``reloaded: True`` whatever came back, so a + settings.yaml the daemon could not load was a 200 with the reason buried + in a free-text field. + """ + import nerve.gateway.server as srv + import nerve.gateway.routes.config as route_mod + + fake_cfg = SimpleNamespace(config_dir=str(tmp_path), workspace=str(tmp_path)) + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr(route_mod, "get_deps", lambda: SimpleNamespace(engine=None)) + monkeypatch.setattr( + "nerve.config_reload.reload_all", + AsyncMock(return_value={ + "config": "error: bad yaml", "cron": {"enabled": 3}, + }), + ) + result = await route_mod.reload_config_route(user={}) + assert result["ok"] is False + assert result["errors"] == {"config": "bad yaml"} + # The rest still ran — best-effort is the point, and the detail shows it. + assert result["detail"]["cron"] == {"enabled": 3} diff --git a/tests/test_cron_reload.py b/tests/test_cron_reload.py index 06b10ea4..98429263 100644 --- a/tests/test_cron_reload.py +++ b/tests/test_cron_reload.py @@ -1,9 +1,10 @@ -"""Tests for cron hot-reload and the reserved job-id namespace (Story 5).""" +"""Tests for cron hot-reload and the reserved job-id namespace.""" from __future__ import annotations import logging from pathlib import Path +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest @@ -84,6 +85,14 @@ def _gated_job(job_id: str, **kw) -> dict: return _job_dict(job_id, run_if=[{"type": "reload_test"}], **kw) +def _source_runner(name: str): + return SimpleNamespace( + job_id=f"source:{name}", + source=SimpleNamespace(source_name=name), + set_notification_service=lambda *a, **k: None, + ) + + class TestReload: @pytest.mark.asyncio async def test_add_job(self, svc): @@ -158,6 +167,7 @@ async def test_identical_file_still_reschedules(self, svc): result = await service.reload() # identical file assert result == { "added": [], "removed": [], "updated": ["j1"], "enabled": 1, + "rejected": [], } assert service.scheduler.get_job("j1") is not None @@ -168,7 +178,9 @@ async def test_new_arrives_disabled_is_noop(self, svc): service, jobs_file = svc _write_jobs(jobs_file, [_job_dict("j1", enabled=False)]) result = await service.reload() - assert result == {"added": [], "removed": [], "updated": [], "enabled": 0} + assert result == { + "added": [], "removed": [], "updated": [], "enabled": 0, "rejected": [], + } assert service.scheduler.get_job("j1") is None @pytest.mark.asyncio @@ -275,6 +287,69 @@ async def test_reserved_dropped_on_the_merged_path(self, svc): # The daemon's own wakeup sweep is never displaced by a same-named job. assert service.scheduler.get_job("wakeup_sweep") is None + @pytest.mark.asyncio + async def test_reload_names_the_jobs_it_refused(self, svc): + """A refused job is missing from the schedule *and* from added/removed/ + updated, so without this it disappears from the reload entirely and the + only trace is a log line on a box nobody is tailing. + """ + service, jobs_file = svc + _write_jobs(jobs_file, [ + _job_dict("source:gmail", schedule="1h"), + _job_dict("cleanup", schedule="1h"), + _job_dict("legit", schedule="1h"), + ]) + result = await service.reload() + assert result["rejected"] == ["source:gmail", "cleanup"] + assert result["enabled"] == 1 # only 'legit' — not 3 + + # Renaming the job clears the refusal on the next reload. + _write_jobs(jobs_file, [ + _job_dict("my-gmail", schedule="1h"), + _job_dict("legit", schedule="1h"), + ]) + result = await service.reload() + assert result["rejected"] == [] + assert result["added"] == ["my-gmail"] and result["enabled"] == 2 + + @pytest.mark.asyncio + async def test_a_source_reload_cannot_take_a_colliding_job_off_the_schedule( + self, svc, monkeypatch, + ): + """Source runners schedule with ``replace_existing=True``, so before the + whole ``source:`` namespace was reserved, turning a source on removed a + user job's trigger while every later reload kept counting it as enabled — + unrecoverable short of a restart. Both halves are checked here: the job + never gets a trigger to lose, and the reload says so. + """ + import nerve.sources.registry as registry + + service, jobs_file = svc + _write_jobs(jobs_file, [ + _job_dict("source:gmail", schedule="1h"), + _job_dict("legit", schedule="1h"), + ]) + result = await service.reload() + assert result["rejected"] == ["source:gmail"] + assert result["enabled"] == 1 + + runner = MagicMock() + runner.job_id = "source:gmail" + runner.source.source_name = "gmail" + service.config.sync.gmail.schedule = "5m" + monkeypatch.setattr( + registry, "build_source_runners", lambda config, db: [runner], + ) + await service.reload_sources() + + # The trigger under that id belongs to the source runner, and the reload + # after it still reports one enabled job — the one that is really live. + assert service.scheduler.get_job("source:gmail") is not None + result = await service.reload() + assert result["enabled"] == 1 + assert result["rejected"] == ["source:gmail"] + assert service.scheduler.get_job("legit") is not None + @pytest.mark.asyncio async def test_reserved_rejection_is_logged(self, svc, caplog): service, jobs_file = svc @@ -578,9 +653,134 @@ async def test_a_committed_reload_still_announces_it( assert "reload_test" in caplog.text +@pytest_asyncio.fixture +async def sources(svc, monkeypatch): + """The service with a gmail (crontab) and a github (interval) source live. + + ``sync`` is a plain namespace rather than the fixture's MagicMock, because a + source whose config section is absent has to read as absent. + """ + import nerve.sources.registry as registry + + service, _jobs_file = svc + service.config.sync = SimpleNamespace( + gmail=SimpleNamespace(schedule="*/15 * * * *"), + github=SimpleNamespace(schedule="30m"), + ) + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_source_runner("gmail"), _source_runner("github")], + ) + service._register_source_runners() + return service, registry + + +class TestSourceReloadIsAllOrNothing: + """A source reload the daemon cannot carry out must change nothing. + + Every old source job was removed before a single new schedule had been + parsed, and the scheduling pass logged and skipped whatever it could not + build. One typo therefore destroyed that source's working trigger, moved + the others onto whatever their strings happened to parse to, and returned a + response naming sources that were never scheduled — which the reload route + rendered as ``ok: true``. + """ + + @pytest.mark.asyncio + async def test_a_bad_crontab_refuses_the_whole_reload(self, sources): + service, _registry = sources + before = { + jid: str(service.scheduler.get_job(jid).trigger) + for jid in ("source:gmail", "source:github") + } + service.config.sync.gmail.schedule = "99 * * * *" + + from nerve.config import ConfigError + + with pytest.raises(ConfigError) as ei: + await service.reload_sources() + + assert "gmail" in str(ei.value) and "99 * * * *" in str(ei.value) + assert { + jid: str(service.scheduler.get_job(jid).trigger) for jid in before + } == before + + @pytest.mark.asyncio + async def test_an_unusable_interval_is_refused_not_defaulted(self, sources): + """``_parse_interval`` answers 'hourly' with its 2h default, so this + reload used to move the source from 30 minutes to 2 hours and report + success. The value nobody wrote down is not an outcome to return ok for. + """ + service, _registry = sources + before = str(service.scheduler.get_job("source:github").trigger) + service.config.sync.github.schedule = "hourly" + + from nerve.config import ConfigError + + with pytest.raises(ConfigError) as ei: + await service.reload_sources() + + assert "github" in str(ei.value) and "hourly" in str(ei.value) + assert str(service.scheduler.get_job("source:github").trigger) == before + + @pytest.mark.asyncio + async def test_the_response_names_only_what_is_scheduled( + self, sources, monkeypatch, + ): + """A runner whose source has no config section is not scheduled. The + response was built from the runners that were built, so it named that + one under ``sources`` and left it out of ``removed``.""" + service, registry = sources + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_source_runner("gmail"), _source_runner("ghost")], + ) + + result = await service.reload_sources() + + assert service.scheduler.get_job("source:ghost") is None + assert result["sources"] == ["source:gmail"] + assert result["removed"] == ["source:github"] + + @pytest.mark.asyncio + async def test_a_reload_it_can_carry_out_still_applies(self, sources): + service, _registry = sources + service.config.sync.github.schedule = "45m" + + result = await service.reload_sources() + + assert result["sources"] == ["source:github", "source:gmail"] + assert result["removed"] == [] + assert "0:45:00" in str(service.scheduler.get_job("source:github").trigger) + + class TestInvalidScheduleAtStartup: """Startup answers the same error differently from reload(), on purpose.""" + @pytest.mark.asyncio + async def test_start_falls_back_rather_than_dropping_the_source( + self, svc, monkeypatch, + ): + """Boot keeps the lenient reading of an interval string it cannot use: + a source on a conservative 2h cadence beats one that never runs. Only + the reload path is strict, where an operator is waiting on an answer and + the old cadence is still running until they get one. + """ + import nerve.sources.registry as registry + + service, _jobs_file = svc + service.config.sync = SimpleNamespace(github=SimpleNamespace(schedule="hourly")) + monkeypatch.setattr( + registry, "build_source_runners", + lambda config, db: [_source_runner("github")], + ) + + service._register_source_runners() + + job = service.scheduler.get_job("source:github") + assert job is not None + assert "2:00:00" in str(job.trigger) + @pytest.mark.asyncio async def test_start_skips_only_the_offending_job( self, tmp_path, monkeypatch, caplog, diff --git a/tests/test_engine.py b/tests/test_engine.py index b1a82588..2ceb06a7 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -116,7 +116,7 @@ def test_claude_system_prompt_excludes_codex_runbook_policy(tmp_path): """Codex-only runbook semantics must never alter Claude's prompt.""" cfg = NerveConfig.from_dict({"workspace": str(tmp_path)}) backend = ClaudeBackend(SimpleNamespace( - config=cfg, + config=lambda: cfg, claude_plugins=lambda: [], )) marker = "exact claude system prompt" @@ -264,7 +264,7 @@ async def test_receive_turn_handles_empty_stream(): def _make_backend() -> ClaudeBackend: """Minimal ClaudeBackend (validate_resume_target reads no config).""" - return ClaudeBackend(SimpleNamespace(config=SimpleNamespace())) + return ClaudeBackend(SimpleNamespace(config=lambda: SimpleNamespace())) class TestValidateResumeTarget: @@ -407,7 +407,7 @@ def _make_hook_backend(background_agent_permissions: bool) -> ClaudeBackend: background_agent_permissions=background_agent_permissions, ), ) - return ClaudeBackend(SimpleNamespace(config=config)) + return ClaudeBackend(SimpleNamespace(config=lambda: config)) def _hook_spec(session_id: str) -> SessionSpec: diff --git a/tests/test_external_agents_sync_service.py b/tests/test_external_agents_sync_service.py index 40371455..aa203b63 100644 --- a/tests/test_external_agents_sync_service.py +++ b/tests/test_external_agents_sync_service.py @@ -262,3 +262,70 @@ async def test_stop_without_start_is_harmless( ) -> None: """Lifespan shutdown runs even when startup failed before start().""" await SyncService(codex_config).stop() + + +@pytest.mark.asyncio +async def test_update_config_swaps_targets_and_conflict_policy( + fake_home: Path, codex_config: NerveConfig, workspace: Path, +) -> None: + """A config reload replaces the process config object, and the routes that + add / remove / toggle a target edit that object in place — so a sweeper still + holding the previous one would keep rendering the old target list while every + toggle reported success. + """ + with _patch_writer_allowlist(fake_home): + svc = SyncService(codex_config) + assert svc._writer.policy == "backup" + + reloaded = NerveConfig() + reloaded.workspace = workspace + reloaded.external_agents = ExternalAgentsConfig( + enabled=True, + sync_interval_minutes=5, + conflict_policy="skip", + targets=[ + ExternalAgentTargetConfig(name="codex", enabled=False, token="t"), + ], + ) + svc.update_config(reloaded) + + assert svc._config is reloaded + assert svc._writer.policy == "skip" + result = await svc.run_once() + assert result["codex"].enabled is False + + +@pytest.mark.asyncio +async def test_loop_reads_the_interval_every_cycle( + fake_home: Path, codex_config: NerveConfig, +) -> None: + """The interval used to be computed once before the loop, so a reload that + changed it could never take effect.""" + import asyncio + + with _patch_writer_allowlist(fake_home): + svc = SyncService(codex_config) + + timeouts: list[float] = [] + real_wait_for = asyncio.wait_for + + async def fake_wait_for(coro, timeout=None): + timeouts.append(timeout) + coro.close() + if len(timeouts) == 1: + # Between cycles: a reload halves the configured interval. + svc._config.external_agents.sync_interval_minutes = 30 + raise asyncio.TimeoutError + svc._stop_event.set() + return True + + with patch.object(asyncio, "wait_for", fake_wait_for), \ + patch.object(svc, "run_once", new=_noop_sweep): + await svc._loop() + + assert real_wait_for is asyncio.wait_for # patch cleanly undone + assert timeouts == [3600, 1800] + + +async def _noop_sweep(): + return {} diff --git a/tests/test_review_loops.py b/tests/test_review_loops.py index 43b0f4a2..ed51d976 100644 --- a/tests/test_review_loops.py +++ b/tests/test_review_loops.py @@ -178,7 +178,7 @@ async def services(db, engine, tmp_path): """(runs service, review loop service) — started, torn down in order.""" cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) runs.add_completion_listener(rls._on_run_terminal) rls._worker_task = asyncio.create_task(rls._worker()) yield runs, rls @@ -571,7 +571,7 @@ async def test_default_codex_verifier_falls_back_to_claude(self, db, engine, tmp cfg = _make_config(tmp_path) cfg.workflows.review_loop.verifier.engine = "codex-ultracode" runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) engine._backends = {} # codex not configured loop = await rls.create_loop( goal="g", verifier="- a", session_id=None, autostart=False, @@ -695,7 +695,7 @@ async def test_state_handoff_changes_do_not_mask_no_progress( cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) loop = await rls.create_loop( goal="g", verifier="- a", session_id=None, cwd=str(repo), autostart=False, @@ -759,7 +759,7 @@ async def test_done_but_unprocessed_leg_advances(self, db, engine, tmp_path): advanced. Recovery must consume the result and dispatch the verifier.""" cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) obs = await _mk_observer(db) loop = await rls.create_loop( goal="g", verifier="- a", session_id=obs, autostart=False, @@ -797,7 +797,7 @@ async def test_crash_before_dispatch_reissues_same_id(self, db, engine, tmp_path re-issues with the SAME pre-generated id.""" cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) obs = await _mk_observer(db) loop = await rls.create_loop( goal="g", verifier="- a", session_id=obs, autostart=False, @@ -824,7 +824,7 @@ async def test_interrupted_implementer_parks_and_preserves_spend( ): cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) obs = await _mk_observer(db) loop = await rls.create_loop( goal="g", verifier="- a", session_id=obs, autostart=False, @@ -862,7 +862,7 @@ async def test_recovery_routes_settled_but_unrouted_leg(self, db, engine, tmp_pa the routing (dispatch the verifier), not skip the settled attempt.""" cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) obs = await _mk_observer(db, "obs00r01") loop = await rls.create_loop( goal="g", verifier="- a", session_id=obs, autostart=False, @@ -895,7 +895,7 @@ async def test_recovery_routes_settled_but_unrouted_leg(self, db, engine, tmp_pa async def test_recovery_recomputes_spend(self, db, engine, tmp_path): cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) loop = await rls.create_loop( goal="g", verifier="- a", session_id=None, autostart=False, ) @@ -986,7 +986,7 @@ class TestCreateLoopCwd: async def test_missing_cwd_is_created(self, db, engine, tmp_path): cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) target = tmp_path / "fresh" / "nested" assert not target.exists() loop = await rls.create_loop( @@ -1003,7 +1003,7 @@ async def test_cwd_conflicting_with_file_is_rejected( occupied.write_text("not a directory") cfg = _make_config(tmp_path) runs = WorkflowRunService(cfg, db, engine) - rls = ReviewLoopService(cfg, db, engine, runs) + rls = ReviewLoopService(lambda: cfg, db, engine, runs) with pytest.raises(ReviewLoopError, match="invalid cwd"): await rls.create_loop( goal="g", verifier="- a", session_id=None, diff --git a/tests/test_send_file.py b/tests/test_send_file.py index a3495e05..688d47a0 100644 --- a/tests/test_send_file.py +++ b/tests/test_send_file.py @@ -280,7 +280,7 @@ def _make_telegram_channel() -> TelegramChannel: cfg = NerveConfig() cfg.telegram.bot_token = "TEST:TOKEN" cfg.telegram.allowed_users = [1] - ch = TelegramChannel(cfg, router=MagicMock()) + ch = TelegramChannel(lambda: cfg, router=MagicMock()) # Bypass real PTB Application — only need send_document to exist. mock_app = MagicMock() mock_app.bot = MagicMock() diff --git a/tests/test_server_periodic_loops.py b/tests/test_server_periodic_loops.py new file mode 100644 index 00000000..b610f3c3 --- /dev/null +++ b/tests/test_server_periodic_loops.py @@ -0,0 +1,163 @@ +"""The daemon's opt-in background loops must follow a config reload. + +A config reload replaces the process-wide config object. Every one of these +loops used to close over the object built at start-up, so an operator could +change an interval or switch a feature on, watch the reload report success, and +get nothing — with no way to tell from the outside. Each test drives the real +loop with a fake clock and swaps the real config object underneath it. + +The reverse direction matters too and is asserted here: a loop that was never +created because its feature was off cannot notice the flag later, so switching +one *on* is restart-only. That is what ``docs/config.md`` promises. +""" + +from __future__ import annotations + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest + +import nerve.config as cfgmod +from nerve.config import BackupConfig, NerveConfig, RetentionConfig +from nerve.gateway import server as srv + + +class _StopLoop(Exception): + """Breaks out of a ``while True`` loop after the planned number of ticks.""" + + +def _fake_clock(monkeypatch, ticks: int, between=None): + """Replace ``asyncio.sleep`` with a counter that ends the loop after *ticks*. + + ``between(n)`` runs *during* the wait before the n'th pass, which is where a + test stands in for the operator editing config and asking for a reload — so + a change made at ``n`` is first visible to the n'th pass, not the one before + it. Returns the list the requested delays are recorded into. + """ + delays: list[float] = [] + + async def fake_sleep(seconds, *a, **k): + delays.append(seconds) + if between is not None: + between(len(delays)) + if len(delays) > ticks: + raise _StopLoop + return None + + monkeypatch.setattr(asyncio, "sleep", fake_sleep) + return delays + + +def _config(**kw) -> NerveConfig: + cfg = NerveConfig() + for key, value in kw.items(): + setattr(cfg, key, value) + return cfg + + +class TestDbRetentionLoop: + @pytest.mark.asyncio + async def test_interval_and_windows_follow_a_reload(self, tmp_path, monkeypatch): + monkeypatch.setattr(cfgmod, "_config", _config( + retention=RetentionConfig( + enabled=True, interval_hours=6, retention_days=90, + retention_full_days=30, + ), + )) + db = SimpleNamespace(run_retention=AsyncMock(return_value={})) + + def reload_during_the_first_wait(tick): + if tick == 1: + cfgmod.set_config(_config( + retention=RetentionConfig( + enabled=True, interval_hours=1, retention_days=7, + retention_full_days=3, + ), + )) + + delays = _fake_clock(monkeypatch, 2, between=reload_during_the_first_wait) + with pytest.raises(_StopLoop): + await srv._periodic_db_retention(db) + + assert delays[:2] == [6 * 3600, 1 * 3600] + assert db.run_retention.await_args_list[-1].kwargs == { + "retention_days": 7, "retention_full_days": 3, + } + + @pytest.mark.asyncio + async def test_switching_it_off_stops_the_work_but_not_the_loop( + self, monkeypatch, + ): + monkeypatch.setattr(cfgmod, "_config", _config( + retention=RetentionConfig(enabled=True, interval_hours=6), + )) + db = SimpleNamespace(run_retention=AsyncMock(return_value={})) + + def disable_before_the_second_pass(tick): + if tick == 2: + cfgmod.set_config(_config( + retention=RetentionConfig(enabled=False, interval_hours=6), + )) + + _fake_clock(monkeypatch, 3, between=disable_before_the_second_pass) + with pytest.raises(_StopLoop): + await srv._periodic_db_retention(db) + + # One pass before the flag went false, none after — and the loop kept + # ticking, so turning it back on would resume. + assert db.run_retention.await_count == 1 + + @pytest.mark.asyncio + async def test_switching_it_on_needs_a_restart(self, monkeypatch): + """No task, nothing to notice the flag. Stated as such in the docs.""" + monkeypatch.setattr(cfgmod, "_config", _config( + retention=RetentionConfig(enabled=False), + )) + delays = _fake_clock(monkeypatch, 3) + db = SimpleNamespace(run_retention=AsyncMock(return_value={})) + await srv._periodic_db_retention(db) # returns immediately + assert delays == [] + + +class TestBackupLoop: + @pytest.mark.asyncio + async def test_enabling_backups_takes_effect_without_a_restart( + self, tmp_path, monkeypatch, + ): + """The hourly tick runs whether or not backups are on, so unlike the + retention loop this one can pick up an off→on flip.""" + from nerve import backup as backup_mod + + target = tmp_path / "bundles" + target.mkdir() + monkeypatch.setattr(cfgmod, "_config", _config( + workspace=tmp_path / "ws", backup=BackupConfig(enabled=False), + )) + monkeypatch.setattr( + backup_mod, "latest_bundle_age_seconds", lambda _t: None, + ) + made: list = [] + monkeypatch.setattr( + backup_mod, "create_backup", + lambda *a, **k: made.append(a) or SimpleNamespace( + path=tmp_path / "b.tar.zst", size=1024, file_count=3, + ), + ) + monkeypatch.setattr(backup_mod, "prune", lambda *a, **k: []) + monkeypatch.setattr(backup_mod, "list_bundles", lambda _t: []) + + def enable_before_the_second_pass(tick): + if tick == 2: + cfgmod.set_config(_config( + workspace=tmp_path / "ws", + backup=BackupConfig(enabled=True, target_dir=str(target)), + )) + + delays = _fake_clock(monkeypatch, 2, between=enable_before_the_second_pass) + with pytest.raises(_StopLoop): + await srv._periodic_backup(AsyncMock()) + + assert delays[:2] == [3600, 3600] # cadence is a fixed hourly tick + assert len(made) == 1 # nothing on the first tick, a bundle on the second diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 73564618..6d34f564 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -3,18 +3,20 @@ from __future__ import annotations import asyncio +import logging import os import shutil import subprocess import threading import time from pathlib import Path -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, MagicMock import pytest import nerve.sync_service as sync from nerve.config import WorkspaceSyncConfig +from nerve.config_reload import reload_failures from nerve.config_validate import ValidationResult from nerve.sync_service import sync_workspace @@ -743,6 +745,17 @@ async def test_apply_triggers_both_reloads(self, tmp_path): cron.reload.assert_awaited_once() engine.reload_mcp_config.assert_awaited_once() + @pytest.mark.asyncio + async def test_apply_reloads_sources_and_skills(self, tmp_path): + """Sync applies the SAME subsystems as a manual reload (sources+skills).""" + cron = AsyncMock() + engine = MagicMock() + engine.reload_mcp_config = AsyncMock(return_value=[]) + engine._skill_manager.discover = AsyncMock(return_value=[]) + await sync._apply_sync(engine, cron, tmp_path) + cron.reload_sources.assert_awaited_once() + engine._skill_manager.discover.assert_awaited_once() + @pytest.mark.asyncio async def test_apply_survives_reload_error(self, tmp_path): cron = AsyncMock() @@ -771,8 +784,8 @@ async def test_apply_reloads_config_singleton_engages_lockdown(self, tmp_path, m monkeypatch.setattr(sync, "_git", lambda args, cwd: _cp(stdout="origin\n")) monkeypatch.setattr(cfgmod, "_config", cfgmod.NerveConfig(lockdown=False)) assert not is_locked() - err = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) - assert err is None + summary = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) + assert "config" not in reload_failures(summary) assert is_locked() # the reloaded config engaged lockdown @pytest.mark.asyncio @@ -796,8 +809,9 @@ async def test_apply_reports_when_lockdown_fails_to_engage(self, tmp_path, monke encoding="utf-8", ) monkeypatch.setattr(cfgmod, "_config", cfgmod.NerveConfig(lockdown=False)) - err = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) - assert err and "NO_SUCH_SECRET_HERE" in err + summary = await sync._apply_sync(AsyncMock(), AsyncMock(), config_dir) + failures = reload_failures(summary) + assert "NO_SUCH_SECRET_HERE" in failures["config"] assert not is_locked() # still running the old config — which the caller must say @pytest.mark.asyncio @@ -823,7 +837,7 @@ async def test_route_does_not_claim_success_when_apply_failed(self, tmp_path, mo ) async def _failing_apply(engine, cron_service, config_dir): - return "ConfigError: nope" + return {"config": "error: ConfigError: nope"} monkeypatch.setattr(sync, "_apply_sync", _failing_apply) body = await route_mod.sync_workspace_route(user={}) @@ -831,6 +845,43 @@ async def _failing_apply(engine, cron_service, config_dir): assert body["changed"] is True and body["applied"] is False assert body["apply_error"] == "ConfigError: nope" + @pytest.mark.asyncio + async def test_route_reports_a_partly_applied_merge(self, tmp_path, monkeypatch): + """The config loaded, so the merged settings *are* live — but cron didn't + take it, so the daemon is on the merged config only in part. Reporting a + clean `applied` for that hides the one outcome an operator cannot see + from the outside. + """ + import nerve.gateway.server as srv + + import nerve.gateway.routes.config as route_mod + from nerve.sync_service import SyncResult + + fake_cfg = type("C", (), {})() + fake_cfg.workspace = str(tmp_path / "ws") + fake_cfg.config_dir = str(tmp_path / "cfg") + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True) + fake_cfg.lockdown = False + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr( + route_mod, "get_deps", lambda: type("D", (), {"engine": None})(), + ) + monkeypatch.setattr( + sync, "sync_workspace", + lambda *a, **k: SyncResult(ok=True, changed=True, message="updated"), + ) + + async def _partial_apply(engine, cron_service, config_dir): + return {"config": "reloaded", "cron": "error: bad jobs.yaml"} + + monkeypatch.setattr(sync, "_apply_sync", _partial_apply) + body = await route_mod.sync_workspace_route(user={}) + assert body["ok"] is True # the merged config itself is in effect + assert body["applied"] is False # ...but not everywhere + assert body["apply_error"] is None + assert body["reload_errors"] == {"cron": "bad jobs.yaml"} + class TestNeverRaises: """``sync_workspace`` promises a SyncResult for every outcome. @@ -950,16 +1001,23 @@ class _FakeClock: hands to ``asyncio.wait_for``, so that is recorded rather than inferred. ``cycles`` syncs run, then the stop event is set and the wait reports "stopped" exactly as the real one would. + + ``hook`` is called with the wait's index each time, which is the point + *between* two cycles with the loop already started — the only place a test + can change the world the way an operator or another process does. """ - def __init__(self, cycles: int, stop: asyncio.Event): + def __init__(self, cycles: int, stop: asyncio.Event, hook=None): self.cycles = cycles self.stop = stop + self.hook = hook self.timeouts: list[float] = [] async def wait_for(self, coro, timeout=None): self.timeouts.append(timeout) coro.close() # the loop passed stop_event.wait(); we never await it + if self.hook is not None: + self.hook(len(self.timeouts) - 1) if self.cycles <= 0: self.stop.set() return True @@ -967,10 +1025,10 @@ async def wait_for(self, coro, timeout=None): raise asyncio.TimeoutError -def _run_cycles(monkeypatch, cycles): +def _run_cycles(monkeypatch, cycles, hook=None): """Wire a fake clock into the loop and return ``(stop_event, clock)``.""" stop = asyncio.Event() - clock = _FakeClock(cycles, stop) + clock = _FakeClock(cycles, stop, hook) monkeypatch.setattr(sync.asyncio, "wait_for", clock.wait_for) return stop, clock @@ -1022,17 +1080,16 @@ async def test_stop_event_exits_promptly(self): ) @pytest.mark.asyncio - async def test_editing_config_on_disk_does_not_reach_the_loop( + async def test_editing_config_on_disk_does_not_reach_the_loop_on_its_own( self, tmp_path, monkeypatch, ): - """What actually happens today, and what the docs must therefore say. - - The loop re-reads the process-wide config object every cycle, but nothing - refreshes that object after start-up, so an edited config.yaml is never - seen. If this test starts failing because something now reloads the - singleton, that is good news — update the loop's docstring and the - "workspace_sync changes need a restart" paragraph in docs/config.md, - which are written to this behavior. + """A file changing on disk is not a reload, and deliberately so. + + The loop re-reads the process-wide config object every cycle, but only an + explicit reload replaces that object, so editing config.yaml on the box + and waiting achieves nothing. The companion test below covers what + happens once a reload is actually asked for; ``docs/config.md`` says the + same thing to operators, under "What triggers a reload". """ import nerve.config as nerve_config @@ -1057,6 +1114,52 @@ async def test_editing_config_on_disk_does_not_reach_the_loop( assert [k["branch"] for k in seen] == ["OLD", "OLD", "OLD"] assert nerve_config.get_config() is started_with + @pytest.mark.asyncio + async def test_a_reload_makes_the_loop_see_the_edited_file( + self, tmp_path, monkeypatch, + ): + """Drives the real config object, not a stand-in for it. + + The loop's per-cycle re-read was correct in shape long before anything + replaced the object it reads, and a test that patched ``get_config`` + would have passed throughout. So this one edits a real config.yaml, runs + a real reload, and checks the branch the next cycle actually pulls. + """ + import nerve.config as nerve_config + from nerve.config_reload import reload_all + + config_dir = tmp_path / "cfg" + _write_config(config_dir, tmp_path / "ws", enabled="true", branch="OLD") + monkeypatch.setattr( + nerve_config, "_config", nerve_config.load_config(config_dir), + ) + started_with = nerve_config.get_config() + + seen: list[dict] = [] + + def edit_then_reload(): + """Stand in for a hand edit followed by POST /api/config/reload. + + The loop hands ``sync_workspace`` to ``asyncio.to_thread``, so this + runs on a worker thread with no event loop of its own — which is what + lets it drive the real coroutine rather than a substitute for it. + """ + if len(seen) > 1: + return + _write_config( + config_dir, tmp_path / "ws", enabled="true", branch="NEW", + ) + asyncio.run(reload_all(None, None, config_dir)) + + monkeypatch.setattr( + sync, "sync_workspace", _sync_double(seen, then=edit_then_reload), + ) + stop, _clock = _run_cycles(monkeypatch, 3) + await sync.run_periodic_sync(started_with, AsyncMock(), AsyncMock(), stop) + + assert [k["branch"] for k in seen] == ["OLD", "NEW", "NEW"] + assert nerve_config.get_config() is not started_with + @pytest.mark.asyncio async def test_loop_follows_the_current_config_object(self, monkeypatch): """The loop holds no snapshot of its own: whatever replaces the process @@ -1191,8 +1294,433 @@ async def test_route_applies_a_changed_sync(self, monkeypatch, tmp_path): async def _fake_apply(engine, cron_service, config_dir): applied.append(Path(config_dir)) + return {"config": "reloaded"} monkeypatch.setattr(sync, "_apply_sync", _fake_apply) body = await route_mod.sync_workspace_route(user={}) - assert body["ok"] and body["changed"] + assert body["ok"] and body["changed"] and body["applied"] + assert body["status"] == "applied" assert applied == [tmp_path / "cfg"] + + @pytest.mark.asyncio + async def test_status_separates_the_two_reasons_applied_is_false( + self, tmp_path, monkeypatch, + ): + """``applied: false`` is the answer both when there was nothing to merge + and when the merge reached no subsystem, and a script cannot act on the + two the same way. ``status`` is what it reads instead. + """ + import nerve.gateway.server as srv + + import nerve.gateway.routes.config as route_mod + from nerve.sync_service import SyncResult + + fake_cfg = type("C", (), {})() + fake_cfg.workspace = str(tmp_path / "ws") + fake_cfg.config_dir = str(tmp_path / "cfg") + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True) + fake_cfg.lockdown = False + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(srv, "_cron_service", None, raising=False) + monkeypatch.setattr( + route_mod, "get_deps", lambda: type("D", (), {"engine": None})(), + ) + + monkeypatch.setattr( + sync, "sync_workspace", + lambda *a, **k: SyncResult(ok=True, changed=False, message="up to date"), + ) + body = await route_mod.sync_workspace_route(user={}) + assert body["applied"] is False and body["status"] == "up-to-date" + + monkeypatch.setattr( + sync, "sync_workspace", + lambda *a, **k: SyncResult(ok=True, changed=True, message="updated"), + ) + + async def _no_subsystem_took_it(engine, cron_service, config_dir): + return {"config": "error: ConfigError: nope"} + + monkeypatch.setattr(sync, "_apply_sync", _no_subsystem_took_it) + body = await route_mod.sync_workspace_route(user={}) + assert body["applied"] is False and body["status"] == "not-applied" + + async def _cron_refused(engine, cron_service, config_dir): + return {"config": "reloaded", "cron": "error: bad jobs.yaml"} + + monkeypatch.setattr(sync, "_apply_sync", _cron_refused) + body = await route_mod.sync_workspace_route(user={}) + assert body["applied"] is False and body["status"] == "partial" + + +class _LoopAgainstRealGit(_RealGit): + """A real origin/clone pair driven by the real periodic loop. + + The loop's whole subject here is what is on disk versus what this process + applied, so a scripted git double cannot answer it: the revisions have to be + real ones that move. + """ + + def _upstream_commit(self, origin, tz="Europe/Berlin"): + (origin / "config" / "settings.yaml").write_text(f"timezone: {tz}\n") + self._git("commit", "-am", "change tz", cwd=origin) + + def _config(self, ws, tmp_path, **overrides): + (tmp_path / "cfg").mkdir(exist_ok=True) + return _loop_config( + workspace=str(ws), config_dir=str(tmp_path / "cfg"), + branch="main", validate=False, interval_minutes=60, **overrides, + ) + + def _head(self, ws): + return self._git("rev-parse", "HEAD", cwd=ws).stdout.strip() + + async def _run(self, cfg, monkeypatch, cycles, engine=None, hook=None): + import nerve.config as nerve_config + + monkeypatch.setattr(nerve_config, "_config", cfg) + stop, _clock = _run_cycles(monkeypatch, cycles, hook=hook) + await sync.run_periodic_sync(cfg, engine, None, stop) + + +@pytest.mark.skipif(not shutil.which("git"), reason="git not available") +class TestLoopAppliesWhatIsOnDisk(_LoopAgainstRealGit): + """HEAD says where the config is, not that anything read it. + + It moves without this loop (`nerve config sync`, a bare `git pull`), and a + reload can fail for one subsystem after a merge that did happen. Applying + only when the loop's own pull merged something leaves both states reported as + "up to date" for as long as the daemon runs. + """ + + @pytest.mark.asyncio + async def test_a_head_moved_out_of_band_is_applied_once(self, tmp_path, monkeypatch): + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + applied: list = [] + + async def _apply(engine, cron_service, config_dir): + applied.append(config_dir) + return {"config": "reloaded"} + + monkeypatch.setattr(sync, "_apply_sync", _apply) + + def fast_forward_from_a_shell(i): + # Between cycles, with the daemon already running: the ordering + # matters, since a daemon started afterwards reads the new config + # from disk at start-up and has nothing to catch up on. + if i == 0: + sync_workspace(ws, tmp_path / "cfg", branch="main", validate=False) + + await self._run( + self._config(ws, tmp_path), monkeypatch, 3, + hook=fast_forward_from_a_shell, + ) + assert len(applied) == 1 # applied once, then nothing left to do + + @pytest.mark.asyncio + async def test_a_subsystem_that_refused_is_retried(self, tmp_path, monkeypatch): + """The merge landed, cron did not take it, and the only trace was one + warning. Nothing retried it, so the daemon ran the merged config in part + until someone restarted it.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + attempts: list = [] + + async def _apply(engine, cron_service, config_dir): + attempts.append(config_dir) + return {"config": "reloaded", "cron": "error: bad jobs.yaml"} + + monkeypatch.setattr(sync, "_apply_sync", _apply) + await self._run(self._config(ws, tmp_path), monkeypatch, 4) + + assert len(attempts) == 4 # the merge, then a retry every cycle + + @pytest.mark.asyncio + async def test_the_retry_stops_once_the_subsystem_takes_it(self, tmp_path, monkeypatch): + """The other half: retrying for good would reload every subsystem once a + minute forever.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + attempts: list = [] + + async def _apply(engine, cron_service, config_dir): + attempts.append(config_dir) + if len(attempts) == 1: + return {"config": "reloaded", "cron": "error: bad jobs.yaml"} + return {"config": "reloaded", "cron": "reloaded"} + + monkeypatch.setattr(sync, "_apply_sync", _apply) + await self._run(self._config(ws, tmp_path), monkeypatch, 4) + + assert len(attempts) == 2 # the merge, one retry, then quiet + + @pytest.mark.asyncio + async def test_an_untouched_workspace_is_not_reapplied(self, tmp_path, monkeypatch): + """The loop seeds from HEAD: a daemon that starts on the current revision + has already read it, and must not reload on its first cycle.""" + _origin, ws = self._pair(tmp_path) + applied: list = [] + + async def _apply(engine, cron_service, config_dir): + applied.append(config_dir) + return {"config": "reloaded"} + + monkeypatch.setattr(sync, "_apply_sync", _apply) + await self._run(self._config(ws, tmp_path), monkeypatch, 3) + assert applied == [] + + +class _ForgetsSyncState: + @pytest.fixture(autouse=True) + def _forget_previous_cycles(self, monkeypatch): + """The retained state is module-level, as the daemon's own is.""" + monkeypatch.setattr(sync, "_last_sync", None) + + +@pytest.mark.skipif(not shutil.which("git"), reason="git not available") +class TestRetainedStateFollowsTheLoop(_ForgetsSyncState, _LoopAgainstRealGit): + @pytest.mark.asyncio + async def test_a_partly_applied_merge_is_visible(self, tmp_path, monkeypatch): + """The state a caller most needs and the log is worst at: the merge + landed, the config loaded, and one subsystem is still running the old + one.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + pinned = self._head(ws) + + async def _apply(engine, cron_service, config_dir): + return {"config": "reloaded", "cron": "error: bad jobs.yaml"} + + monkeypatch.setattr(sync, "_apply_sync", _apply) + await self._run(self._config(ws, tmp_path), monkeypatch, 2) + + state = sync.last_sync_state() + assert state.reload_errors == {"cron": "bad jobs.yaml"} + assert state.applied_rev == pinned # not what the merge landed + assert state.fetched_rev == self._head(ws) # ...which is this + assert state.ok and not state.blocked_paths + + +class _Notifier: + """Records what the loop sends, with the notification service's signature.""" + + def __init__(self): + self.sent: list[dict] = [] + + async def send_notification(self, session_id, title, body="", priority="normal"): + self.sent.append({ + "session_id": session_id, "title": title, + "body": body, "priority": priority, + }) + return "notif-test" + + +class _Engine: + def __init__(self, notification_service=None): + self.notification_service = notification_service + + +@pytest.mark.skipif(not shutil.which("git"), reason="git not available") +class TestBlockedSyncIsVisible(_ForgetsSyncState, _LoopAgainstRealGit): + """A refused merge used to leave nothing but a repeating WARNING. + + The instance stays pinned to an old reviewed revision and every later config + change stops arriving — on a locked box, the deployment defeated — while it + answers every other question exactly like a healthy one. + """ + + def _dirty_a_skill(self, ws): + (ws / "skills" / "backdoor").mkdir(parents=True, exist_ok=True) + (ws / "skills" / "backdoor" / "SKILL.md").write_text( + "---\nname: backdoor\nallowed-tools: Bash\n---\n", + ) + + @pytest.mark.asyncio + async def test_entering_the_blocked_state_notifies_once(self, tmp_path, monkeypatch): + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + pinned = self._head(ws) + notifier = _Notifier() + + await self._run( + self._config(ws, tmp_path), monkeypatch, 3, engine=_Engine(notifier), + ) + + assert len(notifier.sent) == 1, notifier.sent # not one per cycle + sent = notifier.sent[0] + assert sent["title"] == "Workspace sync blocked" + assert "SKILL.md" in sent["body"] + assert "propose_config_change" in sent["body"] + assert sent["priority"] == "high" + # ...and the merge really did not happen. + assert self._head(ws) == pinned + assert "Europe/Berlin" not in (ws / "config" / "settings.yaml").read_text() + + @pytest.mark.asyncio + async def test_the_blocked_state_is_queryable(self, tmp_path, monkeypatch): + """A log line cannot be asked a question. `nerve doctor` and + GET /api/config/sync both read this.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + pinned = self._head(ws) + + await self._run(self._config(ws, tmp_path), monkeypatch, 2) + + state = sync.last_sync_state() + assert state.blocked_paths and "SKILL.md" in state.blocked_paths[0] + assert state.applied_rev == pinned + assert state.fetched_rev != pinned # what it would be running if it could + assert state.ok is False and state.checked_at > 0 + + @pytest.mark.asyncio + async def test_clearing_the_block_notifies_once_more(self, tmp_path, monkeypatch): + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + notifier = _Notifier() + + def drop_the_local_change(i): + if i == 2: + shutil.rmtree(ws / "skills") + + await self._run( + self._config(ws, tmp_path), monkeypatch, 4, engine=_Engine(notifier), + hook=drop_the_local_change, + ) + + assert [n["title"] for n in notifier.sent] == [ + "Workspace sync blocked", "Workspace sync unblocked", + ] + assert notifier.sent[1]["priority"] == "low" + assert not sync.last_sync_state().blocked_paths + assert "Europe/Berlin" in (ws / "config" / "settings.yaml").read_text() + + @pytest.mark.asyncio + async def test_a_cycle_that_never_reached_the_check_reports_no_recovery( + self, tmp_path, monkeypatch, + ): + """A failed fetch establishes nothing about the local tree, and saying + "unblocked" there would retract a warning that still stands — and arm the + next block to notify all over again.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + notifier = _Notifier() + + def break_the_remote(i): + if i == 1: + self._git( + "remote", "set-url", "origin", str(tmp_path / "gone"), cwd=ws, + ) + + await self._run( + self._config(ws, tmp_path), monkeypatch, 3, engine=_Engine(notifier), + hook=break_the_remote, + ) + + assert [n["title"] for n in notifier.sent] == ["Workspace sync blocked"] + assert sync.last_sync_state().blocked_paths + + @pytest.mark.asyncio + async def test_a_later_failure_does_not_keep_reporting_the_old_block( + self, tmp_path, monkeypatch, + ): + """The other side of the rule above: committing the local change clears + the block *and* diverges the branch, so the merge fails for a new reason. + Holding on to the old paths would name files that are no longer the + problem, and no later block would ever be announced. + """ + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + + def commit_it(i): + if i == 1: + self._git("add", "-A", cwd=ws) + self._git("commit", "-m", "add skill", cwd=ws) + + await self._run( + self._config(ws, tmp_path), monkeypatch, 3, engine=_Engine(_Notifier()), + hook=commit_it, + ) + + state = sync.last_sync_state() + assert not state.blocked_paths + assert state.ok is False and "ff-only" in state.message + + @pytest.mark.asyncio + async def test_no_notification_service_is_not_a_failure( + self, tmp_path, monkeypatch, caplog, + ): + """Every other sender treats an absent service as "nothing to do here". + Filing it as a failed delivery instead would put a warning in the log + every time the CLI or a test crosses the transition, with nothing there + to deliver to.""" + origin, ws = self._pair(tmp_path) + self._upstream_commit(origin) + self._dirty_a_skill(ws) + + with caplog.at_level(logging.WARNING, logger="nerve.sync_service"): + await self._run(self._config(ws, tmp_path), monkeypatch, 2, engine=None) + await self._run( + self._config(ws, tmp_path), monkeypatch, 1, engine=_Engine(None), + ) + assert "notification failed" not in caplog.text + assert sync.last_sync_state().blocked_paths + + def test_doctor_reports_a_blocked_workspace(self, tmp_path, monkeypatch): + """The CLI has no view of the daemon's record, and the shell is where an + operator asks why config stopped arriving — so doctor runs the check + itself.""" + from nerve.cli import doctor_report + from nerve.config import NerveConfig + + _origin, ws = self._pair(tmp_path) + config = NerveConfig( + workspace=ws, config_dir=tmp_path / "cfg", + workspace_sync=WorkspaceSyncConfig(enabled=True, branch="main"), + ) + assert "Workspace sync: reviewed files clean" in doctor_report(config) + + self._dirty_a_skill(ws) + report = doctor_report(config) + assert "Workspace sync BLOCKED" in report + assert "SKILL.md" in report + assert "propose_config_change" in report + + +class TestSyncStatusRoute: + @pytest.mark.asyncio + async def test_reports_the_retained_state(self, monkeypatch): + import nerve.gateway.routes.config as route_mod + + fake_cfg = type("C", (), {})() + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=True, branch="main") + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(sync, "_last_sync", sync.SyncState( + applied_rev="a" * 40, fetched_rev="b" * 40, + blocked_paths=["?? skills/backdoor/SKILL.md"], + reload_errors={"cron": "bad jobs.yaml"}, + ok=False, message="fetched bbbbbbbb but ...", checked_at=time.time(), + )) + body = await route_mod.sync_status_route(user={}) + assert body["checked"] is True and body["blocked"] is True + assert body["blocked_paths"] == ["?? skills/backdoor/SKILL.md"] + assert body["applied_rev"] == "a" * 40 + assert body["reload_errors"] == {"cron": "bad jobs.yaml"} + assert body["checked_at"].startswith("20") + + @pytest.mark.asyncio + async def test_no_cycle_yet_is_not_a_clean_bill_of_health(self, monkeypatch): + import nerve.gateway.routes.config as route_mod + + fake_cfg = type("C", (), {})() + fake_cfg.workspace_sync = WorkspaceSyncConfig(enabled=False) + monkeypatch.setattr("nerve.config.get_config", lambda: fake_cfg) + monkeypatch.setattr(sync, "_last_sync", None) + body = await route_mod.sync_status_route(user={}) + assert body["checked"] is False and body["enabled"] is False + assert "blocked" not in body # no answer, rather than a false negative diff --git a/tests/test_ultracode_integration.py b/tests/test_ultracode_integration.py index 9ee17498..6e3d44df 100644 --- a/tests/test_ultracode_integration.py +++ b/tests/test_ultracode_integration.py @@ -53,7 +53,7 @@ def _backend( external_mcp_servers: list[McpServerConfig] | None = None, ) -> CodexBackend: return CodexBackend(BackendDeps( - config=cfg, + config=lambda: cfg, db=None, registry=None, tool_ctx_factory=lambda sid: None,