diff --git a/CLAUDE.md b/CLAUDE.md index e1d59c0..fbc8268 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,7 +20,7 @@ agentic-cli/ │ ├── __init__.py # Package exports, lazy imports │ ├── config.py # BaseSettings (pydantic-settings) │ ├── settings_mixins.py # Composable settings field groups -│ ├── settings_persistence.py # save_settings() (excludes SECRET_FIELDS) +│ ├── settings_persistence.py # Trust-split save (PROJECT_SETTABLE_KEYS → project, rest → user config; excludes SECRET_FIELDS) │ ├── constants.py # Shared constants, truncate() │ ├── file_utils.py # atomic_write_json / atomic_write_text │ ├── logging.py diff --git a/src/agentic_cli/__init__.py b/src/agentic_cli/__init__.py index a7f486f..558523b 100644 --- a/src/agentic_cli/__init__.py +++ b/src/agentic_cli/__init__.py @@ -39,7 +39,7 @@ validate_settings, reload_settings, ) -from agentic_cli.settings_persistence import SettingsPersistence +from agentic_cli.settings_persistence import SettingsPersistence, SettingsSaveResult from agentic_cli.workflow.settings import WorkflowSettingsMixin from agentic_cli.settings_mixins import AppSettingsMixin, CLISettingsMixin @@ -83,6 +83,7 @@ def __getattr__(name: str): "SettingsContext", "SettingsValidationError", "SettingsPersistence", + "SettingsSaveResult", "get_settings", "set_settings", "set_context_settings", diff --git a/src/agentic_cli/cli/app.py b/src/agentic_cli/cli/app.py index c5854ee..6fb83c2 100644 --- a/src/agentic_cli/cli/app.py +++ b/src/agentic_cli/cli/app.py @@ -26,7 +26,7 @@ from agentic_cli.logging import Loggers, configure_logging if TYPE_CHECKING: - from pathlib import Path + from agentic_cli.settings_persistence import SettingsSaveResult from agentic_cli.workflow import GoogleADKWorkflowManager, EventType, WorkflowEvent from agentic_cli.workflow.base_manager import BaseWorkflowManager from agentic_cli.workflow.config import AgentConfig @@ -258,16 +258,18 @@ def _build_ui_items(self) -> list[Any]: # Sort by order and return items only return [item for _, item in sorted(items, key=lambda x: x[0])] - async def save_settings(self) -> "Path": - """Save current settings to project config file (./.{app_name}/settings.json). + async def save_settings(self) -> "SettingsSaveResult": + """Save current settings, split by trust. - Uses SettingsPersistence to save non-default settings to the - project-level config file. Secrets (API keys) are never saved. + Allowlisted keys go to the project config + (./.{app_name}/settings.json). User-scoped keys differing from their + defaults go to the user config (~/.{app_name}/settings.json), where + the loader trusts them — the project file may only carry allowlisted + keys since P0-1. Secrets (API keys) are never saved. Returns: - Path to the saved config file + SettingsSaveResult with the written path(s) """ - from pathlib import Path from agentic_cli.settings_persistence import SettingsPersistence persistence = SettingsPersistence(self._settings.app_name) diff --git a/src/agentic_cli/cli/settings_command.py b/src/agentic_cli/cli/settings_command.py index 8ff9b44..bd78959 100644 --- a/src/agentic_cli/cli/settings_command.py +++ b/src/agentic_cli/cli/settings_command.py @@ -55,9 +55,14 @@ async def execute(self, args: str, app: "BaseCLIApp") -> None: # Apply settings changes await app.apply_settings(result) - # Save settings to project config file + # Save settings, split by trust (allowlisted → project config, + # user-scoped → user config) try: - path = await app.save_settings() - app.session.add_success(f"Settings saved to {path}") + saved = await app.save_settings() + message = f"Settings saved to {saved.project_path}" + if saved.user_path is not None: + keys = ", ".join(saved.user_scoped_keys) + message += f"; user-scoped ({keys}) saved to {saved.user_path}" + app.session.add_success(message) except Exception as e: app.session.add_warning(f"Settings applied but not saved: {e}") diff --git a/src/agentic_cli/config.py b/src/agentic_cli/config.py index f3aae68..9287a8d 100644 --- a/src/agentic_cli/config.py +++ b/src/agentic_cli/config.py @@ -37,7 +37,14 @@ from agentic_cli.workflow.settings import WorkflowSettingsMixin from agentic_cli.workflow.models import ModelRegistry from agentic_cli.settings_mixins import AppSettingsMixin, CLISettingsMixin -from agentic_cli.settings_persistence import get_project_config_path, get_user_config_path +# PROJECT_SETTABLE_KEYS lives in settings_persistence so the save-side split +# and the load-side filter below share one definition (and because the reverse +# import would be circular). See its definition for the trust rationale. +from agentic_cli.settings_persistence import ( + PROJECT_SETTABLE_KEYS as _PROJECT_SETTABLE_KEYS, + get_project_config_path, + get_user_config_path, +) from agentic_cli.logging import Loggers logger = Loggers.config() @@ -55,34 +62,6 @@ ] -# Deny-by-default allowlist: the ONLY keys a project ./.{app}/settings.json (or -# a cwd-relative .env) may set. A cloned/untrusted repo must not be able to flip -# a security boundary — executor backend, container image/user, bind mounts, -# outputs dir, OS-sandbox policy, shell backend, raw LLM logging, workspace dir, -# permission rules, or secrets. Every entry below is a benign field that cannot -# select code execution, filesystem/mount scope, container identity/image, -# network policy, secrets, or sensitive logging. Anything not clearly benign — -# and any new field — is excluded automatically. Real environment variables and -# the user ~/.{app}/settings.json remain fully trusted. -_PROJECT_SETTABLE_KEYS = frozenset({ - # model / behavior - "default_model", "thinking_effort", "orchestrator", - "context_window_trigger_tokens", "context_window_target_tokens", - # retry / request timeouts (not code paths) - "retry_max_attempts", "retry_initial_delay", "retry_backoff_factor", - "anthropic_request_timeout", "python_executor_timeout", "sandbox_timeout", - # sandbox RESOURCE limits (not backend / image / mounts / user / network) - "sandbox_max_sessions", "sandbox_memory_mb", "sandbox_cpus", "sandbox_pids_limit", - # non-exec tool config - "search_backend", - "webfetch_cache_ttl_seconds", "webfetch_max_content_bytes", "webfetch_max_pdf_bytes", - # persistence backend selection (NOT the credential-bearing postgres_uri) - "session_store", - # display / logging verbosity (NOT raw_llm_logging) - "log_level", "log_format", "verbose_thinking", -}) - - class _AllowlistFilterSource(PydanticBaseSettingsSource): """Wrap an untrusted settings source, keeping only allowlisted keys. diff --git a/src/agentic_cli/settings_persistence.py b/src/agentic_cli/settings_persistence.py index 3926efe..ca1cd0b 100644 --- a/src/agentic_cli/settings_persistence.py +++ b/src/agentic_cli/settings_persistence.py @@ -1,10 +1,17 @@ """Settings persistence utilities. Provides functionality to save settings to JSON files for layered configuration. -Settings are saved to ./.{app_name}/settings.json (project config) by default. + +Saving splits by trust (mirroring the P0-1 load-side allowlist in +``agentic_cli.config``): allowlisted keys go to the project +``./.{app_name}/settings.json``; every other (user-scoped) key goes to the +trusted user ``~/.{app_name}/settings.json``. Without the split, a key the +loader refuses to read from the project file would be written there and +silently dropped on the next start. """ import json +from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING, Any @@ -21,6 +28,37 @@ "postgres_uri", # connection string embeds user:password@host }) +# Deny-by-default allowlist: the ONLY keys a project ./.{app}/settings.json (or +# a cwd-relative .env) may set. A cloned/untrusted repo must not be able to flip +# a security boundary — executor backend, container image/user, bind mounts, +# outputs dir, OS-sandbox policy, shell backend, raw LLM logging, workspace dir, +# permission rules, or secrets. Every entry below is a benign field that cannot +# select code execution, filesystem/mount scope, container identity/image, +# network policy, secrets, or sensitive logging. Anything not clearly benign — +# and any new field — is excluded automatically. Real environment variables and +# the user ~/.{app}/settings.json remain fully trusted. +# +# Used by BOTH sides of persistence: the load-side filter +# (config._AllowlistFilterSource) and the save-side split +# (SettingsPersistence.save), so writer and reader cannot drift apart. +PROJECT_SETTABLE_KEYS = frozenset({ + # model / behavior + "default_model", "thinking_effort", "orchestrator", + "context_window_trigger_tokens", "context_window_target_tokens", + # retry / request timeouts (not code paths) + "retry_max_attempts", "retry_initial_delay", "retry_backoff_factor", + "anthropic_request_timeout", "python_executor_timeout", "sandbox_timeout", + # sandbox RESOURCE limits (not backend / image / mounts / user / network) + "sandbox_max_sessions", "sandbox_memory_mb", "sandbox_cpus", "sandbox_pids_limit", + # non-exec tool config + "search_backend", + "webfetch_cache_ttl_seconds", "webfetch_max_content_bytes", "webfetch_max_pdf_bytes", + # persistence backend selection (NOT the credential-bearing postgres_uri) + "session_store", + # display / logging verbosity (NOT raw_llm_logging) + "log_level", "log_format", "verbose_thinking", +}) + # Identity fields set by the application, not the user IDENTITY_FIELDS = frozenset({ "app_name", @@ -38,6 +76,21 @@ def get_user_config_path(app_name: str) -> Path: return Path.home() / f".{app_name}" / "settings.json" +@dataclass(frozen=True) +class SettingsSaveResult: + """Where a settings save landed. + + ``project_path`` always receives the allowlisted keys (or, with an + explicit ``path=``, the full legacy dump). ``user_path`` is set only when + user-scoped (non-allowlisted) keys were written to the user config; + ``user_scoped_keys`` lists the keys added/updated/removed there. + """ + + project_path: Path + user_path: Path | None = None + user_scoped_keys: tuple[str, ...] = () + + def get_user_project_grants_path(app_name: str) -> Path: """Path to interactively-granted permission rules (``~/.{app_name}/project_grants.json``), keyed by resolved project path. @@ -90,24 +143,37 @@ def save( self, settings: "BaseSettings", path: Path | None = None, - ) -> Path: - """Save settings to JSON config file. - - By default saves to project config (./.{app_name}/settings.json). - Secrets (API keys) and identity fields are never saved. - All other user-configurable settings are saved regardless of - whether they match the schema default, because subclasses may - have different effective defaults. + ) -> SettingsSaveResult: + """Save settings, split by trust to match the load-side allowlist. + + Default save writes two files: + + - Project config (./.{app_name}/settings.json) receives ONLY + allowlisted (``PROJECT_SETTABLE_KEYS``) fields, always all of them — + regardless of whether they match the schema default, because + subclasses may have different effective defaults. The file is fully + rewritten, which also heals stale pre-P0-1 files holding keys the + loader now ignores. + - User config (~/.{app_name}/settings.json) receives the remaining + user-scoped fields, but only those differing from the settings + class default (so code-default changes keep applying for untouched + fields, and the user file stays minimal). A user-scoped field back + at its default is REMOVED from the file. Keys this method does not + manage (hand-stored secrets, unknown/domain keys) are preserved + verbatim; a malformed user file raises rather than being clobbered. + + Secrets (API keys) and identity fields are never written anywhere. + With an explicit ``path=``, the legacy behavior is kept: one full + dump (minus secrets/identity) to that file, no split. Args: settings: Settings instance to save - path: Optional custom path (defaults to project_config_path) + path: Optional custom path (single-file legacy dump) Returns: - Path to the saved config file + SettingsSaveResult with the written path(s) """ - target_path = path or self.project_config_path - target_path.parent.mkdir(parents=True, exist_ok=True) + from agentic_cli.file_utils import atomic_write_text # Get settings as dict, excluding secrets and identity fields data = settings.model_dump( @@ -118,11 +184,77 @@ def save( # Convert Path objects to strings for JSON serialization data = self._serialize_paths(data) - # Write atomically - from agentic_cli.file_utils import atomic_write_text - atomic_write_text(target_path, json.dumps(data, indent=2, default=str)) + if path is not None: + # Explicit target: legacy single-file full dump. + path.parent.mkdir(parents=True, exist_ok=True) + atomic_write_text(path, json.dumps(data, indent=2, default=str)) + return SettingsSaveResult(project_path=path) + + project_data = {k: v for k, v in data.items() if k in PROJECT_SETTABLE_KEYS} + user_updates, user_removals = self._split_user_scoped(settings, data) + + project_path = self.project_config_path + project_path.parent.mkdir(parents=True, exist_ok=True) + atomic_write_text( + project_path, json.dumps(project_data, indent=2, default=str) + ) + + user_path = self.user_config_path + # Merge-write: never clobber keys we don't manage (e.g. hand-stored + # API keys). A malformed user file raises instead of being replaced. + existing: dict[str, Any] = {} + if user_path.exists(): + existing = json.loads(user_path.read_text()) + if not isinstance(existing, dict): + raise ValueError( + f"User settings file is not a JSON object: {user_path}" + ) + merged = dict(existing) + removed = tuple(k for k in sorted(user_removals) if k in existing) + for key in removed: + del merged[key] + merged.update(user_updates) + + if merged == existing: + return SettingsSaveResult(project_path=project_path) + + user_path.parent.mkdir(parents=True, exist_ok=True) + atomic_write_text(user_path, json.dumps(merged, indent=2, default=str)) + return SettingsSaveResult( + project_path=project_path, + user_path=user_path, + user_scoped_keys=tuple(sorted(user_updates)) + removed, + ) - return target_path + def _split_user_scoped( + self, settings: "BaseSettings", data: dict[str, Any] + ) -> tuple[dict[str, Any], set[str]]: + """Partition non-allowlisted dumped fields by deviation from default. + + Returns (updates, removals): ``updates`` maps user-scoped keys whose + live value differs from the settings class default to their dumped + value; ``removals`` holds user-scoped keys back at their default, + whose stale entries should leave the user file. Comparison uses the + instance's own class fields, so subclass default overrides are + respected. + """ + from pydantic_core import PydanticUndefined + + updates: dict[str, Any] = {} + removals: set[str] = set() + fields = type(settings).model_fields + for key, value in data.items(): + if key in PROJECT_SETTABLE_KEYS: + continue + field = fields.get(key) + if field is None: + continue + default = field.get_default(call_default_factory=True) + if default is not PydanticUndefined and getattr(settings, key) == default: + removals.add(key) + else: + updates[key] = value + return updates, removals def load(self, path: Path | None = None) -> dict[str, Any]: """Load settings from JSON config file. diff --git a/tests/test_settings_persistence.py b/tests/test_settings_persistence.py index 8014327..c05a5b5 100644 --- a/tests/test_settings_persistence.py +++ b/tests/test_settings_persistence.py @@ -88,6 +88,161 @@ def test_save_includes_default_values(self, tmp_path): assert "thinking_effort" in data +class TestTrustSplitSave: + """P0-1 follow-up: save() must agree with the allowlist-filtered loader. + + Default save splits by trust: allowlisted keys → project settings.json, + non-allowlisted (user-scoped) keys → user ~/.{app}/settings.json, so a + /settings change to a security-relevant key survives a restart instead of + being silently dropped by _AllowlistFilterSource. + """ + + @pytest.fixture + def isolated_fs(self, tmp_path, monkeypatch): + """Isolate cwd + HOME so save/load hit temp files only.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(home)) + for var in ( + "AGENTIC_RAW_LLM_LOGGING", + "AGENTIC_STATEFUL_EXECUTOR_BACKEND", + "AGENTIC_DEFAULT_MODEL", + ): + monkeypatch.delenv(var, raising=False) + return tmp_path + + def _persistence(self): + return SettingsPersistence(app_name="agentic_cli") + + def test_default_save_keeps_project_file_allowlisted(self, isolated_fs): + """Project file receives only allowlisted keys (and heals stale ones).""" + from agentic_cli.config import BaseSettings + from agentic_cli.settings_persistence import PROJECT_SETTABLE_KEYS + + # Pre-seed a stale kitchen-sink project file (pre-P0-1 artifact) + proj_dir = isolated_fs / ".agentic_cli" + proj_dir.mkdir() + (proj_dir / "settings.json").write_text( + json.dumps({"raw_llm_logging": True, "default_model": "stale-model"}) + ) + + settings = BaseSettings( + raw_llm_logging=True, default_model="claude-sonnet-4-6" + ) + self._persistence().save(settings) + + data = json.loads((proj_dir / "settings.json").read_text()) + assert set(data) <= PROJECT_SETTABLE_KEYS + assert data["default_model"] == "claude-sonnet-4-6" + assert "raw_llm_logging" not in data + + def test_default_save_writes_user_scoped_keys_to_user_config(self, isolated_fs): + """Non-allowlisted keys that differ from defaults land in user config.""" + from agentic_cli.config import BaseSettings + + settings = BaseSettings( + raw_llm_logging=True, stateful_executor_backend="local" + ) + self._persistence().save(settings) + + user_file = isolated_fs / "home" / ".agentic_cli" / "settings.json" + assert user_file.exists() + data = json.loads(user_file.read_text()) + assert data["raw_llm_logging"] is True + assert data["stateful_executor_backend"] == "local" + + def test_default_save_at_defaults_does_not_create_user_config(self, isolated_fs): + """All-default settings write nothing user-scoped.""" + from agentic_cli.config import BaseSettings + + self._persistence().save(BaseSettings()) + + assert not (isolated_fs / "home" / ".agentic_cli" / "settings.json").exists() + + def test_user_config_merge_preserves_unmanaged_keys(self, isolated_fs): + """Hand-stored secrets and unknown keys in user config survive a save.""" + from agentic_cli.config import BaseSettings + + user_dir = isolated_fs / "home" / ".agentic_cli" + user_dir.mkdir(parents=True) + (user_dir / "settings.json").write_text( + json.dumps({"anthropic_api_key": "sk-stored", "domain_custom_key": 7}) + ) + + settings = BaseSettings(raw_llm_logging=True) + self._persistence().save(settings) + + data = json.loads((user_dir / "settings.json").read_text()) + assert data["anthropic_api_key"] == "sk-stored" + assert data["domain_custom_key"] == 7 + assert data["raw_llm_logging"] is True + + def test_revert_to_default_removes_user_config_key(self, isolated_fs): + """Reverting a user-scoped key to its default removes it from user config.""" + from agentic_cli.config import BaseSettings + + user_dir = isolated_fs / "home" / ".agentic_cli" + user_dir.mkdir(parents=True) + (user_dir / "settings.json").write_text( + json.dumps({"raw_llm_logging": True, "domain_custom_key": 7}) + ) + + settings = BaseSettings() # loads raw_llm_logging=True from user config + settings.update_setting("raw_llm_logging", False) # user reverts + self._persistence().save(settings) + + data = json.loads((user_dir / "settings.json").read_text()) + assert "raw_llm_logging" not in data + assert data["domain_custom_key"] == 7 # unmanaged key untouched + + def test_round_trip_user_scoped_change_persists(self, isolated_fs): + """THE regression: a /settings change to a non-allowlisted key must + survive save + fresh load, without tripping the untrusted-key filter.""" + import structlog + + from agentic_cli.config import BaseSettings + + settings = BaseSettings() + settings.update_setting("stateful_executor_backend", "local") + self._persistence().save(settings) + + with structlog.testing.capture_logs() as logs: + reloaded = BaseSettings() + + assert reloaded.stateful_executor_backend == "local" + assert not any( + e.get("event") == "untrusted_project_setting_ignored" for e in logs + ) + + def test_explicit_path_keeps_legacy_full_dump(self, isolated_fs): + """save(path=...) still writes the full single-file dump.""" + from agentic_cli.config import BaseSettings + + out = isolated_fs / "export" / "settings.json" + result = self._persistence().save( + BaseSettings(raw_llm_logging=True), path=out + ) + + data = json.loads(out.read_text()) + assert data["raw_llm_logging"] is True # non-allowlisted key retained + assert result.project_path == out + assert result.user_path is None + + def test_save_result_reports_split(self, isolated_fs): + """Default save reports both paths and the user-scoped keys.""" + from agentic_cli.config import BaseSettings + + result = self._persistence().save(BaseSettings(raw_llm_logging=True)) + + assert result.project_path == isolated_fs / ".agentic_cli" / "settings.json" + assert ( + result.user_path + == isolated_fs / "home" / ".agentic_cli" / "settings.json" + ) + assert "raw_llm_logging" in result.user_scoped_keys + + class TestAtomicWrite: """Tests for atomic settings write (C2)."""