Studio: round 5e Codex hardening (4 follow-ups)
1. parallel_calls validator now clamps instead of 422-rejecting.
The Pydantic schema was `int Field(ge=1, le=20)`, which was a
regression from the pre-PR OpenAI-extra behaviour: a non-Codex
client that sent the field with a legacy value like 0 (or a
stray string from a misconfigured wrapper) now got a 422 even
though the route silently ignores the field on every non-Codex
provider. Replaced with a `field_validator(mode="before")` that
coerces any input to the [1, 20] range, keeping the schema docs
self-documenting while accepting legacy inputs.
2. Buffered Codex result with `final_response=None` no longer
leaks `TurnResult(...)` Python object repr into the chat. The
upstream SDK documents `TurnResult.final_response` as nullable
for turns that perform tool work without producing a final
assistant message; the previous `... or str(result)` fallback
would render the repr as visible assistant text. New
`_buffered_result_text` helper returns the empty string in that
case so the stream finishes cleanly with no extra content
chunk. Same fix applied to `_run_codex_synthesis`.
3. Device-login SSE no longer forwards arbitrary subprocess output
to the browser. The previous code yielded every CLI line under
`{type:"log"}`, which on a shimmed binary could leak refresh
tokens, auth JSON, or local config paths into the authenticated
stream. Filtered to a known-safe vocabulary ("Welcome to
Codex", "Initializing", "Successfully logged in", etc.).
`device_url` and `device_code` events still fire as before.
4. CodexLoginButton no longer calls `window.open` from inside an
awaited SSE handler. Browser popup blockers (Firefox, Safari,
Chrome strict) silently block popups triggered outside a fresh
user gesture, so the auto-open was unreliable. Replaced with a
prominent "Open verification page" button styled as an anchor;
the click handler is a real user gesture and is never blocked.
The URL string is still shown below the button for copy/paste.
Tests: 46 cases total (was 43). New regressions cover the
parallel_calls clamp path on three garbage inputs, the buffered
TurnResult-with-None-final repr leak guard, and the device-login
log filter (asserts refresh tokens / auth.json paths are dropped
while known-safe progress lines pass through).
This commit is contained in:
Daniel Han
parent
9b4bd11c9f
commit
f97a800d5b
4 changed files with 279 additions and 40 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -66,14 +66,14 @@ export function CodexLoginButton({ onLoggedIn }: Props) {
|
|||
) as AsyncGenerator<CodexLoginEvent>) {
|
||||
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"}
|
||||
</Button>
|
||||
{deviceUrl && (
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Verification URL:{" "}
|
||||
<a
|
||||
href={deviceUrl}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="underline"
|
||||
<div className="space-y-1">
|
||||
<Button
|
||||
type="button"
|
||||
variant="outline"
|
||||
size="sm"
|
||||
asChild
|
||||
>
|
||||
{deviceUrl}
|
||||
</a>
|
||||
</p>
|
||||
{/* Opens via a real anchor click so popup blockers cannot
|
||||
interfere -- the popup-block path used to apply when
|
||||
`window.open` was triggered from inside an awaited
|
||||
event handler instead of a fresh user gesture. */}
|
||||
<a
|
||||
href={deviceUrl}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
>
|
||||
Open verification page
|
||||
</a>
|
||||
</Button>
|
||||
<p className="break-all text-[11px] text-muted-foreground">
|
||||
Or copy: {deviceUrl}
|
||||
</p>
|
||||
</div>
|
||||
)}
|
||||
{deviceCode && (
|
||||
<p className="text-xs text-muted-foreground">
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue