Add thread/shellCommand to app server API surface#14988
Conversation
|
@codex review |
etraut-openai
force-pushed
the
etraut/threadshellcommand
branch
from
89bb769 to
15be728
Compare
![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
codex/codex-rs/core/src/rollout/recorder.rs
Line 155 in 89bb769
Preserve formatted command output when persisting rollouts
thread/shellCommand adds formatted_output to CommandExecution, but rollout sanitization still clears it before persistence. As a result, thread/read and snapshot replay lose model-formatted exec text after reload/thread switch, despite the new API surfacing this field. This causes replayed command output to differ from live output and drops truncation/format metadata.
ℹ️ 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".
etraut-openai
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Preserve raw bytes when forwarding command output deltas
ExecCommandOutputDelta chunks are converted with String::from_utf8_lossy(...) before emission. Non-UTF8 bytes are replaced with U+FFFD, then later re-encoded, so output is irreversibly corrupted. Because exec legacy events are now shadowed in the TUI adapter, clients no longer have a lossless fallback path for these chunks.
ℹ️ 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".
etraut-openai
force-pushed
the
etraut/threadshellcommand
branch
from
731792e to
0341d1d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0341d1d6d0
ℹ️ 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".
|
@codex review |
etraut-openai
force-pushed
the
etraut/threadshellcommand
branch
from
548529a to
5b23c8d
Compare
| #[ts(export_to = "v2/")] | ||
| pub struct ThreadShellCommandParams { | ||
| pub thread_id: String, | ||
| pub command: String, |
There was a problem hiding this comment.
hmm, we use this for command/exec, should we be consistent?
/// Command argv vector. Empty arrays are rejected.
pub command: Vec<String>,
There was a problem hiding this comment.
command/exec requires the caller to do their own shell expansion. This new call assumes that the string will be sent to the shell "as is" for interpretation by the shell itself. I don't think it makes sense to make this a vector of strings in this case.
| aggregated_output: Option<String>, | ||
| /// The command's output after applying the model-facing formatter/truncation policy. | ||
| #[serde(default)] | ||
| formatted_output: String, |
There was a problem hiding this comment.
Do we need to expose this? I'd avoid, since this could bloat the payload quite a bit. Could we just rely on the existing aggregated_output?
There was a problem hiding this comment.
Good point. I've changed this.
| #[serde_as(as = "serde_with::base64::Base64")] | ||
| #[schemars(with = "String")] | ||
| #[ts(type = "string")] | ||
| pub delta: Vec<u8>, |
There was a problem hiding this comment.
hmm this seems like it could be a breaking change
There was a problem hiding this comment.
After thinking about this more, I don't think that clients are generally going to be able to do much with raw byte output anyway. We'll stick with best-effort conversion to u8 text. I've undone the change to this structure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66af6244ad
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 502037815c
ℹ️ 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".
etraut-openai
force-pushed
the
etraut/threadshellcommand
branch
from
91c5b21 to
acb0aa0
Compare
| } | ||
| EventMsg::ExecCommandOutputDelta(exec_command_output_delta_event) => { | ||
| let item_id = exec_command_output_delta_event.call_id.clone(); | ||
| let delta = String::from_utf8_lossy(&exec_command_output_delta_event.chunk).to_string(); |
There was a problem hiding this comment.
nit: felt cleaner to keep it at the top. non-blocking though
2 participants
This PR adds a new
thread/shellCommandapp server API so clients can implement!shell commands. These commands are executed within the sandbox, and the command text and output are visible to the model.The internal implementation mirrors the current TUI
!behavior.CommandExecutionthread items, including source and formatted output metadatatui_app_serverexec rendering pathThis PR also wires
tui_app_serverto submit!commands through the new API.