Studio: fail closed when Codex SDK cannot enforce safety pins
Round 6 reviewer noted that the warn-and-proceed path in `_start_thread_with_system` is "failing open" on a server-side chat surface: an SDK rev that does not expose ApprovalMode or SandboxMode would log a warning then call `thread_start(model=...)` with NO safety kwargs, letting the model run under the SDK's `auto_review` default. For a route that takes a user-controlled prompt and can spawn shell commands or file writes, that is the wrong tradeoff. Now fails closed: when `_safe_thread_safety_kwargs()` returns the empty dict the helper raises `CodexUnavailableError`, which the route layer translates to a 503 with a clear error message telling the operator to upgrade `openai_codex` (or set the explicit override env var). The error message names the override so users who hit this on a pre-release alpha can opt in with eyes open rather than discovering the unsafe default after the fact. `UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS=1` is the deliberately-verbose escape hatch. Variable name long and explicit so it does not creep into production environments by accident, kept on the codex subprocess safe-list so the round 6 SDK env-scrub wrapper does not delete it before the gate sees it. Tests: 50 cases total (was 49). The previous old-SDK test was renamed and replaced by two new ones: - `test_thread_start_fails_closed_when_safety_unavailable` asserts the raise fires and `thread_start` is never called. - `test_thread_start_allows_unsafe_defaults_with_explicit_opt_in` asserts the override env var lets the request through and `thread_start` runs without the safety kwargs (with a logged warning). The `_install_fake_codex_sdk` helper now injects fake ApprovalMode and SandboxMode by default so the general translation tests do not need to opt into the override; the two round-6b tests above pass `with_safety_enums=False` to exercise the fail-closed branch.
This commit is contained in:
parent
8ee60019a4
commit
2874abbfec
3 changed files with 125 additions and 23 deletions
|
|
@ -93,6 +93,12 @@ _SAFE_CODEX_ENV_KEYS: tuple[str, ...] = (
|
|||
"PROGRAMDATA",
|
||||
"CODEX_HOME",
|
||||
"CODEX_OPENAI_API_KEY",
|
||||
# Studio-internal override for the round 6b fail-closed safety
|
||||
# pin gate. Kept in the safe-list so the round 6 SDK env-scrub
|
||||
# wrapper does not delete it from `os.environ` before
|
||||
# `_start_thread_with_system` checks it. The variable is not a
|
||||
# secret; the codex subprocess receiving it is harmless.
|
||||
"UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS",
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -795,20 +795,41 @@ async def _start_thread_with_system(
|
|||
``_safe_thread_safety_kwargs``) -- the upstream defaults would let
|
||||
a model decide on its own to execute shell commands or write to
|
||||
the operator's filesystem, which is not appropriate for a
|
||||
server-side chat surface with no per-action approval UI.
|
||||
server-side chat surface with no per-action approval UI. If the
|
||||
installed SDK rev cannot expose those enums we fail closed by
|
||||
default (raise ``CodexUnavailableError``) rather than silently
|
||||
falling through to upstream's ``auto_review`` default. Power
|
||||
users on a dev install can set ``UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS=1``
|
||||
to override -- the variable name is deliberately long and explicit
|
||||
so it does not creep into production environments by accident.
|
||||
"""
|
||||
import os as _os
|
||||
|
||||
safety_kwargs = _safe_thread_safety_kwargs()
|
||||
if not safety_kwargs:
|
||||
# The SDK rev does not expose ApprovalMode / SandboxMode. The
|
||||
# provider still runs (so we do not brick users on older builds),
|
||||
# but operators need to see this in their logs.
|
||||
logger.warning(
|
||||
"codex_provider.safety_kwargs_unavailable",
|
||||
note = (
|
||||
allow_unsafe = _os.environ.get(
|
||||
"UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS", ""
|
||||
).strip().lower() in ("1", "true", "yes", "on")
|
||||
if not allow_unsafe:
|
||||
# Fail closed: the user sees a clear 503 with a typed
|
||||
# error rather than discovering after the fact that
|
||||
# Codex ran with auto_review approvals.
|
||||
raise CodexUnavailableError(
|
||||
"Installed openai_codex SDK does not expose ApprovalMode "
|
||||
"/ SandboxMode; Codex threads will use SDK defaults "
|
||||
"(auto_review approvals, unspecified sandbox). Upgrade "
|
||||
"the SDK to pin safe Studio defaults."
|
||||
"/ SandboxMode, so Studio cannot pin the safe deny_all / "
|
||||
"read_only defaults required for a server-side chat "
|
||||
"surface. Upgrade openai_codex to a build that exports "
|
||||
"those enums, or set "
|
||||
"UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS=1 to opt in to the "
|
||||
"SDK's auto_review default on a trusted dev host."
|
||||
)
|
||||
logger.warning(
|
||||
"codex_provider.safety_kwargs_unavailable_override",
|
||||
note = (
|
||||
"UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS is set; Codex "
|
||||
"threads will use the SDK auto_review default with no "
|
||||
"explicit sandbox. This should only be enabled on a "
|
||||
"trusted dev host."
|
||||
),
|
||||
)
|
||||
base_kwargs: dict[str, Any] = {"model": model, **safety_kwargs}
|
||||
|
|
|
|||
|
|
@ -116,13 +116,31 @@ class _FakeAsyncCodex:
|
|||
return _FakeThread(self._chunks, self._final)
|
||||
|
||||
|
||||
def _install_fake_codex_sdk(monkeypatch, async_codex_cls):
|
||||
def _install_fake_codex_sdk(monkeypatch, async_codex_cls, *, with_safety_enums = True):
|
||||
"""Drop a fake ``codex_app_server`` module into sys.modules so the
|
||||
production lazy-import path picks it up without the real SDK
|
||||
being installed.
|
||||
|
||||
``with_safety_enums=True`` (the default) also injects fake
|
||||
``ApprovalMode`` + ``SandboxMode`` so the round 6b fail-closed
|
||||
path in ``_safe_thread_safety_kwargs`` is not triggered for every
|
||||
test that just wants to exercise stream translation. The two
|
||||
dedicated round 6b tests (fail_closed / explicit_opt_in) pass
|
||||
``with_safety_enums=False`` so they can prove the fail-closed
|
||||
branch fires when those enums are missing.
|
||||
"""
|
||||
fake_mod = types.ModuleType("codex_app_server")
|
||||
fake_mod.AsyncCodex = async_codex_cls # type: ignore[attr-defined]
|
||||
if with_safety_enums:
|
||||
fake_mod.ApprovalMode = types.SimpleNamespace( # type: ignore[attr-defined]
|
||||
deny_all = "DENY_ALL",
|
||||
auto_review = "AUTO_REVIEW",
|
||||
)
|
||||
fake_mod.SandboxMode = types.SimpleNamespace( # type: ignore[attr-defined]
|
||||
read_only = "READ_ONLY",
|
||||
workspace_write = "WORKSPACE_WRITE",
|
||||
danger_full_access = "DANGER_FULL_ACCESS",
|
||||
)
|
||||
monkeypatch.setitem(sys.modules, "codex_app_server", fake_mod)
|
||||
# importlib.util.find_spec walks finders, not sys.modules; patch
|
||||
# it directly so the lazy-import gate accepts the fake.
|
||||
|
|
@ -913,6 +931,16 @@ class TestCodexHardenedRegressions:
|
|||
fake_mod = _types.ModuleType("openai_codex")
|
||||
fake_mod.AsyncCodex = _Async # type: ignore[attr-defined]
|
||||
fake_mod.AppServerConfig = _FakeAppServerConfig # type: ignore[attr-defined]
|
||||
# Round 6b: safety enums must be present or the fail-closed
|
||||
# path raises before AppServerConfig ever gets consulted.
|
||||
fake_mod.ApprovalMode = _types.SimpleNamespace( # type: ignore[attr-defined]
|
||||
deny_all = "DENY_ALL",
|
||||
auto_review = "AUTO",
|
||||
)
|
||||
fake_mod.SandboxMode = _types.SimpleNamespace( # type: ignore[attr-defined]
|
||||
read_only = "READ_ONLY",
|
||||
workspace_write = "WW",
|
||||
)
|
||||
monkeypatch.setitem(sys.modules, "openai_codex", fake_mod)
|
||||
real_find_spec = _iu.find_spec
|
||||
monkeypatch.setattr(
|
||||
|
|
@ -1477,12 +1505,17 @@ class TestCodexHardenedRegressions:
|
|||
os.environ.get("HF_TOKEN") == "must_survive"
|
||||
), "scrubbed env leaked permanently when SDK construction failed"
|
||||
|
||||
def test_thread_start_skips_safety_kwargs_on_old_sdk(self, monkeypatch):
|
||||
"""If the installed SDK does not expose ApprovalMode or
|
||||
SandboxMode (older rev / alias), thread_start must still run
|
||||
-- failing closed would brick anyone on a pre-release build.
|
||||
The provider logs a warning and proceeds without the kwargs.
|
||||
def test_thread_start_fails_closed_when_safety_unavailable(self, monkeypatch):
|
||||
"""Round 6b: if the installed SDK cannot expose ApprovalMode or
|
||||
SandboxMode, the provider MUST fail closed rather than
|
||||
silently fall through to the SDK's `auto_review` default. A
|
||||
server-side chat surface with no per-action approval UI
|
||||
cannot tolerate the model deciding on its own to run shell
|
||||
commands. The error surfaces as a typed CodexUnavailableError
|
||||
the route layer translates to 503.
|
||||
"""
|
||||
# Make sure the override env var is NOT set.
|
||||
monkeypatch.delenv("UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS", raising = False)
|
||||
seen_kwargs: list[dict] = []
|
||||
|
||||
class _Async:
|
||||
|
|
@ -1496,8 +1529,54 @@ class TestCodexHardenedRegressions:
|
|||
seen_kwargs.append(dict(kw))
|
||||
return _FakeThread(chunks = ["ok"])
|
||||
|
||||
# Fake SDK without ApprovalMode / SandboxMode.
|
||||
_install_fake_codex_sdk(monkeypatch, _Async)
|
||||
_install_fake_codex_sdk(monkeypatch, _Async, with_safety_enums = False)
|
||||
from core.inference.codex_provider import (
|
||||
CodexUnavailableError,
|
||||
stream_codex,
|
||||
)
|
||||
|
||||
async def _collect():
|
||||
async for _ in stream_codex(
|
||||
messages = [{"role": "user", "content": "hi"}],
|
||||
model = "gpt-5.5",
|
||||
parallel_calls = 1,
|
||||
):
|
||||
pass
|
||||
|
||||
with pytest.raises(CodexUnavailableError) as exc_info:
|
||||
asyncio.run(_collect())
|
||||
assert "ApprovalMode" in str(exc_info.value) or "SandboxMode" in str(
|
||||
exc_info.value
|
||||
)
|
||||
assert not seen_kwargs, (
|
||||
"thread_start must NOT have been called when safety pins "
|
||||
"could not be applied"
|
||||
)
|
||||
|
||||
def test_thread_start_allows_unsafe_defaults_with_explicit_opt_in(
|
||||
self, monkeypatch
|
||||
):
|
||||
"""When the operator deliberately sets the
|
||||
UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS escape hatch, the provider
|
||||
proceeds without the safety pins (logs a warning) instead of
|
||||
raising. This is the dev-only override for pre-release alpha
|
||||
SDK builds that have not yet exposed ApprovalMode/SandboxMode.
|
||||
"""
|
||||
monkeypatch.setenv("UNSLOTH_CODEX_ALLOW_UNSAFE_DEFAULTS", "1")
|
||||
seen_kwargs: list[dict] = []
|
||||
|
||||
class _Async:
|
||||
async def __aenter__(self):
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *a):
|
||||
return False
|
||||
|
||||
async def thread_start(self, **kw):
|
||||
seen_kwargs.append(dict(kw))
|
||||
return _FakeThread(chunks = ["ok"])
|
||||
|
||||
_install_fake_codex_sdk(monkeypatch, _Async, with_safety_enums = False)
|
||||
from core.inference.codex_provider import stream_codex
|
||||
|
||||
async def _collect():
|
||||
|
|
@ -1509,14 +1588,10 @@ class TestCodexHardenedRegressions:
|
|||
pass
|
||||
|
||||
asyncio.run(_collect())
|
||||
assert seen_kwargs, "thread_start never called"
|
||||
assert seen_kwargs, "thread_start never called under override"
|
||||
kw = seen_kwargs[0]
|
||||
assert "approval_mode" not in kw, (
|
||||
"should not pass an unknown approval_mode value on an "
|
||||
"SDK that does not expose the enum"
|
||||
)
|
||||
assert "approval_mode" not in kw
|
||||
assert "sandbox" not in kw
|
||||
# Model still passed so the request is well-formed.
|
||||
assert kw.get("model") == "gpt-5.5"
|
||||
|
||||
def test_device_login_log_filter_drops_unknown_lines(self, monkeypatch):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue