From ec7f917c58a31395e454c04aeaccc7b1dd3449db Mon Sep 17 00:00:00 2001 From: agentforce314 Date: Sun, 9 Aug 2026 01:34:47 -0700 Subject: [PATCH] feat(permissions): keep defaultMode in config.json, not a second settings file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `permissions.defaultMode` was read and written only in the standalone `~/.clawcodex/settings.json`. That file surprised the one person it exists to serve: Full Access is a FLOOR, a persisted mode outranks it, so a value sitting in a file nobody knew about made interactive sessions silently ask for approval with no sign of why. There is already an obvious home. `config.json` -> `settings.permissions` is the source of truth for `allowBypassPermissionsMode` and `disableBypassPermissionsMode` (has_allow_bypass_permissions_mode / is_bypass_permissions_mode_disabled both read it), so the MODE belongs beside its own flags rather than in a separate store. - Reads: config.json first in the user tier, then the legacy settings.json, so an existing choice keeps working untouched. - Writes: config.json only. A stale legacy value is shadowed by the new write, which is what makes this a migration rather than a second store. - `permissions` doubles as a flat rule LIST in the schema, so a mode write starts a dict rather than clobbering rules, and a list reads as "no mode". - Global tier only (`load_global`, not the merged view): a repo-committed `.clawcodex/config.json` must not be able to raise the mode, matching the trust split the repo-scoped settings files already get. Managed policy stays where it is — a root-owned lockdown in /etc/clawcodex cannot live in a file the user can edit — as do the repo tiers, which are per-project and deliberately untrusted for loosening. Co-Authored-By: Claude --- src/permissions/modes.py | 69 +++++++++-- .../test_permission_default_mode_in_config.py | 111 ++++++++++++++++++ 2 files changed, 168 insertions(+), 12 deletions(-) create mode 100644 tests/test_permission_default_mode_in_config.py diff --git a/src/permissions/modes.py b/src/permissions/modes.py index 979058bfe..af8a269b2 100644 --- a/src/permissions/modes.py +++ b/src/permissions/modes.py @@ -201,6 +201,31 @@ def _read_settings_file_default_mode(path: str) -> PermissionMode | None: return mode # type: ignore[return-value] +def _read_config_default_mode() -> PermissionMode | None: + """``config.json`` → ``settings.permissions.defaultMode`` (user tier). + + Global tier only, deliberately: ``load_global`` rather than the merged + view, because a repo-committed ``.clawcodex/config.json`` must not be able + to raise the mode — that is the same trust split the repo-scoped settings + files get below. + """ + try: + from src.config import ConfigManager + + perms = ConfigManager().load_global().get("settings", {}).get("permissions") + except Exception: # noqa: BLE001 — an unreadable config tier is simply absent + log.debug("config.json unreadable for defaultMode", exc_info=True) + return None + if not isinstance(perms, dict): + # `permissions` is also modeled as a flat rule LIST elsewhere in the + # schema; a list here just means no mode is configured. + return None + mode = perms.get("defaultMode") + if not isinstance(mode, str) or mode not in EXTERNAL_PERMISSION_MODES: + return None + return mode # type: ignore[return-value] + + def read_settings_default_mode( cwd: str | None = None, *, @@ -257,6 +282,16 @@ def read_settings_default_mode( except Exception: # noqa: BLE001 — no managed policy on this platform log.debug("managed settings unavailable for defaultMode", exc_info=True) + if trusted is None: + # User tier. `~/.clawcodex/config.json` is the primary home: its + # `settings.permissions` block is already the source of truth for + # `allowBypassPermissionsMode` / `disableBypassPermissionsMode` (see + # has_allow_bypass_permissions_mode and is_bypass_permissions_mode_disabled), + # so the MODE belongs beside them rather than in a second file whose + # existence surprised the one person it was meant to serve. The + # standalone settings.json stays readable as a fallback so an existing + # choice keeps working; it is no longer where new ones are written. + trusted = _read_config_default_mode() if trusted is None: trusted = _read_settings_file_default_mode(user_settings_path()) @@ -310,22 +345,32 @@ def set_settings_default_mode(mode: PermissionMode) -> bool: :data:`EXTERNAL_PERMISSION_MODES` member. Writing it would clobber a real prior choice with a value nothing consumes. """ - from .settings_paths import settings_path_for_destination - from .types import PermissionUpdateSetMode - from .updates import persist_permission_update - if mode not in EXTERNAL_PERMISSION_MODES: log.debug("refusing to persist non-external defaultMode %r", mode) return False - return persist_permission_update( - PermissionUpdateSetMode( - type="setMode", - destination="userSettings", - mode=mode, - ), - settings_path_for_destination=settings_path_for_destination, - ) + # Writes go to config.json, beside the bypass flags that already live in + # `settings.permissions`, so a user has one file to read and edit. The + # legacy settings.json is still READ (read_settings_default_mode), but + # nothing writes it any more — a stale value there is shadowed by the + # write here, which is what makes this a migration and not a second store. + try: + from src.config import ConfigManager + + cm = ConfigManager() + data = cm.load_global() + settings = dict(data.get("settings") or {}) + perms = settings.get("permissions") + # `permissions` doubles as a flat rule LIST in the schema; only a dict + # can carry the mode, so start one rather than clobbering rules. + settings["permissions"] = {**perms, "defaultMode": mode} if isinstance(perms, dict) else {"defaultMode": mode} + data["settings"] = settings + cm.save_global(data) + cm.invalidate() + return True + except Exception: # noqa: BLE001 — a failed write must not fail the set + log.debug("config.json defaultMode persist failed", exc_info=True) + return False def is_elevated_without_sandbox() -> bool: diff --git a/tests/test_permission_default_mode_in_config.py b/tests/test_permission_default_mode_in_config.py new file mode 100644 index 000000000..b3542e12e --- /dev/null +++ b/tests/test_permission_default_mode_in_config.py @@ -0,0 +1,111 @@ +"""`permissions.defaultMode` lives in config.json, not a second settings file. + +`config.json` -> `settings.permissions` was already the source of truth for +`allowBypassPermissionsMode` / `disableBypassPermissionsMode`, so the MODE +belongs beside them. The standalone `~/.clawcodex/settings.json` stays +readable so an existing choice keeps working, but nothing writes it any more. + +The user-visible bug behind this: Full Access is only a FLOOR, and a +persisted defaultMode outranks it — so a mode stored in a file the user did +not know existed made an interactive session silently ask for approval. +""" + +from __future__ import annotations + +import json + +import pytest + +from src.permissions.modes import ( + read_settings_default_mode, + resolve_interactive_permission_state, + set_settings_default_mode, +) + + +@pytest.fixture +def config_file(tmp_path, monkeypatch): + """Point config.json at a temp file and clear the manager cache. + + Invalidated on the way OUT as well as in: the manager is a process-wide + singleton, so leaving it holding this temp config after monkeypatch + restores the real path hands the next test someone else's config-home. + """ + path = tmp_path / "config.json" + path.write_text("{}") + monkeypatch.setattr("src.config.get_global_config_path", lambda: path) + import src.config as config_mod + + config_mod._get_default_manager().invalidate() + try: + yield path + finally: + config_mod._get_default_manager().invalidate() + + +def _write(path, perms): + path.write_text(json.dumps({"settings": {"permissions": perms}})) + import src.config as config_mod + + config_mod._get_default_manager().invalidate() + + +def test_reads_the_mode_from_config_json(config_file) -> None: + _write(config_file, {"defaultMode": "acceptEdits"}) + + assert read_settings_default_mode(None) == "acceptEdits" + + +def test_writes_the_mode_into_config_json(config_file) -> None: + assert set_settings_default_mode("plan") is True + + saved = json.loads(config_file.read_text()) + assert saved["settings"]["permissions"]["defaultMode"] == "plan" + + +def test_a_write_keeps_the_neighbouring_bypass_flags(config_file) -> None: + _write(config_file, {"allowBypassPermissionsMode": True}) + + set_settings_default_mode("default") + + perms = json.loads(config_file.read_text())["settings"]["permissions"] + assert perms["defaultMode"] == "default" + assert perms["allowBypassPermissionsMode"] is True + + +def test_a_rule_list_is_not_clobbered(config_file) -> None: + """`permissions` doubles as a flat rule LIST in the schema — a mode write + must start a dict rather than overwrite whatever rules were there.""" + config_file.write_text(json.dumps({"settings": {"permissions": [{"tool": "Bash"}]}})) + import src.config as config_mod + + config_mod._get_default_manager().invalidate() + + assert read_settings_default_mode(None) is None + assert set_settings_default_mode("plan") is True + assert json.loads(config_file.read_text())["settings"]["permissions"]["defaultMode"] == "plan" + + +def test_nothing_configured_leaves_full_access_standing(config_file) -> None: + mode, _, _ = resolve_interactive_permission_state( + permission_mode_cli=None, + dangerously_skip_permissions=False, + allow_dangerously_skip_permissions=False, + cwd=None, + ) + + assert mode == "bypassPermissions" + + +def test_a_stored_mode_still_outranks_the_full_access_floor(config_file) -> None: + """The exact shape of the reported bug, now in the file people can find.""" + _write(config_file, {"defaultMode": "default"}) + + mode, _, _ = resolve_interactive_permission_state( + permission_mode_cli=None, + dangerously_skip_permissions=False, + allow_dangerously_skip_permissions=False, + cwd=None, + ) + + assert mode == "default"