From 8ee60019a4705b0fcefdc689674ffa85bdecbd10 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Sun, 24 May 2026 17:30:01 +0000 Subject: [PATCH] Studio: round 6 Codex hardening (5 follow-ups) Reviewer round 6 surfaced five real follow-ups on top of rounds 5 through 5g. Each one is a fix for an asymmetric guard or a wrong- shape lookup in the new Codex provider code: 1. Re-gate `installed=True` on having BOTH the SDK AND a `codex` binary on PATH. Round 5 widened the gate to SDK-only, but the login route still shells out to the binary, so an SDK-only host would surface a Codex row whose Sign-in button immediately failed with "codex CLI not found on PATH". The canonical `openai-codex` package depends on `openai-codex-cli-bin` which places the shim on PATH for free, so the common install still lights up; the gate just refuses to advertise a provider Studio cannot actually drive end-to-end. 2. `_safe_thread_safety_kwargs` now also probes `.api` and `.generated.v2_all` for `SandboxMode`. Upstream `openai_codex` exports `ApprovalMode` at the top level but `SandboxMode` lives under `openai_codex.generated.v2_all`. The previous lookup returned `{}` on the canonical SDK install, so every thread_start ran with the unsafe `auto_review` default. Submodule probe resolves the canonical layout and keeps backwards-compat with builds that DID re-export at the top level. 3. `_coerce_text` now applies the answer-event-type filter on the object path too. The upstream SDK emits typed payload classes like `CommandExecutionOutputDelta`, `FileChangeDelta`, `ToolCallDelta`, `PatchApplyDelta`, etc., all of which carry a `.delta` string of local stdout / file paths / tool args. The dict path already filtered these out; the object path used to return `.delta` unconditionally, so a real SDK install could leak tool output into the visible chat reply. 4. `_ScrubbedEnvAsyncCodex` is now process-wide concurrency-safe AND fails-closed if the SDK constructor raises: - Refcount each scrubbed key under an `asyncio.Lock` so a fan-out wrapper that exits early cannot restore a secret while another wrapper is still inside SDK startup (round 6 reproduced this: wrapper A exited, wrapper B's SDK saw the restored HF_TOKEN). - Move `_async_codex_cls()` and its `__aenter__` INSIDE a try/except in `__aenter__`; on failure, run the release path so the scrubbed env vars are restored even though `__aexit__` never fires for the failed construction. 5. `_run_cli` now detaches into its own process group via `start_new_session=True` (Unix) / `CREATE_NEW_PROCESS_GROUP` (Windows) and kills the whole group on timeout, matching the protected path in `stream_codex_device_login`. A shimmed `codex login status` that forks a helper and blocks no longer leaves the child running after we killed the parent. Frontend follow-up: chat-adapter now routes every rendered yield through a `renderFullContent()` helper so the Codex per-tab text accumulated in earlier `_toolEvent` frames is preserved when the synthesis content delta arrives. Previously the next regular content yield rebuilt `parts` from `cumulativeText` alone and the tab section vanished from the final assistant message. Tests: 49 cases total (was 47). New regressions: - `installed_requires_both_cli_and_sdk` (round 6 revert). - `safety_kwargs_finds_sandbox_mode_in_submodule` (canonical SDK layout where `SandboxMode` is in `.generated.v2_all`). - `scrubbed_env_construction_failure_restores_env` (no permanent env leak when the SDK constructor raises). - Expanded `coerce_text_drops_non_answer_event_types` to also exercise the object-shape code path with `CommandExecutionOutputDelta`, `FileChangeDelta`, `ToolCallDelta`, `PatchApplyDelta`, `PlanUpdateDelta`, `AgentReasoningDelta`, plus the positive `AgentMessageDelta` allow-through. --- .../core/inference/codex_availability.py | 71 +++++-- .../backend/core/inference/codex_provider.py | 164 ++++++++++++--- studio/backend/tests/test_codex_provider.py | 192 ++++++++++++++++-- .../src/features/chat/api/chat-adapter.ts | 31 ++- 4 files changed, 398 insertions(+), 60 deletions(-) diff --git a/studio/backend/core/inference/codex_availability.py b/studio/backend/core/inference/codex_availability.py index 9699d2224f..7383c57311 100644 --- a/studio/backend/core/inference/codex_availability.py +++ b/studio/backend/core/inference/codex_availability.py @@ -158,19 +158,35 @@ async def _run_cli(args: list[str], *, timeout: float = 4.0) -> tuple[int, str, """Run a short ``codex`` CLI command and return (rc, stdout, stderr). The probe uses 4s as the wall-clock cap because ``codex --version`` - and ``codex auth status`` both return in well under a second on a + and ``codex login status`` both return in well under a second on a healthy install. A longer probe would block the ``/api/codex/status`` route -- and that route fires on every chat page load, so a tight cap matters. + + Subprocess lifecycle: detached into its own process group on Unix + via ``start_new_session=True`` (matching ``stream_codex_device_login``) + so a hung child cannot survive ``proc.kill()`` on timeout. Without + this, a shimmed ``codex login status`` that forks a helper then + blocks would leave the helper running after we killed the parent. + Windows uses ``CREATE_NEW_PROCESS_GROUP`` for the analogous + isolation. Round 6 reviewer caught the asymmetry with the + device-login path that already had this guard. """ + import os + import signal + + spawn_kwargs: dict[str, Any] = { + "stdout": asyncio.subprocess.PIPE, + "stderr": asyncio.subprocess.PIPE, + "env": _codex_subprocess_env(), + } + if os.name == "posix": + spawn_kwargs["start_new_session"] = True + elif os.name == "nt": + spawn_kwargs["creationflags"] = 0x00000200 # CREATE_NEW_PROCESS_GROUP + try: - proc = await asyncio.create_subprocess_exec( - "codex", - *args, - stdout = asyncio.subprocess.PIPE, - stderr = asyncio.subprocess.PIPE, - env = _codex_subprocess_env(), - ) + proc = await asyncio.create_subprocess_exec("codex", *args, **spawn_kwargs) except FileNotFoundError: return -1, "", "codex binary not on PATH" except Exception as exc: @@ -184,9 +200,21 @@ async def _run_cli(args: list[str], *, timeout: float = 4.0) -> tuple[int, str, try: stdout_b, stderr_b = await asyncio.wait_for(proc.communicate(), timeout = timeout) except asyncio.TimeoutError: + # Kill the whole process group, not just the parent, so any + # child the codex CLI forked also dies. + if os.name == "posix": + try: + os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + except (ProcessLookupError, PermissionError): + pass + else: + try: + proc.send_signal(signal.CTRL_BREAK_EVENT) # type: ignore[attr-defined] + except Exception: + pass proc.kill() try: - await proc.wait() + await asyncio.wait_for(proc.wait(), timeout = 1.0) except Exception: pass return -1, "", f"codex {' '.join(args)} timed out after {timeout:.1f}s" @@ -268,12 +296,16 @@ async def probe_codex_availability() -> dict[str, Any]: Returns a dict with keys: - * ``installed`` (bool) -- True iff Studio can actually drive Codex. - The SDK's `openai-codex-cli-bin` runtime is what backs - `AsyncCodex(...)`, so an importable SDK alone is sufficient even - with no standalone `codex` on PATH. We still report `cli_path` - separately so the UI can surface "CLI also present" / "SDK - bundled runtime only" without changing the gate. + * ``installed`` (bool) -- True iff Studio can actually drive Codex + end-to-end: BOTH the Python SDK (for chat) AND a `codex` + executable on PATH (for the device-auth login flow). The + canonical `openai-codex` package depends on `openai-codex-cli-bin` + which installs the `codex` shim into the venv's `bin/`, so the + common SDK-only install in fact gets the CLI on PATH for free + and this gate triggers correctly. Hosts that import the SDK + from a wheel without that runtime dep stay hidden because the + login flow would otherwise fail with "codex CLI not found on + PATH" after the user clicked Sign in. * ``cli_path`` (str | None) -- absolute path to the CLI, or None. * ``sdk_importable`` (bool) -- the Python SDK is importable. * ``logged_in`` (bool) -- best-effort auth check; meaningless when @@ -285,9 +317,12 @@ async def probe_codex_availability() -> dict[str, Any]: sdk_ok = _sdk_importable() payload: dict[str, Any] = { - # SDK alone is enough -- it bundles the codex runtime. A - # standalone CLI on PATH is additional but optional. - "installed": sdk_ok, + # Gate on BOTH because the login flow shells out to `codex`. + # Round 5 briefly set this to `sdk_ok` alone, but round 6 + # caught that the login route would then fail with + # `codex CLI not found on PATH` after the user clicked + # Sign in, leaving them with an unusable provider row. + "installed": bool(cli_path) and sdk_ok, "cli_path": cli_path, "sdk_importable": sdk_ok, "logged_in": False, diff --git a/studio/backend/core/inference/codex_provider.py b/studio/backend/core/inference/codex_provider.py index ca3dad0e37..35a2ef4393 100644 --- a/studio/backend/core/inference/codex_provider.py +++ b/studio/backend/core/inference/codex_provider.py @@ -93,6 +93,10 @@ def _codex_sdk_env_override() -> dict[str, str]: return {key: "" for key in os.environ if key not in safe} +_SCRUBBED_ENV_LOCK = asyncio.Lock() +_SCRUBBED_ENV_REFCOUNT: dict[str, int] = {} + + class _ScrubbedEnvAsyncCodex: """Async-context wrapper that swaps `os.environ` for the lifetime of a Codex SDK session. @@ -101,40 +105,85 @@ class _ScrubbedEnvAsyncCodex: `AppServerConfig(env=...)`. The SDK starts its app-server with `env = os.environ.copy()`, so removing secrets from the parent process env right before construction keeps them out of the child. - Restore happens on exit, and the restore is `setdefault`-style so - concurrent wrappers do not clobber each other's state. + + Concurrency model: a process-wide asyncio lock serialises the + enter/exit critical section, and a per-key refcount tracks how + many concurrent wrappers are currently "holding" the scrub. A + key is only restored when the last wrapper using it exits. This + fixes two issues round 6 caught: + + 1. Two concurrent fan-out workers used to race: wrapper A could + restore `HF_TOKEN` while wrapper B was still inside SDK + startup, letting B's spawned app-server inherit the secret. + The refcount keeps the key scrubbed for the full overlap + window. + 2. If the SDK constructor raised before `__aenter__` returned, + Python never called `__aexit__`, so the deleted keys leaked + permanently. Construction now happens INSIDE the try/except + in `__aenter__`, and the scrub is rolled back on failure. """ def __init__(self, async_codex_cls: Any): self._async_codex_cls = async_codex_cls self._inner: Any = None - self._saved_env: dict[str, str] | None = None + # Keys this wrapper instance contributed to the refcount, so + # __aexit__ knows exactly which counters to decrement (avoids + # racing with concurrent wrappers that scrub a different set). + self._held_keys: list[str] = [] + # Snapshot of the original values at the time of the FIRST + # wrapper that scrubbed each key, so restoration uses the + # real pre-scrub value. + self._restored_via_us: dict[str, str] = {} async def __aenter__(self) -> Any: import os overrides = _codex_sdk_env_override() - saved: dict[str, str] = {} - for key in overrides: - if key in os.environ: - saved[key] = os.environ[key] - del os.environ[key] - self._saved_env = saved - self._inner = self._async_codex_cls() - return await self._inner.__aenter__() + async with _SCRUBBED_ENV_LOCK: + for key in overrides: + if key not in os.environ and _SCRUBBED_ENV_REFCOUNT.get(key, 0) == 0: + continue + if _SCRUBBED_ENV_REFCOUNT.get(key, 0) == 0: + # First wrapper to scrub this key -- save the + # original so the very last wrapper to release it + # can restore the right value. + self._restored_via_us[key] = os.environ[key] + del os.environ[key] + _SCRUBBED_ENV_REFCOUNT[key] = _SCRUBBED_ENV_REFCOUNT.get(key, 0) + 1 + self._held_keys.append(key) + try: + self._inner = self._async_codex_cls() + return await self._inner.__aenter__() + except BaseException: + # Roll back the scrub if SDK construction / enter fails; + # otherwise the deleted env vars would leak permanently. + await self._release_held_keys() + raise async def __aexit__(self, exc_type, exc, tb): - import os - try: if self._inner is not None: return await self._inner.__aexit__(exc_type, exc, tb) finally: - saved = self._saved_env or {} - for key, value in saved.items(): - # Only restore keys we removed; setdefault avoids - # clobbering a concurrent caller's value. - os.environ.setdefault(key, value) + await self._release_held_keys() + + async def _release_held_keys(self) -> None: + import os + + async with _SCRUBBED_ENV_LOCK: + for key in self._held_keys: + current = _SCRUBBED_ENV_REFCOUNT.get(key, 0) + if current <= 0: + continue + _SCRUBBED_ENV_REFCOUNT[key] = current - 1 + if current - 1 == 0: + # Last wrapper holding this key -- restore the + # original value if WE were the first to scrub it, + # or pull from any other wrapper's saved snapshot. + if key in self._restored_via_us: + os.environ.setdefault(key, self._restored_via_us[key]) + self._held_keys.clear() + self._restored_via_us.clear() def _open_async_codex(async_codex_cls: Any) -> Any: @@ -399,6 +448,18 @@ def _coerce_text(payload: Any) -> str: visible text. Tool / command / plan deltas are dropped so local stdout, file paths, or tool-call arguments never flow into the Chat Completions content stream. + + Both dict-shaped events (tests + some pre-release SDKs) AND + object-shaped events (the real upstream SDK's typed notification + classes) are gated -- if the payload exposes a `type` attribute + or key whose value is not in the answer-event allow-list, we + return the empty string regardless of whether `.delta` or `.text` + is present. Round 6 reviewer caught the object-shape gap: the + upstream SDK can emit `item/commandExecution/outputDelta`, + `item/fileChange/outputDelta`, etc. as typed objects, all of + which carry `.delta` strings containing local stdout, patches, + or tool arguments. Without the object-side filter those strings + would have flowed straight into visible assistant text. """ if payload is None: return "" @@ -419,6 +480,33 @@ def _coerce_text(payload: Any) -> str: return "" if isinstance(payload, list): return "".join(_coerce_text(item) for item in payload) + # Object path: gate on `payload.type` if present, AND on the class + # name as a fallback (the upstream SDK uses class names like + # `AgentMessageDeltaNotification` / `CommandExecutionOutputDelta` + # so a denylist-by-substring catches typed payloads that lack a + # `type` attribute). + ev_type_obj = getattr(payload, "type", None) + if isinstance(ev_type_obj, str) and ev_type_obj not in _ANSWER_EVENT_TYPES: + return "" + cls_name = payload.__class__.__name__ + # Allow only class names that contain "Message" or "Delta" without + # also containing a tool / command / plan / file marker. + cls_lower = cls_name.lower() + if any( + marker in cls_lower + for marker in ( + "command", + "exec", + "file", + "patch", + "plan", + "tool", + "reason", + "stdout", + "stderr", + ) + ): + return "" text_attr = getattr(payload, "text", None) if isinstance(text_attr, str): return text_attr @@ -637,18 +725,46 @@ def _safe_thread_safety_kwargs() -> dict[str, Any]: * ``sandbox = SandboxMode.read_only`` -- the policy that bans file writes and disables network access. - Returns an empty dict when the installed SDK is too old to expose - either symbol; the caller then issues a structured warning and - proceeds without the safety pins. Failing closed (refusing to - run) on an older SDK would brick users on pre-release alpha - builds for no security gain -- the auto_review default is - upstream's choice, not a Studio regression. + Probes multiple locations: ``ApprovalMode`` is exported at the + top-level ``openai_codex`` package, but ``SandboxMode`` lives in + ``openai_codex.generated.v2_all`` (re-exported into + ``openai_codex.api``) and is NOT in the top-level __init__. + Round 6 reviewer caught this -- looking only at the top-level + module returned ``{}``, silently degrading to the auto_review + default on the canonical SDK install. + + Returns an empty dict only when the installed SDK is so different + that neither path resolves -- the caller then issues a + structured warning and proceeds. Failing closed (refusing to + run) on a future SDK rev would brick users for no security gain + -- the auto_review default is upstream's choice, not a Studio + regression. """ sdk_mod = sys.modules.get("openai_codex") or sys.modules.get("codex_app_server") if sdk_mod is None: return {} + + # ApprovalMode: top-level export on canonical SDK. approval_mode_cls = getattr(sdk_mod, "ApprovalMode", None) + + # SandboxMode: try top-level, then `.api`, then `.generated.v2_all`. + # We do not eagerly import these submodules because the SDK may + # not expose them and we do not want to crash the request on an + # ImportError. importlib.import_module gives us a typed failure. sandbox_mode_cls = getattr(sdk_mod, "SandboxMode", None) + if sandbox_mode_cls is None: + for sub in ("api", "generated.v2_all"): + mod_name = getattr(sdk_mod, "__name__", "") + if not mod_name: + continue + try: + submod = importlib.import_module(f"{mod_name}.{sub}") + except Exception: + continue + sandbox_mode_cls = getattr(submod, "SandboxMode", None) + if sandbox_mode_cls is not None: + break + if approval_mode_cls is None or sandbox_mode_cls is None: return {} deny_all = getattr(approval_mode_cls, "deny_all", None) diff --git a/studio/backend/tests/test_codex_provider.py b/studio/backend/tests/test_codex_provider.py index 27f40c7e77..69a1ff74e2 100644 --- a/studio/backend/tests/test_codex_provider.py +++ b/studio/backend/tests/test_codex_provider.py @@ -1012,24 +1012,39 @@ class TestCodexHardenedRegressions: assert '"content": "hello "' in body assert '"content": "from turn.stream"' in body - def test_installed_true_when_sdk_only(self, monkeypatch): - """SDK alone is sufficient: openai-codex-cli-bin ships the - runtime that backs `AsyncCodex(...)`, so the picker must be - shown even when no standalone `codex` lives on PATH. + def test_installed_requires_both_cli_and_sdk(self, monkeypatch): + """Round 6 revert: the login route shells out to `codex`, so + marking `installed=True` on SDK-only would surface a Codex + provider row whose Sign-in button immediately fails. The + canonical `openai-codex` package installs `openai-codex-cli-bin` + which puts the `codex` shim on PATH, so common installs still + light up correctly; the gate just refuses to advertise a + provider Studio cannot actually drive. """ from core.inference import codex_availability as ca + # SDK present, no CLI -> hidden (cannot complete login). monkeypatch.setattr(ca, "_which_codex", lambda: None) monkeypatch.setattr(ca, "_sdk_importable", lambda: True) - payload = asyncio.run(ca.probe_codex_availability()) - assert payload["installed"] is True + assert payload["installed"] is False assert payload["cli_path"] is None assert payload["sdk_importable"] is True - # logged_in stays False because the version/login probes only - # run when a CLI is present (they shell out to it). That is the - # expected behaviour, not a bug. - assert payload["logged_in"] is False + + # CLI present, SDK missing -> still hidden (cannot drive chat). + monkeypatch.setattr(ca, "_which_codex", lambda: "/usr/bin/codex") + monkeypatch.setattr(ca, "_sdk_importable", lambda: False) + + async def fake_version(): + return "codex-cli 0.133.0" + + async def fake_logged_in(): + return True + + monkeypatch.setattr(ca, "_detect_version", fake_version) + monkeypatch.setattr(ca, "_detect_logged_in", fake_logged_in) + payload2 = asyncio.run(ca.probe_codex_availability()) + assert payload2["installed"] is False def test_base_instructions_kwarg_preferred(self, monkeypatch): """The upstream openai_codex SDK uses `base_instructions` for @@ -1181,6 +1196,11 @@ class TestCodexHardenedRegressions: that must NOT be rendered as assistant text -- otherwise local stdout, file paths, or tool-call arguments would leak into the Chat Completions reply. + + Round 6 also requires the object-shape path to gate on type + and class name; the upstream SDK emits typed notification + objects (CommandExecutionOutputDelta, FileChangeDelta, etc.) + with `.delta` strings that would otherwise leak. """ from core.inference.codex_provider import _coerce_text @@ -1189,8 +1209,7 @@ class TestCodexHardenedRegressions: assert _coerce_text({"type": "completed", "text": "done"}) == "done" assert _coerce_text({"type": "text_delta", "delta": "x"}) == "x" - # Non-answer event types are silenced even when they expose a - # delta string that looks like prose. + # Non-answer dict event types are silenced. for ev_type in ( "command.delta", "command_output", @@ -1208,6 +1227,53 @@ class TestCodexHardenedRegressions: f"{ev_type} leaked text into assistant reply: " f"{_coerce_text(payload)!r}" ) + + # Object-shape gate: typed payloads whose class name contains + # a tool/command/file/patch/plan marker drop the .delta too. + class CommandExecutionOutputDelta: + delta = "SECRET_STDOUT" + + class FileChangeDelta: + delta = "secret/file/path" + + class ToolCallDelta: + text = "tool_arg_payload" + + class PatchApplyDelta: + delta = "diff --git a/secret" + + class PlanUpdateDelta: + delta = "plan content" + + class AgentReasoningDelta: + delta = "internal CoT" + + for obj in ( + CommandExecutionOutputDelta(), + FileChangeDelta(), + ToolCallDelta(), + PatchApplyDelta(), + PlanUpdateDelta(), + AgentReasoningDelta(), + ): + assert _coerce_text(obj) == "", ( + f"object-shape {obj.__class__.__name__} leaked: " + f"{_coerce_text(obj)!r}" + ) + + # Object with explicit type attr also drops if not in allow-list. + class _WithType: + type = "command.delta" + delta = "leak" + + assert _coerce_text(_WithType()) == "" + + # Object-shape answer events DO pass through. + class AgentMessageDelta: + delta = "real assistant text" + + assert _coerce_text(AgentMessageDelta()) == "real assistant text" + # Plain strings and untyped dicts still pass through (legacy path). assert _coerce_text("raw text") == "raw text" assert _coerce_text({"text": "no type tag"}) == "no type tag" @@ -1309,6 +1375,108 @@ class TestCodexHardenedRegressions: kw.get("sandbox") == "READ_ONLY_SENTINEL" ), f"sandbox not pinned to read_only: {kw}" + def test_safety_kwargs_finds_sandbox_mode_in_submodule(self, monkeypatch): + """Round 6 caught that `SandboxMode` is exported by the + upstream SDK from `openai_codex.generated.v2_all`, NOT from + the top-level `openai_codex` package. The previous lookup + used `getattr(sdk_mod, 'SandboxMode', None)` only and returned + None for the canonical SDK install, silently degrading to + the unsafe auto_review default. + """ + 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"]) + + # Build a fake openai_codex that DOES NOT expose SandboxMode + # at the top level -- only inside `.generated.v2_all`. + import importlib.util as _iu + + fake_root = types.ModuleType("openai_codex") + fake_root.AsyncCodex = _Async # type: ignore[attr-defined] + fake_root.ApprovalMode = types.SimpleNamespace( # type: ignore[attr-defined] + deny_all = "DENY_ALL", + auto_review = "AUTO", + ) + # Submodule chain `.generated.v2_all` + fake_generated = types.ModuleType("openai_codex.generated") + fake_v2 = types.ModuleType("openai_codex.generated.v2_all") + fake_v2.SandboxMode = types.SimpleNamespace( # type: ignore[attr-defined] + read_only = "READ_ONLY", + workspace_write = "WW", + ) + fake_generated.v2_all = fake_v2 # type: ignore[attr-defined] + fake_root.generated = fake_generated # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "openai_codex", fake_root) + monkeypatch.setitem(sys.modules, "openai_codex.generated", fake_generated) + monkeypatch.setitem(sys.modules, "openai_codex.generated.v2_all", fake_v2) + real_find_spec = _iu.find_spec + monkeypatch.setattr( + "importlib.util.find_spec", + lambda n, *a, **kw: ( + types.SimpleNamespace() + if n in ("openai_codex", "codex_app_server") + else real_find_spec(n, *a, **kw) + ), + ) + + from core.inference.codex_provider import stream_codex + + async def _collect(): + async for _ in stream_codex( + messages = [{"role": "user", "content": "hi"}], + model = "gpt-5.5", + parallel_calls = 1, + ): + pass + + asyncio.run(_collect()) + assert seen_kwargs, "thread_start never called" + kw = seen_kwargs[0] + assert ( + kw.get("approval_mode") == "DENY_ALL" + ), f"approval_mode not pinned even with submodule SandboxMode: {kw}" + assert ( + kw.get("sandbox") == "READ_ONLY" + ), f"sandbox not pinned via submodule lookup: {kw}" + + def test_scrubbed_env_construction_failure_restores_env(self, monkeypatch): + """Round 6: if the SDK constructor raises before __aenter__ + returns, the previous wrapper never called __aexit__ so the + scrubbed env vars leaked permanently. Now the scrub is rolled + back on failure. + """ + from core.inference.codex_provider import _ScrubbedEnvAsyncCodex + + monkeypatch.setenv("HF_TOKEN", "must_survive") + + class _FailingAsync: + def __init__(self): + raise RuntimeError("SDK construction failed") + + async def _run(): + wrapper = _ScrubbedEnvAsyncCodex(_FailingAsync) + try: + async with wrapper: + pass + except RuntimeError: + pass + + asyncio.run(_run()) + # HF_TOKEN must be restored even though __aexit__ never fired + # for the failed construction. + assert ( + 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 diff --git a/studio/frontend/src/features/chat/api/chat-adapter.ts b/studio/frontend/src/features/chat/api/chat-adapter.ts index 086aa25319..8cd6b903d6 100644 --- a/studio/frontend/src/features/chat/api/chat-adapter.ts +++ b/studio/frontend/src/features/chat/api/chat-adapter.ts @@ -1164,6 +1164,17 @@ export function createOpenAIStreamAdapter(): ChatModelAdapter { } return lines.join(""); } + + // All chat-content yields go through this so the Codex per-tab + // output is always concatenated with the normal SSE text. The + // synthesis is emitted by the backend BOTH as a `codex_gather` + // tool event AND as a normal content delta; rendering both + // would duplicate it. Render the tabs above (header / tab text) + // separately from cumulativeText (which carries the synthesis + // content delta) so the user sees `[tabs] ... [synthesis]`. + function renderFullContent(): string { + return cumulativeText + renderCodexBuffer(); + } // Tracks whether we are currently inside a `` block opened by // a `delta.reasoning_content` chunk. Kimi (kimi-k2.6, kimi-k2-thinking) // and DeepSeek's reasoner stream their thinking as a separate @@ -1663,8 +1674,7 @@ export function createOpenAIStreamAdapter(): ChatModelAdapter { } else if (toolEvent.type === "codex_gather") { codexGatherEmitted = true; } - const codexBlock = renderCodexBuffer(); - const codexParts = parseAssistantContent(cumulativeText + codexBlock); + const codexParts = parseAssistantContent(renderFullContent()); yield { content: [...toolCallParts, ...codexParts], }; @@ -1781,8 +1791,12 @@ export function createOpenAIStreamAdapter(): ChatModelAdapter { }; } } - // Yield cumulative state so tool UI updates (tools first, text after) - const textParts = parseAssistantContent(cumulativeText); + // Yield cumulative state so tool UI updates (tools first, text after). + // Use renderFullContent() so any Codex per-tab text accumulated + // in earlier _toolEvent frames is preserved when the synthesis + // delta arrives -- without this the tabs would briefly appear + // and then vanish when the regular content path overwrote them. + const textParts = parseAssistantContent(renderFullContent()); yield { content: [...toolCallParts, ...textParts], metadata: { @@ -1912,7 +1926,9 @@ export function createOpenAIStreamAdapter(): ChatModelAdapter { "", ); } - const parts = parseAssistantContent(cumulativeText); + // renderFullContent() preserves any Codex per-tab text the + // fan-out branch accumulated into codexTabBuffers. + const parts = parseAssistantContent(renderFullContent()); if ( parts.some((part) => part.type === "reasoning") && @@ -2016,7 +2032,10 @@ export function createOpenAIStreamAdapter(): ChatModelAdapter { yield { content: [ ...toolCallParts, - ...parseAssistantContent(cumulativeText), + // renderFullContent() ensures the Codex per-tab text is in + // the FINAL message too -- otherwise the synthesis delta on + // the regular content path would have erased it. + ...parseAssistantContent(renderFullContent()), ...sourceParts, ], metadata: {