fix(llm): auto-downgrade permission_mode under root so agents run out of the box - #12
Conversation
… of the box The SDK default bypassPermissions makes the claude CLI pass --dangerously-skip-permissions, which it refuses under root/sudo (exit 1) — so every agent failed in root containers/CI unless the user knew to set an override. _permission_mode() now resolves: explicit config override wins, else KEEL_PERMISSION_MODE env var, else bypassPermissions for a normal user but 'default' when euid==0 (auto-fix instead of crash). Verified live: a real Claude round-trip through the adapter as root now succeeds (previously crashed with ProcessError). 4 regression tests; full suite 77 green, ruff clean. Note: supersedes the KEEL_PERMISSION_MODE-only change on claude/cool-ramanujan-bD7zK (keeps the env var, adds the root auto-fix).
…-mode-root # Conflicts: # core/llm/claude.py
There was a problem hiding this comment.
Code Review
This pull request introduces a safer resolution for the Claude SDK permission mode, specifically handling root/sudo environments where the default bypassPermissions causes the underlying CLI to fail. It resolves the permission mode by checking explicit configuration, the KEEL_PERMISSION_MODE environment variable, or automatically downgrading to default when running as root. Unit tests are also added to verify this logic. Feedback on the changes includes defensively handling cases where self.config.extra might be None to avoid an AttributeError, and ensuring that tests are hermetic by explicitly clearing the KEEL_PERMISSION_MODE environment variable during test execution.
| explicit = self.config.extra.get("permission_mode") or os.environ.get( | ||
| "KEEL_PERMISSION_MODE" | ||
| ) |
There was a problem hiding this comment.
To prevent potential AttributeError if self.config.extra is parsed as None (for example, if extra: is defined but left empty in config.yaml), we should use a defensive check like (self.config.extra or {}) before calling .get().
| explicit = self.config.extra.get("permission_mode") or os.environ.get( | |
| "KEEL_PERMISSION_MODE" | |
| ) | |
| explicit = (self.config.extra or {}).get("permission_mode") or os.environ.get( | |
| "KEEL_PERMISSION_MODE" | |
| ) |
| def test_default_downgrades_to_default_as_root(monkeypatch): | ||
| monkeypatch.setattr(os, "geteuid", lambda: 0, raising=False) | ||
| assert _client()._permission_mode() == "default" |
There was a problem hiding this comment.
The test test_default_downgrades_to_default_as_root can fail if the KEEL_PERMISSION_MODE environment variable is already set in the test runner's environment. To ensure the test is hermetic and robust, we should explicitly delete this environment variable using monkeypatch.delenv, similar to how it is done in test_default_bypasses_for_normal_user.
| def test_default_downgrades_to_default_as_root(monkeypatch): | |
| monkeypatch.setattr(os, "geteuid", lambda: 0, raising=False) | |
| assert _client()._permission_mode() == "default" | |
| def test_default_downgrades_to_default_as_root(monkeypatch): | |
| monkeypatch.delenv("KEEL_PERMISSION_MODE", raising=False) | |
| monkeypatch.setattr(os, "geteuid", lambda: 0, raising=False) | |
| assert _client()._permission_mode() == "default" |
The bug
The SDK default
permission_mode="bypassPermissions"makes theclaudeCLI pass--dangerously-skip-permissions, which it refuses under root/sudo (exit 1). So every agent crashed withProcessErrorin root containers / CI unless the user knew to set an override.Reproduced live as root, then fixed and verified live: a real Claude round-trip through the adapter as root now succeeds (returned the expected sentinel) where it previously crashed.
The fix
_permission_mode()resolves in this order:extra.permission_modeinconfig.yaml(always wins)KEEL_PERMISSION_MODEenv var (override every agent at once)bypassPermissionsfor a normal user, butdefaultwheneuid==0— auto-fix instead of crash4 regression tests; full suite 77 green, ruff clean.
Relationship to #11
This supersedes the
KEEL_PERMISSION_MODE-only change onclaude/cool-ramanujan-bD7zK(#11): it keeps that env var and adds the root auto-downgrade. Merge #11 first; this branch touches the same lines ofcore/llm/claude.py, so resolve the conflict by taking this branch's_permission_mode()(it contains both behaviors).https://claude.ai/code/session_01T3HcA2F6EaxyQeNUjbBuVn
Generated by Claude Code