From 074a9b7f7b19d502a5a60ef84609e1edcc5019b1 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 5 Nov 2026 06:10:54 -0800 Subject: [PATCH] fix(setup): let the most specific sandbox.enabled win in configured() /sandbox writes enabled: false to the project's settings.local.json, but configured() ORed every scope, so a user-scope true hid it and the session got no warning. Take the first explicit value instead, as Claude Code and sandbox-status-line.sh do. sandbox_enabled_in_project() keeps its any-file answer: the update findings use it to ask whether the project uses the setup at all, and a toggle-off should not stop them. Generated-by: Claude Opus 5 --- .../skills/setup/setup_preflight/isolated.py | 28 +++++++++++---- tools/setup-preflight/tests/test_isolated.py | 36 +++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/isolated.py b/plugins/magpie-setup/skills/setup/setup_preflight/isolated.py index 7acd54307..57144eaf4 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/isolated.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/isolated.py @@ -147,20 +147,32 @@ def current(root: Path) -> str | None: return shipped() -def _sandbox_enabled(paths: list[Path]) -> bool: +def _sandbox_settings(paths: list[Path]) -> list[bool]: + """Every explicit `sandbox.enabled` in `paths`, in the order given.""" + settings = [] for path in paths: try: loaded = json.loads(path.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError): continue sandbox = loaded.get("sandbox") if isinstance(loaded, dict) else None - if isinstance(sandbox, dict) and sandbox.get("enabled") is True: - return True - return False + enabled = sandbox.get("enabled") if isinstance(sandbox, dict) else None + if isinstance(enabled, bool): + settings.append(enabled) + return settings + + +def _project_settings(root: Path) -> list[Path]: + return [root / ".claude" / name for name in ("settings.local.json", "settings.json")] def sandbox_enabled_in_project(root: Path) -> bool: - return _sandbox_enabled([root / ".claude" / name for name in ("settings.json", "settings.local.json")]) + """Whether any project settings file enables the sandbox. + + This asks whether the project uses the setup at all, so a `/sandbox` + toggle-off in `settings.local.json` does not hide a shared `true`. + """ + return True in _sandbox_settings(_project_settings(root)) def configured(root: Path, home: Path | None = None) -> bool: @@ -175,12 +187,16 @@ def configured(root: Path, home: Path | None = None) -> bool: that cannot be read counts as not configured: inside the sandbox the user's settings are read-denied, so callers that run there see `False` and must treat it as "not known to be configured", never as proof of absence. + + The most specific explicit setting wins, as it does for Claude Code and + `sandbox-status-line.sh`: `/sandbox` writes `false` to the project's + `settings.local.json`, and that switches off a sandbox a wider scope enables. """ block = _load_stamp(root).get(BLOCK) if isinstance(block, dict) and block: return block.get("enabled") is not False user = (home if home is not None else Path.home()) / ".claude" - return sandbox_enabled_in_project(root) or _sandbox_enabled([user / "settings.json"]) + return next(iter(_sandbox_settings([*_project_settings(root), user / "settings.json"])), False) #: Shown to the user once per session, at session start, while the isolated diff --git a/tools/setup-preflight/tests/test_isolated.py b/tools/setup-preflight/tests/test_isolated.py index 79f52850e..22f77198b 100644 --- a/tools/setup-preflight/tests/test_isolated.py +++ b/tools/setup-preflight/tests/test_isolated.py @@ -64,6 +64,13 @@ def test_a_sandboxed_project_never_recorded_is_asked_to_run_the_update(project: assert finding.facts["recorded"] is None +def test_a_sandbox_toggled_off_locally_still_gets_the_update_findings(project: Path) -> None: + framework_tree(project) + enable_sandbox(project) + (project / ".claude" / "settings.local.json").write_text('{"sandbox": {"enabled": false}}') + assert codes(isolated_setup_findings(project, today=TODAY)) == ["isolated-setup-changed"] + + def test_an_upgrade_that_changes_setup_files_proposes_the_update(project: Path) -> None: framework_tree(project) before = isolated.current(project) @@ -241,6 +248,35 @@ def test_a_sandbox_in_the_project_settings_is_configured(project: Path) -> None: assert isolated.configured(project) is True +@pytest.mark.parametrize( + ("local", "shared", "user", "expected"), + [ + (False, None, True, False), # /sandbox switched off, whole-user install on + (None, False, True, False), + (False, True, None, False), + (True, False, None, True), + (None, None, True, True), # the project says nothing, so the user file decides + ], +) +def test_the_most_specific_sandbox_setting_wins( + project: Path, + isolated_home: Path, + local: bool | None, + shared: bool | None, + user: bool | None, + expected: bool, +) -> None: + (project / ".claude").mkdir() + (isolated_home / ".claude").mkdir() + for path, enabled in ( + (project / ".claude" / "settings.local.json", local), + (project / ".claude" / "settings.json", shared), + (isolated_home / ".claude" / "settings.json", user), + ): + path.write_text(json.dumps({} if enabled is None else {"sandbox": {"enabled": enabled}})) + assert isolated.configured(project) is expected + + def test_unreadable_user_settings_are_not_configured(project: Path, isolated_home: Path) -> None: (isolated_home / ".claude").mkdir() (isolated_home / ".claude" / "settings.json").write_text("{not json")