Tighten panic on double truncation #6701
Conversation
![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.
ℹ️ 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 let Ok(json) = serde_json::from_str::<serde_json::Value>(content) { | ||
| if let Some(output) = json.get("output") | ||
| && output | ||
| .as_str() | ||
| .unwrap_or("") | ||
| .starts_with("Total output lines:") | ||
| && output.as_str().unwrap_or("").contains("omitted") | ||
| { | ||
| error_or_panic(format!( | ||
| "FunctionCallOutput content was already truncated before ContextManager::record_items; this would cause double truncation {content}" | ||
| )); |
There was a problem hiding this comment.
When content is the serialized Vec<ContentBlock> that we build in FunctionCallOutputPayload::from(&CallToolResult), serde_json::from_str succeeds and returns a Value::Array. Because arrays (and even Value::String) do not have an output key, this new branch short-circuits and we never reach the raw content.contains("Total output lines:") check. The previous implementation caught these double-truncation cases because it examined the string regardless of shape; now the guard silently fails for exactly the shapes our shell/MCP tool outputs use, so double truncation will slip through. We should still run the raw-string fallback whenever the parsed JSON doesn’t contain an output field.
Useful? React with 👍 / 👎.
aibrahim-oai
force-pushed
the
tighten_panic_double_truncation
branch
from
a56a0f4 to
e91a3b8
Compare
No description provided.