Studio: stream live tool output with SSE heartbeats, fix web page extraction, and surface interrupted turns#7083
Conversation
…extraction Server-side python/terminal tools now stream incremental stdout to the chat UI while running (new tool_output SSE event), and every blocking tool execution emits heartbeat keepalives so reverse proxies (Cloudflare tunnels cap idle streams at ~100s) cannot drop the connection mid-turn. The tool loop routes also emit a stall keepalive during silent prompt prefill between tool iterations. The final role=tool message the model sees is byte-identical to before, so tool-call parsing, nudging, and healing are untouched. web_search page fetches now extract main content: GitHub repo root pages are rewritten to the README API (with HTML fallback), hidden/aria-hidden client error placeholders are dropped, conversion scopes to article/main, and known boilerplate fragments are stripped. Non-HTML responses are returned raw instead of being run through the HTML converter. The frontend renders live-scrolling tool output inside running python and terminal cards, and a chat stream that ends without a terminal signal now surfaces an explicit interrupted state with a Retry action instead of silently ending the turn.
|
@codex review |
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements live tool-output streaming and heartbeats for server-side tool execution, alongside main-content extraction and boilerplate stripping for the web fetch tool. The backend changes introduce a streaming wrapper for blocking tools and enhance HTML-to-Markdown conversion, while the frontend is updated to render live stdout/stderr and handle stream interruptions. The review comments correctly identify a few issues: a NameError in _fetch_url_raw due to the missing max_chars parameter, a bug where hidden void elements are not suppressed during HTML parsing, and a UX improvement opportunity to implement smart auto-scrolling in the live output UI component.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _fetch_url_raw( | ||
| url: str, | ||
| max_chars: int = _MAX_PAGE_CHARS, | ||
| timeout: int = 30, | ||
| ) -> str: | ||
| """Fetch a URL and return plain text content (HTML tags stripped). | ||
| extra_headers: dict | None = None, | ||
| ) -> tuple[str | None, str, str]: |
There was a problem hiding this comment.
The extracted _fetch_url_raw function does not accept max_chars as a parameter, but the unmodified lines inside it likely still reference max_chars to calculate max_bytes (e.g., max_bytes = max_chars * 4). This will result in a NameError: name 'max_chars' is not defined at runtime when fetching a URL.
To fix this, add max_chars to the signature of _fetch_url_raw with a default value of _MAX_PAGE_CHARS.
def _fetch_url_raw(
url: str,
timeout: int = 30,
extra_headers: dict | None = None,
max_chars: int = _MAX_PAGE_CHARS,
) -> tuple[str | None, str, str]:There was a problem hiding this comment.
In the current head _fetch_url_raw does not reference max_chars at all: line 1593 is max_bytes = _MAX_FETCH_BYTES (512 KiB) and the body is read with resp.read(max_bytes). There is no NameError, and the download is already byte-capped inside the function, so no signature change is needed.
| max_chars, | ||
| ) | ||
|
|
||
| err, body, content_type = _fetch_url_raw(url, timeout = timeout) |
There was a problem hiding this comment.
Pass the max_chars parameter to _fetch_url_raw to ensure the download size is correctly capped and to avoid any potential NameError inside _fetch_url_raw.
| err, body, content_type = _fetch_url_raw(url, timeout = timeout) | |
| err, body, content_type = _fetch_url_raw(url, timeout = timeout, max_chars = max_chars) |
There was a problem hiding this comment.
The download size is already capped independently of max_chars: _fetch_url_raw reads resp.read(_MAX_FETCH_BYTES) (512 KiB). max_chars is a character cap applied later on the extracted text via _truncate_page_text, so passing it into _fetch_url_raw is unnecessary and the suggested call would fail (no such parameter).
| # ------------------------------------------------------------------ | ||
| # Structural bookkeeping shared by every start tag (skip/hidden/scope). | ||
| def _enter_tag(self, tag: str, attr_dict: dict) -> bool: | ||
| """Track open/hidden/scope state; return True when the tag's content | ||
| should be rendered (False = suppressed).""" | ||
| if tag not in _VOID_TAGS: | ||
| self._open_tags.append(tag) | ||
| if _is_hidden_element(attr_dict): | ||
| self._hidden_marks.append(len(self._open_tags) - 1) | ||
| if self._scope_tags is not None and tag in self._scope_tags: | ||
| self._scope_depth += 1 | ||
| if self._hidden_marks: | ||
| return False | ||
| if self._scope_tags is not None and self._scope_depth == 0: |
There was a problem hiding this comment.
Void elements (like <hr> or <img>) that have the hidden or aria-hidden="true" attribute are currently not suppressed because they are skipped in the tag not in _VOID_TAGS check. This causes them to be rendered even when they should be hidden.
To fix this, check if the void tag is hidden and return False immediately to suppress its rendering.
def _enter_tag(self, tag: str, attr_dict: dict) -> bool:
"""Track open/hidden/scope state; return True when the tag's content
should be rendered (False = suppressed)."""
is_hidden = _is_hidden_element(attr_dict)
if tag not in _VOID_TAGS:
self._open_tags.append(tag)
if is_hidden:
self._hidden_marks.append(len(self._open_tags) - 1)
elif is_hidden:
return False
if self._scope_tags is not None and tag in self._scope_tags:
self._scope_depth += 1
if self._hidden_marks:
return False
if self._scope_tags is not None and self._scope_depth == 0:
return False
return TrueThere was a problem hiding this comment.
Good catch. Fixed in b08ffc8: _enter_tag now returns False for a hidden void element (elif branch), since void tags never join the open-element stack and so cannot carry a hidden mark for a later close to pop. A hidden
/
now emits nothing. (
, hidden
, and a visible
guard.
| useEffect(() => { | ||
| const el = scrollRef.current; | ||
| if (el) { | ||
| el.scrollTop = el.scrollHeight; | ||
| } | ||
| }, [output]); |
There was a problem hiding this comment.
The useEffect hook unconditionally scrolls the output pane to the bottom on every new chunk of output. If a tool produces a large amount of output over time, this will forcefully scroll the user back to the bottom even if they have scrolled up to read earlier output.
Consider implementing a smart scroll behavior that only auto-scrolls if the user is already near the bottom of the container.
useEffect(() => {
const el = scrollRef.current;
if (el) {
const isNearBottom = el.scrollHeight - el.scrollTop - el.clientHeight < 40;
if (isNearBottom) {
el.scrollTop = el.scrollHeight;
}
}
}, [output]);
There was a problem hiding this comment.
Fixed in b08ffc8. The pane now tracks a pinnedToBottom ref updated by an onScroll handler (within 40px of the bottom counts as pinned), and the effect only auto-follows the tail when pinned. Reading earlier output no longer yanks the user back down; scrolling back to the bottom resumes following. Tracking the pinned state via onScroll rather than measuring after the append avoids the classic append-then-measure race.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed727fe24f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err is not None: | ||
| return err | ||
|
|
||
| is_html = "html" in content_type or (not content_type and _looks_like_html(body)) |
There was a problem hiding this comment.
When a fetched page omits Content-Type, resp.headers.get_content_type() defaults to "text/plain", so this not content_type branch is never reached and an HTML body from such a server is returned as raw markup instead of being converted to readable Markdown. This regresses direct URL fetches for pages with missing/invalid content-type headers; use the raw header presence (or sniff whenever the declared type is the default) before deciding the response is non-HTML.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13f7676. _fetch_url_raw now reports an empty content type when the Content-Type header is absent (get_content_type() otherwise defaults to text/plain), and _fetch_page_text sniffs the body whenever the declared type is not HTML, so header-less and mislabeled text/plain HTML pages are converted again, matching the pre-PR behavior. Genuinely non-HTML bodies stay raw. Added tests for absent header + HTML, absent header + plain text, and explicit text/plain + HTML body.
| reader.join(timeout = 5) | ||
| return "".join(chunks), timed_out |
There was a problem hiding this comment.
When the tool timeout is disabled (tool_call_timeout >= 9999, passed here as timeout is None), this fixed 5-second join still returns if a child/grandchild inherited stdout after the main process exits. The previous communicate(timeout=None) path waited for EOF, but the streaming path can now report a partial result after ~5 seconds and leave the descendant running; for unlimited timeouts this drain should wait until EOF or cancellation instead of using a bounded join.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13f7676. When timeout is None, _drain_process_output now waits for EOF like communicate(timeout=None) did, however long a grandchild holds the pipe open, stopping early only when the cancel event fires (it then kills the process group so the reader sees EOF). Finite timeouts keep the bounded remaining-budget join. Added a regression test where a background grandchild inherits stdout and writes 7 seconds after the main shell exits; the late output is now included with timeout=None.
| # line as it prints instead of the pipe's 8 KB buffer flushes. | ||
| if output_callback is not None: | ||
| safe_env = dict(safe_env) | ||
| safe_env.setdefault("PYTHONUNBUFFERED", "1") |
There was a problem hiding this comment.
When live output is enabled for a Python tool, adding PYTHONUNBUFFERED=1 makes the child observe a different environment than the non-streaming path, so code such as os.getenv("PYTHONUNBUFFERED") produces a different model-visible result even though the wrapper promises identical tool results. This affects every streamed local Python execution unless the variable was already set, so the buffering change should not be exposed through the child's environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13f7676. The PYTHONUNBUFFERED injection is removed, so the child invocation (argv and env) is byte-identical with and without streaming. The tradeoff is documented in the code: CPython block-buffers stdout when piped, so live granularity depends on the child flushing; unflushed output arrives in ~8 KB chunks or at exit, the final result is unchanged, and SSE heartbeats cover the quiet gaps. Alternatives were rejected for the same model-visibility reason: -u differs via sys.flags, stdbuf injects LD_PRELOAD/STDBUF vars into the env.
…arity in tool streaming Content-Type sniffing: get_content_type() defaults to text/plain when the header is absent, so the sniffing fallback never fired and header-less HTML came back as raw markup. Report an empty type for a missing header and sniff the body whenever the declared type is not HTML, so mislabeled text/plain HTML pages are converted like before the extraction change. Unlimited timeout drain: with tool_call_timeout disabled the old path used communicate(timeout=None) and waited for EOF, but the streaming drain capped the post-exit drain at a 5 second join, truncating output from a grandchild that holds stdout open. When timeout is None, drain until EOF or the cancel event fires; finite timeouts keep the bounded remaining-budget join. Env parity: drop the PYTHONUNBUFFERED=1 injection on the streaming path so the child invocation is byte-identical with and without streaming (the env var was model-visible via os.getenv). Live streaming granularity now depends on the child flushing; unflushed output arrives in ~8 KB chunks or at exit and the final result is unchanged, with SSE heartbeats covering the gaps.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13f767674b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._open_tags.append(tag) | ||
| if _is_hidden_element(attr_dict): | ||
| self._hidden_marks.append(len(self._open_tags) - 1) |
There was a problem hiding this comment.
When a hidden element is a tag with optional end tags, such as <p hidden> or <li hidden>, a following sibling start tag implicitly closes it in browser HTML parsing. This stack only records the new start tag and waits for an explicit end tag, so inputs like <p hidden>ad<p>real content</p> leave _hidden_marks active and suppress the visible sibling content; web fetches from pages that omit these common end tags can therefore return an empty or truncated main body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and worse than described: an unclosed
or
. Regression tests added for the p, li, and hr cases.
| "button", | ||
| "select", | ||
| "datalist", | ||
| "aside", |
There was a problem hiding this comment.
When documentation uses <aside> for note/warning callouts inside <article> or <main>, this global skip removes the entire subtree before main-content rendering. The web fetch then omits visible warnings or notes that were previously rendered as normal blocks by _BLOCK_TAGS; skip only page-furniture asides, or only asides outside the selected main content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This matches the reference behavior the module mirrors: Mozilla Readability's _prepArticle unconditionally strips aside from the extracted article content (this._clean(articleContent, "aside")). Docs frameworks render note/warning callouts as div.admonition (Sphinx, MkDocs Material, Docusaurus), not aside, so the practical loss is in-article furniture, which is exactly what the skip is for. Keeping the global skip.
| const liveId = | ||
| (backendToolCallId | ||
| ? toolConfirmationIdsByBackendId.get(backendToolCallId) | ||
| : undefined) || | ||
| backendToolCallId || | ||
| toolCallParts[toolCallParts.length - 1]?.toolCallId || |
There was a problem hiding this comment.
When two chat threads generate at the same time, backend tool IDs are not globally unique (for example safetensors starts each run at call_0, and GGUF can fall back to call_${idx}). This uses just the backend ID as the global toolLiveOutput key, so live stdout from two running tools with the same ID can be appended to the same pane, and one run's tool_end/final cleanup can clear the other's live output; include the thread/session scope used for confirmations in this key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: compare mode mounts two runtimes streaming at once (and the main chat view stays mounted while off-route), so call_0 can collide. Fixed in f073f74: the live/full output store keys are now prefixed with a pane scope (modelType plus pairId) provided via context by the pane's ChatRuntimeProvider, so writer and reader derive the same key from the same props, and end-of-run cleanup clears only its own keys instead of the whole map. Scoped by pane rather than thread id because unpersisted new threads have no stable id the components could recompute, and panes are the only concurrent writers in a tab.
| if etype == "heartbeat": | ||
| # Tool-execution wrapper heartbeat: keep the SSE stream | ||
| # alive through proxy idle timeouts (comment lines are | ||
| # ignored by SSE parsers). | ||
| yield _OPENAI_PASSTHROUGH_SSE_KEEPALIVE |
There was a problem hiding this comment.
When Anthropic Messages is run with disable_parallel_tool_use and a later server-side tool is being dropped, drop_until_tool_end consumes every event before this heartbeat branch. A long dropped tool will produce heartbeat events every 10s, which also prevents the 15s stall timer from firing, but none of those keepalives reach the client until tool_end; proxy idle timeouts can still interrupt the stream in exactly the case this heartbeat path is meant to cover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the drop branch ran before the heartbeat branch, and the outer stall keepalive never fires while the generator keeps producing events, so the dropped call's whole server-side execution was a silent window. Fixed in f073f74 by forwarding heartbeats before the drop_until_tool_end skip. Added a test driving _anthropic_tool_stream with a dropped second call, asserting its keepalives come through and the dropped call emits no second tool_use block.
A model writing a large tool call (a full python game is minutes of generation) produced nothing on the stream: the structured path accumulated delta.tool_calls fragments silently after the provisional card, and the text path's DRAINING state consumed everything until stream end. The user saw a dead Running spinner while the model was in fact writing code, and the byte-silent SSE segment was also the window where proxies drop the connection. New tool_args SSE events stream the arguments as they generate. The structured path forwards each fragment once a provisional card exists (backlog first, so the card starts from the top of the call). The text path sniffs the drained call for an enabled tool name and streams the raw call text under the id the stream-end parser assigns its first call (call_0), so the final tool_start reconciles the same card; the sniff is gated on enabled names plus the provisional size floor, and prose or ordinary JSON answers never spawn a card. The safetensors loop streams the drained render_html call to its existing provisional card the same way. The chat adapter accumulates the raw stream per card and feeds a partial JSON parse (call envelopes and stringified arguments unwrapped) into the part's args, so the python and terminal cards render the code live and the render_html canvas builds while streaming; both cards now say Writing code / Writing command during this phase via useToolArgsStatus. Display only: the parser input, the executed call, and the conversation the model sees are byte-identical, covered by new loop-level tests for the structured path, the text path, and the no-tool JSON answer.
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31861eacfa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| yield { | ||
| "type": "tool_start", | ||
| "tool_name": _sniffed, | ||
| "tool_call_id": _text_args_id, |
There was a problem hiding this comment.
When a drained text tool call is large enough to create this provisional tool_start but later fails _parse_tool_calls_from_text (for example malformed or incomplete python XML/JSON with Auto-Heal unable to recover), the not tool_calls branch returns at the end of the turn without emitting a matching tool_end. That leaves the UI with a permanently running provisional tool card even though the stream completes; the safetensors path handles this by closing the provisional card on parse failure, but the GGUF text path needs the same cleanup before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The DRAINING false-positive branch (sniff opened a card, stream-end parse failed) returned before the provisional-card cleanup loop, leaving the card spinning forever. Fixed in f073f74: that branch now closes unresolved provisional cards with an empty tool_end before delivering the text as content. Regression test added (truncated call with auto-heal off).
|
|
||
| idle_polls = 0 | ||
| chunk = item + _drain_pending() | ||
| if not stream_capped: |
There was a problem hiding this comment.
When a python/terminal tool keeps producing stdout after TOOL_OUTPUT_STREAM_MAX_CHARS is reached, subsequent chunks still reset the idle heartbeat counter but are skipped by this guard, so the wrapper yields neither tool_output nor heartbeat until the process exits. For a noisy long-running command this recreates the idle SSE window the change is trying to avoid; keep emitting periodic heartbeats while capped output is being discarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and slightly worse than described: a tool that keeps printing past the cap keeps the queue non-empty, so the loop never hits an idle poll at all and no heartbeat could fire regardless of the counter reset. Fixed in f073f74: post-cap chunks are drained and discarded with one poll-interval sleep each, and each drain counts toward the heartbeat cadence, so keepalives flow at the configured interval however chatty the tool is. Test added.
| if main_content: | ||
| for scope in (frozenset({"article"}), frozenset({"main"})): | ||
| text = _strip_boilerplate_lines(_render(source_html, scope)) | ||
| if len(text) >= _MIN_MAIN_CONTENT_CHARS: |
There was a problem hiding this comment.
For pages whose real <article> or <main> content is under 200 characters (short docs, README stubs, release notes, or status pages), this length check rejects the correct scoped extraction and falls back to rendering the whole document, which reintroduces generic nav/sidebar/cookie text that main_content=True is meant to remove. Prefer a non-empty scoped result, or only fall through when the scoped subtree is empty or clearly boilerplate, rather than using size alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The fallback is strictly additive: the whole-document render still contains the short article/main text (minus nav, footer, and hidden subtrees), and boilerplate stripping still applies on that path, so no content is lost; short pages just get less scoping. The floor exists to reject empty article stubs, and at 200 chars it is already more permissive than Mozilla Readability's DEFAULT_CHAR_THRESHOLD of 500 for the same accept/reject decision. Keeping it as is.
…ata habits Live testing surfaced two issues in the tool streaming UX. First, a long python stdout ended in '... (truncated' in the finished card: the model-visible result is capped by tools._truncate (_MAX_OUTPUT_CHARS, previously 8000 chars) to protect the context window, and the card rendered that capped text even though the live stream had already shown everything. The cap stays (raised to 16000, overridable via UNSLOTH_TOOL_RESULT_MAX_CHARS) but display and model concerns are now split: the adapter preserves the accumulated live stream on tool_end whenever it captured more than the final result, and the finished python/terminal cards prefer it. The live-stream ceiling rises from 16 KB to 400 KB (chunks batch per poll, so SSE stays cheap), and both the live pane and the finished card render only the last 2000 lines with a Show all control so a huge output cannot jank the DOM. The truncation notice now tells the model the user saw the full output and that written files persist in the working directory. The final result string remains byte-identical with and without streaming. Second, models trained on ChatGPT code-interpreter transcripts write to /mnt/data, which does not exist here (the sandbox CWD is a per-thread persistent dir). Three layers, all identical across streaming and non-streaming paths: the python/terminal tool descriptions gain one sentence saying to use relative paths in the persistent CWD; a failed execution whose output shows a missing-file error on a known code-interpreter prefix (/mnt/data, /mnt/outputs, /home/sandbox, /workspace) gets a model-visible retry hint appended after truncation so it always survives; and a sitecustomize shim on the sandbox PYTHONPATH remaps those prefixes onto the CWD in open()/os.makedirs() with a one-line stderr notice, covering the python tool and any Python launched from the terminal tool without touching the exec wrapper (so tracebacks keep their line numbers). Bash-level file operations cannot be redirected without root or mount namespaces, so they rely on the description and the hint.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90f4dd8811
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _makedirs(name, *args, **kwargs): | ||
| return original_makedirs(_remap(name), *args, **kwargs) | ||
|
|
||
| builtins.open = _open |
There was a problem hiding this comment.
Patching only builtins.open misses pathlib.Path.open() / Path.write_text(), which call io.open directly, so common model-generated code like Path('/mnt/data/out.txt').write_text(...) still raises FileNotFoundError instead of being remapped into the sandbox. This undermines the new code-interpreter path shim for pathlib-based file writes; patch io.open as well or otherwise cover pathlib.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: pathlib's Path.open (and read_text, write_text) calls io.open directly, so the builtins.open patch never saw it. Fixed in f073f74 by installing the remap on io.open alongside builtins.open. Test added covering Path.write_text and read_text against /mnt/data, identical with and without streaming.
| useChatRuntimeStore | ||
| .getState() | ||
| .setToolFullOutput(id, liveOutput); |
There was a problem hiding this comment.
When a long call_0 output is preserved here, a later turn can reuse the same backend id (call_0/call_1 are generated per response for local tool calls) and return a shorter result, but this path never clears or overwrites toolFullOutput[id] unless the new live output is longer. The finished python/terminal card then prefers the stale preserved text over the new result, so users can see output from a previous tool run; clear the preserved entry on tool_start or when the condition is false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: toolFullOutput persisted for the whole session while local ids restart at call_0 every response (and every tool iteration within a turn), so a later call's finished card could show a previous call's preserved stream. Fixed in f073f74: tool_start now clears stale live and preserved entries for its key, and the keys are additionally pane-scoped (see the live-output key thread).
| GitHub's ``<div data-show-on-forbidden-error hidden>`` "Uh oh! There was | ||
| an error while loading." blocks) ship in the HTML but are only shown by | ||
| JavaScript on error, so they must not reach the Markdown output.""" | ||
| if "hidden" in attr_dict and attr_dict.get("hidden") != "false": |
There was a problem hiding this comment.
HTML hidden is a boolean attribute, so an element with hidden="false" is still not rendered by browsers. This check lets those subtrees through, which means pages or framework output that serialize falsey hidden state this way can leak client-side placeholders or other hidden boilerplate into fetched page text despite the new hidden-element stripping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct: hidden is an enumerated attribute (hidden / until-found) and the spec's invalid value default is the Hidden state, so hidden="false" is hidden in browsers and was leaking into the Markdown. Fixed in f073f74: any present hidden attribute now hides the subtree; aria-hidden="false" behavior is unchanged. Test added.
|
|
||
| def _reader() -> None: | ||
| try: | ||
| for line in iter(proc.stdout.readline, ""): |
There was a problem hiding this comment.
When a terminal command emits bytes that are invalid for the locale encoding, this reader thread raises UnicodeDecodeError at readline(), but the exception dies in the daemon thread and _bash_exec returns (no output). The non-streaming path reports the decode error, and agentic terminal calls now use this streaming path, so binary or non-UTF-8 command output can be silently lost; capture reader exceptions or decode with replacement as the Python tool does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, with a twist: UnicodeDecodeError is a ValueError subclass, so the reader's pipe-closed handler swallowed it and silently truncated the streamed output, while the non-streaming communicate() raised and surfaced an Execution error, so the two paths diverged. Fixed in f073f74 by giving the terminal Popen encoding utf-8 with errors replace, matching _python_exec, so both paths decode through the same TextIOWrapper config and neither can die on invalid bytes. Identity test with invalid UTF-8 output added.
…d collisions Review follow-ups on the tool streaming work: - _html_to_md: treat any present hidden attribute value as hidden (it is an enumerated attribute whose invalid value default is the Hidden state, so hidden="false" is still hidden), and implement HTML5 optional end tags so an unclosed <p hidden> or <li hidden> ends at the next sibling start tag instead of swallowing every following sibling until the parent closes - tool_stream_exec: keep heartbeats flowing after the live-output cap; a tool that keeps printing past the cap kept the queue non-empty, so neither tool_output nor heartbeat events were emitted and the SSE stream went silent past proxy idle timeouts - routes/inference: forward tool heartbeats before the disable_parallel_tool_use drop window swallows events, so a dropped call that executes server-side cannot leave the Anthropic stream silent - llama_cpp: close the provisional text tool card with a tool_end when the drained call fails to parse (DRAINING false-positive path), so the card cannot spin forever while the text is delivered as content - tools: decode terminal output as utf-8 with errors=replace like the python tool; invalid bytes used to raise UnicodeDecodeError from communicate() on the non-streaming path and silently truncate the streaming reader, so the two paths diverged - sitecustomize: patch io.open alongside builtins.open; pathlib Path.open, read_text and write_text call io.open directly and bypassed the remap - frontend: scope the toolLiveOutput/toolFullOutput store keys by pane (modelType and pairId) and clear stale entries on tool_start; backend ids like call_0 repeat across turns and across concurrently streaming panes (compare mode), so a later turn or another pane could display the wrong preserved output, and run-end cleanup now clears only its own keys Each backend fix carries a regression test that fails on the previous code; the byte-identity tests between streaming and non-streaming stay green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
unsloth/studio/frontend/src/features/chat/tool-output-scope.ts
Lines 26 to 27 in f073f74
Include thread identity in preserved output keys
This scope is shared by every thread in the same pane, while local backend tool ids repeat as call_0, call_1, etc. Because toolFullOutput is only cleared when a later tool_start reuses the id, a long-output tool in one thread can leave preserved text under this key; opening another thread or historical message with a shorter python/terminal card using the same id can then display the previous thread's output. Include a thread/message identity in the key or clear preserved full output on thread switches.
Use an escaped separator instead of a literal NUL
This template literal contains an actual NUL byte, which makes Git treat tool-output-scope.ts as binary (git diff only reports “Binary files differ”). That prevents normal textual review/blame for future changes to this source file; use an escaped separator such as \0 (same runtime key) or a printable delimiter so the file remains text.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| id && | ||
| liveOutput.length > | ||
| ((toolEvent.result as string) ?? "").length | ||
| ) { |
There was a problem hiding this comment.
When a python/terminal tool prints enough output and then times out, this length-only check stores toolFullOutput; the finished cards prefer that over toolEvent.result, so the user sees the captured stdout but loses the final Execution timed out after ... result. The same can hide other short final annotations after long output. Gate this on an actual truncation result or append/preserve the final result text so completed failed tools still show why they stopped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4078886. The finished python/terminal card shows the full live stream only when the result is just its truncated prefix; otherwise it appends the result so the failure/exit tail (timeout notice, Exit code N) is never hidden, and the copy button copies both. Logic lives in a shared preferFullToolOutput (tool-output-scope.ts) used by both tool cards. Frontend tsc and build pass.
| return text[:limit] + ( | ||
| f"\n\n... (truncated, {len(text)} chars total; the user was shown " | ||
| "the full output, and any files the code wrote persist in the " | ||
| "working directory)" |
There was a problem hiding this comment.
This truncation notice is added by _truncate() for every python/terminal execution, including non-streaming chat/API calls and direct execute_tool() uses where no tool_output events are delivered to the user. For long outputs in those paths the model-visible result says the user was shown the full output even though the client only receives the truncated text, which can make the assistant rely on output the user never saw. Make the wording conditional on an actually streamed UI path, or keep the generic truncation notice here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4078886. The truncation notice no longer claims the user saw the full output. Making it conditional on an attached output_callback would break the streaming vs non-streaming byte-identity invariant the tests assert (test_python_exec_*_identical_with_streaming), so the wording is mode-neutral and stays truthful for non-streaming chat/API and direct execute_tool() callers: it states the full output is not retained here and that any files the code wrote persist in the working directory. Added a regression test over the truncation path asserting byte identity across modes and the absence of the false claim.
| import os | ||
| import sys | ||
|
|
||
| _PREFIXES = ("/mnt/data", "/mnt/outputs", "/home/sandbox", "/workspace") |
There was a problem hiding this comment.
The tool description now explicitly tells models that /tmp/outputs is one of the nonexistent absolute paths to avoid, but this remap list omits that prefix. If generated Python creates or writes /tmp/outputs/..., sitecustomize will leave it in the host /tmp instead of the per-session sandbox workdir, so the file is not preserved/served with the conversation and can bleed across sessions; add this prefix to the remap and matching missing-path hint list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4078886, with a guard. Unlike /mnt/data and the other prefixes, /tmp exists on the sandbox host and code can legitimately create /tmp/outputs (e.g. Path('/tmp/outputs').mkdir(), which goes through the unpatched os.mkdir). Unconditionally remapping would shadow that real directory, so the shim remaps /tmp/outputs into the per-conversation workdir only while it does not already exist (a real one passes straight through) and /tmp/outputs also joins the missing-path retry-hint list. This keeps the tool description truthful and heals the habit without surprising code that made its own /tmp/outputs. Added a hermetic _remap test (test_sandbox_sitecustomize.py).
… truthful Finished python/terminal cards preferred the fuller live stream by length alone, so a tool that printed a lot then timed out or exited non-zero showed the captured stdout but dropped the final result's status (timeout notice, Exit code N). preferFullToolOutput now shows the stream when the result is just its truncated prefix, and appends the result otherwise so the failure tail always survives and the copy button copies both. The result truncation notice claimed the user was shown the full output, but the same wrapper serves non-streaming chat/API and direct execute_tool() callers where nothing is streamed to anyone. The notice is now mode-neutral and stays byte-identical with and without an output_callback, keeping the streaming vs non-streaming invariant intact. The sandbox sitecustomize shim now remaps /tmp/outputs into the working directory only while it does not already exist, so a real /tmp/outputs the user's own code created is never shadowed; /tmp/outputs also joins the missing-path retry-hint list.
|
@codex review |
…ned only when at bottom
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Include the message/run identity in tool output keys
When a chat contains multiple local tool runs, backend tool ids such as call_0 restart on each assistant response, but this key only combines the pane scope and toolCallId. Finished Python/terminal cards read toolFullOutput through the same key, so a later long-output call_0 clears or overwrites the entry used by earlier call_0 cards in the same pane; those older cards can lose their full output or display the newer tool’s output. Add a per-message/run identifier to the key, or persist the full output on the specific tool part.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| builtins.open = _open | ||
| # pathlib.Path.open / write_text / read_text call io.open directly, not |
There was a problem hiding this comment.
When generated code prepares code-interpreter paths with Path('/tmp/outputs').mkdir(...) or Path('/mnt/data').mkdir(parents=True, ...), pathlib calls os.mkdir, not os.makedirs, so it bypasses this remap. On hosts where /tmp/outputs can be created, later open('/tmp/outputs/...') is no longer remapped because the conditional prefix now exists; on locked-down hosts the /mnt/data setup can still fail before the write. Route os.mkdir/Path.mkdir through _remap too so common pathlib setup stays in the conversation workdir.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 796a7a1. os.mkdir now goes through the same _remap as os.makedirs. Patching os.mkdir alone was not enough though: for Path('/mnt/data').mkdir(parents=True, exist_ok=True) pathlib hits FileExistsError on the mapped target and then probes the (intentionally unpatched) os.stat via Path.is_dir() against the never-present /mnt/data, so it still re-raised. So Path.mkdir is patched too, remapping the receiver up front so the whole parents/exist_ok dance lands on the working directory and stays idempotent. Hermetic tests cover Path.mkdir(parents=True) for the convention path plus a passthrough for real paths.
| if item is done_sentinel: | ||
| break | ||
|
|
||
| chunk = item + _drain_pending() |
There was a problem hiding this comment.
After a tool exceeds the 400 KB live-output cap, this still concatenates the current item with every queued chunk before the capped branch discards it. For a chatty command such as yes or a tight print loop, the producer can enqueue far more than one poll interval's worth of text, so the cap no longer bounds memory/CPU during the rest of the run. Once stream_capped is true, drain and drop queued items without building a combined string.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 796a7a1. Past the cap the wrapper no longer builds item + _drain_pending(): a new _drain_and_drop() pulls the current chunk and every queued sibling off the queue without concatenating them, so a chatty tool (yes, a tight print loop) can no longer make the cap meaningless by flooding memory/CPU with discarded strings. Each drain still sleeps one poll interval and counts as an idle poll, so the heartbeat cadence is unchanged and test_heartbeats_continue_while_capped_output_flows stays green.
…mkdir
Past the live-output cap stream_tool_execution built item + _drain_pending()
(the current chunk joined with every queued sibling) only to discard it in the
capped branch, so a chatty tool (yes, a tight print loop) could enqueue far
more than one poll interval of text and blow past the memory/CPU ceiling the
cap exists to enforce. Drain and drop queued items without building a combined
string, still counting each drain toward the heartbeat cadence so the SSE
keepalive survives.
Generated code often prepares code-interpreter paths with
Path('/mnt/data').mkdir(parents=True, exist_ok=True); pathlib drives that
through os.mkdir (not the patched os.makedirs) per component and, on
FileExistsError, probes the unpatched os.stat via Path.is_dir(), so the setup
raised before open() ever ran. Patch os.mkdir with the same remap and patch
Path.mkdir so the whole parents/exist_ok dance lands on the mapped working
directory and stays idempotent; real paths still pass through.
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47dede19b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| reader.join(timeout = remaining) | ||
| if reader.is_alive(): | ||
| timed_out = True | ||
| _kill_process_tree(proc) |
There was a problem hiding this comment.
When a command starts a child that inherits stdout and then the main process exits, this branch runs after proc.wait() has already returned; _kill_process_tree immediately returns for an exited proc, so the descendant is never signaled. In finite-timeout runs the tool can return while that child keeps running/holding the pipe, and in timeout=None runs cancellation leaves the child behind; store the process group before waiting and kill that group here even after the parent has exited.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2e43a7c. _kill_process_tree short-circuits on the reaped parent, so a grandchild inheriting stdout was never signaled. _drain_process_output now captures the setsid process group before waiting and SIGKILLs that group at both give-up points (finite-timeout reader-still-alive and timeout=None cancel). Added test_bash_exec_finite_timeout_kills_grandchild_holding_stdout, which fails without the fix.
| chunk = item + _drain_pending() | ||
| idle_polls = 0 | ||
| if streamed_chars + len(chunk) > TOOL_OUTPUT_STREAM_MAX_CHARS: | ||
| chunk = chunk[: max(0, TOOL_OUTPUT_STREAM_MAX_CHARS - streamed_chars)] |
There was a problem hiding this comment.
When a chatty tool fills the queue before the cap has been tripped, this concatenates the current item with every pending chunk before enforcing TOOL_OUTPUT_STREAM_MAX_CHARS. A yes-style command or tight print loop can therefore make the first over-cap drain allocate/process far more than the 400 KB live-output cap; the capped branch now drops without joining, but the crossing batch needs the same bounded drain/truncate behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2e43a7c. The crossing batch now bounds its drain: _drain_pending stops joining once the remaining live-output budget is exceeded and discards the surplus in place (extracted into _drain_queue, mirroring _drain_and_drop), so a yes-style burst can no longer allocate far past the cap on the first over-cap batch. The truncated output stays byte-identical to joining everything; covered by test_drain_queue_bounds_the_over_cap_batch and test_over_cap_crossing_batch_streams_capped_output.
…bsolute path
Models invent absolute paths from seeing their CWD (a DeepSeek run did
open('/home/ubuntu/Sandbox/flappy_bird.html', 'w') and died with
FileNotFoundError). A prefix list cannot enumerate these, so the sitecustomize
shim gains a write-mode fallback in open()/io.open(): when a write/create-mode
open targets an absolute path outside the CWD whose parent directory does not
exist, redirect it to the basename in the CWD and emit the same one-line stderr
notice, echoing the original path. The prefix remaps still run first (they cover
reads and preserve subpaths); read modes never hit the fallback so real system
files fail or succeed truthfully; bytes paths pass through. The fallback is not
applied to mkdir/makedirs/Path.mkdir, since creating an arbitrary absolute
directory can legitimately succeed on the host, and that decision is documented
in a comment.
The model-visible retry hint now echoes the real failing path (parsed from the
traceback tail) instead of the canned /mnt/data example, and fires for any
absolute path outside the working directory, not just the enumerated prefixes,
while a relative miss still gets no hint.
The shim wrapper still adds one frame to tracebacks that surface open() errors;
suppressing only our frame has no clean standard mechanism (a wrapper always
adds a frame), so the frame is left as an accepted compromise.
Tests: hallucinated absolute write remaps to the CWD basename across w/a/x/w+;
reads of a missing absolute path pass through untouched; writes to an existing
external dir pass through; prefix subpaths still preserved; end-to-end write
fallback lands the file in the sandbox workdir identically with and without
streaming; the hint echoes the actual path for convention and non-convention
absolute paths alike.
Resolve two conflicts: keep both new imports in tools.py (time, urllib.parse); in the GGUF tool loop keep the stream_tool_execution worker-thread wrapper and forward thread_id into execute_tool so main's per-thread MCP stdio session scoping is preserved alongside live tool-output streaming.
The pane-scope and tool-output-key separators were literal NUL (0x00) bytes, which made git treat the file as binary and hide its diff and blame. Write them as \u0000 escapes instead; the runtime key value is unchanged.
stream_tool_execution ran its yield loop with no try/finally, so a gen.close() on client disconnect (GeneratorExit at a yield) skipped the worker join and never signalled cancellation. A tool that does not poll cancel_event mid-flight (web_search, MCP, search_knowledge_base) then kept request teardown blocked until the tool's own timeout. Thread the request cancel_event into the wrapper, set it only on the abnormal-exit path so a clean multi-tool turn is unaffected, and bound the worker join to a few seconds; the daemon worker cannot outlive the process.
The sandbox sitecustomize shim remapped code-interpreter prefixes (/mnt/data, /workspace, ...) onto the working directory for every open mode, including reads. A read of a path that truly did not exist was silently redirected onto a same-basename workdir file instead of raising on the path the model used, hiding real missing-input errors. Remap writes and creates as before, but remap a read only when the mapped workdir target already exists (re-reading a just-written artifact); otherwise keep the original absolute path so the failure stays truthful.
The web fetch applied timeouts per network operation, so a GitHub README API attempt plus its HTML fallback plus up to five redirect hops could run well past the tool timeout, and nothing aborted once the client had disconnected. Add a single wall-clock deadline shared across the API attempt, the fallback, every redirect hop and the body read, cap each hop's socket timeout at the time left on the budget, and poll cancel_event. SSRF host pinning, per-hop redirect revalidation, the five-hop cap and the size cap are unchanged.
Resolve three conflicts against the newer main (permission-mode work, #7079): keep both the tool-output-scope exports and PermissionModeDropdown in chat/index.ts; in the GGUF loop keep main's permission-mode normalization and the expanded tools import alongside the stream_tool_execution wrapper (thread_id and cancel_event still threaded); union the sandbox_site dir constant with main's auto-mode safety-detection block in tools.py.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2adccd72af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "Blocked: URL is missing a hostname.", "", "" | ||
|
|
||
| port = parsed.port or (443 if parsed.scheme == "https" else 80) | ||
| ok, reason, pinned_ip = _validate_and_resolve_host(parsed.hostname, port) |
There was a problem hiding this comment.
When a web_fetch is cancelled/disconnected before the first request (or while DNS is slow), this still enters _validate_and_resolve_host() before any _fetch_budget_exceeded() check, and getaddrinfo() is not bounded by the new deadline or cancel_event. In that scenario the tool worker can remain stuck in DNS despite the new overall timeout/cancellation path; check the budget before each resolve (including redirects) or make resolution itself bounded/cancellable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 8399841: _resolve_with_budget now checks the deadline and cancel_event before every host resolve (initial and each redirect) and runs getaddrinfo on a budget-polled worker thread, so a cancelled or slow DNS lookup aborts within the overall fetch budget instead of blocking in getaddrinfo.
| "ul", | ||
| "ol", | ||
| "dl", | ||
| "pre", | ||
| "blockquote", |
There was a problem hiding this comment.
When the GitHub README API returns raw Markdown that begins with a valid raw-HTML flow block such as <ul>, <pre>, or <blockquote>, these entries make _looks_like_html() classify the whole README as HTML. _fetch_page_text() then runs the Markdown through html_to_markdown, which collapses subsequent Markdown headings, lists, and fenced code into a single line, corrupting README content; restrict these ambiguous tags for README bodies or use response metadata/extension to distinguish real HTML.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, confirmed. A raw-Markdown README opening with a block tag (
- ,
- ,
- ,
,) was sniffed as HTML and collapsed by html_to_markdown. Fixed in 3d06f9004: the README API body is now sniffed with a stricter document-level check (doctype or a leading //), so only a real .html README is converted and a Markdown README is returned as-is. The general page-fetch path is unchanged.
for more information, see https://pre-commit.ci
The bounded worker join added for disconnect safety still ran on an abnormal close, so a client disconnect could wait the full join timeout; and the safetensors and Anthropic tool streams closed their generator synchronously on the event loop, unlike the GGUF path. On abnormal exit the daemon worker is abandoned, so join with a zero timeout instead of waiting; offload the safetensors and Anthropic gen.close to a thread to match GGUF; and surface a heartbeat as soon as cancel_event is set while the worker is silent so the route regains control at once instead of after a heartbeat interval.
The overall fetch deadline did not cover host resolution or the response body read, and query-mode web_search ignored cancellation. Resolve hosts (initial and every redirect) on a budget-polled helper so a slow or pre-cancelled getaddrinfo aborts on time; read the capped body in chunks with the budget re-checked between them so a slow-drip server cannot stretch a single read past the deadline; and gate the blocking DDGS query on cancel_event on both sides. SSRF host pinning, per-hop redirect revalidation, the five-hop cap and the size cap are unchanged.
The one-shot remap notice fired while computing the mapping, so a read that kept its original path emitted a false notice and spent the notice a later genuine remap needed. Only emit it once _remap_open commits to the redirect. Separately, os.open classified O_TRUNC / O_APPEND without O_CREAT as creating, but those cannot create a missing file, so a missing target now stays truthful (only O_CREAT maps to the creating mode).
for more information, see https://pre-commit.ci
…leading block tag The GitHub README API returns the raw file, almost always Markdown. _looks_like_html classified a Markdown README opening with a block tag (<ul>, <ol>, <dl>, <pre>, <blockquote>) as HTML, so _fetch_page_text ran it through html_to_markdown and collapsed its headings, lists and fenced code into a single line. Sniff the README body with a stricter document-level check (doctype or a leading <html>/<head>/<body>) so only a real .html README is converted; the general page path is unchanged.
for more information, see https://pre-commit.ci
The local Anthropic tool-stream and plain-stream paths called _anthropic_stream_error_event(e) with force defaulting to False, so an unclassified mid-stream failure (llama-server crash, decode OOM, a dropped upstream socket) returned no event. The except block then fell through to emitter.finish(), emitting a normal message_delta and message_stop that masked a truncated turn as a clean finish. Pass force = True at both fall-through sites so an unclassified failure emits a 500 SSE error event and returns, matching the Anthropic passthrough path that already forces it. Add regression tests covering both stream paths.
for more information, see https://pre-commit.ci
…eir own output Backend tool ids restart at call_0 every assistant response, and the transient toolLiveOutput/toolFullOutput store maps were keyed by pane scope plus that bare backend id. Two turns in the same pane therefore shared one key: the stale-clear at tool_start only guards the forward direction, so when a later call_0 finished and wrote its preserved full output, every earlier still-mounted finished card reading the same key re-rendered and displayed the newer tool's output instead of its own. Mint one per-run-unique part id per backend id (call_0:<uuid>) and route tool_start/output/args/end through a single resolver so all events for a call resolve the same id. The durable part carries the unique id, so the finished-card readers derive a collision-free key with no change, and the awaiting-confirmation path keeps its own synthesized id. Outbound replay stays paired (the assistant tool_call id and the role=tool result tool_call_id both come from the part id) and gains unique ids across turns, which strict providers require.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Problem
Three defects reported from a chat session on
unsloth/DeepSeek-V4-Flash-GGUF(UD-IQ1_S) behind a--secureCloudflare tunnel, all in the server-side tool stack (web_search, python, terminal):Web page fetches returned page furniture instead of content. Fetching a GitHub repo page produced "Uh oh! There was an error while loading. Please reload this page.", "You can't perform that action at this time.", skip-links, nav, and the language-stats sidebar. GitHub ships those error placeholders in the HTML behind the
hiddenattribute (JS reveals them on failure), and the HTML-to-Markdown converter rendered the whole document, hidden nodes included.python/terminal tools blocked with no streamed output.
execute_toolran synchronously inside the tool loop generator; between thetool_startandtool_endevents (up to the 300 s timeout) not a single byte was written to the SSE stream. The UI showed an indefinite "Running..." spinner with zero output.Responses silently stopped mid-turn. The llama-server log shows
W srv stop: cancel taskat ~10.3k tokens: the tunnel dropped the idle SSE connection (Cloudflare caps idle streams at roughly 100 s), Studio's disconnect watcher setcancel_event, and the upstream generation was aborted. The frontend treated the resulting clean EOF as a normal end of stream, so the turn just ended with no indication anything went wrong. Silence windows longer than the proxy cap come from two places in the tool path: tool execution itself, and prompt prefill between tool iterations (the conversation had grown to ~10k tokens, matching the cancel log).Changes
Live tool output + heartbeats (backend)
core/inference/tool_stream_exec.py:stream_tool_execution()runs the blocking tool in a worker thread and turns it into a generator that yields{"type": "tool_output", ...}chunks (live stdout/stderr) and{"type": "heartbeat"}events every 10 s of silence, then returns the tool's result viaStopIteration.value. Both the GGUF loop (llama_cpp.py) and the safetensors loop (safetensors_agentic.py) now execute every tool through this wrapper, so web_search and MCP calls get heartbeats too.tools.py:execute_tool/_python_exec/_bash_execaccept an optionaloutput_callback; subprocess stdout is drained line by line through the sameTextIOWrapper, so the returned result string is byte-identical with or without streaming. The finalrole=toolmessage the model sees is unchanged; tool-call parsing, dedup, nudging, and healing are untouched. A regression test asserts the exact message equality with and without streaming.routes/inference.py): the GGUF and safetensors tool streams forwardtool_outputas SSE data events and mapheartbeatto the existing: keep-aliveSSE comment. On top of that, the next-event wait in the tool streams (and the Anthropic/v1/messagestool stream) now emits a keepalive comment whenever the backend generator is silent for 15 s, which covers long prompt prefill between tool iterations, the second silence source behind the Cloudflare drops.Web page extraction (backend)
_html_to_md.py: subtrees withhidden/aria-hidden="true"are never rendered (this alone removes GitHub's error placeholders); newmain_content=Truemode scopes conversion to<article>(GitHub renders READMEs there) then<main>with a whole-document fallback, and strips a fixed list of known boilerplate fragments (skip-links, session banners, cookie prompts) from short lines outside code fences.<template>,<dialog>,<button>,<select>,<aside>are skipped.tools.py:github.com/{owner}/{repo}root pages are rewritten to the unauthenticated GitHub README API (/repos/{owner}/{repo}/readme, raw Accept), with fallback to the HTML fetch on any failure (404, rate limit). Non-HTML responses (raw.githubusercontent, plain text, markdown) are returned as-is instead of being collapsed by the HTML renderer. SSRF pinning, redirect validation, and size caps are unchanged.Interrupted-state UI (frontend)
chat-api.ts: the SSE reader tracks terminal signals; EOF withoutdata: [DONE]or afinish_reasonchunk now throwsStreamInterruptedErrorinstead of ending the turn silently.chat-adapter.ts+thread.tsx: an interrupted or failed turn shows an inline error banner on the message with a Retry button (ActionBar reload), plus a "Response interrupted" toast; the partial answer is kept.tool_outputevents append to a transienttoolLiveOutputstore map; running python/terminal cards render a live-scrolling output pane (auto-scrolls like the thinking block), the card mounts expanded while running, and a collapsed multi-tool group force-opens while output streams. The pane is transient; the persisted card still shows the final result.Audit: other long blocking non-streaming segments in the chat pipeline
Fixed in this PR (tool path):
Already covered before this PR:
_openai_admission_wait_stream_chunks).Noted, not changed here:
probe_timeoutand run before the response starts (pre-header, so a proxy 524 is possible on a down MCP server; existing cool-off limits it).chat_messagesitself; left as a follow-up. The interrupted state + Retry covers recovery.Tool-call argument streaming
A model writing a large tool call (a full python game is minutes of generation) still produced nothing on the stream: the structured path accumulated
delta.tool_callsfragments silently behind the provisional card, and the text path's DRAINING state consumed everything until stream end. The user saw a dead "Running" spinner while the model was writing code, and the byte-silent window was also where proxies drop the stream.New
tool_argsSSE events now stream the arguments as they generate: the structured path forwards fragments once a provisional card exists; the text path sniffs the drained call for an enabled tool name and streams the raw call text under the id the stream-end parser assigns (call_0), so the finaltool_startreconciles the same card; the safetensors loop streams the drainedrender_htmlcall to its existing provisional card. The chat adapter feeds a partial JSON parse (call envelopes and stringified arguments unwrapped) into the part's args, so the python/terminal cards render the code live ("Writing code" phase viauseToolArgsStatus) and the render_html canvas builds while streaming. Display only: the parser input, the executed call, and the conversation the model sees are byte-identical (loop-level tests cover the structured path, the text path, and the ordinary-JSON-answer case).Verification
studio/backend/testssuite on this branch: 6627 passed; the 20 failures (test_training_worker_flash_attn,test_mcp_stdio_improvements, 3 intest_openai_tool_passthrough) reproduce identically on cleanorigin/mainin the same environment (pre-existing, unrelated). New suites:test_tool_output_streaming.py(15 tests: byte-identical results, heartbeat cadence, stream cap, loop-level regression that the model-visible tool message is unchanged andtool_outputlands betweentool_start/tool_end) andtest_web_fetch_extraction.py(19 tests over snapshotted GitHub page fragments). Ruff clean on touched files.npm run build(tsc -b + vite) clean. No existing vitest specs to extend.unsloth/Qwen3.5-4B-MTP-GGUF(UD-Q8_K_XL): a python tool printing 12 lines over 60 s streamed 13tool_outputSSE events at exact 5 s cadence; max inter-byte gap on the wire dropped from 60 s+ to 5 s. A silent 35 s tool produced: keep-alivecomments every 10 s (max gap 10 s, well under the ~100 s proxy cap). The chat UI showed the expanded tool card with live-scrolling output while running.https://github.com/unslothai/unslothreturned the README ("README of ... fetched via the GitHub README API"); all boilerplate fragments absent.unsloth/DeepSeek-V4-Flash-GGUF(UD-IQ1_S, the reporter's model): "Create a Flappy Bird game in HTML" with Search + Code pills and Thinking High completed a 262 s, 14k-token turn with web sources and python tool calls, no mid-turn drop.W srv stop: cancel taskllama-server log line as the original incident, which the heartbeats now prevent from happening spuriously.Follow-ups from live testing (90f4dd8)
Two issues surfaced while testing the streaming live:
Truncated python output in the finished card. The model-visible tool result is capped by
tools._truncate(_MAX_OUTPUT_CHARS, previously 8000 chars) to protect the context window, and the finished card rendered that capped text even though the live stream had already shown everything. Display and model concerns are now split: the adapter preserves the accumulated live stream ontool_endwhenever it captured more than the final result, and the finished python/terminal cards prefer it. The live-stream ceiling rises from 16 KB to 400 KB (chunks batch per poll interval, so the SSE channel stays cheap), and both the live pane and the finished card render only the last 2000 lines with a "Show all" control so a very large output cannot jank the DOM. The model-visible cap rises to 16000 chars (matching the web-page cap), is overridable viaUNSLOTH_TOOL_RESULT_MAX_CHARS, and its notice now tells the model the user saw the full output and that written files persist in the working directory. The final result string remains byte-identical with and without streaming.Models writing to
/mnt/data(ChatGPT code-interpreter habit). A DeepSeek run didopen('/mnt/data/flappy_bird.html', 'w')and died withFileNotFoundError; the sandbox CWD is a per-thread persistent directory. Three layers, all identical across streaming and non-streaming paths:/mnt/dataand friends do not exist;/mnt/data,/mnt/outputs,/home/sandbox,/workspace) gets a model-visible retry hint, detected on the pre-truncation output and appended after truncation so it always survives;sitecustomizeshim on the sandboxPYTHONPATHremaps those prefixes onto the CWD inopen()/os.makedirs()and prints a one-line stderr notice so the model learns the real location. It covers the python tool and any Python launched from the terminal tool (env inheritance) without touching the exec wrapper, so tracebacks keep their line numbers. Bash-level file operations cannot be redirected without root or mount namespaces, so they rely on the description and the hint.New tests: truncation-notice format, env cap override, hint detection and end-to-end hint on python/bash failures, remap end-to-end (file lands in the sandbox workdir, notice in the result, streaming/non-streaming identical), and unremapped calls (
os.listdir) falling back to the hint.test_tool_output_streaming.pynow has 24 tests;test_sandbox_tools.pyextends the env whitelist test for the shimPYTHONPATH.Generalized sandbox write remap (48ac7d8)
A prefix list cannot enumerate the paths a model invents from seeing its own CWD (a DeepSeek run did
open('/home/ubuntu/Sandbox/flappy_bird.html', 'w'), outside every prefix, and died withFileNotFoundError), so thesitecustomizeshim now adds a write-mode fallback: a write/create-modeopen()/io.open()on an absolute path outside the CWD whose parent directory is missing is redirected to the basename in the CWD, while reads, existing external directories, andmkdirstay untouched. The retry hint now echoes the actual failing path (parsed from the traceback tail) and fires for any absolute path outside the working directory, not just the enumerated prefixes.