diff --git a/studio/backend/core/inference/codex_provider.py b/studio/backend/core/inference/codex_provider.py index bb6ab71e56..db790f623e 100644 --- a/studio/backend/core/inference/codex_provider.py +++ b/studio/backend/core/inference/codex_provider.py @@ -589,11 +589,34 @@ async def _stream_thread_run( # Only reached when no streaming helper emitted anything, so this # is the first (and only) execution of the turn. result = await thread.run(prompt) - text = _coerce_text(result) or getattr(result, "final_response", "") or str(result) + text = _buffered_result_text(result) if text: yield text +def _buffered_result_text(result: Any) -> str: + """Extract assistant text from a buffered ``TurnResult``. + + The upstream SDK documents ``TurnResult.final_response`` as + nullable -- a turn that performs only tool work and completes + without a final assistant message will set it to ``None``. The + previous ``... or str(result)`` fallback then sent a Python + object repr (``TurnResult(...)``) into the chat, which surfaced + as visible garbage to the user. Returning the empty string for + that case lets the OpenAI-shape stream finish cleanly with no + extra content chunk -- the usage / stop / [DONE] frames still + fire, and the chat UI simply shows no assistant text rather + than a misleading object dump. + """ + text = _coerce_text(result) + if text: + return text + final = getattr(result, "final_response", None) + if isinstance(final, str) and final: + return final + return "" + + def _safe_thread_safety_kwargs() -> dict[str, Any]: """Return the safe ``approval_mode`` + ``sandbox`` kwargs for thread_start. @@ -1023,9 +1046,10 @@ async def _run_codex_synthesis( codex, model, system, synthesis_prompt ) result = await thread.run(synthesis_prompt) - return ( - _coerce_text(result) or getattr(result, "final_response", "") or str(result) - ) + # Use the same buffered extraction as `_stream_thread_run` so a + # synthesis turn whose `final_response` is None returns an empty + # string instead of a `TurnResult(...)` Python object repr. + return _buffered_result_text(result) except Exception as exc: logger.warning("codex_provider.synthesis_failed", error = str(exc)) return "" @@ -1116,6 +1140,35 @@ async def stream_codex_device_login() -> AsyncGenerator[dict[str, Any], None]: rc: int = -1 cancelled = False + # Allow-list of substrings the upstream `codex login --device-auth` + # command prints during the normal flow. Anything outside this list + # is treated as opaque and not forwarded to the browser, so a + # shimmed binary that prints auth JSON, refresh tokens, local + # config paths, or unexpected stderr cannot leak that content + # through Studio's authenticated SSE stream. The URL and code + # extracted above are emitted separately as `device_url` / + # `device_code` events and are not affected by this filter. + safe_log_patterns: tuple[str, ...] = ( + "welcome to codex", + "initializing", + "open this", + "open:", + "open the", + "verification", + "enter this one-time code", + "enter the code", + "waiting", + "successfully logged in", + "logged in", + "signed in", + "browser opened", + "press ctrl", + ) + + def _safe_to_forward(text: str) -> bool: + lowered = text.lower() + return any(pat in lowered for pat in safe_log_patterns) + try: assert proc.stdout is not None while True: @@ -1134,7 +1187,11 @@ async def stream_codex_device_login() -> AsyncGenerator[dict[str, Any], None]: if cm: yield {"type": "device_code", "code": cm.group(1)} code_emitted = True - yield {"type": "log", "line": line} + # Only forward lines from the known safe vocabulary; opaque + # output (file paths, tokens, JSON, error messages) stays in + # backend logs only. + if line and _safe_to_forward(line): + yield {"type": "log", "line": line} except (asyncio.CancelledError, GeneratorExit): cancelled = True raise diff --git a/studio/backend/models/inference.py b/studio/backend/models/inference.py index 65a73dd65f..e079414357 100644 --- a/studio/backend/models/inference.py +++ b/studio/backend/models/inference.py @@ -788,19 +788,45 @@ class ChatCompletionRequest(BaseModel): ) parallel_calls: int = Field( default = 1, - ge = 1, - le = 20, description = ( "[x-unsloth] Codex provider only. When > 1, fan the chat turn " "out across N parallel Codex calls and synthesise a unified " "final answer. Each parallel attempt is rendered as its own tab " "in the chat UI; a final 'Synthesis' tab carries the merged " - "output. Bounded to [1, 20] by pydantic so a runaway value can't " - "saturate the local CLI. Defaults to 1 (single-call shape). " + "output. Silently clamped to [1, 20] by `_clamp_parallel_calls` " + "so a runaway value cannot saturate the local CLI -- using a " + "validator (rather than `ge=1, le=20`) keeps backwards " + "compatibility with pre-PR clients that sent the field as a " + "stray OpenAI extra (e.g. `0` for 'no fan-out') and would " + "otherwise hit a 422. Defaults to 1 (single-call shape). " "Silently ignored on every provider other than `codex`." ), ) + @field_validator("parallel_calls", mode = "before") + @classmethod + def _clamp_parallel_calls(cls, value: Any) -> int: + """Coerce ``parallel_calls`` to [1, 20] without rejecting weird inputs. + + Pre-PR behaviour was to silently ignore unknown / out-of-range + OpenAI extras; using ``ge=1, le=20`` on the Field would have + regressed that by returning a 422 to any non-Codex client that + happened to set the field to 0 or omit it as ``None``. Coerce + the value here instead so the schema stays self-documenting + ([1, 20]) while accepting legacy inputs. + """ + if value is None: + return 1 + try: + n = int(value) + except (TypeError, ValueError): + return 1 + if n < 1: + return 1 + if n > 20: + return 20 + return n + @model_validator(mode = "after") def _resolve_missing_tool_call_ids(self) -> "ChatCompletionRequest": """Fill missing tool_call_id by walking back to the preceding assistant. diff --git a/studio/backend/tests/test_codex_provider.py b/studio/backend/tests/test_codex_provider.py index 372bee770e..2b52bc5f94 100644 --- a/studio/backend/tests/test_codex_provider.py +++ b/studio/backend/tests/test_codex_provider.py @@ -16,9 +16,10 @@ Covers: * Parallel-calls fan-out: ``parallel_calls > 1`` spawns N async tasks and emits ``codex_tab_open`` / ``codex_tab_chunk`` / ``codex_tab_close`` events plus a final ``codex_gather`` synthesis event. -* Request validator: ``parallel_calls`` is clamped to [1, 20] by - pydantic so a runaway value is rejected with 422 before any Codex - task is spawned. +* Request validator: ``parallel_calls`` is silently clamped to + [1, 20] by a Pydantic field validator (not by ``ge=1, le=20``) so + non-Codex clients that send legacy values like ``0`` continue to + be accepted instead of getting a 422. """ from __future__ import annotations @@ -429,27 +430,47 @@ class TestParallelCallsValidator: ) assert req.parallel_calls == n - def test_request_rejects_below_one(self): + def test_request_clamps_below_one(self): + """Pre-PR clients sometimes sent `parallel_calls=0` as a stray + OpenAI extra and the request was silently accepted; rejecting + with 422 would regress that. The validator now clamps to 1. + """ from models.inference import ChatCompletionRequest - from pydantic import ValidationError - with pytest.raises(ValidationError): - ChatCompletionRequest( + for n in (0, -1, -100): + req = ChatCompletionRequest( model = "gpt-5.4", messages = [{"role": "user", "content": "hi"}], - parallel_calls = 0, + parallel_calls = n, ) + assert req.parallel_calls == 1, f"clamp failed for {n}" - def test_request_rejects_above_twenty(self): + def test_request_clamps_above_twenty(self): + """A runaway value (1000, etc.) is clamped to the 20 cap so it + cannot saturate the local CLI even when the client misbehaves. + """ from models.inference import ChatCompletionRequest - from pydantic import ValidationError - with pytest.raises(ValidationError): - ChatCompletionRequest( + for n in (21, 100, 1000): + req = ChatCompletionRequest( model = "gpt-5.4", messages = [{"role": "user", "content": "hi"}], - parallel_calls = 21, + parallel_calls = n, ) + assert req.parallel_calls == 20, f"clamp failed for {n}" + + def test_request_coerces_garbage_to_one(self): + """Strings / floats / None coerce to 1 instead of 422 so a + legacy or misconfigured client cannot break chat for everyone.""" + from models.inference import ChatCompletionRequest + + for value in (None, "garbage", float("nan")): + req = ChatCompletionRequest( + model = "gpt-5.4", + messages = [{"role": "user", "content": "hi"}], + parallel_calls = value, + ) + assert req.parallel_calls == 1 def test_request_default_is_one(self): """Default = 1 so the field matches the single-call code path @@ -1330,6 +1351,129 @@ class TestCodexHardenedRegressions: # 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): + """The login stream's `log` events must not forward arbitrary + subprocess output. Only an allow-list of known progress + strings reaches the browser; anything else (auth JSON, + tokens, paths, error tails) stays in backend logs. + """ + # Build a synthetic stdout stream with one safe line and one + # unsafe line, then drive the login generator against it. + from core.inference import codex_provider as cp + + class _FakeStdout: + def __init__(self, lines: list[bytes]): + self._lines = list(lines) + + async def readline(self) -> bytes: + if not self._lines: + return b"" + return self._lines.pop(0) + + class _FakeProc: + pid = 99999 + returncode = None + stdout = _FakeStdout( + [ + b"Welcome to Codex\n", + b"Open: https://auth.openai.com/codex/device\n", + b"Enter this one-time code: ABCD-EFGH\n", + b'{"refresh_token": "rt_LEAK_LEAK_LEAK"}\n', + b"/home/u/.codex/auth.json saved\n", + b"Successfully logged in\n", + ], + ) + + async def wait(self): + self.returncode = 0 + return 0 + + def kill(self): + self.returncode = -9 + + def terminate(self): + self.returncode = -15 + + async def _fake_create_subprocess_exec(*a, **kw): + return _FakeProc() + + monkeypatch.setattr( + cp.asyncio, "create_subprocess_exec", _fake_create_subprocess_exec + ) + + events: list[dict] = [] + + async def _collect(): + async for ev in cp.stream_codex_device_login(): + events.append(ev) + + asyncio.run(_collect()) + log_lines = [ev.get("line", "") for ev in events if ev.get("type") == "log"] + joined = "\n".join(log_lines) + # Sensitive content must not have been forwarded. + assert "refresh_token" not in joined, f"token leaked: {joined!r}" + assert "rt_LEAK_LEAK_LEAK" not in joined + assert "auth.json" not in joined, f"local config path leaked: {joined!r}" + # The known-safe progress lines must be present so the UI can + # show the user what is happening. + assert any("Welcome to Codex" in line for line in log_lines) + assert any("Successfully logged in" in line for line in log_lines) + # device_url + device_code events must still fire. + url_events = [ev for ev in events if ev.get("type") == "device_url"] + code_events = [ev for ev in events if ev.get("type") == "device_code"] + assert url_events and url_events[0]["url"].endswith("/codex/device") + assert code_events and code_events[0]["code"] == "ABCD-EFGH" + + def test_buffered_result_none_final_does_not_emit_repr(self, monkeypatch): + """A buffered TurnResult whose final_response is None must NOT + send a Python object repr (``TurnResult(...)``) to the user. + Returning an empty content chunk is the right shape: the + stream still finishes with the usage + stop + [DONE] frames, + but no garbage assistant text appears. + """ + + class _ResultNoFinal: + final_response = None # explicit None + + def __repr__(self): + return "TurnResult(internal=should_not_leak)" + + class _ThreadBuffered: + async def run(self, prompt): + return _ResultNoFinal() + + class _Async: + async def __aenter__(self): + return self + + async def __aexit__(self, *a): + return False + + async def thread_start(self, **kw): + return _ThreadBuffered() + + _install_fake_codex_sdk(monkeypatch, _Async) + from core.inference.codex_provider import stream_codex + + chunks: list[str] = [] + + async def _collect(): + async for c in stream_codex( + messages = [{"role": "user", "content": "hi"}], + model = "gpt-5.5", + parallel_calls = 1, + ): + chunks.append(c) + + asyncio.run(_collect()) + body = "".join(chunks) + assert ( + "TurnResult" not in body + ), f"Python object repr leaked to user content: {body!r}" + assert "should_not_leak" not in body + # Stream still terminated cleanly. + assert "[DONE]" in body + def test_empty_stream_falls_back_to_completed_agent_message(self, monkeypatch): """A successful turn that emits zero ``message.delta`` events but DOES emit a final ``ItemCompletedNotification`` with an diff --git a/studio/frontend/src/features/chat/components/codex-login-button.tsx b/studio/frontend/src/features/chat/components/codex-login-button.tsx index deaacb8a71..9667c8f382 100644 --- a/studio/frontend/src/features/chat/components/codex-login-button.tsx +++ b/studio/frontend/src/features/chat/components/codex-login-button.tsx @@ -66,14 +66,14 @@ export function CodexLoginButton({ onLoggedIn }: Props) { ) as AsyncGenerator) { if (event.type === "device_url" && event.url) { setDeviceUrl(event.url); - // Open the verification page eagerly so the user doesn't - // have to copy the URL out of the log surface. ``noopener`` - // prevents the auth-tab from controlling the Studio window. - try { - window.open(event.url, "_blank", "noopener,noreferrer"); - } catch { - // Ignore -- the URL is still visible in the log. - } + // Do NOT auto-open the verification URL with `window.open`. + // The click handler that started this flow has already + // awaited an SSE event, so the call is no longer in a user + // gesture and most browsers (Firefox, Safari, Chrome with + // strict popup settings) will silently block the popup. + // The URL is rendered as a prominent link below so the + // user can open it in one click without depending on the + // popup heuristic. } else if (event.type === "device_code" && event.code) { setDeviceCode(event.code); } else if (event.type === "log" && event.line) { @@ -114,17 +114,29 @@ export function CodexLoginButton({ onLoggedIn }: Props) { {busy ? "Signing in to Codex…" : "Sign in to Codex"} {deviceUrl && ( -

- Verification URL:{" "} - + +

+ Or copy: {deviceUrl} +

+ )} {deviceCode && (