From 5dcd4cf88b2b5d72ac1ff954dff20425edeed875 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 19:55:02 +0000 Subject: [PATCH 1/9] Let the system actor take a new ID while nothing refers to it In external mode, the environment can name the ID of the agent's system principal. To use that ID for autonomous work, the system actor of a new database must be able to take it once, although v047 makes the ID immutable. Add the reader for NERVE_SYSTEM_PRINCIPAL_ID (canonical lower-case UUID only) and a store method that changes the ID in one transaction: it drops system_actor_id_cannot_change, updates the row and creates the trigger again with the v047 statement. It changes nothing when another actor has the new ID or when a row refers to the system actor. The referencing columns come from the schema (PRAGMA foreign_key_list), so columns that later migrations add are checked too. A read-only variant gives the same answer for CLI commands. Co-Authored-By: Claude Opus 5.5 --- nerve/db/accounts.py | 162 ++++++++++++++++++++ nerve/db/base.py | 13 +- nerve/identity.py | 50 ++++++- tests/conftest.py | 12 ++ tests/test_system_principal.py | 264 +++++++++++++++++++++++++++++++++ 5 files changed, 495 insertions(+), 6 deletions(-) create mode 100644 tests/test_system_principal.py diff --git a/nerve/db/accounts.py b/nerve/db/accounts.py index 751822a6c..d5b9715a6 100644 --- a/nerve/db/accounts.py +++ b/nerve/db/accounts.py @@ -13,6 +13,33 @@ CREDENTIAL_SOURCES = ("config", "local", "none") JWT_SECRET_NAME = "jwt_secret" +# The trigger that keeps the ID of the system actor stable, and the statement +# that v047 creates it with, character for character. Only +# AccountStore._rekey_system_actor drops it, and it creates it again with +# this statement in the same transaction. +SYSTEM_ACTOR_ID_TRIGGER = "system_actor_id_cannot_change" +SYSTEM_ACTOR_ID_TRIGGER_SQL = """CREATE TRIGGER IF NOT EXISTS system_actor_id_cannot_change +BEFORE UPDATE OF id ON actor_refs +FOR EACH ROW WHEN OLD.kind = 'system' AND NEW.id IS NOT OLD.id +BEGIN + SELECT RAISE(ABORT, 'the system actor id cannot change'); +END;""" + +# Every (table, column) that has a foreign key to actor_refs(id), read from +# the schema. A foreign key without a column list refers to the primary key, +# and PRAGMA foreign_key_list gives NULL for its "to" column. +ACTOR_REF_COLUMNS_SQL = """ +SELECT m.name, p."from" + FROM sqlite_master AS m + JOIN pragma_foreign_key_list(m.name) AS p + WHERE m.type = 'table' + AND p."table" = 'actor_refs' COLLATE NOCASE + AND (p."to" IS NULL OR p."to" = 'id' COLLATE NOCASE) + ORDER BY m.name, p."from" +""" + +SYSTEM_ACTOR_ID_TAKEN = "another actor already has the new ID" + # Sentinel for "leave this field alone" in partial updates, distinct from None # (which clears a nullable field). _UNSET = object() @@ -137,6 +164,47 @@ class BootstrapAccount: actor_id: str | None = None +@dataclass(frozen=True) +class SystemActorRekey: + """The result of :meth:`AccountStore._rekey_system_actor`.""" + + # The stored ID of the system actor before and after the call. + previous_id: str + actor_id: str + # Why the ID did not change, or ``None``. + blocker: str | None = None + + @property + def changed(self) -> bool: + return self.previous_id != self.actor_id + + +def _quote_identifier(name: str) -> str: + return '"' + name.replace('"', '""') + '"' + + +def _rekey_probes( + columns: list[tuple[str, str]], current_id: str, new_id: str, +) -> list[tuple[str, tuple[str], str]]: + """The checks that must all find no row before the system actor gets a new ID. + + Each item is ``(sql, params, reason)``. The reason applies when the query + returns a row. ``columns`` comes from :data:`ACTOR_REF_COLUMNS_SQL`, so + the names are from the schema, never from a caller. + """ + probes = [ + ("SELECT 1 FROM actor_refs WHERE id = ? LIMIT 1", (new_id,), SYSTEM_ACTOR_ID_TAKEN), + ] + for table, column in columns: + probes.append(( + f"SELECT 1 FROM {_quote_identifier(table)} " + f"WHERE {_quote_identifier(column)} = ? LIMIT 1", + (current_id,), + f"{table}.{column} refers to the current ID", + )) + return probes + + class AccountStore: """Database mixin; callers use higher-level account and identity services.""" @@ -206,6 +274,71 @@ async def upsert_external_actor( ) written[actor_id] = display_name + # -- system actor ID ----------------------------------------------------- + + async def _actor_ref_columns(self) -> list[tuple[str, str]]: + """Every ``(table, column)`` with a foreign key to ``actor_refs(id)``. + + Read from the schema, so a column that a later migration adds is + included without a change here. + """ + async with self.db.execute(ACTOR_REF_COLUMNS_SQL) as cursor: + return [(row[0], row[1]) async for row in cursor] + + async def _system_actor_rekey_blocker( + self, current_id: str, new_id: str, + ) -> str | None: + """Why the system actor cannot change from ``current_id`` to ``new_id``. + + ``None`` when no actor has ``new_id`` and no column with a foreign key + to ``actor_refs`` holds ``current_id``. + """ + for sql, params, reason in _rekey_probes( + await self._actor_ref_columns(), current_id, new_id, + ): + async with self.db.execute(sql, params) as cursor: + if await cursor.fetchone() is not None: + return reason + return None + + async def _rekey_system_actor(self, new_id: str) -> SystemActorRekey: + """Give the system actor ``new_id`` while no row refers to it. + + One transaction reads the stored ID again, checks + :meth:`_system_actor_rekey_blocker`, drops + ``system_actor_id_cannot_change``, changes the ID, and creates the + trigger again with :data:`SYSTEM_ACTOR_ID_TRIGGER_SQL`. A failure rolls + back all of it, so no other transaction sees the table without the + trigger. When a blocker exists, nothing changes and the result names + it. The cached :attr:`system_actor` is read again in all cases. + """ + async with self._atomic(): + async with self.db.execute( + "SELECT id FROM actor_refs WHERE kind = 'system'" + ) as cursor: + row = await cursor.fetchone() + if row is None: + from nerve.db.base import IdentityInvariantError + + raise IdentityInvariantError( + "actor identity is corrupt: there is no system actor" + ) + previous_id = row[0] + blocker = None + if previous_id != new_id: + blocker = await self._system_actor_rekey_blocker(previous_id, new_id) + if blocker is None: + await self.db.execute( + f"DROP TRIGGER IF EXISTS {SYSTEM_ACTOR_ID_TRIGGER}" + ) + await self.db.execute( + "UPDATE actor_refs SET id = ? WHERE id = ? AND kind = 'system'", + (new_id, previous_id), + ) + await self.db.execute(SYSTEM_ACTOR_ID_TRIGGER_SQL) + await self._cache_system_actor() + return SystemActorRekey(previous_id, self.system_actor_id, blocker) + # -- accounts ------------------------------------------------------------ async def get_account(self, account_id: str) -> dict | None: @@ -744,6 +877,35 @@ def read_setup_required(db_path: Path, *, configured_password: bool) -> bool: conn.close() +def inspect_system_actor(db_path: Path, wanted_id: str) -> tuple[str, str | None] | None: + """Read-only: the stored system actor ID, and why it cannot become ``wanted_id``. + + Returns ``(actor_id, blocker)``. ``blocker`` is ``None`` when the stored + ID is ``wanted_id`` or when the identity bootstrap can change it (see + ``AccountStore._system_actor_rekey_blocker``). Returns ``None`` when the + database does not exist, has no system actor yet, or cannot be read. + """ + conn = _read_only(db_path) + if conn is None: + return None + try: + rows = conn.execute("SELECT id FROM actor_refs WHERE kind = 'system'").fetchall() + if len(rows) != 1: + return None + current_id = str(rows[0][0]) + if current_id == wanted_id: + return current_id, None + columns = [(row[0], row[1]) for row in conn.execute(ACTOR_REF_COLUMNS_SQL)] + for sql, params, reason in _rekey_probes(columns, current_id, wanted_id): + if conn.execute(sql, params).fetchone() is not None: + return current_id, reason + return current_id, None + except sqlite3.Error: + return None + finally: + conn.close() + + def inspect_bootstrap_state(db_path: Path) -> tuple[list[str], bool] | None: """Read-only dry-run state: credential sources and stored-secret presence.""" conn = _read_only(db_path) diff --git a/nerve/db/base.py b/nerve/db/base.py index 10c29f7e3..167c43491 100644 --- a/nerve/db/base.py +++ b/nerve/db/base.py @@ -440,7 +440,11 @@ async def connect(self) -> None: raise async def _cache_system_actor(self) -> None: - """Validate and cache the one system actor created by migrations.""" + """Validate and cache the one system actor created by migrations. + + Called at connect, and again after the identity bootstrap changes the + ID (:meth:`AccountStore._rekey_system_actor`). + """ async with self.db.execute( "SELECT id, display_name FROM actor_refs WHERE kind = 'system'" ) as cursor: @@ -465,8 +469,11 @@ async def _cache_system_actor(self) -> None: def system_actor(self) -> Actor: """The actor for autonomous work, validated and cached at connect. - Schema triggers prevent a change to its id or kind, so no caller has to - read the row again. + Schema triggers prevent a change to its id or kind. The one exception + is the identity bootstrap at startup, which can give the system actor + the ID of the agent's system principal before anything refers to it, + and which then refreshes this cache. So no other caller has to read + the row again. """ if self._system_actor is None: raise RuntimeError("Database not connected. Call connect() first.") diff --git a/nerve/identity.py b/nerve/identity.py index 828f8bf59..4459387ab 100644 --- a/nerve/identity.py +++ b/nerve/identity.py @@ -3,9 +3,12 @@ An :class:`Actor` is the identity carried through an operation: the person who made the request, or the agent's system principal when the work is autonomous (cron, hooks, background agents). Human actors are resolved per request and -passed down the call chain. The system actor cannot change, so the database -caches it at connect (``Database.system_actor``); there is no process-global -"current actor". +passed down the call chain. The database caches the system actor at connect +(``Database.system_actor``); there is no process-global "current actor". The +ID of the system actor changes only in the identity bootstrap at startup, in +external mode, when the environment names the ID of the agent's system +principal (:data:`SYSTEM_PRINCIPAL_ENV`) and no row refers to the old ID yet. +The bootstrap then refreshes the cache. This module sits at the top level, outside both :mod:`nerve.db` and :mod:`nerve.gateway`, because both ends need it: the gateway resolves an actor @@ -30,8 +33,10 @@ from __future__ import annotations +import os from dataclasses import dataclass from typing import TYPE_CHECKING, TypeAlias +from uuid import UUID if TYPE_CHECKING: # pragma: no cover - typing only, never imported at runtime from nerve.db.accounts import AccountStore @@ -43,6 +48,45 @@ ACTOR_KIND_SYSTEM = "system" ACTOR_KINDS = (ACTOR_KIND_HUMAN, ACTOR_KIND_SYSTEM) +# In external mode, the platform that runs the agent can give the ID of the +# agent's system principal in this variable. The identity bootstrap reads it +# once at startup and gives that ID to the system actor (see +# ``nerve.migrate.bootstrap_identity``). Local mode ignores it. +SYSTEM_PRINCIPAL_ENV = "NERVE_SYSTEM_PRINCIPAL_ID" + + +class InvalidSystemPrincipalId(ValueError): + """The system principal ID is not a canonical lower-case UUID.""" + + +def parse_system_principal_id(value: str | None) -> str | None: + """Return the system principal ID in ``value``, or ``None`` when it is unset. + + Unset or blank is ``None``. Any other value must be a UUID in its + canonical lower-case form (``8-4-4-4-12`` hex digits), because the ID is + compared and sent as a string. Raises :class:`InvalidSystemPrincipalId` + for a value in another form, also for a valid UUID in upper case: the + platform sends the canonical form, so another form is a configuration + error. + """ + if value is None or not value.strip(): + return None + try: + canonical = str(UUID(value)) + except ValueError: + canonical = None + if canonical != value: + raise InvalidSystemPrincipalId( + f"{SYSTEM_PRINCIPAL_ENV} must be a UUID in canonical lower-case form, " + f"got {value!r}" + ) + return canonical + + +def system_principal_id_from_env() -> str | None: + """Read and parse ``NERVE_SYSTEM_PRINCIPAL_ID``.""" + return parse_system_principal_id(os.environ.get(SYSTEM_PRINCIPAL_ENV)) + class ActorResolutionError(Exception): """A verified credential names no actor this instance can act for.""" diff --git a/tests/conftest.py b/tests/conftest.py index ec49fdeeb..dad7a38cb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -156,6 +156,18 @@ def _unpin_auth_mode(monkeypatch): unpin_auth_mode() +@pytest.fixture(autouse=True) +def _no_ambient_system_principal(monkeypatch): + """Strip ``NERVE_SYSTEM_PRINCIPAL_ID`` from the developer shell. + + In external mode, the identity bootstrap gives the system actor the ID in + that variable. Tests about it set the variable themselves. + """ + from nerve.identity import SYSTEM_PRINCIPAL_ENV + + monkeypatch.delenv(SYSTEM_PRINCIPAL_ENV, raising=False) + + @pytest.fixture(scope="session", autouse=True) def _deterministic_umask(): """Run the suite under umask 022, whatever the developer's shell has. diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py new file mode 100644 index 000000000..428aff6c6 --- /dev/null +++ b/tests/test_system_principal.py @@ -0,0 +1,264 @@ +"""The system actor takes the system principal ID that the environment names. + +In external mode, the environment can give the ID of the agent's system +principal in ``NERVE_SYSTEM_PRINCIPAL_ID``. The identity bootstrap (server +start and ``nerve init``) gives the system actor that ID while no row refers to +the system actor and no other actor has the ID. It drops the trigger +``system_actor_id_cannot_change`` for that one update and creates it again in +the same transaction. Otherwise the local ID stays, a warning is logged, and +``nerve doctor`` reports the difference. Local mode ignores the variable. +""" + +from __future__ import annotations + +import asyncio +import sqlite3 + +import pytest +import pytest_asyncio + +from nerve.db import Database +from nerve.db.accounts import ( + SYSTEM_ACTOR_ID_TAKEN, + SYSTEM_ACTOR_ID_TRIGGER, + SYSTEM_ACTOR_ID_TRIGGER_SQL, + inspect_system_actor, +) +from nerve.db.migrations import v047_accounts +from nerve.identity import ( + SYSTEM_PRINCIPAL_ENV, + Actor, + InvalidSystemPrincipalId, + parse_system_principal_id, +) + +# Obviously fake IDs. +PRINCIPAL = "00000000-0000-4000-8000-00000000c2c2" +ALICE = "00000000-0000-4000-8000-0000000a11ce" + + +async def _open(path) -> Database: + database = Database(path) + await database.connect() + return database + + +async def _trigger_sql(database: Database) -> str | None: + async with database.db.execute( + "SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = ?", + (SYSTEM_ACTOR_ID_TRIGGER,), + ) as cursor: + row = await cursor.fetchone() + return row[0] if row else None + + +async def _system_rows(database: Database) -> list[str]: + async with database.db.execute( + "SELECT id FROM actor_refs WHERE kind = 'system'" + ) as cursor: + return [row[0] async for row in cursor] + + +async def _refer_to_system_actor(database: Database, reference: str) -> str: + """Make one row refer to the system actor. Returns ``table.column``.""" + system = database.system_actor + if reference == "session": + await database.create_session("s1", source="cron", actor=system, cause=None) + return "sessions.created_by_actor_id" + if reference == "cause": + await database.create_session("s1", source="cron", actor=None, cause=system) + return "sessions.caused_by_actor_id" + if reference == "message": + await database.create_session("s1", actor=None) + await database.add_message("s1", "user", "hello", actor=system) + return "messages.actor_id" + # A column that a later migration adds is found from the schema. + await database._write( + "CREATE TABLE later_story (id INTEGER PRIMARY KEY, " + "caused_by_actor_id TEXT REFERENCES actor_refs(id))" + ) + await database._write( + "INSERT INTO later_story (caused_by_actor_id) VALUES (?)", (system.actor_id,), + ) + return "later_story.caused_by_actor_id" + + +@pytest.fixture +def seeded(monkeypatch): + """The environment names the system principal ID.""" + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, PRINCIPAL) + return PRINCIPAL + + +@pytest_asyncio.fixture +async def database(tmp_path): + database = await _open(tmp_path / "nerve.db") + yield database + await database.close() + + +# --------------------------------------------------------------------------- # +# The value # +# --------------------------------------------------------------------------- # + + +class TestParse: + @pytest.mark.parametrize("value", [None, "", " "]) + def test_unset_or_blank_is_none(self, value): + assert parse_system_principal_id(value) is None + + def test_a_canonical_uuid_is_accepted(self): + assert parse_system_principal_id(PRINCIPAL) == PRINCIPAL + + @pytest.mark.parametrize("value", [ + PRINCIPAL.upper(), + "{" + PRINCIPAL + "}", + "urn:uuid:" + PRINCIPAL, + PRINCIPAL.replace("-", ""), + " " + PRINCIPAL, + PRINCIPAL + "\n", + "not-a-uuid", + "12345", + ]) + def test_any_other_form_is_refused(self, value): + with pytest.raises(InvalidSystemPrincipalId, match=SYSTEM_PRINCIPAL_ENV): + parse_system_principal_id(value) + + +# --------------------------------------------------------------------------- # +# The change of the ID in the database # +# --------------------------------------------------------------------------- # + + +@pytest.mark.asyncio +class TestRekey: + async def test_an_unreferenced_system_actor_takes_the_new_id(self, tmp_path): + database = await _open(tmp_path / "nerve.db") + try: + local_id = database.system_actor_id + result = await database._rekey_system_actor(PRINCIPAL) + + assert result.changed and result.blocker is None + assert (result.previous_id, result.actor_id) == (local_id, PRINCIPAL) + assert database.system_actor == Actor(actor_id=PRINCIPAL, kind="system") + assert await _system_rows(database) == [PRINCIPAL] + assert await database.get_actor_ref(local_id) is None + finally: + await database.close() + + reopened = await _open(tmp_path / "nerve.db") + try: + assert reopened.system_actor_id == PRINCIPAL + finally: + await reopened.close() + + async def test_the_same_id_changes_nothing(self, database): + await database._rekey_system_actor(PRINCIPAL) + result = await database._rekey_system_actor(PRINCIPAL) + assert not result.changed and result.blocker is None + assert database.system_actor_id == PRINCIPAL + + async def test_the_trigger_is_created_again_with_the_v047_statement(self, database): + assert SYSTEM_ACTOR_ID_TRIGGER_SQL in v047_accounts.SQL + fresh = await _trigger_sql(database) + assert fresh is not None + + await database._rekey_system_actor(PRINCIPAL) + + assert await _trigger_sql(database) == fresh + + async def test_the_trigger_still_blocks_other_id_changes(self, database): + local_id = database.system_actor_id + await database._rekey_system_actor(PRINCIPAL) + for other in (local_id, ALICE): + with pytest.raises(sqlite3.IntegrityError, match="system actor id cannot change"): + await database._write( + "UPDATE actor_refs SET id = ? WHERE kind = 'system'", (other,), + ) + assert await _system_rows(database) == [PRINCIPAL] + + @pytest.mark.parametrize("reference", ["session", "cause", "message", "new_column"]) + async def test_a_reference_in_any_actor_column_keeps_the_id(self, database, reference): + local_id = database.system_actor_id + column = await _refer_to_system_actor(database, reference) + + result = await database._rekey_system_actor(PRINCIPAL) + + assert not result.changed + assert result.blocker == f"{column} refers to the current ID" + assert database.system_actor_id == local_id + assert await _system_rows(database) == [local_id] + + async def test_an_id_that_another_actor_has_keeps_the_id(self, database): + local_id = database.system_actor_id + await database.upsert_external_actor(PRINCIPAL, "Alice Example") + + result = await database._rekey_system_actor(PRINCIPAL) + + assert not result.changed and result.blocker == SYSTEM_ACTOR_ID_TAKEN + assert database.system_actor_id == local_id + assert (await database.get_actor_ref(PRINCIPAL))["kind"] == "human" + + async def test_a_failure_rolls_back_the_id_and_the_trigger(self, database, monkeypatch): + import nerve.db.accounts as accounts + + local_id = database.system_actor_id + fresh = await _trigger_sql(database) + monkeypatch.setattr(accounts, "SYSTEM_ACTOR_ID_TRIGGER_SQL", "CREATE TRIGGER broken") + + with pytest.raises(sqlite3.OperationalError): + await database._rekey_system_actor(PRINCIPAL) + + assert await _system_rows(database) == [local_id] + assert await _trigger_sql(database) == fresh + assert database.system_actor_id == local_id + + async def test_two_connections_agree_on_the_new_id(self, tmp_path): + first = await _open(tmp_path / "nerve.db") + second = await _open(tmp_path / "nerve.db") + try: + results = await asyncio.gather( + first._rekey_system_actor(PRINCIPAL), + second._rekey_system_actor(PRINCIPAL), + ) + assert sorted(r.changed for r in results) == [False, True] + assert all(r.blocker is None for r in results) + assert first.system_actor_id == second.system_actor_id == PRINCIPAL + finally: + await first.close() + await second.close() + + async def test_the_checked_columns_come_from_the_schema(self, database): + found = set(await database._actor_ref_columns()) + assert { + ("accounts", "actor_id"), + ("sessions", "created_by_actor_id"), + ("messages", "actor_id"), + } <= found + await database._write( + "CREATE TABLE later_story (a TEXT REFERENCES actor_refs, " + "b TEXT REFERENCES actor_refs(id), c TEXT REFERENCES sessions(id))" + ) + found_after = set(await database._actor_ref_columns()) + assert found_after - found == {("later_story", "a"), ("later_story", "b")} + + async def test_the_read_only_inspection_gives_the_same_answer(self, tmp_path): + path = tmp_path / "nerve.db" + assert inspect_system_actor(path, PRINCIPAL) is None + + database = await _open(path) + try: + local_id = database.system_actor_id + assert inspect_system_actor(path, local_id) == (local_id, None) + assert inspect_system_actor(path, PRINCIPAL) == (local_id, None) + + column = await _refer_to_system_actor(database, "session") + blocker = f"{column} refers to the current ID" + assert inspect_system_actor(path, PRINCIPAL) == (local_id, blocker) + result = await database._rekey_system_actor(PRINCIPAL) + assert result.blocker == blocker + + await database.upsert_external_actor(ALICE, None) + assert inspect_system_actor(path, ALICE) == (local_id, SYSTEM_ACTOR_ID_TAKEN) + finally: + await database.close() From e0d36e3122fefbe2e84674f4814eccf05cdc23e8 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 19:56:23 +0000 Subject: [PATCH 2/9] Give the system actor the system principal ID at startup In external mode, autonomous work must be attributed to the agent's system principal, whose ID the environment names in NERVE_SYSTEM_PRINCIPAL_ID. The identity bootstrap of the server start and of nerve init now reads the variable once and gives the system actor that ID while no row refers to the system actor. A new installation thus takes the ID before it attributes any work. When a row already refers to the local ID, when another actor has the new ID, or when the value is not a canonical lower-case UUID, the local ID stays and the bootstrap logs one warning. Local mode ignores the variable. The dry-run preview of nerve migrate reports the same step. Co-Authored-By: Claude Opus 5.5 --- docs/accounts.md | 31 ++++ docs/config.md | 17 ++ nerve/migrate.py | 153 +++++++++++++++++- tests/test_system_principal.py | 285 +++++++++++++++++++++++++++++++++ 4 files changed, 480 insertions(+), 6 deletions(-) diff --git a/docs/accounts.md b/docs/accounts.md index 3afd38083..7c237337e 100644 --- a/docs/accounts.md +++ b/docs/accounts.md @@ -160,6 +160,8 @@ In external mode: - Startup creates no account and deletes a setup token from an earlier local mode. It still makes the signing secret, because system and MCP tokens need it. +- Startup can give the system actor the ID of the agent's system principal. + See [The system actor in external mode](#the-system-actor-in-external-mode). - Accounts and history from an earlier local mode stay in `nerve.db`. - Each model request names the principal that caused the work, in the `X-Nerve-Inference-Principal` header: the session's cause, or the system @@ -171,6 +173,35 @@ In external mode: > header can act as any person, also as the actor of an account from an earlier > local mode. +### The system actor in external mode + +Each database has one system actor. The schema migration gives it a random +ID. In external mode, the environment can give the ID of the agent's system +principal in `NERVE_SYSTEM_PRINCIPAL_ID` (see +[System principal ID](config.md#system-principal-id)). The identity bootstrap +of `nerve init` and of each server start then gives the system actor that ID +when these two conditions are true: + +- No row refers to the system actor. Nerve finds each column that has a + foreign key to `actor_refs` in the schema, for example `accounts.actor_id`, + `sessions.created_by_actor_id` and `messages.actor_id`, and checks all of + them. +- No other actor has the new ID. + +Thus a new installation gets the ID before it attributes work to the system +actor. After the change, system and MCP tokens act as the new ID, and +autonomous work is attributed to it. + +When a condition is false, the system actor keeps its local ID and Nerve logs +a warning at each start. Nerve does not change rows that refer to the local +ID. `nerve doctor` shows the difference. A value that is not a canonical +lower-case UUID also keeps the local ID and gives a warning. Local mode +ignores the variable. + +The schema trigger `system_actor_id_cannot_change` prevents all other changes +to the ID. The bootstrap removes the trigger, changes the ID and creates the +trigger again in one transaction. + ## Attribution Nerve can record two actor IDs for a change: diff --git a/docs/config.md b/docs/config.md index 604a3c49d..d292c23a1 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1857,6 +1857,23 @@ These requests have no header: memory model calls through the OpenAI-compatible client (the memory library makes that client and takes no headers), and Codex sessions. +### System principal ID + +The environment variable `NERVE_SYSTEM_PRINCIPAL_ID` gives the ID of the +agent's system principal. It has an effect only in external mode. It is not a +configuration key. `nerve init` and the server read it once, in the identity +bootstrap. + +| Value | Behavior | +|---|---| +| Unset or empty | The system actor keeps its ID. | +| A UUID in canonical lower-case form, for example `00000000-0000-4000-8000-000000000001` | The system actor gets this ID when no row refers to the system actor and no other actor has the ID. Otherwise the system actor keeps its local ID and Nerve logs a warning. | +| Any other value, also a UUID in upper case | The system actor keeps its ID and Nerve logs a warning. The server starts. | + +Set the variable before the first `nerve init` or start, so that the change +occurs before Nerve attributes work to the system actor. See +[The system actor in external mode](accounts.md#the-system-actor-in-external-mode). + ## Setup | Key | Type | Default | Description | diff --git a/nerve/migrate.py b/nerve/migrate.py index 9df9b6567..7fab785b1 100644 --- a/nerve/migrate.py +++ b/nerve/migrate.py @@ -63,9 +63,11 @@ tables; this step creates the first account, moves ``auth.password_hash`` onto the account row and out of the configuration files (:func:`_migrate_config_credentials`), and, if ``auth.jwt_secret`` is unset, -creates a signing secret. Only the gateway, at every start, and ``nerve init`` -run it. :func:`migrate` reports what the gateway will do and writes nothing to -``nerve.db``. Other CLI commands open the database with +creates a signing secret. In external mode it creates no account, and it gives +the system actor the ID in ``NERVE_SYSTEM_PRINCIPAL_ID`` +(:func:`_apply_system_principal`). Only the gateway, at every start, and +``nerve init`` run it. :func:`migrate` reports what the gateway will do and +writes nothing to ``nerve.db``. Other CLI commands open the database with :func:`open_production_db`, which loads the signing secret and creates no account. """ @@ -303,6 +305,9 @@ class MigrationReport: # `auth.password_hash` was (or would be) removed from the machine-local # configuration afterwards, because nothing reads it any more. scrubbed_config_password: bool = False + # The system actor got (or would get) the ID in NERVE_SYSTEM_PRINCIPAL_ID + # (see :func:`_apply_system_principal`). + rekeyed_system_actor: bool = False @property def did_anything(self) -> bool: @@ -319,6 +324,7 @@ def did_bootstrap(self) -> bool: or self.retired_stored_secret or self.migrated_config_credential or self.scrubbed_config_password + or self.rekeyed_system_actor ) @@ -1410,15 +1416,18 @@ async def bootstrap_identity( Results go to ``report``. With ``dry_run``, nothing is written. In external mode, the gateway names people, so no account is created or - changed. The signing secret is still made, because system and MCP tokens - need it. + changed. The system actor gets the ID in ``NERVE_SYSTEM_PRINCIPAL_ID`` + when it can (:func:`_apply_system_principal`). The signing secret is + still made, because system and MCP tokens need it. """ from nerve.gateway.auth import is_external_mode report = MigrationReport(dry_run=dry_run) if report is None else report if is_external_mode(): + await _apply_system_principal(db, report, dry_run=dry_run) await ensure_jwt_secret(db, config, report=report, dry_run=dry_run) return report + _note_ignored_system_principal() source = _credential_source_for(config) # Include accounts a dry run would create with the transitional source so @@ -1480,6 +1489,106 @@ async def bootstrap_identity( return report +def _rekey_action(previous_id: str | None, new_id: str, *, dry_run: bool) -> str: + """``previous_id`` is ``None`` when the system actor does not exist yet.""" + from nerve.identity import SYSTEM_PRINCIPAL_ENV + + if previous_id is None: + return f"give the new system actor the ID {new_id}, the ID in {SYSTEM_PRINCIPAL_ENV}" + verb = "change" if dry_run else "changed" + return ( + f"{verb} the system actor ID from {previous_id} to {new_id}, the ID in " + f"{SYSTEM_PRINCIPAL_ENV}" + ) + + +def _warn_kept_system_actor( + report: MigrationReport, current_id: str, reason: str, *, dry_run: bool, +) -> None: + warning = ( + f"the system actor keeps its local ID {current_id}: {reason}. Autonomous " + "work is attributed to the local ID. 'nerve doctor' shows this." + ) + report.warnings.append(warning) + if not dry_run: + logger.warning("Identity: %s", warning) + + +def _rekey_blocked_reason(new_id: str, blocker: str) -> str: + from nerve.identity import SYSTEM_PRINCIPAL_ENV + + return f"{SYSTEM_PRINCIPAL_ENV} names {new_id}, but {blocker}" + + +def _note_ignored_system_principal() -> None: + """Log that ``NERVE_SYSTEM_PRINCIPAL_ID`` has no effect in local mode.""" + from nerve.identity import SYSTEM_PRINCIPAL_ENV + + if os.environ.get(SYSTEM_PRINCIPAL_ENV, "").strip(): + logger.info( + "Identity: %s is set, but it has no effect in local mode; the system " + "actor keeps its local ID", SYSTEM_PRINCIPAL_ENV, + ) + + +async def _apply_system_principal( + db: "Database", report: MigrationReport, *, dry_run: bool, +) -> None: + """Give the system actor the ID of the agent's system principal. + + External mode only. The environment names the ID in + ``NERVE_SYSTEM_PRINCIPAL_ID``, which this reads once per bootstrap, so + once per start. When the value differs from the stored ID, the ID changes + only while no row refers to the system actor and no other actor has the + new ID (:meth:`~nerve.db.accounts.AccountStore._rekey_system_actor`). A + new installation therefore takes the ID at ``nerve init`` or at its first + start, before any work is attributed to it. + + In all other cases the local ID stays: an unset value says nothing, and + an invalid value, a referenced system actor, or a new ID that another + actor has gives one warning. ``nerve doctor`` reports the difference. + """ + from nerve.identity import InvalidSystemPrincipalId, system_principal_id_from_env + + current_id = db.system_actor_id + try: + new_id = system_principal_id_from_env() + except InvalidSystemPrincipalId as e: + _warn_kept_system_actor(report, current_id, str(e), dry_run=dry_run) + return + if new_id is None or new_id == current_id: + return + + if dry_run: + blocker = await db._system_actor_rekey_blocker(current_id, new_id) + if blocker is None: + report.rekeyed_system_actor = True + report.identity_actions.append(_rekey_action(current_id, new_id, dry_run=True)) + else: + _warn_kept_system_actor( + report, current_id, _rekey_blocked_reason(new_id, blocker), dry_run=True, + ) + return + + result = await db._rekey_system_actor(new_id) + if result.changed: + report.rekeyed_system_actor = True + report.identity_actions.append( + _rekey_action(result.previous_id, result.actor_id, dry_run=False) + ) + logger.info( + "Identity: the system actor ID changed from %s to %s", + result.previous_id, result.actor_id, + ) + elif result.blocker is not None: + _warn_kept_system_actor( + report, result.actor_id, _rekey_blocked_reason(new_id, result.blocker), + dry_run=False, + ) + # Otherwise the stored ID was already the new ID: another process changed + # it after this connection cached the system actor. + + async def ensure_jwt_secret( db: "Database", config: NerveConfig, @@ -1570,7 +1679,9 @@ def _inspect_identity(config: NerveConfig, db_path: Path, report: MigrationRepor state = inspect_bootstrap_state(db_path) sources, stored = state if state is not None else ([], False) - if not is_external_mode(): + if is_external_mode(): + _inspect_system_principal(db_path, report) + else: _inspect_accounts(config, sources, report) if config.auth.jwt_secret: if stored: @@ -1581,6 +1692,36 @@ def _inspect_identity(config: NerveConfig, db_path: Path, report: MigrationRepor report.identity_actions.append(_secret_action(dry_run=True)) +def _inspect_system_principal(db_path: Path, report: MigrationReport) -> None: + """Report the system actor step of :func:`bootstrap_identity`, read-only.""" + from nerve.db.accounts import inspect_system_actor + from nerve.identity import InvalidSystemPrincipalId, system_principal_id_from_env + + try: + new_id = system_principal_id_from_env() + except InvalidSystemPrincipalId as e: + report.warnings.append(f"the system actor keeps its local ID: {e}") + return + if new_id is None: + return + state = inspect_system_actor(db_path, new_id) + if state is None: + # No system actor yet: the next start creates one and gives it the ID. + report.rekeyed_system_actor = True + report.identity_actions.append(_rekey_action(None, new_id, dry_run=True)) + return + current_id, blocker = state + if current_id == new_id: + return + if blocker is None: + report.rekeyed_system_actor = True + report.identity_actions.append(_rekey_action(current_id, new_id, dry_run=True)) + else: + _warn_kept_system_actor( + report, current_id, _rekey_blocked_reason(new_id, blocker), dry_run=True, + ) + + def _inspect_accounts( config: NerveConfig, sources: list[str], report: MigrationReport, ) -> None: diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 428aff6c6..400fc5935 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -12,11 +12,16 @@ from __future__ import annotations import asyncio +import logging import sqlite3 +import httpx import pytest import pytest_asyncio +from fastapi import Depends, FastAPI +from nerve import paths +from nerve.config import NerveConfig, set_config from nerve.db import Database from nerve.db.accounts import ( SYSTEM_ACTOR_ID_TAKEN, @@ -25,18 +30,36 @@ inspect_system_actor, ) from nerve.db.migrations import v047_accounts +from nerve.gateway.auth import ( + AUTH_MODE_ENV, + AUTH_MODE_EXTERNAL, + MCP_AUDIENCE, + create_external_mcp_token, + create_mcp_session_token, + create_system_token, + decode_token, + pin_auth_mode, + require_auth, + resolve_actor_from_claims, +) from nerve.identity import ( SYSTEM_PRINCIPAL_ENV, Actor, InvalidSystemPrincipalId, parse_system_principal_id, ) +from nerve.migrate import MigrationReport, _inspect_identity, bootstrap_identity +_SECRET = "test-secret-for-system-principal-pad32" # Obviously fake IDs. PRINCIPAL = "00000000-0000-4000-8000-00000000c2c2" ALICE = "00000000-0000-4000-8000-0000000a11ce" +def _config() -> NerveConfig: + return NerveConfig.from_dict({"auth": {"jwt_secret": _SECRET}}) + + async def _open(path) -> Database: database = Database(path) await database.connect() @@ -83,6 +106,17 @@ async def _refer_to_system_actor(database: Database, reference: str) -> str: return "later_story.caused_by_actor_id" +def _warnings(caplog) -> list[str]: + return [r.getMessage() for r in caplog.records if r.levelno == logging.WARNING] + + +@pytest.fixture +def external(monkeypatch): + """External mode, as ``create_app()`` pins it.""" + monkeypatch.setenv(AUTH_MODE_ENV, AUTH_MODE_EXTERNAL) + pin_auth_mode(AUTH_MODE_EXTERNAL) + + @pytest.fixture def seeded(monkeypatch): """The environment names the system principal ID.""" @@ -262,3 +296,254 @@ async def test_the_read_only_inspection_gives_the_same_answer(self, tmp_path): assert inspect_system_actor(path, ALICE) == (local_id, SYSTEM_ACTOR_ID_TAKEN) finally: await database.close() + + +# --------------------------------------------------------------------------- # +# The identity bootstrap # +# --------------------------------------------------------------------------- # + + +@pytest.mark.asyncio +class TestBootstrap: + async def test_a_fresh_database_takes_the_id(self, database, external, seeded): + local_id = database.system_actor_id + report = await bootstrap_identity(database, _config()) + + assert report.rekeyed_system_actor and report.did_bootstrap + assert report.identity_actions[0] == ( + f"changed the system actor ID from {local_id} to {PRINCIPAL}, the ID in " + f"{SYSTEM_PRINCIPAL_ENV}" + ) + assert not report.warnings + assert database.system_actor_id == PRINCIPAL + assert await database.count_accounts() == 0 + + async def test_a_second_start_changes_nothing(self, database, external, seeded, caplog): + await bootstrap_identity(database, _config()) + with caplog.at_level(logging.INFO, logger="nerve.migrate"): + report = await bootstrap_identity(database, _config()) + assert not report.rekeyed_system_actor and not report.warnings + assert database.system_actor_id == PRINCIPAL + assert "system actor" not in caplog.text + + @pytest.mark.parametrize("reference", ["session", "cause", "message", "new_column"]) + async def test_a_referenced_system_actor_keeps_its_local_id( + self, database, external, seeded, caplog, reference, + ): + local_id = database.system_actor_id + column = await _refer_to_system_actor(database, reference) + + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + report = await bootstrap_identity(database, _config()) + + assert database.system_actor_id == local_id + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 + warning = report.warnings[0] + assert f"keeps its local ID {local_id}" in warning + assert f"{SYSTEM_PRINCIPAL_ENV} names {PRINCIPAL}" in warning + assert f"{column} refers to the current ID" in warning + assert _warnings(caplog) == [f"Identity: {warning}"] + + async def test_an_id_that_another_actor_has_keeps_the_local_id( + self, database, external, seeded, caplog, + ): + local_id = database.system_actor_id + await database.upsert_external_actor(PRINCIPAL, "Alice Example") + + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + report = await bootstrap_identity(database, _config()) + + assert database.system_actor_id == local_id + assert len(report.warnings) == 1 and SYSTEM_ACTOR_ID_TAKEN in report.warnings[0] + assert len(_warnings(caplog)) == 1 + + async def test_local_mode_ignores_the_variable(self, database, seeded, caplog): + local_id = database.system_actor_id + with caplog.at_level(logging.INFO, logger="nerve.migrate"): + report = await bootstrap_identity(database, _config()) + assert database.system_actor_id == local_id + assert not report.rekeyed_system_actor and not report.warnings + assert report.bootstrapped_account + assert "no effect in local mode" in caplog.text + assert _warnings(caplog) == [] + + @pytest.mark.parametrize("value", [PRINCIPAL.upper(), "not-a-uuid"]) + async def test_an_invalid_value_keeps_the_local_id( + self, database, external, monkeypatch, caplog, value, + ): + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, value) + local_id = database.system_actor_id + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + report = await bootstrap_identity(database, _config()) + assert database.system_actor_id == local_id + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 and "canonical" in report.warnings[0] + assert len(_warnings(caplog)) == 1 and local_id in _warnings(caplog)[0] + + async def test_no_variable_changes_nothing(self, database, external): + local_id = database.system_actor_id + report = await bootstrap_identity(database, _config()) + assert database.system_actor_id == local_id + assert not report.rekeyed_system_actor and not report.warnings + + async def test_a_dry_run_reports_and_writes_nothing(self, database, external, seeded): + local_id = database.system_actor_id + report = await bootstrap_identity(database, _config(), dry_run=True) + assert report.rekeyed_system_actor + assert report.identity_actions[0].startswith( + f"change the system actor ID from {local_id} to {PRINCIPAL}" + ) + assert database.system_actor_id == local_id + assert await _system_rows(database) == [local_id] + + async def test_two_starts_at_once_agree_and_warn_about_nothing( + self, tmp_path, external, seeded, + ): + first = await _open(tmp_path / "nerve.db") + second = await _open(tmp_path / "nerve.db") + try: + reports = await asyncio.gather( + bootstrap_identity(first, _config()), + bootstrap_identity(second, _config()), + ) + assert sorted(r.rekeyed_system_actor for r in reports) == [False, True] + assert not any(r.warnings for r in reports) + assert first.system_actor_id == second.system_actor_id == PRINCIPAL + finally: + await first.close() + await second.close() + + +# --------------------------------------------------------------------------- # +# Tokens # +# --------------------------------------------------------------------------- # + + +@pytest.mark.asyncio +class TestTokens: + async def test_system_and_mcp_tokens_resolve_to_the_new_id( + self, database, external, seeded, + ): + await bootstrap_identity(database, _config()) + claims = [ + decode_token(create_system_token(_SECRET), _SECRET), + decode_token( + create_mcp_session_token(_SECRET, "engine-sess-1"), _SECRET, + audience=MCP_AUDIENCE, + ), + decode_token(create_external_mcp_token(_SECRET), _SECRET, audience=MCP_AUDIENCE), + ] + for claim in claims: + actor = await resolve_actor_from_claims(database, claim) + assert actor.actor_id == PRINCIPAL and actor.is_system + + async def test_a_request_with_a_system_token_acts_as_the_new_id( + self, database, external, seeded, wire_identity_store, + ): + set_config(_config()) + try: + await bootstrap_identity(database, _config()) + wire_identity_store(database) + app = FastAPI() + + @app.get("/whoami") + async def whoami(actor: Actor = Depends(require_auth)): + return {"actor_id": actor.actor_id, "kind": actor.kind} + + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://nerve-test", + ) as client: + res = await client.get( + "/whoami", + headers={"Authorization": f"Bearer {create_system_token(_SECRET)}"}, + ) + assert res.status_code == 200 + assert res.json() == {"actor_id": PRINCIPAL, "kind": "system"} + finally: + set_config(NerveConfig()) + + +# --------------------------------------------------------------------------- # +# Entry points: nerve init, server start, the preview # +# --------------------------------------------------------------------------- # + + +class _ProxyThatFails: + """Stops the lifespan right after the identity and setup-token steps.""" + + async def start(self): + raise RuntimeError("stop after the identity steps") + + async def stop(self): + return None + + +class TestEntryPoints: + def test_nerve_init_takes_the_id(self, monkeypatch, seeded): + """``nerve init`` reads the mode and the ID from the environment.""" + from nerve.migrate import bootstrap_identity_sync + + monkeypatch.setenv(AUTH_MODE_ENV, AUTH_MODE_EXTERNAL) + report = bootstrap_identity_sync(NerveConfig()) + assert report.rekeyed_system_actor + assert inspect_system_actor(paths.db_path(), PRINCIPAL) == (PRINCIPAL, None) + + @pytest.mark.asyncio + async def test_the_server_start_takes_the_id_before_it_serves( + self, tmp_path, monkeypatch, seeded, + ): + import nerve.proxy.service as proxy_module + from nerve.gateway import server + + config = NerveConfig() + config.workspace = tmp_path / "ws" + config.workspace.mkdir() + config.proxy.enabled = True + config.mcp_endpoint.enabled = False + set_config(config) + try: + monkeypatch.setenv(AUTH_MODE_ENV, AUTH_MODE_EXTERNAL) + app = server.create_app() + monkeypatch.setattr(proxy_module, "ProxyService", lambda cfg: _ProxyThatFails()) + with pytest.raises(RuntimeError, match="stop after the identity steps"): + async with server.lifespan(app): + pass + finally: + set_config(NerveConfig()) + + assert inspect_system_actor(paths.db_path(), PRINCIPAL) == (PRINCIPAL, None) + + @pytest.mark.asyncio + async def test_the_preview_reports_what_the_next_start_does(self, external, seeded): + report = MigrationReport(dry_run=True) + _inspect_identity(NerveConfig(), paths.db_path(), report) + assert report.rekeyed_system_actor + assert report.identity_actions[0] == ( + f"give the new system actor the ID {PRINCIPAL}, the ID in {SYSTEM_PRINCIPAL_ENV}" + ) + + database = await _open(paths.db_path()) + try: + local_id = database.system_actor_id + report = MigrationReport(dry_run=True) + _inspect_identity(NerveConfig(), paths.db_path(), report) + assert report.rekeyed_system_actor + assert report.identity_actions[0].startswith( + f"change the system actor ID from {local_id} to {PRINCIPAL}" + ) + + column = await _refer_to_system_actor(database, "session") + report = MigrationReport(dry_run=True) + _inspect_identity(NerveConfig(), paths.db_path(), report) + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 and column in report.warnings[0] + finally: + await database.close() + + def test_the_preview_reports_an_invalid_value(self, external, monkeypatch): + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, "not-a-uuid") + report = MigrationReport(dry_run=True) + _inspect_identity(NerveConfig(), paths.db_path(), report) + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 and "canonical" in report.warnings[0] From 0287e46c3c377fe23a38ff7f4f76f94304f47fc6 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 19:57:03 +0000 Subject: [PATCH 3/9] Show the system principal ID state in nerve doctor When the system actor keeps its local ID, autonomous work is not attributed to the agent's system principal, and only the start log says so. nerve doctor now reads NERVE_SYSTEM_PRINCIPAL_ID in its own shell, as it does for NERVE_AUTH_MODE, and shows whether the system actor has that ID, whether the next start gives it the ID, or why it keeps its local ID. It reads nerve.db read-only. Co-Authored-By: Claude Opus 5.5 --- docs/config.md | 5 ++ nerve/cli.py | 42 ++++++++++++++++ tests/test_system_principal.py | 89 +++++++++++++++++++++++++++++++++- 3 files changed, 135 insertions(+), 1 deletion(-) diff --git a/docs/config.md b/docs/config.md index d292c23a1..3067eaa30 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1874,6 +1874,11 @@ Set the variable before the first `nerve init` or start, so that the change occurs before Nerve attributes work to the system actor. See [The system actor in external mode](accounts.md#the-system-actor-in-external-mode). +`nerve doctor` reads the variable in its own shell, as it does for +`NERVE_AUTH_MODE`. It shows one of these: the system actor has the ID, the +next start gives it the ID, or the system actor keeps its local ID, with the +reason. + ## Setup | Key | Type | Default | Description | diff --git a/nerve/cli.py b/nerve/cli.py index 66012a7b4..7c0c84039 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -1574,6 +1574,44 @@ def _telegram_sync_lines(config, warnings: list[str]) -> list[str]: return lines +def _system_principal_report(*, external: bool) -> tuple[list[str], list[str]]: + """Doctor lines about ``NERVE_SYSTEM_PRINCIPAL_ID``: ``(lines, warnings)``. + + Like the auth mode, the value comes from this shell, not from the running + server. Nothing is reported when the variable is unset here. + """ + from nerve.db.accounts import inspect_system_actor + from nerve.identity import ( + SYSTEM_PRINCIPAL_ENV, + InvalidSystemPrincipalId, + system_principal_id_from_env, + ) + + label = f"System actor ({SYSTEM_PRINCIPAL_ENV} from this shell)" + if not external: + if os.environ.get(SYSTEM_PRINCIPAL_ENV, "").strip(): + return [f"[--] {label}: set, but it has no effect in local mode"], [] + return [], [] + try: + wanted = system_principal_id_from_env() + except InvalidSystemPrincipalId as e: + return [], [f"[WARN] {label}: {e}. The server keeps the local system actor ID."] + if wanted is None: + return [], [] + state = inspect_system_actor(paths.db_path(), wanted) + if state is None: + return [f"[--] {label}: no system actor yet; the next start gives it {wanted}"], [] + current, blocker = state + if current == wanted: + return [f"[OK] {label}: {current}"], [] + if blocker is None: + return [f"[--] {label}: {current}; the next start changes it to {wanted}"], [] + return [], [ + f"[WARN] {label}: keeps the local ID {current}, not {wanted}: {blocker}. " + "Autonomous work is attributed to the local ID." + ] + + def doctor_report(config, config_source: str = "", check_api: bool = False) -> str: """Run doctor checks and return the report as a plain-text string. @@ -2012,6 +2050,10 @@ def doctor_report(config, config_source: str = "", check_api: bool = False) -> s "[WARN] auth.password_hash is set but no account uses it — every " "account has its own password now; the configured value does nothing" ) + if mode is not None: + ok, warn = _system_principal_report(external=external) + lines.extend(ok) + warnings.extend(warn) # The setup checklist: the same derivation the setup page uses, over the # facts this process can read. Never prints the token — `nerve status` diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 400fc5935..24cd8fc96 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -21,7 +21,7 @@ from fastapi import Depends, FastAPI from nerve import paths -from nerve.config import NerveConfig, set_config +from nerve.config import NerveConfig, load_config, set_config from nerve.db import Database from nerve.db.accounts import ( SYSTEM_ACTOR_ID_TAKEN, @@ -547,3 +547,90 @@ def test_the_preview_reports_an_invalid_value(self, external, monkeypatch): _inspect_identity(NerveConfig(), paths.db_path(), report) assert not report.rekeyed_system_actor assert len(report.warnings) == 1 and "canonical" in report.warnings[0] + + +# --------------------------------------------------------------------------- # +# nerve doctor # +# --------------------------------------------------------------------------- # + + +class TestDoctor: + @staticmethod + def _config(tmp_path) -> NerveConfig: + config_dir, workspace = tmp_path / "cfg", tmp_path / "ws" + config_dir.mkdir() + (workspace / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text(f"workspace: {workspace}\n", encoding="utf-8") + (config_dir / "config.local.yaml").write_text("{}\n", encoding="utf-8") + return load_config(config_dir) + + @staticmethod + def _report(*, external: bool) -> tuple[list[str], list[str]]: + from nerve.cli import _system_principal_report + + return _system_principal_report(external=external) + + def test_nothing_is_reported_without_the_variable(self): + assert self._report(external=True) == ([], []) + assert self._report(external=False) == ([], []) + + @pytest.mark.parametrize("value", [PRINCIPAL, "not-a-uuid"]) + def test_local_mode_says_the_variable_has_no_effect(self, monkeypatch, value): + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, value) + lines, warnings = self._report(external=False) + assert warnings == [] + assert len(lines) == 1 and lines[0].startswith("[--]") + assert "no effect in local mode" in lines[0] + + def test_an_invalid_value_is_a_warning(self, monkeypatch): + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, "not-a-uuid") + lines, warnings = self._report(external=True) + assert lines == [] + assert len(warnings) == 1 and warnings[0].startswith("[WARN]") + assert "canonical" in warnings[0] and "keeps the local system actor ID" in warnings[0] + + def test_no_database_yet(self, seeded): + lines, warnings = self._report(external=True) + assert warnings == [] + assert lines == [ + f"[--] System actor ({SYSTEM_PRINCIPAL_ENV} from this shell): no system " + f"actor yet; the next start gives it {PRINCIPAL}" + ] + + @pytest.mark.asyncio + async def test_a_pending_change_and_a_kept_local_id(self, seeded): + database = await _open(paths.db_path()) + try: + local_id = database.system_actor_id + lines, warnings = self._report(external=True) + assert warnings == [] + assert lines == [ + f"[--] System actor ({SYSTEM_PRINCIPAL_ENV} from this shell): " + f"{local_id}; the next start changes it to {PRINCIPAL}" + ] + + column = await _refer_to_system_actor(database, "session") + lines, warnings = self._report(external=True) + assert lines == [] + assert len(warnings) == 1 and warnings[0].startswith("[WARN]") + assert f"keeps the local ID {local_id}, not {PRINCIPAL}" in warnings[0] + assert f"{column} refers to the current ID" in warnings[0] + finally: + await database.close() + + @pytest.mark.asyncio + async def test_the_doctor_report_shows_the_id_in_force( + self, tmp_path, external, seeded, + ): + from nerve.cli import doctor_report + + database = await _open(paths.db_path()) + try: + await bootstrap_identity(database, _config()) + finally: + await database.close() + report = doctor_report(self._config(tmp_path)) + assert ( + f"[OK] System actor ({SYSTEM_PRINCIPAL_ENV} from this shell): {PRINCIPAL}" + in report + ) From 41d6aac4f85bf4961ccf0e3a54354126c75d6751 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:31:38 +0000 Subject: [PATCH 4/9] Keep the system actor ID coherent across connections Two processes that start at the same time with different values could both change the ID. The first process then kept a cached ID that no longer existed, and its next write attributed to the system actor failed the foreign key. A connection now changes the ID only while the stored ID is still the ID it cached. Otherwise it keeps the stored ID, refreshes its cache and logs a warning. The checks compare with COLLATE BINARY, as the foreign key does, so a column declared with a collation that this connection does not register cannot stop the start. A check that cannot run keeps the ID. The read-only inspection and the dry run read one snapshot, so a concurrent change cannot give a mixed answer. Co-Authored-By: Claude Opus 5.5 --- docs/accounts.md | 13 ++- docs/config.md | 2 +- nerve/db/accounts.py | 139 +++++++++++++++++-------- nerve/db/base.py | 17 ++++ nerve/migrate.py | 38 +++---- tests/test_system_principal.py | 179 ++++++++++++++++++++++++++++++++- 6 files changed, 315 insertions(+), 73 deletions(-) diff --git a/docs/accounts.md b/docs/accounts.md index 7c237337e..74d25df26 100644 --- a/docs/accounts.md +++ b/docs/accounts.md @@ -180,22 +180,25 @@ ID. In external mode, the environment can give the ID of the agent's system principal in `NERVE_SYSTEM_PRINCIPAL_ID` (see [System principal ID](config.md#system-principal-id)). The identity bootstrap of `nerve init` and of each server start then gives the system actor that ID -when these two conditions are true: +when these conditions are true: - No row refers to the system actor. Nerve finds each column that has a foreign key to `actor_refs` in the schema, for example `accounts.actor_id`, `sessions.created_by_actor_id` and `messages.actor_id`, and checks all of - them. + them. A check that cannot run counts as a row. - No other actor has the new ID. +- No other process changed the ID after this process opened the database. + When two processes start at the same time with different values, the first + change stays. Thus a new installation gets the ID before it attributes work to the system actor. After the change, system and MCP tokens act as the new ID, and autonomous work is attributed to it. -When a condition is false, the system actor keeps its local ID and Nerve logs -a warning at each start. Nerve does not change rows that refer to the local +When a condition is false, the system actor keeps its current ID and Nerve +logs a warning at each start. Nerve does not change rows that refer to that ID. `nerve doctor` shows the difference. A value that is not a canonical -lower-case UUID also keeps the local ID and gives a warning. Local mode +lower-case UUID also keeps the current ID and gives a warning. Local mode ignores the variable. The schema trigger `system_actor_id_cannot_change` prevents all other changes diff --git a/docs/config.md b/docs/config.md index 3067eaa30..822c3c949 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1867,7 +1867,7 @@ bootstrap. | Value | Behavior | |---|---| | Unset or empty | The system actor keeps its ID. | -| A UUID in canonical lower-case form, for example `00000000-0000-4000-8000-000000000001` | The system actor gets this ID when no row refers to the system actor and no other actor has the ID. Otherwise the system actor keeps its local ID and Nerve logs a warning. | +| A UUID in canonical lower-case form, for example `00000000-0000-4000-8000-000000000001` | The system actor gets this ID when no row refers to the system actor, no other actor has the ID, and no other process changed the ID first. Otherwise the system actor keeps its current ID and Nerve logs a warning. See [The system actor in external mode](accounts.md#the-system-actor-in-external-mode). | | Any other value, also a UUID in upper case | The system actor keeps its ID and Nerve logs a warning. The server starts. | Set the variable before the first `nerve init` or start, so that the change diff --git a/nerve/db/accounts.py b/nerve/db/accounts.py index d5b9715a6..2493bfab3 100644 --- a/nerve/db/accounts.py +++ b/nerve/db/accounts.py @@ -166,9 +166,11 @@ class BootstrapAccount: @dataclass(frozen=True) class SystemActorRekey: - """The result of :meth:`AccountStore._rekey_system_actor`.""" + """The result of :meth:`AccountStore._rekey_system_actor` or of + :meth:`AccountStore._preview_system_actor_rekey`.""" - # The stored ID of the system actor before and after the call. + # The stored ID of the system actor before the call, and the ID after it. + # For a preview, ``actor_id`` is the ID that the change would give. previous_id: str actor_id: str # Why the ID did not change, or ``None``. @@ -183,23 +185,44 @@ def _quote_identifier(name: str) -> str: return '"' + name.replace('"', '""') + '"' +@dataclass(frozen=True) +class _Probe: + """One check that must find no row before the system actor gets a new ID.""" + + sql: str + params: tuple[str] + # The checked column, as ``table.column``. + label: str + # Why the ID cannot change when the query returns a row. + reason: str + + def failure(self, error: Exception) -> str: + """Why the ID cannot change when the query cannot run.""" + return f"the check of {self.label} could not run ({error})" + + def _rekey_probes( columns: list[tuple[str, str]], current_id: str, new_id: str, -) -> list[tuple[str, tuple[str], str]]: +) -> list[_Probe]: """The checks that must all find no row before the system actor gets a new ID. - Each item is ``(sql, params, reason)``. The reason applies when the query - returns a row. ``columns`` comes from :data:`ACTOR_REF_COLUMNS_SQL`, so - the names are from the schema, never from a caller. + ``columns`` comes from :data:`ACTOR_REF_COLUMNS_SQL`, so the names are + from the schema, never from a caller. Each comparison uses ``COLLATE + BINARY``, as the foreign key does with ``actor_refs.id``, so the declared + collation of a column is never loaded. """ probes = [ - ("SELECT 1 FROM actor_refs WHERE id = ? LIMIT 1", (new_id,), SYSTEM_ACTOR_ID_TAKEN), + _Probe( + "SELECT 1 FROM actor_refs WHERE id COLLATE BINARY = ? LIMIT 1", + (new_id,), "actor_refs.id", SYSTEM_ACTOR_ID_TAKEN, + ), ] for table, column in columns: - probes.append(( + probes.append(_Probe( f"SELECT 1 FROM {_quote_identifier(table)} " - f"WHERE {_quote_identifier(column)} = ? LIMIT 1", + f"WHERE {_quote_identifier(column)} COLLATE BINARY = ? LIMIT 1", (current_id,), + f"{table}.{column}", f"{table}.{column} refers to the current ID", )) return probes @@ -291,21 +314,51 @@ async def _system_actor_rekey_blocker( """Why the system actor cannot change from ``current_id`` to ``new_id``. ``None`` when no actor has ``new_id`` and no column with a foreign key - to ``actor_refs`` holds ``current_id``. + to ``actor_refs`` holds ``current_id``. A check that cannot run is a + reason too, so the ID changes only when every check found no row. """ - for sql, params, reason in _rekey_probes( - await self._actor_ref_columns(), current_id, new_id, - ): - async with self.db.execute(sql, params) as cursor: - if await cursor.fetchone() is not None: - return reason + for probe in _rekey_probes(await self._actor_ref_columns(), current_id, new_id): + try: + async with self.db.execute(probe.sql, probe.params) as cursor: + found = await cursor.fetchone() is not None + except sqlite3.Error as e: + return probe.failure(e) + if found: + return probe.reason return None + async def _decide_system_actor_rekey(self, new_id: str) -> tuple[str, str | None]: + """Read the stored system actor ID and decide whether it can become ``new_id``. + + Call inside a transaction. Returns ``(stored_id, blocker)``. The stored + ID must still be the ID that this connection has cached: when another + connection changed it, this one does not change it again, because the + other connection uses the ID it set. + """ + async with self.db.execute( + "SELECT id FROM actor_refs WHERE kind = 'system'" + ) as cursor: + row = await cursor.fetchone() + if row is None: + from nerve.db.base import IdentityInvariantError + + raise IdentityInvariantError("actor identity is corrupt: there is no system actor") + stored_id = row[0] + if stored_id == new_id: + return stored_id, None + cached_id = self.system_actor_id + if stored_id != cached_id: + return stored_id, ( + f"another process changed the ID from {cached_id} to {stored_id} " + "after this process read it" + ) + return stored_id, await self._system_actor_rekey_blocker(stored_id, new_id) + async def _rekey_system_actor(self, new_id: str) -> SystemActorRekey: """Give the system actor ``new_id`` while no row refers to it. - One transaction reads the stored ID again, checks - :meth:`_system_actor_rekey_blocker`, drops + One transaction reads the stored ID again, checks it with + :meth:`_decide_system_actor_rekey`, drops ``system_actor_id_cannot_change``, changes the ID, and creates the trigger again with :data:`SYSTEM_ACTOR_ID_TRIGGER_SQL`. A failure rolls back all of it, so no other transaction sees the table without the @@ -313,32 +366,24 @@ async def _rekey_system_actor(self, new_id: str) -> SystemActorRekey: it. The cached :attr:`system_actor` is read again in all cases. """ async with self._atomic(): - async with self.db.execute( - "SELECT id FROM actor_refs WHERE kind = 'system'" - ) as cursor: - row = await cursor.fetchone() - if row is None: - from nerve.db.base import IdentityInvariantError - - raise IdentityInvariantError( - "actor identity is corrupt: there is no system actor" + previous_id, blocker = await self._decide_system_actor_rekey(new_id) + if previous_id != new_id and blocker is None: + await self.db.execute(f"DROP TRIGGER IF EXISTS {SYSTEM_ACTOR_ID_TRIGGER}") + await self.db.execute( + "UPDATE actor_refs SET id = ? WHERE id = ? AND kind = 'system'", + (new_id, previous_id), ) - previous_id = row[0] - blocker = None - if previous_id != new_id: - blocker = await self._system_actor_rekey_blocker(previous_id, new_id) - if blocker is None: - await self.db.execute( - f"DROP TRIGGER IF EXISTS {SYSTEM_ACTOR_ID_TRIGGER}" - ) - await self.db.execute( - "UPDATE actor_refs SET id = ? WHERE id = ? AND kind = 'system'", - (new_id, previous_id), - ) - await self.db.execute(SYSTEM_ACTOR_ID_TRIGGER_SQL) + await self.db.execute(SYSTEM_ACTOR_ID_TRIGGER_SQL) await self._cache_system_actor() return SystemActorRekey(previous_id, self.system_actor_id, blocker) + async def _preview_system_actor_rekey(self, new_id: str) -> SystemActorRekey: + """What :meth:`_rekey_system_actor` would do, from one snapshot, with no write.""" + async with self._read_snapshot(): + previous_id, blocker = await self._decide_system_actor_rekey(new_id) + actor_id = new_id if blocker is None else previous_id + return SystemActorRekey(previous_id, actor_id, blocker) + # -- accounts ------------------------------------------------------------ async def get_account(self, account_id: str) -> dict | None: @@ -883,12 +928,14 @@ def inspect_system_actor(db_path: Path, wanted_id: str) -> tuple[str, str | None Returns ``(actor_id, blocker)``. ``blocker`` is ``None`` when the stored ID is ``wanted_id`` or when the identity bootstrap can change it (see ``AccountStore._system_actor_rekey_blocker``). Returns ``None`` when the - database does not exist, has no system actor yet, or cannot be read. + database does not exist, has no system actor yet, or cannot be read. All + reads use one snapshot, so a concurrent change gives a consistent answer. """ conn = _read_only(db_path) if conn is None: return None try: + conn.execute("BEGIN") rows = conn.execute("SELECT id FROM actor_refs WHERE kind = 'system'").fetchall() if len(rows) != 1: return None @@ -896,9 +943,13 @@ def inspect_system_actor(db_path: Path, wanted_id: str) -> tuple[str, str | None if current_id == wanted_id: return current_id, None columns = [(row[0], row[1]) for row in conn.execute(ACTOR_REF_COLUMNS_SQL)] - for sql, params, reason in _rekey_probes(columns, current_id, wanted_id): - if conn.execute(sql, params).fetchone() is not None: - return current_id, reason + for probe in _rekey_probes(columns, current_id, wanted_id): + try: + found = conn.execute(probe.sql, probe.params).fetchone() is not None + except sqlite3.Error as e: + return current_id, probe.failure(e) + if found: + return current_id, probe.reason return current_id, None except sqlite3.Error: return None diff --git a/nerve/db/base.py b/nerve/db/base.py index 167c43491..537b184c1 100644 --- a/nerve/db/base.py +++ b/nerve/db/base.py @@ -588,6 +588,23 @@ async def _atomic(self) -> AsyncIterator[None]: await self._rollback_quietly() raise + @asynccontextmanager + async def _read_snapshot(self) -> AsyncIterator[None]: + """Run several reads against one snapshot of the database. + + A deferred BEGIN takes no write lock: the snapshot starts at the first + read and lasts until the rollback at the end. The body must not write. + The write lock of this connection is held, so no write on this + connection can commit or end the transaction while the body runs. + """ + async with self._write_lock: + await self._heal_leaked_txn() + try: + await self.db.execute("BEGIN") + yield + finally: + await self._rollback_quietly() + async def _write(self, sql: str, params: tuple | list = ()) -> WriteResult: """Execute one write statement and commit, under the write lock. diff --git a/nerve/migrate.py b/nerve/migrate.py index 7fab785b1..1f5f7f531 100644 --- a/nerve/migrate.py +++ b/nerve/migrate.py @@ -1506,8 +1506,8 @@ def _warn_kept_system_actor( report: MigrationReport, current_id: str, reason: str, *, dry_run: bool, ) -> None: warning = ( - f"the system actor keeps its local ID {current_id}: {reason}. Autonomous " - "work is attributed to the local ID. 'nerve doctor' shows this." + f"the system actor keeps its current ID {current_id}: {reason}. Autonomous " + "work is attributed to that ID. 'nerve doctor' shows this." ) report.warnings.append(warning) if not dry_run: @@ -1544,9 +1544,10 @@ async def _apply_system_principal( new installation therefore takes the ID at ``nerve init`` or at its first start, before any work is attributed to it. - In all other cases the local ID stays: an unset value says nothing, and - an invalid value, a referenced system actor, or a new ID that another - actor has gives one warning. ``nerve doctor`` reports the difference. + In all other cases the current ID stays: an unset value says nothing, and + an invalid value, a referenced system actor, a new ID that another actor + has, or an ID that another process changed first gives one warning. + ``nerve doctor`` reports the difference. """ from nerve.identity import InvalidSystemPrincipalId, system_principal_id_from_env @@ -1560,30 +1561,23 @@ async def _apply_system_principal( return if dry_run: - blocker = await db._system_actor_rekey_blocker(current_id, new_id) - if blocker is None: - report.rekeyed_system_actor = True - report.identity_actions.append(_rekey_action(current_id, new_id, dry_run=True)) - else: - _warn_kept_system_actor( - report, current_id, _rekey_blocked_reason(new_id, blocker), dry_run=True, - ) - return - - result = await db._rekey_system_actor(new_id) + result = await db._preview_system_actor_rekey(new_id) + else: + result = await db._rekey_system_actor(new_id) if result.changed: report.rekeyed_system_actor = True report.identity_actions.append( - _rekey_action(result.previous_id, result.actor_id, dry_run=False) - ) - logger.info( - "Identity: the system actor ID changed from %s to %s", - result.previous_id, result.actor_id, + _rekey_action(result.previous_id, result.actor_id, dry_run=dry_run) ) + if not dry_run: + logger.info( + "Identity: the system actor ID changed from %s to %s", + result.previous_id, result.actor_id, + ) elif result.blocker is not None: _warn_kept_system_actor( report, result.actor_id, _rekey_blocked_reason(new_id, result.blocker), - dry_run=False, + dry_run=dry_run, ) # Otherwise the stored ID was already the new ID: another process changed # it after this connection cached the system actor. diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 24cd8fc96..344f3b4b9 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -106,6 +106,19 @@ async def _refer_to_system_actor(database: Database, reference: str) -> str: return "later_story.caused_by_actor_id" +def _rekey_from_another_connection(path, new_id: str) -> None: + """Change the system actor ID as another process would, and commit.""" + other = sqlite3.connect(path) + try: + other.execute("BEGIN IMMEDIATE") + other.execute(f"DROP TRIGGER {SYSTEM_ACTOR_ID_TRIGGER}") + other.execute("UPDATE actor_refs SET id = ? WHERE kind = 'system'", (new_id,)) + other.execute(SYSTEM_ACTOR_ID_TRIGGER_SQL) + other.commit() + finally: + other.close() + + def _warnings(caplog) -> list[str]: return [r.getMessage() for r in caplog.records if r.levelno == logging.WARNING] @@ -262,6 +275,146 @@ async def test_two_connections_agree_on_the_new_id(self, tmp_path): await first.close() await second.close() + async def test_a_connection_does_not_change_an_id_that_another_one_set(self, tmp_path): + """Both connections cached the local ID. The second one asks for another + ID after the first changed it: it keeps the first one's ID, so the ID + that each connection uses still exists.""" + first = await _open(tmp_path / "nerve.db") + second = await _open(tmp_path / "nerve.db") + try: + local_id = first.system_actor_id + assert (await first._rekey_system_actor(PRINCIPAL)).changed + + result = await second._rekey_system_actor(ALICE) + + assert not result.changed + assert (result.previous_id, result.actor_id) == (PRINCIPAL, PRINCIPAL) + assert result.blocker == ( + f"another process changed the ID from {local_id} to {PRINCIPAL} " + "after this process read it" + ) + assert first.system_actor_id == second.system_actor_id == PRINCIPAL + assert await _system_rows(first) == [PRINCIPAL] + # Both connections can still write rows attributed to the system actor. + await first.create_session("s-first", actor=first.system_actor) + await second.create_session("s-second", actor=second.system_actor) + finally: + await first.close() + await second.close() + + async def test_concurrent_changes_to_different_ids_leave_one_live_id(self, tmp_path): + first = await _open(tmp_path / "nerve.db") + second = await _open(tmp_path / "nerve.db") + try: + results = await asyncio.gather( + first._rekey_system_actor(PRINCIPAL), + second._rekey_system_actor(ALICE), + ) + assert sorted(r.changed for r in results) == [False, True] + winner = next(r.actor_id for r in results if r.changed) + loser = next(r for r in results if not r.changed) + assert loser.blocker.startswith("another process changed the ID") + assert first.system_actor_id == second.system_actor_id == winner + assert await _system_rows(first) == [winner] + await first.create_session("s-first", actor=first.system_actor) + await second.create_session("s-second", actor=second.system_actor) + finally: + await first.close() + await second.close() + + @pytest.mark.parametrize("referenced", [False, True]) + async def test_a_column_collation_that_is_not_registered_does_not_stop_the_check( + self, database, referenced, + ): + """The probe compares with ``COLLATE BINARY``, so a column declared with + a collation that only another connection registers can be checked.""" + local_id = database.system_actor_id + other = sqlite3.connect(database.db_path) + try: + other.create_collation("app_order", lambda a, b: (a > b) - (a < b)) + other.execute( + "CREATE TABLE extension (who TEXT COLLATE app_order " + "REFERENCES actor_refs(id))" + ) + if referenced: + other.execute("INSERT INTO extension (who) VALUES (?)", (local_id,)) + other.commit() + finally: + other.close() + + assert inspect_system_actor(database.db_path, PRINCIPAL) == ( + local_id, "extension.who refers to the current ID" if referenced else None, + ) + result = await database._rekey_system_actor(PRINCIPAL) + if referenced: + assert result.blocker == "extension.who refers to the current ID" + assert database.system_actor_id == local_id + else: + assert result.changed and database.system_actor_id == PRINCIPAL + + async def test_a_check_that_cannot_run_keeps_the_id(self, database, monkeypatch): + import nerve.db.accounts as accounts + + local_id = database.system_actor_id + real = accounts._rekey_probes + + def with_a_broken_probe(columns, current_id, new_id): + broken = accounts._Probe( + "SELECT 1 FROM missing_table WHERE who = ?", (current_id,), + "missing_table.who", "unused", + ) + return [*real(columns, current_id, new_id), broken] + + monkeypatch.setattr(accounts, "_rekey_probes", with_a_broken_probe) + + current, blocker = inspect_system_actor(database.db_path, PRINCIPAL) + assert current == local_id + assert blocker.startswith("the check of missing_table.who could not run (") + result = await database._rekey_system_actor(PRINCIPAL) + assert result.blocker.startswith("the check of missing_table.who could not run (") + assert database.system_actor_id == local_id + assert await _system_rows(database) == [local_id] + + async def test_the_read_only_inspection_uses_one_snapshot(self, database, monkeypatch): + """Another process changes the ID while the inspection runs. The answer + is the state before that change, not a mix of the two states.""" + import nerve.db.accounts as accounts + + local_id = database.system_actor_id + real = accounts._rekey_probes + + def probes_after_a_concurrent_change(columns, current_id, new_id): + _rekey_from_another_connection(database.db_path, new_id) + return real(columns, current_id, new_id) + + monkeypatch.setattr(accounts, "_rekey_probes", probes_after_a_concurrent_change) + + assert inspect_system_actor(database.db_path, PRINCIPAL) == (local_id, None) + + async def test_the_preview_uses_one_snapshot_and_writes_nothing( + self, database, monkeypatch, + ): + import nerve.db.accounts as accounts + + local_id = database.system_actor_id + real = accounts._rekey_probes + + def probes_after_a_concurrent_change(columns, current_id, new_id): + _rekey_from_another_connection(database.db_path, new_id) + return real(columns, current_id, new_id) + + monkeypatch.setattr(accounts, "_rekey_probes", probes_after_a_concurrent_change) + + result = await database._preview_system_actor_rekey(PRINCIPAL) + + assert (result.previous_id, result.actor_id, result.blocker) == ( + local_id, PRINCIPAL, None, + ) + assert not database.db.in_transaction + # The ID that the other connection set, not one that the preview wrote. + assert await _system_rows(database) == [PRINCIPAL] + assert database.system_actor_id == local_id + async def test_the_checked_columns_come_from_the_schema(self, database): found = set(await database._actor_ref_columns()) assert { @@ -340,7 +493,7 @@ async def test_a_referenced_system_actor_keeps_its_local_id( assert not report.rekeyed_system_actor assert len(report.warnings) == 1 warning = report.warnings[0] - assert f"keeps its local ID {local_id}" in warning + assert f"keeps its current ID {local_id}" in warning assert f"{SYSTEM_PRINCIPAL_ENV} names {PRINCIPAL}" in warning assert f"{column} refers to the current ID" in warning assert _warnings(caplog) == [f"Identity: {warning}"] @@ -414,6 +567,30 @@ async def test_two_starts_at_once_agree_and_warn_about_nothing( await first.close() await second.close() + async def test_a_start_that_asks_for_another_id_keeps_the_one_already_set( + self, tmp_path, external, monkeypatch, caplog, + ): + first = await _open(tmp_path / "nerve.db") + second = await _open(tmp_path / "nerve.db") + try: + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, PRINCIPAL) + assert (await bootstrap_identity(first, _config())).rekeyed_system_actor + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, ALICE) + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + report = await bootstrap_identity(second, _config()) + + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 + assert f"keeps its current ID {PRINCIPAL}" in report.warnings[0] + assert "another process changed the ID" in report.warnings[0] + assert len(_warnings(caplog)) == 1 + assert first.system_actor_id == second.system_actor_id == PRINCIPAL + await second.create_session("s-second", actor=second.system_actor) + finally: + await first.close() + await second.close() + # --------------------------------------------------------------------------- # # Tokens # From b3289f0c42c3e085d14ec7b62b79b0ab7172c0ae Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:31:43 +0000 Subject: [PATCH 5/9] Read the system principal ID once per start nerve start runs the identity preview of the config migration before it serves, and with --foreground the server runs in the same process. The preview read NERVE_SYSTEM_PRINCIPAL_ID and logged a warning, and the identity bootstrap then read it again and logged the same warning. The preview of nerve start now leaves out the system actor step, so the start reads the variable once and logs its result once. nerve migrate and nerve upgrade, which do not start the server, still show the step. Co-Authored-By: Claude Opus 5.5 --- nerve/cli.py | 4 +- nerve/migrate.py | 43 +++++++++++++--- tests/test_system_principal.py | 93 ++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 9 deletions(-) diff --git a/nerve/cli.py b/nerve/cli.py index 7c0c84039..d52b85f40 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -874,11 +874,13 @@ def start(ctx: click.Context, foreground: bool) -> None: # Migrate a legacy install to the workspace/config layout if needed # (idempotent, best-effort). Non-destructive — originals kept as *.migrated. - # The gateway runs the identity bootstrap when it starts. + # The gateway runs the identity bootstrap when it starts. It also reads + # NERVE_SYSTEM_PRINCIPAL_ID and logs the result, so the preview here does not. if config is not None: from nerve.migrate import maybe_migrate report = maybe_migrate( config_dir, workspace=getattr(config, "workspace", None), config=config, + preview_system_principal=False, ) if report is not None and report.did_anything: # The config in hand was loaded from the old locations, and migration diff --git a/nerve/migrate.py b/nerve/migrate.py index 1f5f7f531..1679b6d55 100644 --- a/nerve/migrate.py +++ b/nerve/migrate.py @@ -587,6 +587,8 @@ def migrate( legacy_cron_dir: Path | None = None, report: MigrationReport | None = None, config: NerveConfig | None = None, + *, + preview_system_principal: bool = True, ) -> MigrationReport: """Perform the migration for ``config_dir``. Idempotent; safe to re-run. @@ -601,7 +603,8 @@ def migrate( ``config`` is what the identity preview reads (:func:`_preview_identity`). If it is omitted, or the layout half moved files, the preview loads it from - ``config_dir``. + ``config_dir``. ``preview_system_principal`` false leaves the system actor + step out of the preview (see :func:`maybe_migrate`). """ config_dir = Path(config_dir) workspace = Path(workspace) if workspace is not None else _resolve_workspace(config_dir) @@ -610,7 +613,9 @@ def migrate( _migrate_config_yaml(config_dir, workspace, legacy_cron, report) _migrate_cron(workspace, legacy_cron, report) - _preview_identity(config_dir, config, report) + _preview_identity( + config_dir, config, report, system_principal=preview_system_principal, + ) return report @@ -921,6 +926,8 @@ def maybe_migrate( workspace: Path | None = None, legacy_cron_dir: Path | None = None, config: NerveConfig | None = None, + *, + preview_system_principal: bool = True, ) -> MigrationReport | None: """Run migration if needed. Best-effort: never raises (called on startup). @@ -931,6 +938,10 @@ def maybe_migrate( renamed away. Callers need that — ``nerve start`` has to reload the config it is holding, and ``nerve upgrade`` has to print the review prompt for a tracked file that now exists. + + ``nerve start`` passes ``preview_system_principal=False``: the start that + follows reads ``NERVE_SYSTEM_PRINCIPAL_ID`` once and logs its result once, + so the preview leaves that step out. """ report = MigrationReport(dry_run=False) try: @@ -941,6 +952,7 @@ def maybe_migrate( legacy_cron_dir=legacy_cron_dir, report=report, config=config, + preview_system_principal=preview_system_principal, ) except Exception as e: # noqa: BLE001 — must never break upgrade/startup report.error = str(e) @@ -1641,14 +1653,18 @@ async def ensure_jwt_secret( def _preview_identity( - config_dir: Path, config: NerveConfig | None, report: MigrationReport, + config_dir: Path, + config: NerveConfig | None, + report: MigrationReport, + *, + system_principal: bool = True, ) -> None: """Report what the gateway's identity bootstrap will do at its next start. Only the gateway and ``nerve init`` write identity state, so this reads ``nerve.db`` read-only and never creates it. Skipped on a fresh install (no ``config.local.yaml``): the wizard has not yet written the password - and secret. + and secret. ``system_principal`` is passed to :func:`_inspect_identity`. """ from nerve.bootstrap import is_fresh_install @@ -1663,18 +1679,29 @@ def _preview_identity( f"identity bootstrap not shown: the config could not be loaded ({e})" ) return - _inspect_identity(config, paths.db_path(), report) + _inspect_identity(config, paths.db_path(), report, system_principal=system_principal) -def _inspect_identity(config: NerveConfig, db_path: Path, report: MigrationReport) -> None: - """Report what :func:`bootstrap_identity` would do, reading nerve.db read-only.""" +def _inspect_identity( + config: NerveConfig, + db_path: Path, + report: MigrationReport, + *, + system_principal: bool = True, +) -> None: + """Report what :func:`bootstrap_identity` would do, reading nerve.db read-only. + + With ``system_principal`` false, the system actor step is not reported and + ``NERVE_SYSTEM_PRINCIPAL_ID`` is not read. + """ from nerve.db.accounts import inspect_bootstrap_state from nerve.gateway.auth import is_external_mode state = inspect_bootstrap_state(db_path) sources, stored = state if state is not None else ([], False) if is_external_mode(): - _inspect_system_principal(db_path, report) + if system_principal: + _inspect_system_principal(db_path, report) else: _inspect_accounts(config, sources, report) if config.auth.jwt_secret: diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 344f3b4b9..55c148c27 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -725,6 +725,99 @@ def test_the_preview_reports_an_invalid_value(self, external, monkeypatch): assert not report.rekeyed_system_actor assert len(report.warnings) == 1 and "canonical" in report.warnings[0] + @staticmethod + def _install(tmp_path): + """A configured install whose gateway starts the proxy, so that a + failing proxy stops the start right after the identity steps.""" + config_dir, workspace = tmp_path / "cfg", tmp_path / "ws" + config_dir.mkdir() + (workspace / "config").mkdir(parents=True) + (config_dir / "config.yaml").write_text( + f"workspace: {workspace}\n" + "proxy:\n enabled: true\n" + "mcp_endpoint:\n enabled: false\n", + encoding="utf-8", + ) + (config_dir / "config.local.yaml").write_text("{}\n", encoding="utf-8") + return config_dir + + def test_the_start_preview_leaves_out_the_system_actor_step( + self, tmp_path, external, seeded, + ): + from nerve.migrate import maybe_migrate + + config_dir = self._install(tmp_path) + start = maybe_migrate(config_dir, preview_system_principal=False) + assert not start.rekeyed_system_actor + assert not any("system actor" in a for a in start.identity_actions) + # Without the flag (nerve upgrade), the preview shows the step. + upgrade = maybe_migrate(config_dir) + assert upgrade.rekeyed_system_actor + + def test_a_foreground_start_reads_the_variable_once_and_warns_once( + self, tmp_path, monkeypatch, seeded, + ): + from click.testing import CliRunner + + import nerve.identity as identity + import nerve.migrate as migrate_module + import nerve.proxy.service as proxy_module + from nerve import cli + from nerve.gateway import server + + config_dir = self._install(tmp_path) + + async def refer_to_the_system_actor(): + database = await _open(paths.db_path()) + try: + await _refer_to_system_actor(database, "session") + finally: + await database.close() + + asyncio.run(refer_to_the_system_actor()) + + reads: list[str | None] = [] + read_env = identity.system_principal_id_from_env + + def counted_read(): + reads.append(read_env()) + return reads[-1] + + warnings: list[str] = [] + + def record_warning(msg, *args, **kwargs): + warnings.append(msg % args if args else msg) + + def run_server(config): + app = server.create_app() + + async def serve(): + with pytest.raises(RuntimeError, match="stop after the identity steps"): + async with server.lifespan(app): + pass + + asyncio.run(serve()) + + monkeypatch.setattr(identity, "system_principal_id_from_env", counted_read) + monkeypatch.setattr(migrate_module.logger, "warning", record_warning) + monkeypatch.setattr(server, "run_server", run_server) + monkeypatch.setattr(proxy_module, "ProxyService", lambda cfg: _ProxyThatFails()) + monkeypatch.setattr(cli, "_get_daemon_status", lambda: (False, None)) + monkeypatch.setattr(cli, "setup_logging", lambda verbose=False: None) + monkeypatch.setenv(AUTH_MODE_ENV, AUTH_MODE_EXTERNAL) + try: + result = CliRunner().invoke( + cli.main, ["-c", str(config_dir), "start", "-f"], catch_exceptions=False, + ) + finally: + set_config(NerveConfig()) + + assert result.exit_code == 0, result.output + assert reads == [PRINCIPAL] + system_warnings = [w for w in warnings if "system actor" in w] + assert len(system_warnings) == 1 + assert system_warnings[0].startswith("Identity: the system actor keeps its current ID") + # --------------------------------------------------------------------------- # # nerve doctor # From 4a725beb05cf57d537cddc4aca4867fa880d7baf Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:00:12 +0000 Subject: [PATCH 6/9] Let a database take a system principal ID only once A process that opened the database after another process changed the system actor ID cached the new ID, and could then change it again for a different value. The first process then used an ID that no longer existed. The change now records the ID it set, in the same transaction, in an instance_secrets row (the existing name/value table for instance state; the row is not secret). While the row exists, every different ID is refused with the usual warning, so a database changes its system actor ID at most once. The result of a change gives the IDs of its own transaction, not of a later read. The read-only inspection applies the same rule. Co-Authored-By: Claude Opus 5.5 --- docs/accounts.md | 7 +- docs/config.md | 2 +- nerve/db/accounts.py | 52 ++++++++++--- tests/test_system_principal.py | 131 +++++++++++++++++++++++++++++++-- 4 files changed, 170 insertions(+), 22 deletions(-) diff --git a/docs/accounts.md b/docs/accounts.md index 74d25df26..a4af6b2ea 100644 --- a/docs/accounts.md +++ b/docs/accounts.md @@ -187,9 +187,10 @@ when these conditions are true: `sessions.created_by_actor_id` and `messages.actor_id`, and checks all of them. A check that cannot run counts as a row. - No other actor has the new ID. -- No other process changed the ID after this process opened the database. - When two processes start at the same time with different values, the first - change stays. +- The system actor did not take an ID from the variable before. Nerve records + the ID it took in `nerve.db`, in the same transaction as the change, so the + ID changes at most once, also when two processes start with different + values. A backup made with `--no-secrets` does not keep that record. Thus a new installation gets the ID before it attributes work to the system actor. After the change, system and MCP tokens act as the new ID, and diff --git a/docs/config.md b/docs/config.md index 822c3c949..a6fdd2b34 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1867,7 +1867,7 @@ bootstrap. | Value | Behavior | |---|---| | Unset or empty | The system actor keeps its ID. | -| A UUID in canonical lower-case form, for example `00000000-0000-4000-8000-000000000001` | The system actor gets this ID when no row refers to the system actor, no other actor has the ID, and no other process changed the ID first. Otherwise the system actor keeps its current ID and Nerve logs a warning. See [The system actor in external mode](accounts.md#the-system-actor-in-external-mode). | +| A UUID in canonical lower-case form, for example `00000000-0000-4000-8000-000000000001` | The system actor gets this ID when no row refers to the system actor, no other actor has the ID, and the system actor did not take an ID from this variable before. Otherwise the system actor keeps its current ID and Nerve logs a warning. See [The system actor in external mode](accounts.md#the-system-actor-in-external-mode). | | Any other value, also a UUID in upper case | The system actor keeps its ID and Nerve logs a warning. The server starts. | Set the variable before the first `nerve init` or start, so that the change diff --git a/nerve/db/accounts.py b/nerve/db/accounts.py index 2493bfab3..f600dc1d5 100644 --- a/nerve/db/accounts.py +++ b/nerve/db/accounts.py @@ -40,6 +40,18 @@ SYSTEM_ACTOR_ID_TAKEN = "another actor already has the new ID" +# The ``instance_secrets`` row that records the system principal ID the +# system actor took. It is not a secret: ``instance_secrets`` is the existing +# name/value table for instance state. AccountStore._rekey_system_actor writes +# it in the same transaction as the change of the ID, and refuses every later +# different ID while it exists, so a database takes a system principal ID at +# most once. A backup made with ``--no-secrets`` does not keep it. +SYSTEM_PRINCIPAL_CLAIM_NAME = "system_principal_id" + + +def _already_claimed(claimed_id: str) -> str: + return f"the system actor already took the ID {claimed_id} once, and its ID changes only once" + # Sentinel for "leave this field alone" in partial updates, distinct from None # (which clears a nullable field). _UNSET = object() @@ -330,10 +342,13 @@ async def _system_actor_rekey_blocker( async def _decide_system_actor_rekey(self, new_id: str) -> tuple[str, str | None]: """Read the stored system actor ID and decide whether it can become ``new_id``. - Call inside a transaction. Returns ``(stored_id, blocker)``. The stored - ID must still be the ID that this connection has cached: when another - connection changed it, this one does not change it again, because the - other connection uses the ID it set. + Call inside a transaction. Returns ``(stored_id, blocker)``. The ID + changes at most once: a recorded claim + (:data:`SYSTEM_PRINCIPAL_CLAIM_NAME`) refuses every different ID, also + from a process that opened the database after the change. Without a + claim, the stored ID must still be the ID that this connection has + cached: when another connection changed it, this one does not change + it again, because the other connection uses the ID it set. """ async with self.db.execute( "SELECT id FROM actor_refs WHERE kind = 'system'" @@ -346,6 +361,9 @@ async def _decide_system_actor_rekey(self, new_id: str) -> tuple[str, str | None stored_id = row[0] if stored_id == new_id: return stored_id, None + claimed_id = await self._get_instance_secret(SYSTEM_PRINCIPAL_CLAIM_NAME) + if claimed_id is not None: + return stored_id, _already_claimed(claimed_id) cached_id = self.system_actor_id if stored_id != cached_id: return stored_id, ( @@ -359,23 +377,30 @@ async def _rekey_system_actor(self, new_id: str) -> SystemActorRekey: One transaction reads the stored ID again, checks it with :meth:`_decide_system_actor_rekey`, drops - ``system_actor_id_cannot_change``, changes the ID, and creates the - trigger again with :data:`SYSTEM_ACTOR_ID_TRIGGER_SQL`. A failure rolls - back all of it, so no other transaction sees the table without the - trigger. When a blocker exists, nothing changes and the result names - it. The cached :attr:`system_actor` is read again in all cases. + ``system_actor_id_cannot_change``, changes the ID, creates the trigger + again with :data:`SYSTEM_ACTOR_ID_TRIGGER_SQL`, and records the claim + (:data:`SYSTEM_PRINCIPAL_CLAIM_NAME`). A failure rolls back all of it, + so no other transaction sees the table without the trigger, or the new + ID without the claim. When a blocker exists, nothing changes and the + result names it. The result gives the IDs of this transaction. The + cached :attr:`system_actor` is read again in all cases. """ async with self._atomic(): previous_id, blocker = await self._decide_system_actor_rekey(new_id) - if previous_id != new_id and blocker is None: + changed = previous_id != new_id and blocker is None + if changed: await self.db.execute(f"DROP TRIGGER IF EXISTS {SYSTEM_ACTOR_ID_TRIGGER}") await self.db.execute( "UPDATE actor_refs SET id = ? WHERE id = ? AND kind = 'system'", (new_id, previous_id), ) await self.db.execute(SYSTEM_ACTOR_ID_TRIGGER_SQL) + await self.db.execute( + "INSERT INTO instance_secrets (name, value) VALUES (?, ?)", + (SYSTEM_PRINCIPAL_CLAIM_NAME, new_id), + ) await self._cache_system_actor() - return SystemActorRekey(previous_id, self.system_actor_id, blocker) + return SystemActorRekey(previous_id, new_id if changed else previous_id, blocker) async def _preview_system_actor_rekey(self, new_id: str) -> SystemActorRekey: """What :meth:`_rekey_system_actor` would do, from one snapshot, with no write.""" @@ -942,6 +967,11 @@ def inspect_system_actor(db_path: Path, wanted_id: str) -> tuple[str, str | None current_id = str(rows[0][0]) if current_id == wanted_id: return current_id, None + claimed = conn.execute( + "SELECT value FROM instance_secrets WHERE name = ?", (SYSTEM_PRINCIPAL_CLAIM_NAME,), + ).fetchone() + if claimed is not None: + return current_id, _already_claimed(str(claimed[0])) columns = [(row[0], row[1]) for row in conn.execute(ACTOR_REF_COLUMNS_SQL)] for probe in _rekey_probes(columns, current_id, wanted_id): try: diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 55c148c27..e95dd83b6 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -27,6 +27,7 @@ SYSTEM_ACTOR_ID_TAKEN, SYSTEM_ACTOR_ID_TRIGGER, SYSTEM_ACTOR_ID_TRIGGER_SQL, + SYSTEM_PRINCIPAL_CLAIM_NAME, inspect_system_actor, ) from nerve.db.migrations import v047_accounts @@ -106,6 +107,17 @@ async def _refer_to_system_actor(database: Database, reference: str) -> str: return "later_story.caused_by_actor_id" +def _claimed(claimed_id: str) -> str: + return ( + f"the system actor already took the ID {claimed_id} once, and its ID " + "changes only once" + ) + + +async def _claim(database: Database) -> str | None: + return await database._get_instance_secret(SYSTEM_PRINCIPAL_CLAIM_NAME) + + def _rekey_from_another_connection(path, new_id: str) -> None: """Change the system actor ID as another process would, and commit.""" other = sqlite3.connect(path) @@ -259,6 +271,12 @@ async def test_a_failure_rolls_back_the_id_and_the_trigger(self, database, monke assert await _system_rows(database) == [local_id] assert await _trigger_sql(database) == fresh assert database.system_actor_id == local_id + assert await _claim(database) is None + + async def test_the_change_records_the_claim(self, database): + assert await _claim(database) is None + await database._rekey_system_actor(PRINCIPAL) + assert await _claim(database) == PRINCIPAL async def test_two_connections_agree_on_the_new_id(self, tmp_path): first = await _open(tmp_path / "nerve.db") @@ -282,17 +300,13 @@ async def test_a_connection_does_not_change_an_id_that_another_one_set(self, tmp first = await _open(tmp_path / "nerve.db") second = await _open(tmp_path / "nerve.db") try: - local_id = first.system_actor_id assert (await first._rekey_system_actor(PRINCIPAL)).changed result = await second._rekey_system_actor(ALICE) assert not result.changed assert (result.previous_id, result.actor_id) == (PRINCIPAL, PRINCIPAL) - assert result.blocker == ( - f"another process changed the ID from {local_id} to {PRINCIPAL} " - "after this process read it" - ) + assert result.blocker == _claimed(PRINCIPAL) assert first.system_actor_id == second.system_actor_id == PRINCIPAL assert await _system_rows(first) == [PRINCIPAL] # Both connections can still write rows attributed to the system actor. @@ -313,7 +327,7 @@ async def test_concurrent_changes_to_different_ids_leave_one_live_id(self, tmp_p assert sorted(r.changed for r in results) == [False, True] winner = next(r.actor_id for r in results if r.changed) loser = next(r for r in results if not r.changed) - assert loser.blocker.startswith("another process changed the ID") + assert loser.blocker == _claimed(winner) assert first.system_actor_id == second.system_actor_id == winner assert await _system_rows(first) == [winner] await first.create_session("s-first", actor=first.system_actor) @@ -322,6 +336,80 @@ async def test_concurrent_changes_to_different_ids_leave_one_live_id(self, tmp_p await first.close() await second.close() + async def test_a_connection_that_opens_later_cannot_change_the_id_again(self, tmp_path): + """The second connection opens after the change, so it caches the new + ID. The claim still refuses a different ID, and the first connection + keeps an ID that exists.""" + path = tmp_path / "nerve.db" + first = await _open(path) + try: + assert (await first._rekey_system_actor(PRINCIPAL)).changed + second = await _open(path) + try: + assert second.system_actor_id == PRINCIPAL + + result = await second._rekey_system_actor(ALICE) + + assert not result.changed and result.blocker == _claimed(PRINCIPAL) + assert inspect_system_actor(path, ALICE) == (PRINCIPAL, _claimed(PRINCIPAL)) + assert first.system_actor_id == second.system_actor_id == PRINCIPAL + assert await _system_rows(first) == [PRINCIPAL] + await first.create_session("s-first", actor=first.system_actor) + await second.create_session("s-second", actor=second.system_actor) + finally: + await second.close() + finally: + await first.close() + + async def test_the_result_gives_the_ids_of_its_own_transaction( + self, tmp_path, monkeypatch, + ): + """Another connection opens between the commit and the cache refresh + of the first one, and asks for a different ID. It is refused, and the + first connection reports and caches the ID it set.""" + path = tmp_path / "nerve.db" + first = await _open(path) + late: list = [] + refresh = first._cache_system_actor + + async def refresh_after_a_late_change(): + second = await _open(path) + try: + late.append(await second._rekey_system_actor(ALICE)) + finally: + await second.close() + await refresh() + + try: + monkeypatch.setattr(first, "_cache_system_actor", refresh_after_a_late_change) + result = await first._rekey_system_actor(PRINCIPAL) + + assert result.changed and result.actor_id == PRINCIPAL + assert late[0].blocker == _claimed(PRINCIPAL) + assert first.system_actor_id == PRINCIPAL + assert await _system_rows(first) == [PRINCIPAL] + finally: + await first.close() + + async def test_without_a_claim_a_connection_keeps_an_id_that_another_one_set( + self, database, + ): + """A database without the claim row (for example from a backup made + with --no-secrets): the compare with the cached ID still refuses to + change an ID that another connection set.""" + local_id = database.system_actor_id + _rekey_from_another_connection(database.db_path, PRINCIPAL) + assert await _claim(database) is None + + result = await database._rekey_system_actor(ALICE) + + assert not result.changed + assert result.blocker == ( + f"another process changed the ID from {local_id} to {PRINCIPAL} " + "after this process read it" + ) + assert database.system_actor_id == PRINCIPAL + @pytest.mark.parametrize("referenced", [False, True]) async def test_a_column_collation_that_is_not_registered_does_not_stop_the_check( self, database, referenced, @@ -583,7 +671,7 @@ async def test_a_start_that_asks_for_another_id_keeps_the_one_already_set( assert not report.rekeyed_system_actor assert len(report.warnings) == 1 assert f"keeps its current ID {PRINCIPAL}" in report.warnings[0] - assert "another process changed the ID" in report.warnings[0] + assert _claimed(PRINCIPAL) in report.warnings[0] assert len(_warnings(caplog)) == 1 assert first.system_actor_id == second.system_actor_id == PRINCIPAL await second.create_session("s-second", actor=second.system_actor) @@ -591,6 +679,35 @@ async def test_a_start_that_asks_for_another_id_keeps_the_one_already_set( await first.close() await second.close() + async def test_a_later_start_with_another_value_keeps_the_id( + self, tmp_path, external, monkeypatch, caplog, + ): + """The first process keeps running. A process that starts later, with + another value, keeps the first ID and logs one warning.""" + path = tmp_path / "nerve.db" + first = await _open(path) + try: + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, PRINCIPAL) + assert (await bootstrap_identity(first, _config())).rekeyed_system_actor + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, ALICE) + second = await _open(path) + try: + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + report = await bootstrap_identity(second, _config()) + assert not report.rekeyed_system_actor + assert len(report.warnings) == 1 + assert f"keeps its current ID {PRINCIPAL}" in report.warnings[0] + assert _claimed(PRINCIPAL) in report.warnings[0] + assert len(_warnings(caplog)) == 1 + assert second.system_actor_id == PRINCIPAL + finally: + await second.close() + assert first.system_actor_id == PRINCIPAL + await first.create_session("s-first", actor=first.system_actor) + finally: + await first.close() + # --------------------------------------------------------------------------- # # Tokens # From 845559df23955a555bfedcc8dd664d970c6232a4 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:01:54 +0000 Subject: [PATCH 7/9] Leave the system principal ID to the server in a fresh foreground start nerve start --foreground on a fresh install runs the slim nerve init and then serves in the same process. Both ran the identity bootstrap, so the process read NERVE_SYSTEM_PRINCIPAL_ID twice and warned twice about an invalid value. start now passes a hidden init option, and the slim init then leaves the system actor step to the gateway's bootstrap, which reads the variable and logs the result once. Co-Authored-By: Claude Opus 5.5 --- nerve/cli.py | 19 +++++-- nerve/migrate.py | 25 +++++++-- tests/test_system_principal.py | 99 ++++++++++++++++++++++++++-------- 3 files changed, 114 insertions(+), 29 deletions(-) diff --git a/nerve/cli.py b/nerve/cli.py index d52b85f40..67886f8b0 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -222,12 +222,16 @@ def main(ctx: click.Context, config_dir: str | None, verbose: bool) -> None: "--keep-env", is_flag=True, help="Slim setup: pass ANTHROPIC_API_KEY/CLAUDE_CODE_OAUTH_TOKEN from this shell to the daemon", ) +@click.option( + "--serve-next", is_flag=True, hidden=True, + help="Slim setup: this process serves next; leave NERVE_SYSTEM_PRINCIPAL_ID to the gateway", +) @click.pass_context def init( ctx: click.Context, if_needed: bool, non_interactive: bool, inside_docker: bool, terminal: bool, deployment: str | None, workspace_opt: str | None, port: int | None, bind: str | None, no_start: bool, no_browser: bool, no_clipboard: bool, yes: bool, - keep_env: bool, + keep_env: bool, serve_next: bool = False, ) -> None: """Set up Nerve: decide how it runs here, then finish in your browser. @@ -248,6 +252,7 @@ def init( deployment=deployment, workspace=workspace_opt, port=port, bind=bind, start=not no_start, browser=False if no_browser else None, clipboard=not no_clipboard, yes=yes, keep_env=keep_env, + system_principal=not serve_next, ) return @@ -738,6 +743,7 @@ def _slim_init( clipboard: bool, yes: bool, keep_env: bool = False, + system_principal: bool = True, ) -> None: from nerve.bootstrap import SlimFlags, SlimSetup, is_fresh_install @@ -761,11 +767,16 @@ def _slim_init( # The owner account exists before the daemon answers, so the setup # token is available the moment /health is green. No name: the setup - # page asks. + # page asks. ``system_principal`` is false when this process serves + # next (``nerve start -f``): the gateway's bootstrap reads + # NERVE_SYSTEM_PRINCIPAL_ID and logs the result, once. from nerve.migrate import bootstrap_identity_sync try: - bootstrap_identity_sync(config, display_name=None, passwordless=False) + bootstrap_identity_sync( + config, display_name=None, passwordless=False, + system_principal=system_principal, + ) except Exception as e: # noqa: BLE001 — the files are written; say what is missing raise click.ClickException( f"Setup wrote the configuration, but the local owner account could not " @@ -905,7 +916,7 @@ def start(ctx: click.Context, foreground: bool) -> None: # `start --foreground` would otherwise exit 1 and leave an # unsupervised daemon holding the port). click.echo("Fresh install detected — running setup...") - ctx.invoke(init, no_start=True, no_browser=True, no_clipboard=True) + ctx.invoke(init, no_start=True, no_browser=True, no_clipboard=True, serve_next=True) config = ctx.obj.get("config") if config is None: raise click.ClickException( diff --git a/nerve/migrate.py b/nerve/migrate.py index 1679b6d55..f7aca82b3 100644 --- a/nerve/migrate.py +++ b/nerve/migrate.py @@ -1412,6 +1412,7 @@ async def bootstrap_identity( report: MigrationReport | None = None, dry_run: bool = False, display_name: str | None = None, + system_principal: bool = True, ) -> MigrationReport: """Create the local owner account and signing secret if they do not exist. @@ -1431,15 +1432,22 @@ async def bootstrap_identity( changed. The system actor gets the ID in ``NERVE_SYSTEM_PRINCIPAL_ID`` when it can (:func:`_apply_system_principal`). The signing secret is still made, because system and MCP tokens need it. + + ``system_principal`` false leaves the system actor step, and the read of + ``NERVE_SYSTEM_PRINCIPAL_ID``, to a later bootstrap in the same process: + ``nerve start --foreground`` on a fresh install runs ``nerve init`` and + then the gateway, and only the gateway does the step. """ from nerve.gateway.auth import is_external_mode report = MigrationReport(dry_run=dry_run) if report is None else report if is_external_mode(): - await _apply_system_principal(db, report, dry_run=dry_run) + if system_principal: + await _apply_system_principal(db, report, dry_run=dry_run) await ensure_jwt_secret(db, config, report=report, dry_run=dry_run) return report - _note_ignored_system_principal() + if system_principal: + _note_ignored_system_principal() source = _credential_source_for(config) # Include accounts a dry run would create with the transitional source so @@ -1783,6 +1791,7 @@ def bootstrap_identity_sync( *, display_name: str | None = None, passwordless: bool = False, + system_principal: bool = True, ) -> MigrationReport: """Run the identity bootstrap on a new connection, for the installer. @@ -1796,7 +1805,9 @@ def bootstrap_identity_sync( has a password. In external mode, no account is created and ``display_name`` and - ``passwordless`` have no effect. Only the signing secret is made. + ``passwordless`` have no effect. The signing secret is made, and the + system actor gets the ID in ``NERVE_SYSTEM_PRINCIPAL_ID`` unless + ``system_principal`` is false (see :func:`bootstrap_identity`). """ try: asyncio.get_running_loop() @@ -1811,6 +1822,7 @@ def bootstrap_identity_sync( asyncio.run( _bootstrap_with_own_connection( config, paths.db_path(), report, display_name, passwordless, + system_principal=system_principal, ) ) return report @@ -1822,6 +1834,8 @@ async def _bootstrap_with_own_connection( report: MigrationReport, display_name: str | None = None, passwordless: bool = False, + *, + system_principal: bool = True, ) -> None: from nerve.db import Database from nerve.gateway.auth import is_external_mode @@ -1829,7 +1843,10 @@ async def _bootstrap_with_own_connection( db = Database(db_path, workspace=config.workspace) await db.connect() try: - await bootstrap_identity(db, config, report=report, display_name=display_name) + await bootstrap_identity( + db, config, report=report, display_name=display_name, + system_principal=system_principal, + ) if passwordless and not is_external_mode(): await _confirm_passwordless(db, config, report) finally: diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index e95dd83b6..35e8c3d6b 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -13,6 +13,7 @@ import asyncio import logging +import os import sqlite3 import httpx @@ -871,40 +872,32 @@ def test_the_start_preview_leaves_out_the_system_actor_step( upgrade = maybe_migrate(config_dir) assert upgrade.rekeyed_system_actor - def test_a_foreground_start_reads_the_variable_once_and_warns_once( - self, tmp_path, monkeypatch, seeded, - ): - from click.testing import CliRunner - + @staticmethod + def _watch_a_foreground_start(tmp_path, monkeypatch) -> tuple[list, list[str]]: + """Prepare ``nerve start -f``: the server stops right after its identity + bootstrap. Returns the list of env reads and the list of warnings of + ``nerve.migrate``, both filled while the command runs. HOME points + into ``tmp_path``, so a default path cannot reach a real install.""" import nerve.identity as identity import nerve.migrate as migrate_module - import nerve.proxy.service as proxy_module - from nerve import cli + from nerve import cli, setup_token from nerve.gateway import server - config_dir = self._install(tmp_path) - - async def refer_to_the_system_actor(): - database = await _open(paths.db_path()) - try: - await _refer_to_system_actor(database, "session") - finally: - await database.close() - - asyncio.run(refer_to_the_system_actor()) - reads: list[str | None] = [] read_env = identity.system_principal_id_from_env def counted_read(): - reads.append(read_env()) - return reads[-1] + reads.append(os.environ.get(SYSTEM_PRINCIPAL_ENV)) + return read_env() warnings: list[str] = [] def record_warning(msg, *args, **kwargs): warnings.append(msg % args if args else msg) + async def stop_after_the_identity_steps(*args, **kwargs): + raise RuntimeError("stop after the identity steps") + def run_server(config): app = server.create_app() @@ -915,13 +908,39 @@ async def serve(): asyncio.run(serve()) + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) monkeypatch.setattr(identity, "system_principal_id_from_env", counted_read) monkeypatch.setattr(migrate_module.logger, "warning", record_warning) + monkeypatch.setattr(setup_token, "ensure_setup_token", stop_after_the_identity_steps) monkeypatch.setattr(server, "run_server", run_server) - monkeypatch.setattr(proxy_module, "ProxyService", lambda cfg: _ProxyThatFails()) monkeypatch.setattr(cli, "_get_daemon_status", lambda: (False, None)) + monkeypatch.setattr( + cli, "_spawn_daemon", lambda *a, **k: pytest.fail("a foreground start spawns nothing"), + ) monkeypatch.setattr(cli, "setup_logging", lambda verbose=False: None) monkeypatch.setenv(AUTH_MODE_ENV, AUTH_MODE_EXTERNAL) + return reads, warnings + + def test_a_foreground_start_reads_the_variable_once_and_warns_once( + self, tmp_path, monkeypatch, seeded, + ): + from click.testing import CliRunner + + from nerve import cli + + config_dir = self._install(tmp_path) + + async def refer_to_the_system_actor(): + database = await _open(paths.db_path()) + try: + await _refer_to_system_actor(database, "session") + finally: + await database.close() + + asyncio.run(refer_to_the_system_actor()) + reads, warnings = self._watch_a_foreground_start(tmp_path, monkeypatch) try: result = CliRunner().invoke( cli.main, ["-c", str(config_dir), "start", "-f"], catch_exceptions=False, @@ -935,6 +954,44 @@ async def serve(): assert len(system_warnings) == 1 assert system_warnings[0].startswith("Identity: the system actor keeps its current ID") + @pytest.mark.parametrize("value", [PRINCIPAL, "not-a-uuid"]) + def test_a_fresh_foreground_start_reads_the_variable_once( + self, tmp_path, monkeypatch, value, + ): + """``nerve start -f`` on a fresh install runs the slim init and then + serves in the same process. Only the gateway's bootstrap reads the + variable: a valid value is applied once, an invalid one warns once.""" + from click.testing import CliRunner + + from nerve import bootstrap, cli + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, value) + monkeypatch.setattr(bootstrap, "_ensure_web_ui", lambda config_dir: "present") + monkeypatch.setattr(bootstrap, "port_is_free", lambda port, host="127.0.0.1": True) + monkeypatch.setattr( + bootstrap, "propose_port", lambda preferred, bind: (preferred, False), + ) + reads, warnings = self._watch_a_foreground_start(tmp_path, monkeypatch) + config_dir, workspace = tmp_path / "cfg", tmp_path / "ws" + try: + result = CliRunner().invoke( + cli.main, ["-c", str(config_dir), "start", "-f"], + input=f"server\n{workspace}\n8977\nlocalhost\n", catch_exceptions=False, + ) + finally: + set_config(NerveConfig()) + + assert result.exit_code == 0, result.output + assert "Fresh install detected" in result.output + assert (config_dir / "config.local.yaml").exists() + assert reads == [value] + system_warnings = [w for w in warnings if "system actor" in w] + if value == PRINCIPAL: + assert system_warnings == [] + assert inspect_system_actor(paths.db_path(), PRINCIPAL) == (PRINCIPAL, None) + else: + assert len(system_warnings) == 1 and "canonical" in system_warnings[0] + # --------------------------------------------------------------------------- # # nerve doctor # From e568c27172a12ce32ab0a7076b1ea3d4adbad500 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:01:59 +0000 Subject: [PATCH 8/9] Name the current system actor ID in nerve doctor After a successful seed, the current system actor ID is a principal ID, not a local one. nerve doctor still said that the system actor keeps its "local" ID for an invalid or a different value, which misstates where autonomous work is attributed. The doctor, the local-mode note and the migrate preview now say "current ID". Co-Authored-By: Claude Opus 5.5 --- docs/config.md | 2 +- nerve/cli.py | 6 +++--- nerve/migrate.py | 4 ++-- tests/test_system_principal.py | 33 ++++++++++++++++++++++++++++++--- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/docs/config.md b/docs/config.md index a6fdd2b34..a099add6b 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1876,7 +1876,7 @@ occurs before Nerve attributes work to the system actor. See `nerve doctor` reads the variable in its own shell, as it does for `NERVE_AUTH_MODE`. It shows one of these: the system actor has the ID, the -next start gives it the ID, or the system actor keeps its local ID, with the +next start gives it the ID, or the system actor keeps its current ID, with the reason. ## Setup diff --git a/nerve/cli.py b/nerve/cli.py index 67886f8b0..241b0deb2 100644 --- a/nerve/cli.py +++ b/nerve/cli.py @@ -1608,7 +1608,7 @@ def _system_principal_report(*, external: bool) -> tuple[list[str], list[str]]: try: wanted = system_principal_id_from_env() except InvalidSystemPrincipalId as e: - return [], [f"[WARN] {label}: {e}. The server keeps the local system actor ID."] + return [], [f"[WARN] {label}: {e}. The server keeps the current system actor ID."] if wanted is None: return [], [] state = inspect_system_actor(paths.db_path(), wanted) @@ -1620,8 +1620,8 @@ def _system_principal_report(*, external: bool) -> tuple[list[str], list[str]]: if blocker is None: return [f"[--] {label}: {current}; the next start changes it to {wanted}"], [] return [], [ - f"[WARN] {label}: keeps the local ID {current}, not {wanted}: {blocker}. " - "Autonomous work is attributed to the local ID." + f"[WARN] {label}: keeps the current ID {current}, not {wanted}: {blocker}. " + "Autonomous work is attributed to that ID." ] diff --git a/nerve/migrate.py b/nerve/migrate.py index f7aca82b3..a3c706c40 100644 --- a/nerve/migrate.py +++ b/nerve/migrate.py @@ -1547,7 +1547,7 @@ def _note_ignored_system_principal() -> None: if os.environ.get(SYSTEM_PRINCIPAL_ENV, "").strip(): logger.info( "Identity: %s is set, but it has no effect in local mode; the system " - "actor keeps its local ID", SYSTEM_PRINCIPAL_ENV, + "actor keeps its current ID", SYSTEM_PRINCIPAL_ENV, ) @@ -1729,7 +1729,7 @@ def _inspect_system_principal(db_path: Path, report: MigrationReport) -> None: try: new_id = system_principal_id_from_env() except InvalidSystemPrincipalId as e: - report.warnings.append(f"the system actor keeps its local ID: {e}") + report.warnings.append(f"the system actor keeps its current ID: {e}") return if new_id is None: return diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index 35e8c3d6b..fb39e7127 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -1031,7 +1031,7 @@ def test_an_invalid_value_is_a_warning(self, monkeypatch): lines, warnings = self._report(external=True) assert lines == [] assert len(warnings) == 1 and warnings[0].startswith("[WARN]") - assert "canonical" in warnings[0] and "keeps the local system actor ID" in warnings[0] + assert "canonical" in warnings[0] and "keeps the current system actor ID" in warnings[0] def test_no_database_yet(self, seeded): lines, warnings = self._report(external=True) @@ -1042,7 +1042,7 @@ def test_no_database_yet(self, seeded): ] @pytest.mark.asyncio - async def test_a_pending_change_and_a_kept_local_id(self, seeded): + async def test_a_pending_change_and_a_kept_id(self, seeded): database = await _open(paths.db_path()) try: local_id = database.system_actor_id @@ -1057,11 +1057,38 @@ async def test_a_pending_change_and_a_kept_local_id(self, seeded): lines, warnings = self._report(external=True) assert lines == [] assert len(warnings) == 1 and warnings[0].startswith("[WARN]") - assert f"keeps the local ID {local_id}, not {PRINCIPAL}" in warnings[0] + assert f"keeps the current ID {local_id}, not {PRINCIPAL}" in warnings[0] assert f"{column} refers to the current ID" in warnings[0] finally: await database.close() + @pytest.mark.asyncio + async def test_after_a_seed_a_different_or_invalid_value_names_the_current_id( + self, external, monkeypatch, + ): + """The current ID is the seeded principal here, not a local ID.""" + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, PRINCIPAL) + database = await _open(paths.db_path()) + try: + await bootstrap_identity(database, _config()) + finally: + await database.close() + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, ALICE) + lines, warnings = self._report(external=True) + assert lines == [] + assert warnings == [ + f"[WARN] System actor ({SYSTEM_PRINCIPAL_ENV} from this shell): keeps the " + f"current ID {PRINCIPAL}, not {ALICE}: {_claimed(PRINCIPAL)}. Autonomous " + "work is attributed to that ID." + ] + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, "not-a-uuid") + lines, warnings = self._report(external=True) + assert lines == [] + assert len(warnings) == 1 and "keeps the current system actor ID" in warnings[0] + assert "local" not in warnings[0] + @pytest.mark.asyncio async def test_the_doctor_report_shows_the_id_in_force( self, tmp_path, external, seeded, From cd6afa6b2137e6ff29a14ee7fd2e738a2a0fd28c Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:22:59 +0000 Subject: [PATCH 9/9] Keep the system principal claim in a --no-secrets backup A --no-secrets backup emptied instance_secrets, so a restored database lost the record of the system principal ID. A process that opened the restored database later could then change the system actor ID again, and a process that was already running kept an ID that no longer existed. The scrub now deletes every instance_secrets row except that record, which is instance state and not a secret (the same ID is in actor_refs). The signing secret and the setup token are still removed. Co-Authored-By: Claude Opus 5.5 --- docs/accounts.md | 12 ++++-- nerve/backup.py | 30 ++++++++++----- nerve/db/accounts.py | 3 +- tests/test_backup.py | 35 +++++++++++++++++ tests/test_system_principal.py | 70 ++++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 15 deletions(-) diff --git a/docs/accounts.md b/docs/accounts.md index a4af6b2ea..85979c925 100644 --- a/docs/accounts.md +++ b/docs/accounts.md @@ -190,7 +190,8 @@ when these conditions are true: - The system actor did not take an ID from the variable before. Nerve records the ID it took in `nerve.db`, in the same transaction as the change, so the ID changes at most once, also when two processes start with different - values. A backup made with `--no-secrets` does not keep that record. + values. A backup made with `--no-secrets` keeps that record, because it is + not secret. Thus a new installation gets the ID before it attributes work to the system actor. After the change, system and MCP tokens act as the new ID, and @@ -387,9 +388,12 @@ password hashes, and the generated signing secret. Treat the archive as a secret. `--no-secrets` removes account passwords, the signing secret and the setup -completion record from the database snapshot, omits `config.local.yaml`, and -replaces credential values in workspace configuration with environment-variable -placeholders. It fails if a configuration file cannot be parsed safely. +completion record from the database snapshot, and keeps the record of the +system principal ID that the system actor took (see +[The system actor in external mode](#the-system-actor-in-external-mode)). It +omits `config.local.yaml`, and replaces credential values in workspace +configuration with environment-variable placeholders. It fails if a +configuration file cannot be parsed safely. After restoring a `--no-secrets` backup: diff --git a/nerve/backup.py b/nerve/backup.py index 8ebf30814..8a6bb1911 100644 --- a/nerve/backup.py +++ b/nerve/backup.py @@ -8,10 +8,11 @@ accounts and actor identity. ``--no-secrets`` empties its two credentials in the snapshot: the JWT signing secret in ``instance_secrets`` and each account's password hash in ``accounts.credential`` - (see :func:`_scrub_snapshot_secrets`). It also replaces secret values in the - staged workspace ``config/*.yaml`` with ``${VAR}`` placeholders, because a - tracked ``auth.password_hash`` is still a verifier - (see :func:`_sanitised_config`). + (see :func:`_scrub_snapshot_secrets`). The one ``instance_secrets`` row that + is not secret, the record of the system principal ID, stays. It also + replaces secret values in the staged workspace ``config/*.yaml`` with + ``${VAR}`` placeholders, because a tracked ``auth.password_hash`` is still a + verifier (see :func:`_sanitised_config`). - ``memu.sqlite`` — the entire long-term memory - the memU sidecar dirs (``memu-conversations/``, ``memu-manual/``, ``memu-resources/``) - secrets (``certs/``, ``mcp-token``, ``telegram_sync.session``, ``config.local.yaml``) @@ -351,14 +352,21 @@ def _snapshot_db(src: Path, dst: Path) -> None: def _scrub_instance_secrets(snapshot: Path) -> None: - """Empty ``instance_secrets`` in a snapshot copy of nerve.db. + """Delete the secrets from ``instance_secrets`` in a snapshot copy of nerve.db. The table holds the generated JWT signing secret, which can mint tokens for - the live instance, so ``--no-secrets`` must not carry it. This edits the - snapshot before it is checksummed and never touches the live database. - ``secure_delete`` overwrites the freed pages. A snapshot without the table - is left as is. The restored instance generates a new secret on first start. + the live instance, so ``--no-secrets`` must not carry it. Every row is + deleted except the record of the system principal ID that the system + actor took (``nerve.db.accounts.SYSTEM_PRINCIPAL_CLAIM_NAME``): it is + instance state, not a secret, and the same ID is in ``actor_refs``. A + restored database that lost it could change its system actor ID again. + This edits the snapshot before it is checksummed and never touches the + live database. ``secure_delete`` overwrites the freed pages. A snapshot + without the table is left as is. The restored instance generates a new + secret on first start. """ + from nerve.db.accounts import SYSTEM_PRINCIPAL_CLAIM_NAME + conn = _connect(snapshot) try: exists = conn.execute( @@ -367,7 +375,9 @@ def _scrub_instance_secrets(snapshot: Path) -> None: if not exists: return conn.execute("PRAGMA secure_delete=ON") - conn.execute("DELETE FROM instance_secrets") + conn.execute( + "DELETE FROM instance_secrets WHERE name != ?", (SYSTEM_PRINCIPAL_CLAIM_NAME,), + ) conn.commit() finally: conn.close() diff --git a/nerve/db/accounts.py b/nerve/db/accounts.py index f600dc1d5..3cbf1cd13 100644 --- a/nerve/db/accounts.py +++ b/nerve/db/accounts.py @@ -45,7 +45,8 @@ # name/value table for instance state. AccountStore._rekey_system_actor writes # it in the same transaction as the change of the ID, and refuses every later # different ID while it exists, so a database takes a system principal ID at -# most once. A backup made with ``--no-secrets`` does not keep it. +# most once. A backup made with ``--no-secrets`` keeps this row and deletes the +# secrets (nerve.backup._scrub_instance_secrets). SYSTEM_PRINCIPAL_CLAIM_NAME = "system_principal_id" diff --git a/tests/test_backup.py b/tests/test_backup.py index 429e8c1c6..8b80b75ee 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -426,6 +426,41 @@ def test_no_secrets_scrubs_the_stored_signing_secret(nerve_dir, workspace, confi assert _stored_secrets(staging / "state" / "nerve.db") == ["planted-signing-secret"] +def test_no_secrets_keeps_the_system_principal_claim(nerve_dir, workspace, config_dir, tmp_path): + """The record of the system principal ID is instance state, not a secret: + a --no-secrets snapshot keeps it and still deletes every secret row.""" + from nerve.db.accounts import SYSTEM_PRINCIPAL_CLAIM_NAME + + claim = "00000000-0000-4000-8000-00000000c2c2" + _plant_instance_secret(nerve_dir / "nerve.db") + conn = sqlite3.connect(str(nerve_dir / "nerve.db")) + try: + conn.execute( + "INSERT INTO instance_secrets VALUES ('setup_token', 'planted-setup-token')" + ) + conn.execute( + "INSERT INTO instance_secrets VALUES (?, ?)", (SYSTEM_PRINCIPAL_CLAIM_NAME, claim), + ) + conn.commit() + finally: + conn.close() + + stripped = backup_mod.create_backup( + nerve_dir, workspace, tmp_path / "out", config_dir=config_dir, include_secrets=False, + ) + staging = tmp_path / "x" + report = backup_mod.verify_bundle(stripped.path, extract_to=staging) + assert report.ok, report.errors + snapshot = staging / "state" / "nerve.db" + assert _stored_secrets(snapshot) == [claim] + raw = snapshot.read_bytes().decode("latin-1") + assert "planted-signing-secret" not in raw + assert "planted-setup-token" not in raw + assert sorted(_stored_secrets(nerve_dir / "nerve.db")) == sorted( + ["planted-signing-secret", "planted-setup-token", claim] + ) + + # Obviously synthetic, and long enough to be found in the raw file bytes. _PLANTED_HASH = "$2b$12$planted-account-password-hash-not-a-real-one" diff --git a/tests/test_system_principal.py b/tests/test_system_principal.py index fb39e7127..6e3d21e44 100644 --- a/tests/test_system_principal.py +++ b/tests/test_system_principal.py @@ -993,6 +993,76 @@ def test_a_fresh_foreground_start_reads_the_variable_once( assert len(system_warnings) == 1 and "canonical" in system_warnings[0] +# --------------------------------------------------------------------------- # +# Backup and restore # +# --------------------------------------------------------------------------- # + + +class TestBackupRestore: + @pytest.mark.asyncio + async def test_a_no_secrets_restore_keeps_the_claim_and_drops_the_secrets( + self, tmp_path, external, monkeypatch, caplog, + ): + """Seed P1, back up with --no-secrets, restore. A first start with the + same value changes nothing. A later start with another value is + refused while the first process runs, and the first process can still + write rows attributed to the system actor.""" + from nerve import backup as backup_mod + from nerve.db.accounts import JWT_SECRET_NAME + + monkeypatch.setenv("HOME", str(tmp_path / "home")) + source = tmp_path / "source_state" + source.mkdir(mode=0o700) + workspace, config_dir = tmp_path / "ws", tmp_path / "cfg" + workspace.mkdir() + (workspace / "SOUL.md").write_text("soul", encoding="utf-8") + config_dir.mkdir() + (config_dir / "config.yaml").write_text(f"workspace: {workspace}\n", encoding="utf-8") + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, PRINCIPAL) + seeded_db = await _open(source / "nerve.db") + try: + await bootstrap_identity(seeded_db, NerveConfig()) # generates a signing secret + assert seeded_db.system_actor_id == PRINCIPAL + assert await seeded_db._get_instance_secret(JWT_SECRET_NAME) + finally: + await seeded_db.close() + + bundle = backup_mod.create_backup( + source, workspace, tmp_path / "out", config_dir=config_dir, include_secrets=False, + ) + restored = tmp_path / "restored_state" + report = backup_mod.restore_bundle( + bundle.path, restored, tmp_path / "restored_ws", + config_dir=tmp_path / "restored_cfg", + ) + assert report.ok, report.errors + + first = await _open(restored / "nerve.db") + try: + assert await first._get_instance_secret(JWT_SECRET_NAME) is None + assert await _claim(first) == PRINCIPAL + first_report = await bootstrap_identity(first, _config()) + assert not first_report.rekeyed_system_actor and not first_report.warnings + assert first.system_actor_id == PRINCIPAL + + monkeypatch.setenv(SYSTEM_PRINCIPAL_ENV, ALICE) + second = await _open(restored / "nerve.db") + try: + with caplog.at_level(logging.WARNING, logger="nerve.migrate"): + late = await bootstrap_identity(second, _config()) + assert not late.rekeyed_system_actor + assert len(late.warnings) == 1 and _claimed(PRINCIPAL) in late.warnings[0] + assert second.system_actor_id == PRINCIPAL + finally: + await second.close() + + assert await _system_rows(first) == [PRINCIPAL] + await first.create_session("s-first", actor=first.system_actor) + finally: + await first.close() + + # --------------------------------------------------------------------------- # # nerve doctor # # --------------------------------------------------------------------------- #