Skip to content

Bound unified exec output collection#31802

Open
jif-oai wants to merge 3 commits into
mainfrom
jif/bound-unified-exec-collection
Open

Bound unified exec output collection#31802
jif-oai wants to merge 3 commits into
mainfrom
jif/bound-unified-exec-collection

Conversation

@jif-oai

@jif-oai jif-oai commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Why

Unified exec stores process output in a 1 MiB head/tail buffer, but each poll drained that buffer into an uncapped Vec. An executor that kept producing output during the yield window could therefore make one exec_command or write_stdin call retain much more than the intended limit.

Simply joining the retained head and tail would also make omitted output look contiguous and complete.

What changed

  • Collect drained output into another HeadTailBuffer instead of an uncapped Vec.
  • Keep draining process output after the limit is reached so the producer does not become blocked.
  • Preserve the beginning and end of the output using the existing 1 MiB policy.
  • Store retained output in at most two byte containers, dropping empty chunks and coalescing tiny chunks so metadata cannot grow with event count.
  • Insert an explicit omitted-byte marker between the retained head and tail whenever output is dropped.
  • Add regression coverage for repeated full-buffer drains and adversarial empty and tiny chunks.

Scope

This only changes call-local unified-exec output collection and the private head/tail buffer it uses. Executor transport framing and process-event queue limits are separate concerns.

@jif-oai jif-oai requested a review from a team as a code owner July 9, 2026 14:36

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b9ac9c28a

ℹ️ 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".

Comment thread codex-rs/core/src/unified_exec/process_manager.rs
@jif-oai

jif-oai commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1484d97181

ℹ️ 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".

Comment on lines 1278 to 1280
for chunk in drained_chunks {
collected.extend_from_slice(&chunk);
collected.push_chunk(chunk);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve omissions from the drained process buffer

When the process produces more than UNIFIED_EXEC_OUTPUT_MAX_BYTES before this poll gets to drain output_buffer, that source HeadTailBuffer has already dropped a middle segment; drain_chunks() returns only its retained head/tail and clears the omitted count. Pushing those chunks here as ordinary contiguous bytes means the returned tool output can silently splice around the missing middle with no omission marker/count, despite the new marker logic. This needs to propagate the drained buffer's omitted byte count (or otherwise mark the gap) before appending the retained chunks.

Useful? React with 👍 / 👎.

}

collected
collected.to_bytes_with_omission_marker()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the omission marker visible after model truncation

When a poll itself drops output, to_bytes_with_omission_marker() places the marker in the middle of the retained 1 MiB buffer, but ExecCommandToolOutput::response_text() later truncates large raw output by preserving only the beginning and end for the model. For any output above the byte cap and above the model token budget, that second truncation drops this middle marker, so the model sees a normal token-truncation notice but not that unified exec already omitted process bytes. Consider surfacing the omission metadata outside the truncatable middle of raw_output.

Useful? React with 👍 / 👎.

Comment thread codex-rs/core/src/unified_exec/process_manager_tests.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants