Feat/restore image generation history#15223
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
![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: db2240182f
ℹ️ 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".
won-openai
force-pushed
the
feat/restore-image-generation-history
branch
from
180df2d to
18baff0
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/tui_app_server/src/app/app_server_adapter.rs
Lines 993 to 998 in 18baff0
Update test adapter for new saved_path field
ThreadItem::ImageGeneration gained a saved_path field, but this exhaustive match arm still destructures without ../saved_path. The same file also constructs ThreadItem::ImageGeneration test fixtures without the new field, so cargo test -p codex-tui-app-server test builds will fail to compile.
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9452e68d3e
ℹ️ 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".
| model_provider: "fallback".to_string(), | ||
| cwd: PathBuf::new(), | ||
| cli_version: String::new(), | ||
| source: SessionSource::VSCode, |
There was a problem hiding this comment.
flakycode/testcase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 347b9b8cf2
ℹ️ 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".
| RolloutItem::EventMsg(EventMsg::ImageGenerationEnd(mut event)) | ||
| if mode == EventPersistenceMode::Limited && event.saved_path.is_some() => | ||
| { | ||
| // ResponseItem::ImageGenerationCall already persists the inline base64 payload. | ||
| // Keep only the saved-path metadata in limited rollouts to avoid storing the blob twice. | ||
| event.result.clear(); | ||
| RolloutItem::EventMsg(EventMsg::ImageGenerationEnd(event)) |
There was a problem hiding this comment.
Do not clear ImageGenerationEnd.result at persist time solely because saved_path is present. This erases the only portable copy of image bytes; if the file is later deleted/unavailable, resumed threads cannot render the image anymore. The read path already conditionally clears only when saved_path exists on disk, so this unconditional write-time clearing causes data loss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4dca4695f
ℹ️ 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 Path::new(saved_path).is_file() { | ||
| result.clear(); |
There was a problem hiding this comment.
clear_inline_image_results_for_saved_files always blanks ThreadItem::ImageGeneration.result when saved_path exists. This runs for resumed turns regardless of rollout persistence mode, so extended-history sessions lose their inline image payloads even though extended mode is meant to preserve detailed event data. Clients that cannot open the server-local file path get an empty image result.
Useful? React with 👍 / 👎.
won-openai
force-pushed
the
feat/restore-image-generation-history
branch
from
a4dca46 to
0552a98
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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
Restore image generation items in resumed thread history