Merge pull request #105 from shoom1/fix/settings-save-trust-split

fix(settings): trust-split save so /settings changes survive the P0-1 allowlist

Author: shoom1

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

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",

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)

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}")

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.
 

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.